Separate data on TextureAtlasSprite to multiple components #1618
Labels
A-Rendering
Drawing game state to the screen
C-Usability
A simple quality-of-life change that makes Bevy easier to use
Comments
Ratysz
added
A-Rendering
|
I like this. Smaller components are more reusable, more parallelizable and easier to work with. I don't see any strong reason why these should remain in a single component. |
Merged
Closed
github-merge-queue bot
pushed a commit
that referenced
this issue
Jan 16, 2024
# 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](#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 `SpriteBundle`s (@mockersf [comment](#5072 (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 ```diff 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 ```diff 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() }); } ``` --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: François <mockersf@gmail.com> Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>
|
This was fixed by #5103. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What problem does this solve or what need does it fill?
The data that is on
TextureAtlasSpritemakes it difficult to use. It currently has index, color, and flipping information. The only data that makes sense is index, because that determines the sprite from the atlas that is drawn.Normally when you change the color of a sprite, you want to change all of the sprites that are drawn by the entity. You don't need different colors for every sprite. Currently, if you want to change the color, you have to change it individually for every sprite. It's the same with flipping. There isn't a reason you'd want to have different flipping for individual sprites. You'd want to flip all or none at a time.
What solution would you like?
There should be a new component. I'm not sure of exact naming, but something like
SpriteDrawParamsthat contains Color and flipping.What alternative(s) have you considered?
The only alternative if you want to change the color of sprites that are drawn, is to change the color on the current sprite and update it every time it changes. It's the same with flipping, you have to update the flipping every time you change a sprite.
Additional context
Most of the above also applies to
Sprite. It looks like it uses a material for color though. I think whatever solution is implemented should also apply toSprite. I only didn't mention it because I have only usedTextureAtlasSpriteso far. I do think entities should be able to set their own color that gets multiplied with the material color. That's how it's done in Unity.The text was updated successfully, but these errors were encountered: