-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Feat/UI texture atlas #5070
Feat/UI texture atlas #5070
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. From an ergonomics perspective I really like the enum approach. Especially once you migrate the index into the enum's data.
I actually think we should use this same strategy for 2D unless there are serious performance regressions; it makes the mental model and API much better. If we go that route, this should be done as two PRs:
- Migrate 2D sprites to this enum-ful approach.
- Add texture atlas support for UI.
@cart, do you have strong opinions here?
So everything seems to be working, I had some issues with the @alice-i-cecile About
If this approach is accepted I can open a different MR to bring these changes to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. I'm happy with this now, and would love to see the rest of 2D unified under this approach. Adding this to Cart's Attention Board.
EDIT: actually, adding the sibling and blocking this PR on #5072.
Thanks, I opened a second PR for sprites here: #5072 |
a1145d9
to
29b8a7e
Compare
Blocking on #5103; we should use that approach here too :) |
29b8a7e
to
ab4e049
Compare
ab4e049
to
2aafef4
Compare
2aafef4
to
3dd87a5
Compare
5820644
to
a97e8d0
Compare
a97e8d0
to
830ac83
Compare
Third party impl: bevyengine/bevy-assets#253 |
6650529
to
8ff7fa9
Compare
Could you mark this as closing #1169? |
Really useful feature, hope users can use it asap. |
75d8fd0
to
3253eca
Compare
531d1b4
to
6a780c0
Compare
Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: François <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
6a780c0
to
c8763f6
Compare
When using an image as a UI node, the image's intrinsic size is used as part of layout (if/when the layout engine requests that node's content-based size). TextureAtlas also seem to have an intrinsic size (actually two: the size in the atlas and an optional override size). Would it make sense to use those sizes for layout when TextureAtlas is used on a UI node? (my intuition is that it would, but it probably doesn't need to block this PR and could be added later). |
Closing in favor of #8822, which isn't blocked on a refactor. |
# Objective This adds support for using texture atlas sprites in UI. From discussions today in the ui-dev discord it seems this is a much wanted feature. This was previously attempted in #5070 by @ManevilleF however that was blocked #5103. This work can be easily modified to support #5103 changes after that merges. ## Solution I created a new UI bundle that reuses the existing texture atlas infrastructure. I create a new atlas image component to prevent it from being drawn by the existing non-UI systems and to remove unused parameters. In extract I added new system to calculate the required values for the texture atlas image, this extracts into the same resource as the existing UI Image and Text components. This should have minimal performance impact because if texture atlas is not present then the exact same code path is followed. Also there should be no unintended behavior changes because without the new components the existing systems write the extract same resulting data. I also added an example showing the sprite working and a system to advance the animation on space bar presses. Naming is hard and I would accept any feedback on the bundle name! --- ## Changelog > Added TextureAtlasImageBundle --------- Co-authored-by: ickshonpe <[email protected]>
# 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 <[email protected]> Co-authored-by: François <[email protected]> Co-authored-by: IceSentry <[email protected]>
Objective
Add support for texture sheets in
bevy_ui
. (Previous attempt: #3792).Texture sheet support can enable:
Solution
I applied the exact same logic as #5103.
To use texture atlases for UI, adding a
TextureAtlas
component is enough to make it work.Note
This MR is now based on #5103 I will rebase this branch on main once the base MR is merged