Split TextureAtlasSources out of TextureAtlasLayout and make TextureAtlasLayout serializable

[clarfonthey]

Objective

Mostly covers the first point in #13713 (comment)

The idea here is that a lot of people want to load their own texture atlases, and many of them do this by deserializing some custom version of TextureAtlasLayout. This makes that a little easier by providing serde impls for them.

Solution

In order to make TextureAtlasLayout serializable, the custom texture mappings that are added by TextureAtlasBuilder were separated into their own type, TextureAtlasSources. The inner fields are made public so people can create their own version of this type, although because it embeds asset IDs, it's not as easily serializable. In particular, atlases that are loaded directly (e.g. sprite sheets) will not have a copy of this map, and so, don't need to construct it at all.

As an aside, since this is the very first thing in bevy_sprite with serde impls, I've added a serialize feature to the crate and made sure it gets activated when the serialize feature is enabled on the parent bevy crate.

Testing

I was kind of shocked that there isn't anywhere in the code besides a single example that actually used this functionality, so, it was relatively straightforward to do.

In #13713, among other places, folks have mentioned adding custom serialization into their pipelines. It would be nice to hear from people whether this change matches what they're doing in their code, and if it's relatively seamless to adapt to. I suspect that the answer is yes, but, that's mainly the only other kind of testing that can be added.

Migration Guide

TextureAtlasBuilder no longer stores a mapping back to the original images in TextureAtlasLayout; that functionality has been added to a new struct, TextureAtlasSources, instead. This also means that the signature for TextureAtlasBuilder::finish has changed, meaning that calls of the form:

let (atlas_layout, image) = builder.build()?;

Will now change to the form:

let (atlas_layout, atlas_sources, image) = builder.build()?;

And instead of performing a reverse-lookup from the layout, like so:

let atlas_layout_handle = texture_atlases.add(atlas_layout.clone());
let index = atlas_layout.get_texture_index(&my_handle);
let handle = TextureAtlas {
    layout: atlas_layout_handle,
    index,
};

You can perform the lookup from the sources instead:

let atlas_layout = texture_atlases.add(atlas_layout);
let index = atlas_sources.get_texture_index(&my_handle);
let handle = TextureAtlas {
    layout: atlas_layout,
    index,
};

Additionally, TextureAtlasSources also has a convenience method, handle, which directly combines the index and an existing TextureAtlasLayout handle into a new TextureAtlas:

let atlas_layout = texture_atlases.add(atlas_layout);
let handle = atlas_sources.handle(atlas_layout, &my_handle);

Extra notes

In the future, it might make sense to combine the three types returned by TextureAtlasBuilder into their own struct, just so that people don't need to assign variable names to all three parts. In particular, when creating a version that can be loaded directly (like #11873), we could probably use this new type.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[s-puig]

LGTM.

In the future, we should likely get rid of pub texture_ids: HashMap<AssetId<Image>, usize>, altogether and let TextureAtlasBuilder create the layout handles, but I'm fine with this for the time being since it improves upon the existing design.

[clarfonthey]

In the future, we should likely get rid of pub texture_ids: HashMap<AssetId<Image>, usize>, altogether and let TextureAtlasBuilder create the layout handles, but I'm fine with this for the time being since it improves upon the existing design.

What exactly were you thinking for this? Since my thought process was that if the texture atlas were already preprocessed, you'd have to populate this manually if you wanted to use it. Forcing it to go through the TextureAtlasBuilder again seems counter to that.

[clarfonthey]

Since the order seemed mostly random before, I decided to sort these. The big change is that bevy_sprite?/serialize is added here.

[s-puig]

What exactly were you thinking for this? Since my thought process was that if the texture atlas were already preprocessed, you'd have to populate this manually if you wanted to use it. Forcing it to go through the TextureAtlasBuilder again seems counter to that.

Bit of a long text.

IMO, the way we generate atlases is flawed and wrongly encourages using atlas indices. This is particularly apparent when dealing with sprite animations: have an atlas with two sprite-sheets (gabe and alice) with identical animations (10 frame). You have to hard-code that gabe animation goes from 0-9 and alice from 10-19 which is stupid, both go from 0-9 in their local space!

Instead of using atlas indices we should be encouraging atlas layouts:

• Sprites stay in their local space so animations can be reused.
• No longer need a Hashmap in the layout since everything is from that sprite.
• In the future, we no longer would need resources to keep track of what sprite is what, instead we could have an asset loader similar to .gltf and return differents part of the atlas e.g. asset_server.load("atlas.sprites") would return the atlas image, asset_server.load("atlas.sprite#gabe") would return gabe's layout and asset_server.load("atlas_sprite#gabe/walk") would return gabe's animation (wouldn't that be neat?)

Working with atlas layouts right now is a bit annoying though, you need to build your atlas, keep the image handles -which sucks when building a spritesheet from other spritesheets- alive (if it drops the handle will not be the same as in the layout, oops!), grab the index, find the next sprite index (or hard-code the number), grab a slice of the rects then build and add your layouts to the asset server. AtlasBuilder is this way because of #2987.

Now, what i would do:

• Separate AtlasBuilder into two, one with an AssetServer version and one without. This way it can be used in asset context
  • Personally, i would go with a single struct with a state generic so you would have a AtlasBuilder<None> and AtlasBuilder<AssetState> that would be built like so: AtlasBuilder<'a, None>.with_asset_server(self, server: &AssetServer) -> AtlasBuilder<'a, AssetState>.
• AtlasBuilder<'a, AssetState> would have a method layout(&'a mut self) -> LayoutBuilder<'a>. LayoutBuilder would have the usual add_texture operations like AtlasBuilder, but on build() it would create a Handle<AtlasLayout>, store it in AtlasBuilder and return it to the user.
• AtlasBuilder<'a, AssetState>.build() -> (Handle<AtlasLayout>, Handle<Image>) would do the same as now and add the proper rects to all the Handle<AtlasLayout>.

This is a rough sketch and I'm sure i missed things, but it would look something like this:

fn build_atlas(mut asset_server: AssetServer, mut sprites: ResMut<SpriteLibrary>) {
    let mut texture_atlas_builder = TextureAtlasBuilder::default().with_asset_server(&asset_server);
    let gabe_handle: Handle<AtlasLayout> = texture_atlas_builder
                                                            .layout()
                                                            .from_grid("gabe.png", /*parameters here*/)
                                                            .build();
    // Add gabe handle to sprites or spawn or whatever
    let alice_handle: Handle<AtlasLayout> = texture_atlas_builder
                                                            .layout()
                                                            .add_texture("alice00.png")
                                                            .add_texture("alice01.png")
                                                            .build();
    // Same as above
    let (layout, image) = texture_atlas_builder.build();
}

[clarfonthey]

Okay, I feel convinced enough about the argument for future plans that I'm going to just make the field in TextureAtlasSources private for now. We can always make it public later.

[github-actions]

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

[clarfonthey]

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

Was there an update to this action? Since I didn't add a new example; just modified an existing one.

---

Ah, I see, no there wasn't. It appears that my desire to "fix a mistake" in the Cargo.toml file actually caused all Cargo commands to fail, and so it triggered this comment.

Will file an issue for that.

[tychedelia]

LGTM. Not familiar enough with sprite/2d to have a strong opinion about the future facing API choices, but the implementation is fine.