Texture Atlas rework

[ManevilleF]

Objective

Old MR: #5072 Associated UI MR: #5070 Adresses #1618

Unify sprite management

Solution

• Remove the Handle<Image> field in TextureAtlas which is the main cause for all the boilerplate
• Remove the redundant TextureAtlasSprite component
• Renamed TextureAtlas asset to TextureAtlasLayout (suggestion)
• Add a TextureAtlas component, containing the atlas layout handle and the section index

The difference between this solution and #5072 is that instead of the enum approach is that we can more easily manipulate texture sheets without any breaking changes for classic SpriteBundles (@mockersf comment)

Also, this approach is more data oriented extracting the Handle<Image> and avoiding complex texture atlas manipulations to retrieve the texture in both applicative and engine code. With this method, the only difference between a SpriteBundle and a SpriteSheetBundle is an additional component storing the atlas handle and the index.

This solution can be applied to bevy_ui as well (see #5070).

Ping @alice-i-cecile

Changelog

• (BREAKING) Removed TextureAtlasSprite
• (BREAKING) Renamed TextureAtlas to TextureAtlasLayout
• (BREAKING) SpriteSheetBundle:
  • Uses a Sprite instead of a TextureAtlasSprite component
  • Has a texture field containing a Handle<Image> like the SpriteBundle
  • Has a new TextureAtlas component instead of a Handle<TextureAtlasLayout>
• (BREAKING) DynamicTextureAtlasBuilder::add_texture takes an additional &Handle<Image> parameter
• (BREAKING) TextureAtlasLayout::from_grid no longer takes a Handle<Image> parameter
• (BREAKING) TextureAtlasBuilder::finish now returns a Result<(TextureAtlasLayout, Handle<Image>), _>
• bevy_text:
  • GlyphAtlasInfo stores the texture Handle<Image>
  • FontAtlas stores the texture Handle<Image>

Migration Guide

fn my_system(
  mut images: ResMut<Assets<Image>>, 
-  mut atlases: ResMut<Assets<TextureAtlas>>, 
+  mut atlases: ResMut<Assets<TextureAtlasLayout>>, 
  asset_server: Res<AssetServer>
) {
    let texture_handle: asset_server.load("my_texture.png");
-   let layout = TextureAtlas::from_grid(texture_handle, Vec2::new(25.0, 25.0), 5, 5, None, None);
+   let layout = TextureAtlasLayout::from_grid(Vec2::new(25.0, 25.0), 5, 5, None, None);
    let layout_handle = atlases.add(atlas);
    commands.spawn_bundle(SpriteSheetBundle {
-      sprite: TextureAtlasSprite::new(0),
-      texture_atlas: atlas_handle,
+      atlas: TextureAtlas {
+         layout: layout_handle,
+         index: 0
+      },
+      texture: texture_handle,
       ..Default::default()
     });
}

[ManevilleF]

I'm satisfied with the state of this.

@alice-i-cecile I know you like the enum approach but I think this is way more modular and more ecs friendly as there is one single additional component defining one behaviour. @mockersf with this approach there are less breaking changes and no .into() call for the texture.

I think this MR is better than #5072

[superdump]

I’m probably not going to have any time to review this until later in the week as it seems like I probably also need to look at two other PRs that are proposals?

I may have some thoughts about this as I’d like to use a shadow atlas at some point, which is quite dynamic in some senses (updating subregions) but it’s unclear whether that should use the same implementation or a special one, as it is more using a large tiled texture, possibly with various tile sizes, to manage a cache of shadow maps. I dunno. I’ll take a look when I can.

[alice-i-cecile]

@superdump this is an alternative to the enum alternative that merged together single and texture atlas sprites in #5072. We agree that this approach is cleaner and likely to come with fewer weird performance and UX problems, so feel free to review this on its own.

[hymm]

This breaks the 1-1 relation between the texture atlas and the image, right? We lose the encoding that a texture atlas belongs to a specific image. Makes me a little nervous that people would change the image handle and forget to change the atlas handle, which would be confusing. On the other hand this probably makes it easier to have a large texture atlas that might have different grids on it. Hmm... Ideally we'd encode the dependency of the texture atlas on the image somewhere. But we probably don't want that in the component. Maybe just something to consider when dependent assets gets figured out.

I do like that this blurs the line between texture atlas sprites and normal sprites in a very ecs way. i.e. you just need to add a mapping and an index to a normal sprite.

[ManevilleF]

@hymm I don't think the relationship break is an issue. Components are self sufficient, a texture is a texture, a TextureSheet defines what section of any associated texture to draw. There is no" inherent" relationship between an atlas and a texture in my opinion, it's just that usually we combine a specific atlas with a specific texture. But I understand that there could be some cases where people get confused.

Overall I think this allows more "ECS friendly" modularity, and more use cases for texture sheets. Also, this opens the door to a separate MR I opened that adds texture sheet support for UI elements.

[hymm]

I wouldn't say this problem is necessarily blocking, but this PR introduces a new class of errors that users can make. They can now use the wrong atlas with an image, where before they couldn't do this. In some cases this could lead to significant confusion. Consider a situation where the section of the old texture atlas points at a section of the new image that only has transparent pixels. To the user their sprite will have disappeared and they won't know why.

The decreased conceptual difference between sprites and texture atlas sprites and increased flexibility is likely to be worth introducing this new type of error. But I haven't fully formed my opinion on this yet.

[ManevilleF]

@alice-i-cecile rebased on main, I adapted the stress test

[ManevilleF]

On this branch I get:

2022-07-04T14:06:12.538797Z  INFO many_animated_sprites: Sprites: 102400
2022-07-04T14:06:12.543088Z  INFO bevy diagnostic: frame_time                      :    0.058550s (avg 0.059529s)
2022-07-04T14:06:12.543108Z  INFO bevy diagnostic: fps                             :   17.079334  (avg 16.819139)
2022-07-04T14:06:12.543112Z  INFO bevy diagnostic: frame_count                     : 394.000000

And on the main branch I have:

2022-07-04T14:07:44.686063Z  INFO many_animated_sprites: Sprites: 102400
2022-07-04T14:07:44.686125Z  INFO bevy diagnostic: frame_time                      :    0.059174s (avg 0.059465s)
2022-07-04T14:07:44.686138Z  INFO bevy diagnostic: fps                             :   16.899209  (avg 16.833800)
2022-07-04T14:07:44.686142Z  INFO bevy diagnostic: frame_count                     : 393.000000

So I don't see much difference, as expected. Maybe I should use stuff described in profiling.md ?

[alice-i-cecile]

Looks very close. I'd test out the more advanced traces; if nothing else it's good to get comfortable with those tools.

[ManevilleF]

ping @alice-i-cecile @mockersf

[alice-i-cecile]

@ManevilleF can you rebase this?

[ManevilleF]

@alice-i-cecile sure, I'll do that tomorrow along with #5070

[ManevilleF]

Done @alice-i-cecile , I think #5070 could be integrated to bevy 0.9 as well

[ManevilleF]

Rebased again after #5877

[ManevilleF]

Rebased again after #6481

@alice-i-cecile Should I keep rebasing or will this PR validation take some time ? Same for #5070

[alice-i-cecile]

Gradual rebasing is probably going to be easier. I think that this should be easier enough to merge, but I'd like @superdump's and @StarArawn's opinions :)

[ManevilleF]

We still didn't made a decision about @superdump suggestion of splitting the index and the atlas, we were waiting for @mockersf input on this

[rparrett]

FWIW, I am slightly on "team no split." Although I made some convincing arguments to myself on both sides.

Apart from that, I think Rob's naming suggestions would be a huge improvement. Maybe we should we make that change and see what happens?

[ManevilleF]

Rebased on main and applied @superdump renaming suggestions. I updated the MR description to fit the new naming

[dubrowgn]

This looks like it is effectively reverting https://github.com/bevyengine/bevy/pull/6057, which I would honestly be in favor of personally. However, we ended up with a worse version of the old TextureAtlas::from_grid_with_padding(), which takes Options just to immediately unwrap them. If we are going to split TextureAtlas::from_grid() again, can we get rid of the Options also, and just take Vec2 instead for padding and offset?

[ManevilleF]

This looks like it is effectively reverting #6057, which I would honestly be in favor of personally. However, we ended up with a worse version of the old TextureAtlas::from_grid_with_padding(), which takes Options just to immediately unwrap them. If we are going to split TextureAtlas::from_grid() again, can we get rid of the Options also, and just take Vec2 instead for padding and offset?

That must be a rebase issue, since this PR is much older than #6057 . I'll look into it

[ManevilleF]

@dubrowgn I rollbacked the changes I made to the from_grid impl. I agree with you, the API is quite bad with the two Option<Vec2> but I don't think it's relevant for this PR. Once it is merged I'll try to rework the atlas API

@alice-i-cecile I think this is good to go. Are we missing some reviews ? (I didn't know I could only pick one reviewer so you might have received mulitple notifications)

[alice-i-cecile]

Yeah, we just need two reviewers. Ideally I'd like @StarArawn, @mockersf and @hakolao to take a look, and I'll do my best myself (I have used texture atlases at work).

[ManevilleF]

@alice-i-cecile I applied your suggestions and added some extra doc comments.

Btw I keep telling myself that I should wait for this to be merged before making big API changes but I have a urge to remove TextureAtlasLayout::size which is used absolutely nowhere and can be wrong if the layout is custom built

[ManevilleF]

Rebased on main after conflict with #6861

[rparrett]

This should close #1618 as well, I think

[mnmaita]

@alice-i-cecile is this one ready to go? Looks like discussion items were all resolved. @ManevilleF would need to rebase again though.

[alice-i-cecile]

Yep, I'll do another review pass here. @mnmaita feel free to leave your feedback too; it really does help us.

[alice-i-cecile]

@sammyduc can you review this? I'd like to ensure that your use case in #8633 will continue to work.

[alice-i-cecile]

Closing as this has now been adopted.