Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tile entities are leaked upon recursively despawning the Map entity #18

Closed
forbjok opened this issue May 11, 2021 · 10 comments
Closed

Tile entities are leaked upon recursively despawning the Map entity #18

forbjok opened this issue May 11, 2021 · 10 comments
Assignees
Labels
bug Something isn't working

Comments

@forbjok
Copy link
Contributor

forbjok commented May 11, 2021

When the Map entity is despawned with .despawn_recursive(), all tile entities belonging to that tilemap continue to exist even though they are no longer visible. As far as I can tell, this is due to the tile entities not being marked as children of the Map entity.

@StarArawn StarArawn self-assigned this May 11, 2021
@StarArawn StarArawn added the bug Something isn't working label May 11, 2021
@StarArawn
Copy link
Owner

Thanks for the report! I think we likely will want a map.despawn(commands, map_entity); api for this. I can't have tiles be children of chunks for various reasons.

@forbjok
Copy link
Contributor Author

forbjok commented May 11, 2021

They don't necessarily need to be children of chunks, do they?
Both chunks and tiles could all just be children of the Map entity itself.

@StarArawn
Copy link
Owner

Part of the issue with having tiles be a child of the map entity or the chunk entity is that tiles don't have standard transforms, and bevy currently complains about that. We could give them standard transforms, but then its just a lot of extra data that is not needed and people might assume that changing a tile transform would change where a tile is located which isn't supported and isn't easy to support.

@forbjok
Copy link
Contributor Author

forbjok commented May 11, 2021

tiles don't have standard transforms, and bevy currently complains about that

If that's the case, maybe we should suggest fixing it in Bevy. I don't see any reason why entities should be forced to have a Transform in order to be part of a hierarchy. Parent-child relationships are useful for a lot of things not related to positioning, such as in this case, for organizational purposes to make it easier to despawn all sub-entities when despawning the container entity.

@StarArawn
Copy link
Owner

StarArawn commented May 11, 2021

tiles don't have standard transforms, and bevy currently complains about that

If that's the case, maybe we should suggest fixing it in Bevy. I don't see any reason why entities should be forced to have a Transform in order to be part of a hierarchy.

Bevy relations RFC would fix this issue. :)

See:
bevyengine/rfcs#18

@forbjok
Copy link
Contributor Author

forbjok commented May 11, 2021

tiles don't have standard transforms, and bevy currently complains about that

If that's the case, maybe we should suggest fixing it in Bevy. I don't see any reason why entities should be forced to have a Transform in order to be part of a hierarchy.

Bevy relations RFC would fix this issue. :)

See:
bevyengine/rfcs#18

Yeah, this would probably allow it, assuming you can have the ECS enforce the deletion of certain relation types when their container entity is despawned.
This seems like a much more complex change though, and way overkill if the goal is simply to ensure sub-entities are despawned.

@StarArawn
Copy link
Owner

@forbjok I ran some tests with this, and I discovered a performance drop almost in half due to having a parent per child. If I instead have a map_query.despawn_layer(layer_id) would that be acceptable?

@forbjok
Copy link
Contributor Author

forbjok commented May 19, 2021

Interesting. I wonder why that would affect performance when tiles don't have transforms that would need to be propagated. Have you looked into why it's affecting performance?

@StarArawn
Copy link
Owner

The performance is mainly lost due to how bevy_transform propagates the transform information for parents/children. It always iterates over the children regardless of whether they have transforms or not.

@StarArawn
Copy link
Owner

You can now use map_query.depsawn_map to despawn an entire map including tiles. Optionally you can despawn layers with depsawn_layer or tiles with: despawn_layer_tiles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants