Migrate UI bundles to required components by VitalyAnkh · Pull Request #15898 · bevyengine/bevy

VitalyAnkh · 2024-10-14T10:50:19Z

Objective

Migrate UI bundles to required components, fixes Migrate UI bundles to required components #15889

Solution

deprecate NodeBundle in favor of Node
deprecate ImageBundle in favor of UiImage
deprecate ButtonBundle in favor of Button

Testing

CI.

Migration Guide

Replace all uses of NodeBundle with Node. e.g.

     commands
-        .spawn(NodeBundle {
-            style: Style {
+        .spawn((
+            Node::default(),
+            Style {
                 width: Val::Percent(100.),
                 align_items: AlignItems::Center,
                 justify_content: JustifyContent::Center,
                 ..default()
             },
-            ..default()
-        })
+        ))

Replace all uses of ButtonBundle with Button. e.g.

                     .spawn((
-                        ButtonBundle {
-                            style: Style {
-                                width: Val::Px(w),
-                                height: Val::Px(h),
-                                // horizontally center child text
-                                justify_content: JustifyContent::Center,
-                                // vertically center child text
-                                align_items: AlignItems::Center,
-                                margin: UiRect::all(Val::Px(20.0)),
-                                ..default()
-                            },
-                            image: image.clone().into(),
+                        Button,
+                        Style {
+                            width: Val::Px(w),
+                            height: Val::Px(h),
+                            // horizontally center child text
+                            justify_content: JustifyContent::Center,
+                            // vertically center child text
+                            align_items: AlignItems::Center,
+                            margin: UiRect::all(Val::Px(20.0)),
                             ..default()
                         },
+                        UiImage::from(image.clone()),
                         ImageScaleMode::Sliced(slicer.clone()),
                     ))

Replace all uses of ImageBundle with UiImage. e.g.

-    commands.spawn(ImageBundle {
-        image: UiImage {
+    commands.spawn((
+        UiImage {
             texture: metering_mask,
             ..default()
         },
-        style: Style {
+        Style {
             width: Val::Percent(100.0),
             height: Val::Percent(100.0),
             ..default()
         },
-        ..default()
-    });
+    ));

crates/bevy_ui/src/widget/button.rs

examples/ui/ui_texture_slice_flip_and_tile.rs

crates/bevy_ui/src/ui_node.rs

examples/ui/ui_material.rs

alice-i-cecile · 2024-10-16T23:26:16Z

Doesn't gets drawn because of the Without query filter in extract_ui_materials.

This means we can't have BackgroundColor in the required components for either Node or Style. We could swap this to store an Option and then branch on it in this system. It's a bit wasteful to store it but not a big deal.

The other open question here is the Node -> Style vs Style -> Node question. I think I have a mild preference for the latter, since it makes the migration nicer for both now and 0.16, when we're planning to move many of the fields from Style into Node. The lack of Node::default is also a big win for me.

On the other hand though, commands.spawn(Style {..}) is super unclear to beginners and has no obvious semantic meaning. So maybe this way is better. @cart, final call please?

ickshonpe · 2024-10-17T11:07:40Z

Neither Node nor Style has very helpful semantics, maybe we could do a simple renaming for whatever the core component is for now like to UiNode or UiStyle so it's clear to new users that this is a UI component whilst retaining enough familiarity that exisiting users won't be too confused.

I find the Ui prefix a bit ugly though.

ickshonpe · 2024-10-17T11:50:33Z

Another option. Node is mostly only used internally. We could rename the existing Node node type to ResolvedLayout or ComputedNode or whatever without causing too much friction. Then replace it with a ZST named Node carrying the required components that doesn't need to be constructed using default().

alice-i-cecile · 2024-10-17T13:30:54Z

Moving to ResolvedLayout or something seems reasonable: that seems like the broad shape of the design we want in the future.

MiniaczQ

Not much to contribute here, mostly agreed with other comments.

I don't think UI components need Ui in the name, since UiText was heavily voted against, so there wouldn't be consistency either way

cart · 2024-10-17T19:20:18Z

The other open question here is the Node -> Style vs Style -> Node question. I think I have a mild preference for the latter, since it makes the migration nicer for both now and 0.16, when we're planning to move many of the fields from Style into Node. The lack of Node::default is also a big win for me.

This is conceptually wrong. "Node" is the driving context. "Style" is a property of a "Node". The cost of training people to think otherwise is too high. The current port is already an ergonomic win. We can claim the remaining wins when we break up Style.

I find the Ui prefix a bit ugly though.

Me too. I prefer the short names. Instead of prefixing everything we should just pick concept names that are "unique enough", with a bias toward short and snappy to please the "ui ergonomics" crowd. I personally think Node is fine. We can/should discuss this later, not here.

Moving to ResolvedLayout or something seems reasonable: that seems like the broad shape of the design we want in the future.

Yeah this seems reasonable. But we should discuss this elsewhere.

Then replace it with a ZST named Node carrying the required components that doesn't need to be constructed using default().

I still strongly believe Node should contain the "style properties that all nodes have" such as width, height, etc. But thats a post 0.15 conversation.

…MaterialNode. Port to Required Components.

cart

I just pushed a change that:

Added support for BackgroundColor to UI material nodes, allowing require(BackgroundColor) to work as expected. (resolving the conversation above).
Renames UiMaterialHandle to MaterialNode
Ports UI materials to Required Components. MaterialNode now requires Node (because it "is a" Node).
Deprecate MaterialNodeBundle

I think this PR is now ready to merge

ickshonpe · 2024-10-17T20:46:40Z

crates/bevy_ui/src/render/mod.rs

            entity: (*entity, extracted_uinode.main_entity),
            sort_key: (
-                FloatOrd(extracted_uinode.stack_index as f32),
+                FloatOrd(extracted_uinode.stack_index as f32 + stack_z_offsets::BACKGROUND_COLOR),


Could just set extract_ui_material_nodes after extract_ui_background_colors instead.
TransparentUi uses a stable sort, so if two phase items have the same stack index then the z-order is determined by the ordering of the extraction systems.

Gotcha. I don't have strong opinions one way or the other. I doubt a single per-entity float add will meaningfully tip the performance scales. Doing this per-entity gives us more granularity than "per thing encapsulated by a system", but we also currently have no use case where this matters. I also like that it puts the sort behavior front-and-center in a centralized place. The current behavior is pretty implicit.

I don't care about the performance impact but I don't like the inconsistancy very much and it breaks any existing code that rendered borders or text above ui materials on the same node.

Perhaps, but it also buys flexibility for future scenarios and decouples sort order from system execution order (which feels like a good thing as execution order could easily need to be different than sort order) .

I still don't have super strong opinions here. I think I prefer the explicitness of defining offsets. But I wouldn't strongly object to a change that moves this to using system execution order.

# Objective The `ui_material` recently gained a harsh red background via #15898. I'm assuming this was added for testing and forgotten about. https://pixel-eagle.com/project/b25a040a-a980-4602-b90c-d480ab84076d/run/5268/compare/5259?screenshot=UI%20(User%20Interface)/ui_material.png ## Solution Remove the red background ## Testing `cargo run --example ui_material`

Migrate nodes to required components

7a69264

VitalyAnkh marked this pull request as draft October 14, 2024 10:50

Migrate more examples

29bd18f

alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 14, 2024

alice-i-cecile added this to the 0.15 milestone Oct 14, 2024

VitalyAnkh added 2 commits October 15, 2024 13:09

Migrate ImageBundle and ButtonBundle

1da36cc

Merge from main

578eb88

VitalyAnkh added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Oct 15, 2024

MiniaczQ self-requested a review October 15, 2024 14:31

cart reviewed Oct 15, 2024

View reviewed changes

crates/bevy_ui/src/widget/button.rs Outdated Show resolved Hide resolved

examples/ui/ui_texture_slice_flip_and_tile.rs Outdated Show resolved Hide resolved

VitalyAnkh added 8 commits October 16, 2024 10:26

Update for cart's review

208c41c

Migrate some documentation and examples

7b5413e

Merge branch 'main' into migrate_ui_bundles

3ee1372

Migrate more examples

6865958

Migrate all NodeBundles

ab7bccc

Migrate all examples

837e78b

Fix CI

8784827

Fix CI

57d1324

VitalyAnkh added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 16, 2024

VitalyAnkh marked this pull request as ready for review October 16, 2024 10:32

VitalyAnkh removed 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 Oct 16, 2024

VitalyAnkh requested a review from alice-i-cecile October 16, 2024 10:33

ickshonpe suggested changes Oct 16, 2024

View reviewed changes

crates/bevy_ui/src/ui_node.rs Show resolved Hide resolved

ickshonpe suggested changes Oct 16, 2024

View reviewed changes

examples/ui/ui_material.rs Outdated Show resolved Hide resolved

alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Oct 16, 2024

Merge branch 'main' into migrate_ui_bundles

fb90cac

VitalyAnkh added 2 commits October 17, 2024 11:15

Resolve conflicts

b865f1e

Fix CI

4b57f47

MiniaczQ approved these changes Oct 17, 2024

View reviewed changes

Support BackgroundColor for MaterialNode. Rename UiMaterialHandle to …

1866af7

…MaterialNode. Port to Required Components.

cart approved these changes Oct 17, 2024

View reviewed changes

alice-i-cecile approved these changes Oct 17, 2024

View reviewed changes

alice-i-cecile enabled auto-merge October 17, 2024 20:19

ickshonpe suggested changes Oct 17, 2024

View reviewed changes

Remove explicit doc path

1b7c918

ickshonpe approved these changes Oct 17, 2024

View reviewed changes

alice-i-cecile added this pull request to the merge queue Oct 17, 2024

Merged via the queue into bevyengine:main with commit eb19a9e Oct 17, 2024

cart mentioned this pull request Oct 18, 2024

Merge Style properties into Node. Use ComputedNode for computed properties. #15975

Merged

rparrett mentioned this pull request Oct 18, 2024

Fix red background in ui_material example #15978

Merged

IQuick143 mentioned this pull request Nov 3, 2024

Outdated migration guide for 15898 bevyengine/bevy-website#1767

Closed

Uh oh!

Conversation

VitalyAnkh commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Migration Guide

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile commented Oct 16, 2024

Uh oh!

ickshonpe commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ickshonpe commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Oct 17, 2024

Uh oh!

MiniaczQ left a comment

Choose a reason for hiding this comment

Uh oh!

cart commented Oct 17, 2024

Uh oh!

cart left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ickshonpe Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cart Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

ickshonpe Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

cart Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

VitalyAnkh commented Oct 14, 2024 •

edited

Loading

ickshonpe commented Oct 17, 2024 •

edited

Loading

ickshonpe commented Oct 17, 2024 •

edited

Loading

cart left a comment •

edited

Loading

ickshonpe Oct 17, 2024 •

edited

Loading