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).

EDIT: I also applied this solution to Bevy UI

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>
• bevy_ui:
  • (BREAKING) Removed UiAtlasImage , the atlas bundle is now identical to the ImageBundle with an additional TextureAtlas

Migration Guide

• Sprites

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(layout);
    commands.spawn(SpriteSheetBundle {
-      sprite: TextureAtlasSprite::new(0),
-      texture_atlas: atlas_handle,
+      atlas: TextureAtlas {
+         layout: layout_handle,
+         index: 0
+      },
+      texture: texture_handle,
       ..Default::default()
     });
}

• UI

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(layout);
    commands.spawn(AtlasImageBundle {
-      texture_atlas_image: UiTextureAtlasImage {
-           index: 0,
-           flip_x: false,
-           flip_y: false,
-       },
-      texture_atlas: atlas_handle,
+      atlas: TextureAtlas {
+         layout: layout_handle,
+         index: 0
+      },
+      image: UiImage {
+           texture: texture_handle,
+           flip_x: false,
+           flip_y: false,
+       },
       ..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

[alice-i-cecile]

This is nice! I think that this is an uncontroversial improvement to the UX of working with texture atlases.

Given that I expect it to be very rare to swap between ordinary sprites and sprite atlases at run time, I think this approach makes more sense that the enum design you showed before.

[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.

[alice-i-cecile]

I've merged in the relevant new stress test now; would be curious to see the before and after.

[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]

lgtm

i have a mild feeling that TextureAtlas should be named something more specific since it is not an actual atlas itself, rather the info that identifies a sub-image stored within an atlas ... something like TextureAtlasIndex maybe. but i'm not blocking on that.

It stores both a handle to the layout, which kinda is an atlas, and the index, so maybe something like TextureCutter or Section ?

[robtfm]

TextureSection sounds good to me, but @alice-i-cecile or another significant user of atlases should probably opine.

[JMS55]

TextureSlice, TextureSubregion, maybe TextureAtlasFoo.

[alice-i-cecile]

Very happy to defer renaming for a follow-up :p

[atlv24]

Topologically an atlas consists of a set of charts, so maybe TextureChart

[IceSentry]

LGTM

I'm fine with the current name, there might be something better but we can rename that later.

I didn't do a super deep review of everything but I tried running everything and overall the approach seems nice.

[alice-i-cecile]

Merging!

[CooCooCaCha]

TextureSlice matches up with 9-slice, which also refers to a piece of a texture. One more alternative is "patch".

[alice-i-cecile]

Merge conflicts aren't trivial, but once they're resolved ping me and I'll merge this in for you :)

[ManevilleF]

@alice-i-cecile done ! The camera driven UI PR had a few conflcits with this

[alice-i-cecile]

Pulled locally and the relevant examples work for me :) Merging!

[odecay]

It seems like removing extract_atlas_uinodes here also means that the ordering constraints referencing RenderUiSystem::ExtractAtlasNode aren't doing anything since nothing is in that set now. If its no longer needed we should remove it.

[hymm]

Could you make an issue for this?