Tiles probably should not be "children" of tilemaps

[rparrett]

There are a few reasons one might make use of Bevy's hierarchy:

1. Transform propagation
2. Visibility propagation
3. Recursive despawns

I am reasonably sure that 1 and 2 are not applicable to bevy_ecs_tilemap. Tiles do not have transforms and visibility propagation to tiles is not needed -- tilemaps that are not visible will not be rendered.

Using bevy's hierarchy comes with a performance penalty because due to visibility propagation work being done. (Also, prior to 0.15, transform propagation affected us too. See bevyengine/bevy#14384)

That just leaves 3. The relationship between a tilemap and its tiles is already defined in the tilemap's TileStorage, so using Bevy's hierarchy is just duplicating this data. Instead of using recursive despawns, we could add an OnRemove hook that drains the TileStorage and despawns its entities.

I saw this show up in profiling, but I'm writing this issue from memory and it has been a few weeks. I don't remember how significant this was.

[ChristopherBiscardi]

related to this, we should probably add an example of how we'd expect people to add things like colliders to the right locations relating to the tilemap/tile positioning.

[adrien-bon]

Not really in favor of this change.
I'd like to add a reason n°4 : keeping a well organized entities tree.
Logically, it makes sense that tiles are children of tilemaps.

If the situation of having a Parent entity with the Visibility component with lots of Children without this component induce a performance hit, maybe it should be considered a bug in the engine and fixed in Bevy itself (as it was done forTransform).

Also, I'm not a Bevy expert at all, but after taking a quick look at the code it seems like it should not: https://github.com/bevyengine/bevy/blob/release-0.15.0/crates/bevy_render/src/view/visibility/mod.rs#L398 (to compare with the fix for Transform).

[rparrett]

Appreciate the feedback. We should dig back into the profiler, because my memory may be incorrect. I'm not sure how big of an issue it is either way.

I should amend point #3 -- currently, "bevy visibility propagation" in bevy_ecs_tilemap is not even possible. Tilemaps have Visibility, but tiles do not, they use our own TileVisible component. I think this is intentional choice related to how the renderer works.

I could see users wanting to attach stuff to their tiles and have visibility propagation work, but right now it won't. So it seems like a bit of a footgun. Maybe we should use Visibility for tiles. Visibilty propagation would then work for individual tiles. But Visibility::Visible on a tile wouldn't work quite like users might expect (at least not without modifications to our renderer, but I'm not sure if it would be reasonable to show tiles when their tilemap is not "visible.")

[ChristopherBiscardi]

Sounds like we should document answers to:

1. Is TileVisible required, or would Bevy's builtin Visibility suffice.

I asked in #engine-dev about Visibility and if its intended use was a match for our use case and I believe it is. Visibility in Bevy is a rendering concern, while TileVisibility in bevy_ecs_tilemap is also a rendering concern.

Using Visibility to control both entire-tilemap rendering and tile rendering seems to make sense but it still might be worth doing a bit of a deeper dive into the places Visibility affects bevy's rendering before deciding to do it. Its always possible Bevy's Visibility implementation changes, but IMO our end-goal is to upstream tilemaps so being closer to upstream is better for that too.

As for if this is even a possible change on the bevy_ecs_tilemap side, I think we need a bit more investigation.

The only library-side use of the TileVisible is in Extract for building the PackedTileData/ExtractedTile, which eventually makes its way into chunks.

There's also InheritedVisibility, which is more for "is this in the viewport" but is set via propagation and comes from Bevy already.

2. What performance penalty is there

Biggest thing for this issue. We need some kind of profile to know what the baseline is and what this change could do perf wise. Doesn't seem like we have the numbers as of right now to say if there's even an issue.

0.14->0.15 improved perf something like 3x, so if there is a similar fix to the Transform fix to be made that could be really nice.

re: the original issue

Transform propagation... Tiles do not have transforms

Tiles can totally have transforms. This is part of why I'd like to have a "how do we handle colliders" type example. Tiles are entities and thus can have any component on them, including transforms for collider positioning which feels like the first thing someone would think of for positioning colliders.

Also worth noting that the collectathon example from bevy_ecs_ldtk has Transforms on it's tiles, and I assume that this is fairly common.

fn log_tiles(query: Query<(Entity, &TilePos, Has<Transform>)>) {
    for (entity, tilepos, has_transform) in &query {
        info!(?entity, ?tilepos, ?has_transform);
    }
}

2024-12-30T02:29:59.483449Z  INFO collectathon: entity=381v1#4294967677 tilepos=TilePos { x: 7, y: 1 } has_transform=true
2024-12-30T02:29:59.483455Z  INFO collectathon: entity=382v1#4294967678 tilepos=TilePos { x: 7, y: 2 } has_transform=true
2024-12-30T02:29:59.483461Z  INFO collectathon: entity=383v1#4294967679 tilepos=TilePos { x: 7, y: 3 } has_transform=true
2024-12-30T02:29:59.483468Z  INFO collectathon: entity=384v1#4294967680 tilepos=TilePos { x: 7, y: 4 } has_transform=true
2024-12-30T02:29:59.483475Z  INFO collectathon: entity=385v1#4294967681 tilepos=TilePos { x: 7, y: 5 } has_transform=true
2024-12-30T02:29:59.483483Z  INFO collectathon: entity=386v1#4294967682 tilepos=TilePos { x: 7, y: 6 } has_transform=true

The relationship between a tilemap and its tiles is already defined in the tilemap's TileStorage

TileStorage is a bit weird and by its current implementation this doesn't have to be true. I feel like the only reason it exists is to do efficient tile-position based entity access. In that sense its not really storage, but rather a user-maintained index. TilePos -> Entity.

We've started using it more with 0.15's release (the on_remove_tilemap observer) but it really does not get used anywhere else library-side except for helpers. This can be demonstrated by updating a TIlePos which in turn doesn't have any effect on TileStorage. A user has to both remove the original tile position entry and set the new tile position entry to move a tile in the TileStorage.

The crate's core functionality actually works without TileStorage entirely. The only thing that "requires" it is the TilemapBundle, which is an interesting note because you can spawn everything without the TileStorage if you don't use the bundle. This technically allows entirely alternative forms of indexing without the overhead of maintaining this specific TilePos->Entity mapping. A game of Snake on a tilemap for example, could store a Vec of entities representing the snake's parts and never need to access them by position.

[rparrett]

Okay, I am pretty convinced that hierarchies of tilemaps and tiles can be useful.

We need some kind of profile to know what the baseline is and what this change could do perf wise. Doesn't seem like we have the numbers as of right now to say if there's even an issue.

I agree. It would be nice to get an idea of how

• A naive bevy hierarchy of sprites
• A hierarchy-less tilemap
• A hierarchy-full tilemap (perhaps also with Visibility for tiles)

compare.

TileStorage is a bit weird and by its current implementation this doesn't have to be true.

I'd only point out that according to our examples, TileStorage is the canonical way to get all tiles for a tilemap to despawn them.

I'd love for users to be able to just despawn or despawn_recursive an entire tilemap.

[adrien-bon]

As for if this is even a possible change on the bevy_ecs_tilemap side, I think we need a bit more investigation.

Don't know if it makes sense to actually have Visibility from Bevy (and associated required components) for tiles.

Currently, we have entities for tilemaps and tiles but we actually render chunks, which are not entities.
We could probably make it work so the Visibility on a tile is actually used for baking the corresponding chunk, but:

• It would probably impact performances: in my understanding both InheritedVisibility and ViewVisibility are reset each frame, which would mean having Changed tiles every frames. In the end, I believe it means the extract code has to redraw all chunks every frame (maybe not so bad since I think it was pretty much what happened before 0.16 and retained rendering)
• Maybe another performance issue with Visibility propagation
• Bevy Visibility component seems to be a better fit for entities which are directly rendered. If we do have this Visibility component on tiles it could trick the user into thinking it's the tile that's actually rendered.

I guess we could have the same discussion about Transform.
Yes, you can have a Transform on a tile but nothing happens if you change its value: tile does not move.
What actually matters is the TilePos component which is used by the extraction code to position the tile in the chunk.

I think we should have the same strategy for Transform and Visibility: either we recomend using these components on tiles or we discourage their use in favor of bevy_ecs_tilemap specific ones (namely TilePos and TileVisibility). So if we do the change for Visibility we probably need to do the same for Transform.

0.14->0.15 improved perf something like 3x, so if there is a similar fix to the Transform fix to be made that could be really nice.

Maybe also has to do with the retained rendering stuff and not only the Transform propagation ?
But it's definitivelly something to test. Should be pretty easy: juste remove Visibility from the tilemap (we could also test without Transform).

Also worth noting that the collectathon example from bevy_ecs_ldtk has Transforms on it's tiles, and I assume that this is fairly common.

Yes, and it's also how it is currently done for bevy_ecs_tiled when adding colliders from tiles.
The Transform component on tiles is used for propagation from the tilemap down to the colliders.

I'm actually changing this to attach (and merge them when possible) colliders from tiles on the tilemap rather than on tiles.
I did not do tracing but it's sensibly a huge performances gain, eventhough decrease in colliders numbers has probably a huge impact.

I believe this kind of approach should be encouraged instead of spawning one collider per tile which does not work on "medium sized" maps.

A naive bevy hierarchy of sprites

I'd be very interested to see how this solution performs.
It would be very nice to delegate most of the rendering work to Bevy and get ride of the chunking mechanisms (which btw have some issue for isometric maps).
It may work thanks to retained rendering.

If we have this kind of architecture, we could definitivelly have Transform and Visibility on tiles since that's actually what would be rendered.

I'd love for users to be able to just despawn or despawn_recursive an entire tilemap.

I believe it already works ? :)

[rparrett]

I believe it already works ? :)

Well, yes, if you place all of your tiles in a hierarchy.

My big question is "do we want to teach Bevy users to do this universally?" Because we could easily provide a mechanism for this based on TileStorage that has no perf implications.

I'd be very interested to see how this solution performs.
It may work thanks to retained rendering.

Sprite performance is actually significantly worse in 0.15. I haven't done this sort of comparison with bevy_ecs_tilemap since before 0.12 though, which is when we started using instancing for sprites, which was pretty major.

[ChristopherBiscardi]

seems like 0.16 is going to improve relationships (and perf) as well: bevyengine/bevy#17398