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 #15889

Closed
alice-i-cecile opened this issue Oct 13, 2024 · 13 comments · Fixed by #15898
Closed

Migrate UI bundles to required components #15889

alice-i-cecile opened this issue Oct 13, 2024 · 13 comments · Fixed by #15898
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Design This issue requires design work to think about how it would best be accomplished S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon
Milestone

Comments

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Oct 13, 2024

Almost all of our bundle types have been migrated to required components after #14791. However, UI bundles have not been done yet.

#15550 will conflict with that, but since that PR needs more time to bake, we should press on with this work for the 0.15 release.

The strategy to do so is laid out here: https://hackmd.io/@bevy/required_components/%2FpuAA8c18TzeMhjclo36vEQ#Combined-Proposal-1-Conservative-Port-no-major-reworks-Selected

@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! labels Oct 13, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 13, 2024
@alice-i-cecile alice-i-cecile added D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Uncontroversial This work is generally agreed upon labels Oct 13, 2024
@ickshonpe
Copy link
Contributor

ickshonpe commented Oct 13, 2024

Ah I'm stupid I was worried this was going to be terrible but I hadn't realised required components were transitive until now 😅

I think though that Style might be better a choice instead of Node for the primary component. I've been thinking of a few constructions where it might be useful to have unstyled Nodes. We might want to support alternative layout algorithms as well and it'd be easier if they could output Nodes and use the existing UI extraction setup.

@cart
Copy link
Member

cart commented Oct 14, 2024

I think though that Style might be better a choice instead of Node for the primary component. I've been thinking of a few constructions where it might be useful to have unstyled Nodes. We might want to support alternative layout algorithms as well and it'd be easier if they could output Nodes and use the existing UI extraction setup.

I think most of Style's fields should be migrated to Node, the rest should be moved elsewhere, and Style should be removed. So in a way, I agree.

@alice-i-cecile
Copy link
Member Author

I think most of Style's fields should be migrated to Node, the rest should be moved elsewhere, and Style should be removed. So in a way, I agree.

Do you want to do that now, or wait for 0.16? I firmly agree about the ultimate action though.

@ickshonpe
Copy link
Contributor

ickshonpe commented Oct 14, 2024

Do you mean that Style and Node should be consolidated into one component containing both the layout constraints and the resolved layout values? Or is it more like Style being renamed to Node and the resolved layout values being moved somewhere else.

I'd prefer it if we had more granular UI components anyway.

@VitalyAnkh
Copy link
Contributor

VitalyAnkh commented Oct 15, 2024

I also prefer more granular UI components, i.e., keeping Node and Style separated. I'm waiting for the community to reach a consensus before migrating the examples in the PR (there are about 300 instances that need to be migrated in the examples).

@VitalyAnkh VitalyAnkh added the S-Needs-Design This issue requires design work to think about how it would best be accomplished label Oct 15, 2024
@cart
Copy link
Member

cart commented Oct 15, 2024

Do you want to do that now, or wait for 0.16? I firmly agree about the ultimate action though.

I think we should do the conservative port as outlined in the hackmd and do more involved changes in a later release.

Do you mean that Style and Node should be consolidated into one component containing both the layout constraints and the resolved layout values? Or is it more like Style being renamed to Node and the resolved layout values being moved somewhere else.

I think Node should be the "driving" public facing interface containing all of the "styleable" properties inherent to a UI node. For computed values (such as calculated_size), we should move those to one or more separate components.

There should be no more generic Style component. You "style" fields on specific components, you don't set Style.

@ickshonpe
Copy link
Contributor

I think Node should be the "driving" public facing interface containing all of the "styleable" properties inherent to a UI node. For computed values (such as calculated_size), we should move those to one or more separate components.

That sounds good. I've never liked how we keep the relatively non-distinct geometric layout constraints in a component called Style

It still bothers me though that the ouput component containing the computed layout is going to be the primary defining component, even if it's only intended to be temporary it doesn't feel very pleasant. Couldn't we do a renaming with Style becoming Node (and the primary component) and Node becoming ResolvedNode (or whatever) to go along with the required components migration?

@alice-i-cecile
Copy link
Member Author

I'd really try to reduce breaking changes for right now: this migration is already going to be really painful.

@ickshonpe
Copy link
Contributor

ickshonpe commented Oct 16, 2024

If we don't want breaking changes then Style could be the central component for now, using Node (without changes) doesn't make sense with the new require api. Required components is meant to make it unnecessary to explicitly spawn essential nonconfigurable components but since it's relatively uncommon to spawn completely unstyled UI nodes VitalyAnkh's migration PR is full of code like:

commands
.spawn((
    Node::default(),
    Style {
        width: Val::Percent(100.),
        height: Val::Percent(100.),
        ..default()
    }
))

which seems obviously wrong to me. I realise this really is not all that important though and it will be fixed in 0.16. I'll leave this alone now and go and fix some bugs.

@ickshonpe
Copy link
Contributor

Last thing, we could put require on both Style and Node?

@VitalyAnkh
Copy link
Contributor

VitalyAnkh commented Oct 16, 2024

It's frustrating to see numerous instances of Node::default() following a Style in the examples. But this approach makes migration to Bevy 0.15 more intuitive for users.

@ickshonpe Are you proposing that we make Style require Node and use Style as the public interface? If we pursue this approach—moving most of Style's components into Node and using Node as the public UI interface in 0.16—we'd be creating an awkward transition (Node -> Style in 0.15, then Style -> Node in 0.16).

I suggest we either make no changes beyond the required component migration, or implement a comprehensive overhaul. A middle ground approach might lead to unnecessary complexity.

@ickshonpe
Copy link
Contributor

ickshonpe commented Oct 16, 2024

@ickshonpe Are you proposing that we make Style require Node and use Style as the public interface? If we pursue this approach—moving most of Style's components into Node and using Node as the public UI interface in 0.16—we'd be creating an awkward transition (Node -> Style in 0.15, then Style -> Node in 0.16).

I don't think it makes the transition any more awkward or unituitive. It isn't Node -> Style, it's NodeBundle to Style + required components. Bevy users are much more familiar with Style than Node which is normally only used internally. For the 0.16 transition Style gets removed and the fields are moved to Node which would just mean a change from:

commands.spawn(
    Style {
        width: Val::Percent(100.),
        height: Val::Percent(100.),
        ..default()
    }
);

to

commands.spawn(
    Node {
        width: Val::Percent(100.),
        height: Val::Percent(100.),
        ..default()
    }
);

I guess? Or maybe we'll have bsn in by then, I'm not sure about the timeline.

@VitalyAnkh
Copy link
Contributor

@ickshonpe You're right. I totally agree with you. I will update the PR. Thanks!

github-merge-queue bot pushed a commit that referenced this issue Oct 17, 2024
# Objective

- Migrate UI bundles to required components, fixes #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.
```diff
     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.
```diff
                     .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.
```diff
-    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()
-    });
+    ));
 ```

---------

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
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-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Design This issue requires design work to think about how it would best be accomplished S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants