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

Migrate UI bundles to required components #15898

Merged
merged 17 commits into from
Oct 17, 2024

Conversation

VitalyAnkh
Copy link
Contributor

@VitalyAnkh VitalyAnkh commented Oct 14, 2024

Objective

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

@VitalyAnkh VitalyAnkh marked this pull request as draft October 14, 2024 10:50
@alice-i-cecile 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 alice-i-cecile added this to the 0.15 milestone Oct 14, 2024
@VitalyAnkh VitalyAnkh added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Oct 15, 2024
@MiniaczQ MiniaczQ self-requested a review October 15, 2024 14:31
@VitalyAnkh 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 VitalyAnkh marked this pull request as ready for review October 16, 2024 10:32
@VitalyAnkh 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
@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Oct 16, 2024
@alice-i-cecile
Copy link
Member

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
Copy link
Contributor

ickshonpe commented Oct 17, 2024

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
Copy link
Contributor

ickshonpe commented Oct 17, 2024

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
Copy link
Member

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

Copy link
Contributor

@MiniaczQ MiniaczQ left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

cart commented Oct 17, 2024

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.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I just pushed a change that:

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

I think this PR is now ready to merge

@@ -823,7 +842,7 @@ pub fn queue_uinodes(
pipeline,
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),
Copy link
Contributor

@ickshonpe ickshonpe Oct 17, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@alice-i-cecile 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
28 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Oct 20, 2024
# 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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate UI bundles to required components
5 participants