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

Texture atlases should use atlas layouts, not atlas indices #15365

Open
alice-i-cecile opened this issue Sep 22, 2024 · 2 comments
Open

Texture atlases should use atlas layouts, not atlas indices #15365

alice-i-cecile opened this issue Sep 22, 2024 · 2 comments
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Contentious There are nontrivial implications that should be thought through

Comments

@alice-i-cecile
Copy link
Member

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();
}

Originally posted by @s-puig in #15344 (comment)

@alice-i-cecile
Copy link
Member Author

Moved this into an issue so it doesn't get buried, but @s-puig I'm very much in favor of this design and really appreciate the great writeup.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Contentious There are nontrivial implications that should be thought through labels Sep 22, 2024
@s-puig
Copy link
Contributor

s-puig commented Oct 5, 2024

Had second thoughts about this.

Instead of adding an Asset context into TextureAtlasBuilder, we could push it into a SystemParam kinda like Gizmos.
This way we reduce user boilerplate (no longer need to load all the image assets, wait until they are done and grab the references), allow some control (sequential load for wasm builds?) and preserve context about what is being added which would pave the way for managed atlases (rebuild atlases without the dropped layouts, atlas building on async tasks, etc..)

The main difference would be that instead of getting an atlas right now, we pinky promise to populate the atlas/layouts eventually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

No branches or pull requests

2 participants