Skip to content
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

UI texture atlas support #8822

Merged
merged 25 commits into from
Jun 19, 2023
Merged

Conversation

mwbryant
Copy link
Contributor

@mwbryant mwbryant commented Jun 11, 2023

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

@github-actions
Copy link
Contributor

You added a new example but didn't update the readme. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets labels Jun 11, 2023
Copy link
Contributor

@ickshonpe ickshonpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it works fine, I identified lots of problems but they are all really down to the confused state of the render module and not the fault of the PR author.

Work still needed:

  • Check that the UV coordinates on flipped images are correct after clipping (an easy way to do this would be to change the image in the Overflow example to an image from a texture atlas).
  • Check that clipped images aren't stretched or shrunk with a scale factor not equal to 1.
  • Needs a system similar to update_image_content_size_system that adds a Measure so that the UI can automatically size the image to preserve its aspect ratio instead of requiring a user to constrain the image's node to the exact dimensions.

I'm 90%+ sure this passes the first two checks, but need to make certain as those bugs have been reintroduced a few times.

crates/bevy_ui/src/node_bundles.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/node_bundles.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/render/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/render/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/render/mod.rs Outdated Show resolved Hide resolved
examples/ui/ui_texture_atlas.rs Outdated Show resolved Hide resolved
examples/ui/ui_texture_atlas.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/render/mod.rs Outdated Show resolved Hide resolved
@mwbryant
Copy link
Contributor Author

uv_rect has been removed from ExtractedUiNode, now there should be no change to the existing flow. I also tested with the overflow example (and blundered some git commands....) as was suggested and clipping works:
shot_2023-Jun-12-at-09:52:20

@nicopap nicopap removed their request for review June 12, 2023 16:31
@mwbryant
Copy link
Contributor Author

New version now uses a standalone extract system and has a less confusing API by only using the parameters on UiTextureAtlasSprite that we actually use.

@mwbryant
Copy link
Contributor Author

Also here is clipping working in the overflow example with flip x and y set and texture atlases being used in the scaling example. This should satisfy the other concerns in the top level comment if I am understanding them correctly (blur is caused by linear sampling, it's intended and changed the same way pixel art is used elsewhere):
shot_2023-Jun-12-at-12:04:54
shot_2023-Jun-12-at-12:07:02

@mwbryant
Copy link
Contributor Author

With Measure setting system added this should be ready to go. I believe I have covered everything @ickshonpe requested and pointed out in their review. From my testing, every feature I am aware of works and none of the existing examples are broken (and there shouldn't be a mechanism for this to affect them)

Copy link
Contributor

@wilk10 wilk10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this!! I think this will make many users happy.

I have only left non blocking nits, feel free to ignore if you prefer.

Did you check if it works also when a normal UI node is a child of a node with an AtlasImageBundle? For example: Animated border around a normal button

crates/bevy_ui/src/node_bundles.rs Show resolved Hide resolved
crates/bevy_ui/src/node_bundles.rs Show resolved Hide resolved
crates/bevy_ui/src/render/mod.rs Show resolved Hide resolved
crates/bevy_ui/src/render/mod.rs Show resolved Hide resolved
examples/ui/ui_texture_atlas.rs Show resolved Hide resolved
crates/bevy_ui/src/render/mod.rs Show resolved Hide resolved
crates/bevy_ui/src/render/mod.rs Outdated Show resolved Hide resolved
@ickshonpe
Copy link
Contributor

Thanks a lot for this!! I think this will make many users happy.
I have only left non blocking nits, feel free to ignore if you prefer.
Did you check if it works also when a normal UI node is a child of a node with an AtlasImageBundle? For example: Animated border around a normal button

With ImageBundle and AtlasImageBundle UI nodes you shouldn't give them child nodes as the MeasureFunc won't work. Instead, use a regular NodeBundle with its BackgroundColor set to white and insert either the UiImage component or, to display atlas images with this PR, Handle<TextureAtlas> and UiTextureAtlasSprite. Maybe there needs to be a separate bundle for image nodes without a MeasureFunc` as it's really unintuitive how the current API works.

Ok I didn't know that. But if it's the same behaviour as ImageBundle then it's not relevant for this PR specifically. Maybe it's worth a separate issue then?

Oh yes definitely a separate issue, I didn't mean that as a suggestion for this PR

@github-actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

examples/ui/ui_texture_atlas.rs Outdated Show resolved Hide resolved
examples/ui/ui_texture_atlas.rs Show resolved Hide resolved
examples/ui/ui_texture_atlas.rs Outdated Show resolved Hide resolved
examples/ui/ui_texture_atlas.rs Show resolved Hide resolved
@ickshonpe
Copy link
Contributor

It looks good now. Just left some non-essential suggestions for the example you can apply if you like.

@pkupper
Copy link
Contributor

pkupper commented Jun 16, 2023

Considering #5103 may be merged soon, how does this compare to #5070 which is based on that Texture Atlas Rework PR? I personally find the API proposed in #5070 slightly more ergonomic.

@mwbryant
Copy link
Contributor Author

Considering #5103 may be merged soon, how does this compare to #5070 which is based on that Texture Atlas Rework PR? I personally find the API proposed in #5070 slightly more ergonomic.

If #5103 is merged then I can rework this pr to use the new interface and then API will be identical, the only difference is they don't create a new bundle for texture atlas ui nodes which is debatable.

Looking at #5070 shows that it's current state is pretty similar to the first draft of this PR. Since then there's been some work to ensure scaling, clipping and the measure systems all work with this PR and that is missing from #5070 (from a quick glance). This makes me personally believe that after #5103 is merged this would be the correct PR to add for support in UI but I'm obviously biased toward my own work.

Also #5070 uses the existing extract system instead of creating a new one. That was my original approach but we opted to change into a standalone system because of the complexity working with atlases added. With the new API for atlases it might make sense to revert this back to the single system approach.

@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Jun 19, 2023
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 19, 2023
@alice-i-cecile
Copy link
Member

@mwbryant can you mention @ManevilleF in the initial post so we can remember to include their work in the release notes?

mwbryant and others added 5 commits June 19, 2023 15:24
Co-authored-by: ickshonpe <david.curthoys@googlemail.com>
Co-authored-by: ickshonpe <david.curthoys@googlemail.com>
Co-authored-by: ickshonpe <david.curthoys@googlemail.com>
Co-authored-by: ickshonpe <david.curthoys@googlemail.com>
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice example, a fairly direct (and well-debated) implementation, great docs. Let's merge this now: we can refine it with use.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 19, 2023
Merged via the queue into bevyengine:main with commit 8b5bf42 Jun 19, 2023
24 checks passed
@mwbryant mwbryant deleted the ui_texture_atlas branch June 19, 2023 23:58
@s-puig s-puig mentioned this pull request Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Enhancement A new feature S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants