Texture atlas rework #10099

s-puig · 2023-10-12T22:47:03Z

Adopted #5103
Not ready for review yet

Description temporarily copied from #5103

Objective

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

Unify sprite management

Solution

Remove the Handle field in TextureAtlas which is the main cause for all the boilerplate
Remove the redundant TextureAtlasSprite component
Renamed TextureAtlas asset to TextureAtlasLayout (Texture Atlas rework #5103 (comment))
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 #5072 (comment))

Also, this approach is more data oriented extracting the Handle 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).

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 like the SpriteBundle
- Has a new TextureAtlas component instead of a Handle
(BREAKING) DynamicTextureAtlasBuilder::add_texture takes an additional &Handle parameter
(BREAKING) TextureAtlasLayout::from_grid no longer takes a Handle parameter
(BREAKING) TextureAtlasBuilder::finish now returns a Result<(TextureAtlasLayout, Handle), _>
bevy_text:
- GlyphAtlasInfo stores the texture Handle
- FontAtlas stores the texture Handle

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

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Co-authored-by: François <mockersf@gmail.com>

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

s-puig · 2023-10-12T23:12:16Z

Rebased as close as i could to the original PR with still a few things to polish from the merge. Opening mainly for discussion.

UI texture atlas support #8822 added atlas textures to UI. Should we merge the extract_atlas_nodes into a single system (extract_ui_nodes) like the author suggested in this PR? Leave it to a second PR?
Original author made an AtlasTexture component composed of AtlasTextureLayout and an index. IMO, it should be separated into 2 components (AtlasTextureLayout and TextureIndex(usize)) since it would allow better reusability and flexibility (e.g. eventual SpriteArray or SpriteAnimation). What are your thoughts?

alice-i-cecile · 2023-10-13T00:15:55Z

cc @ManevilleF @mwbryant if you two have time and interest to participate in the discussion <3

mwbryant · 2023-10-13T00:29:50Z

UI texture atlas support #8822 added atlas textures to UI. Should we merge the extract_atlas_nodes into a single system (extract_ui_nodes) like the author suggested in this PR? Leave it to a second PR?

If you can, I would strongly recommend doing this in the same PR. Keeping the UI and regular sprite usage in sync is important for consistency. There shouldn't be anything stopping this and it would probably simplify some of the complexity/duplication I introduced :)

I can also handle it in a follow-up if you ping me and remind me.

Thank you for taking this on by the way, unifying the two ways we handle sprites is very important in my opinion. A lot of people start with single sprites for prototyping and then swap over to sheets and this will reduce friction there.

mnmaita

This is a great change, thanks for adopting this PR!

mnmaita · 2023-10-13T02:26:18Z

crates/bevy_sprite/src/texture_atlas.rs

-    /// Generate a [`TextureAtlas`] by splitting a texture into a grid where each
-    /// `tile_size` by `tile_size` grid-cell is one of the textures in the
+    /// Generate a [`TextureAtlasLayout`] as a grid where each
+    /// `tile_size` by `tile_size` grid-cell is one of *section* in the


Suggested change

/// `tile_size` by `tile_size` grid-cell is one of *section* in the

/// `tile_size` by `tile_size` grid-cell is one *section* in the

Minor language nit here.

ManevilleF · 2023-10-14T16:15:39Z

Hi, thanks for adopting my old PR. About your questions:

I think you should handle both UI and sprites in the same PR, I originally opened two separate pull requests (Texture Atlas rework #5103 and Feat/UI texture atlas #5070) because the functionality did not exist at all UI side
About splitting the TextureAtlas component, there was a discussion and we decided to use a single component as it wouldn't make sense to split it and have a single index component (Feat/UI texture atlas #5070 (comment) Texture Atlas rework #5103 (comment)) and it might make the API/Bundle less clear but the situation may have changed

# Conflicts: # crates/bevy_sprite/src/render/mod.rs

s-puig · 2023-10-14T17:58:39Z

Unified #8822 and undid #9099 since it was no longer needed.

@mwbryant could you take a look to ensure i didn't make any mistakes?

About splitting the TextureAtlas component, there was a discussion and we decided to use a single component as it wouldn't make sense to split it and have a single index component (#5070 (comment) #5103 (comment)) and it might make the API/Bundle less clear but the situation may have changed

@mockersf thoughts about splitting AtlasTexture component into Handle<TextureAtlasLayout> TextureIndex(usize)?

mwbryant · 2023-10-17T16:18:38Z

crates/bevy_ui/src/node_bundles.rs

+/// Note:
+/// This bundle is identical to [`ImageBundle`] with an additional [`TextureAtlas`] component.


This was contested in the original PR. I think we opted to make the bundle for feature visibility but it's unclear to me if having similar bundles is good practice. The duplication seems risky if they become desynced in the future. I don't think Bevy has a good pattern for indicating a bundle can add a component to get new functionality though

mwbryant · 2023-10-17T16:22:29Z

@mwbryant could you take a look to ensure i didn't make any mistakes?

I don't see anything obviously wrong after a pass over the ui side of this. Thank you!

ManevilleF · 2023-10-18T10:09:13Z

examples/2d/sprite_sheet.rs

+    let atlas = TextureAtlasLayout::from_grid(Vec2::new(24.0, 24.0), 7, 1, None, None);
+    let texture_atlas = texture_atlases.add(atlas);


Maybe the two variables should be renamed atlas_layout and layout_handle for clarity

alice-i-cecile · 2023-11-15T18:55:26Z

Closing out due to inactivity for now. Feel free to reopen if you find you have the time to tackle this.

s-puig · 2023-11-18T18:28:24Z

I still plan to tackle this (unless someone wants to take it!) but i have been a bit busy lately (and for the foreseeable future). Hopefully i can get back to it ~December.

alice-i-cecile · 2023-11-18T20:58:10Z

Sounds good, reopen this when you do :)

> Replaces #5213 # Objective Implement sprite tiling and [9 slice scaling](https://en.wikipedia.org/wiki/9-slice_scaling) for `bevy_sprite`. Allowing slice scaling and texture tiling. Basic scaling vs 9 slice scaling: ![Traditional_scaling_vs_9-slice_scaling](https://user-images.githubusercontent.com/26703856/177335801-27f6fa27-c569-4ce6-b0e6-4f54e8f4e80a.svg) Slicing example: <img width="481" alt="Screenshot 2022-07-05 at 15 05 49" src="https://user-images.githubusercontent.com/26703856/177336112-9e961af0-c0af-4197-aec9-430c1170a79d.png"> Tiling example: <img width="1329" alt="Screenshot 2023-11-16 at 13 53 32" src="https://github.com/bevyengine/bevy/assets/26703856/14db39b7-d9e0-4bc3-ba0e-b1f2db39ae8f"> # Solution - `SpriteBundlue` now has a `scale_mode` component storing a `SpriteScaleMode` enum with three variants: - `Stretched` (default) - `Tiled` to have sprites tile horizontally and/or vertically - `Sliced` allowing 9 slicing the texture and optionally tile some sections with a `Textureslicer`. - `bevy_sprite` has two extra systems to compute a `ComputedTextureSlices` if necessary,: - One system react to changes on `Sprite`, `Handle<Image>` or `SpriteScaleMode` - The other listens to `AssetEvent<Image>` to compute slices on sprites when the texture is ready or changed - I updated the `bevy_sprite` extraction stage to extract potentially multiple textures instead of one, depending on the presence of `ComputedTextureSlices` - I added two examples showcasing the slicing and tiling feature. The addition of `ComputedTextureSlices` as a cache is to avoid querying the image data, to retrieve its dimensions, every frame in a extract or prepare stage. Also it reacts to changes so we can have stuff like this (tiling example): https://github.com/bevyengine/bevy/assets/26703856/a349a9f3-33c3-471f-8ef4-a0e5dfce3b01 # Related - [ ] Once #5103 or #10099 is merged I can enable tiling and slicing for texture sheets as ui # To discuss There is an other option, to consider slice/tiling as part of the asset, using the new asset preprocessing but I have no clue on how to do it. Also, instead of retrieving the Image dimensions, we could use the same system as the sprite sheet and have the user give the image dimensions directly (grid). But I think it's less user friendly --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: ickshonpe <david.curthoys@googlemail.com> Co-authored-by: Alice Cecile <alice.i.cecil@gmail.com>

ManevilleF and others added 10 commits May 13, 2023 11:17

Texture Atlas rework

163fb78

Unify SpriteBundle and SpriteSheetBundle

c18c9e4

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Apply suggestions from code review

3a6fa5a

Co-authored-by: François <mockersf@gmail.com>

Docs improvements

214d10d

Rebased on main

dc918ec

texture atlas layout in the texture atlas module

92b84ef

Rebase on main

bfcd2fa

Apply suggestions from code review

1b0969d

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Merge "main" into "texture_atlas_rework"

d38deb4

Formatting

a7dd1b9

hymm self-requested a review October 12, 2023 23:19

hymm added this to the 0.13 milestone Oct 12, 2023

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 labels Oct 13, 2023

mnmaita reviewed Oct 13, 2023

View reviewed changes

s-puig added 2 commits October 14, 2023 18:49

Unify texture atlas of bevy_ui

c866622

Merge branch 'main' into texture_atlas_rework

3869f59

# Conflicts: # crates/bevy_sprite/src/render/mod.rs

mwbryant reviewed Oct 17, 2023

View reviewed changes

ManevilleF reviewed Oct 18, 2023

View reviewed changes

alice-i-cecile closed this Nov 15, 2023

ManevilleF mentioned this pull request Nov 16, 2023

Sprite slicing and tiling #10588

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Texture atlas rework #10099

Texture atlas rework #10099

s-puig commented Oct 12, 2023

s-puig commented Oct 12, 2023

alice-i-cecile commented Oct 13, 2023

mwbryant commented Oct 13, 2023

mnmaita left a comment

mnmaita Oct 13, 2023

ManevilleF commented Oct 14, 2023

s-puig commented Oct 14, 2023

mwbryant Oct 17, 2023

mwbryant commented Oct 17, 2023

ManevilleF Oct 18, 2023

alice-i-cecile commented Nov 15, 2023

s-puig commented Nov 18, 2023

alice-i-cecile commented Nov 18, 2023

	/// `tile_size` by `tile_size` grid-cell is one of section in the
	/// `tile_size` by `tile_size` grid-cell is one section in the

		/// Note:
		/// This bundle is identical to [`ImageBundle`] with an additional [`TextureAtlas`] component.

		let atlas = TextureAtlasLayout::from_grid(Vec2::new(24.0, 24.0), 7, 1, None, None);
		let texture_atlas = texture_atlases.add(atlas);

Texture atlas rework #10099

Texture atlas rework #10099

Conversation

s-puig commented Oct 12, 2023

Objective

Solution

Changelog

Migration Guide

s-puig commented Oct 12, 2023

alice-i-cecile commented Oct 13, 2023

mwbryant commented Oct 13, 2023

mnmaita left a comment

Choose a reason for hiding this comment

mnmaita Oct 13, 2023

Choose a reason for hiding this comment

ManevilleF commented Oct 14, 2023

s-puig commented Oct 14, 2023

mwbryant Oct 17, 2023

Choose a reason for hiding this comment

mwbryant commented Oct 17, 2023

ManevilleF Oct 18, 2023

Choose a reason for hiding this comment

alice-i-cecile commented Nov 15, 2023

s-puig commented Nov 18, 2023

alice-i-cecile commented Nov 18, 2023