Texture Atlas caching for sprite sheet bundles

[Trouv]

Currently, a new texture atlas is created and added to the Assets<TextureAtlas> asset store as part of the logic for the #[sprite_sheet_bundle] field attribute of #[derive(LdtkEntity)]. This means that, even if two entities' visuals are part of the same tileset, two different texture atlases will be created for them, instead of them sharing the same asset with two separate asset handles.

This isn't that big of a deal, since the actual data inside a texture atlas is pretty limited. There shouldn't be any memory leakage either. But still, the current implementation isn't ideal.

Make it so that texture atlases aren't re-added to the asset store if they have been added previously. One approach could be to add an Option<Handle<TextureAtlas>> to the TilesetMap, in addition to the plain image handle. However, I am concerned that this approach may trigger AssetEvent::Modified when we update the field from None to Some, which could lead to unnecessary and constant reloading. An alternative approach might just search the asset store for an appropriate texture atlas handle, but this could be difficult since TextureAtlas isn't Eq/PartialEq.

[miyoku157]

I don't think we can use Option<Handle<TextureAtlas>> inside TilesetMap: The TilesetMap would look like this: pub type TilesetMap = HashMap<i32, (Handle<Image>, Option<Handle<TextureAtlas>>)>;

• First i think you're right about a possible AssetEvent::Modified being launch as we have to dereference to modify the variable
• The second problem is that if you add this, the tileset_map. We have two choices, either make it mutable which will require the ldtk_asset above it to be mutable. And after that it can generate bugs because mutable reference block lots more than immutable. The other choice is to use interior mutability to change the Option variable. Nevertheless, i've already tried this solution, and as bevy is multithreaded, it requires the multithreaded version (Mutex or other). So i think maybe it's overkill?

I've still make a version of a possible caching Atlas here: https://github.com/miyoku157/bevy_ecs_ldtk/tree/caching_texture Nevertheless, there are still some errors with the different way of using LdtkEntity. The no_grid doesn't appear. The way i've done it is that a tileset have a TextureAtlas associated. But if you're using no_grid, is it the same TextureAtlas? Maybe that's why the pumpkin aren't showing anymore

Which leads me to think that we should rethink the loading of the assets, and create the Texture atlas at the same time we loads the tileset (at least, the one configured inside ldtk json file). But it's a big change. What do you think?

[miyoku157]

If two different TextureAtlas can exist for the same Image, it cannot work with a hashmap, Which is what i've done currently. So i'm wondering, can the user use no_grid and grid ldtkentity together? Or is it something that should be avoid/forbid?

[Trouv]

Sorry for being slow to respond to this

Which leads me to think that we should rethink the loading of the assets, and create the Texture atlas at the same time we loads the tileset (at least, the one configured inside ldtk json file). But it's a big change. What do you think?

Actually - I know there are some low-priority plans to make bevy_ecs_tilemap support using texture atlases as tilesets. This is something I've been wanting for a while, so I've considered contributing it myself but haven't gotten around to it. When this does eventually get in then it would be even more reason to just create the texture atlases as dependent assets or whatever during the asset loading process. So, yes, this is something I'm interested in.

If two different TextureAtlas can exist for the same Image, it cannot work with a hashmap, Which is what i've done currently. So i'm wondering, can the user use no_grid and grid ldtkentity together? Or is it something that should be avoid/forbid?

Hmm, I think no_grid is a case where just creating a new texture atlas on the spot is more reasonable. In the case of no_grid the texture atlas only has 1 rect in it, so it's not taking up a ton of space anyway.

[miyoku157]

Hmm, if that so, i think it could be a better solution to look at the bevy_ecs_tilemap support for that. Or contribute to it. Moreover, i saw that bevy also rework TextureAtlas for the 0.11: https://github.com/bevyengine/bevy/pull/5103 For the no_grid, even if it doesn't take lots of space. If we can make multiple Texture atlas from one tileset, we can't juste handle a Hashmap<i32, Option<Handle<TextureAtlas>>, it has to be a set or hashset.

[Trouv]

For the no_grid, even if it doesn't take lots of space. If we can make multiple Texture atlas from one tileset, we can't juste handle a Hashmap<i32, Option<Handle>, it has to be a set or hashset.

Yeah that's true, I'm just saying for the no_grid case I don't think we need caching that badly so maybe we just don't do it. We can warn users in the docs that no caching will occur.

(removing the good-first-issue label since this is actually pretty complicated)