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

Have a separate implicit viewport node per root node + make viewport node Display::Grid #9637

Merged
merged 9 commits into from
Sep 19, 2023

Conversation

nicoburns
Copy link
Contributor

@nicoburns nicoburns commented Aug 29, 2023

Objective

Make bevy_ui "root" nodes more intuitive to use/style by:

  • Removing the implicit flexbox styling (such as stretch alignment) that is applied to them, and replacing it with more intuitive CSS Grid styling (notably with stretch alignment disabled in both axes).
  • Making root nodes layout independently of each other. Instead of there being a single implicit "viewport" node that all root nodes are children of, there is now an implicit "viewport" node per root node. And layout of each tree is computed separately.

Solution

  • Remove the global implicit viewport node, and instead create an implicit viewport node for each user-specified root node.
  • Keep track of both the user-specified root nodes and the implicit viewport nodes in a separate Vec.
  • Use the window's size as the available_space parameter to Taffy.compute_layout rather than setting it on the implicit viewport node (and set the viewport to height: 100%; width: 100% to make this "just work").

Changelog

  • Bevy UI now lays out root nodes independently of each other in separate layout contexts.
  • The implicit viewport node (which contains each user-specified root node) is now Display::Grid with align_items and justify_items both set to Start.

Migration Guide

  • Bevy UI now lays out root nodes independently of each other in separate layout contexts. If you were relying on your root nodes being able to affect each other's layouts, then you may need to wrap them in a single root node.
  • The implicit viewport node (which contains each user-specified root node) is now Display::Grid with align_items and justify_items both set to Start. You may need to add height: Val::Percent(100.) to your root nodes if you were previously relying on being implicitly set.

@nicoburns nicoburns added A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Aug 29, 2023
@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Aug 29, 2023
@rparrett
Copy link
Contributor

rparrett commented Aug 30, 2023

There seem to be quite a few more examples that were relying on this behavior.

Some changes are quite harmless like "text in bottom left moved to top left." It also seems like there must have been some sort of inherent padding somehow because some things that were previously in the top left are now even more top-left.

But some are very broken.

imdirdiff-out.png

@ickshonpe
Copy link
Contributor

ickshonpe commented Aug 30, 2023

There seem to be quite a few more examples that were relying on this behavior.

Some changes are quite harmless like "text in bottom left moved to top left." It also seems like there must have been some sort of inherent padding somehow because some things that were previously in the top left are now even more top-left.

But some are very broken.

A lot of the problems are because of how I optimised the examples to take advantage of the implicit root node before. Most of it should be trivial to fix. It doesn't look like this PR should introduce any new bugs.

Not against this but I've got a few vague reservations. I'll come back later after I've tried experimenting a bit.

@nicoburns
Copy link
Contributor Author

@rparrett How did you generate those visual diffs? That's very helpful!

I've fixed a bunch of the examples (mostly just adding height: Val::Percent(100.0) to the root node - but in a few cases actually adding a wrapper root node). But looking at some of the other examples, I think perhaps we ought to be block this on DioxusLabs/taffy#530. The reason being that it seems common to position top-level UI nodes using absolute positioning (this is especially used in examples for debug text), and this PR would currently break that functionality (but if we land DioxusLabs/taffy#530, then that would continue to "just work").

As an aside: I experienced a lot of issues with click detection not working (i.e. buttons not doing anything when clicked) when testing examples. But it seems this is also the case on main and is not related to this PR.

@rparrett
Copy link
Contributor

rparrett commented Aug 30, 2023

It's a bit involved at the moment:

  • checkout base commit
  • cargo run -p example-showcase -- run --screenshot
  • mv screenshots base-screenshots
  • checkout another commit
  • cargo run -p example-showcase -- run --screenshot again

At that point you have two folders full of screenshots. I hacked together this thing to generate a super basic html report from those.

It takes a good while to do the screenshotting part and your PC pretty difficult to use with windows popping up and stealing focus while it is happening (macos fix). Patching in #9220 (and using --in-ci) helps a bit with the UI examples that run with desktop_app mode. Otherwise you can move your mouse frantically over the window if things don't appear to be progressing.

crates/bevy_ui/src/layout/mod.rs Outdated Show resolved Hide resolved
@@ -151,6 +152,7 @@ mod game {
NodeBundle {
style: Style {
width: Val::Percent(100.0),
height: Val::Percent(100.0),
Copy link
Contributor

@ickshonpe ickshonpe Aug 31, 2023

Choose a reason for hiding this comment

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

The margins no longer display correctly for the new game section here.
Not sure what is wrong? Might not be something introduced by this PR but an already existing bug. I seem to remember someone describing a similar issue on discord but they didn't provide a code sample, only a screenshot, so not sure if it is related.

Copy link
Contributor Author

@nicoburns nicoburns Sep 15, 2023

Choose a reason for hiding this comment

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

Yeah, that's bizarre. I don't understand what is going on here. Not only is the black node taller in main, but the second line of text is rendered higher up the screen!

Main:

Screenshot 2023-09-15 at 23 12 33

This PR:

Screenshot 2023-09-15 at 23 12 04

Copy link
Contributor Author

@nicoburns nicoburns Sep 15, 2023

Choose a reason for hiding this comment

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

@ickshonpe I have a feeling this may have to do with non-deterministic text measurement. Because by removing the root node (and putting in some borders for debugging the layout), I was able to make it do this:

Screenshot 2023-09-16 at 00 10 15

Which it definitely shouldn't do. I suspect the bug we are seeing in the actual game_menu example is the text measurement returning a single-line height for preliminary measurements. And then a 2-line height we re-measured with the dimensions returned from the first measurement. FYI, cosmic-text recently fixed a very similar issue pop-os/cosmic-text#175.

Code for above screenshot:

//! This example will display a simple menu using Bevy UI where you can start a new game,
//! change some settings or quit. There is no actual game, it will just display the current
//! settings for 5 seconds before going back to the menu.

use bevy::prelude::*;

const TEXT_COLOR: Color = Color::rgb(0.9, 0.9, 0.9);

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .run();
}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());
    // commands
    //     .spawn(
    //         NodeBundle {
    //             style: Style {
    //                 width: Val::Percent(100.0),
    //                 height: Val::Percent(100.0),
    //                 // center children`
    //                 align_items: AlignItems::Center,
    //                 justify_content: JustifyContent::Center,
    //                 border: UiRect::all(Val::Px(4.0)),
    //                 ..default()
    //             },
    //             border_color: Color::RED.into(),
    //             ..default()
    //         },
    //     )
    //     .with_children(|parent| {
    // First create a `NodeBundle` for centering what we want to display
    commands
        .spawn(NodeBundle {
            style: Style {
                // This will display its children in a column, from top to bottom
                flex_direction: FlexDirection::Column,
                // `align_items` will align children on the cross axis. Here the main axis is
                // vertical (column), so the cross axis is horizontal. This will center the
                // children
                align_items: AlignItems::Center,
                // margin: UiRect::all(Val::Auto),
                border: UiRect::all(Val::Px(4.0)),
                ..default()
            },
            border_color: Color::BLUE.into(),
            background_color: Color::BLACK.into(),
            ..default()
        })
        .with_children(|parent| {
            // Display two lines of text, the second one with the current settings
            parent
                .spawn(NodeBundle {
                    style: Style {
                        align_items: AlignItems::Start,
                        justify_content: JustifyContent::Start,
                        border: UiRect::all(Val::Px(4.0)),
                        margin: UiRect::all(Val::Px(50.0)),
                        ..default()
                    },
                    border_color: Color::RED.into(),
                    background_color: Color::GRAY.into(),
                    ..default()
                })
                .with_children(|parent| {
                    parent.spawn(
                        TextBundle::from_section(
                            "Will be back to the menu shortly...",
                            TextStyle {
                                font_size: 80.0,
                                color: TEXT_COLOR,
                                ..default()
                            },
                        )
                    );
                });

            parent
                .spawn(NodeBundle {
                    style: Style {
                        align_items: AlignItems::Start,
                        justify_content: JustifyContent::Start,
                        border: UiRect::all(Val::Px(4.0)),
                        margin: UiRect::all(Val::Px(50.0)),
                        ..default()
                    },
                    border_color: Color::RED.into(),
                    background_color: Color::GRAY.into(),
                    ..default()
                })
                .with_children(|parent| {
                    parent.spawn(
                        TextBundle::from_sections([
                            TextSection::new(
                                "quality: Medium",
                                TextStyle {
                                    font_size: 60.0,
                                    color: Color::BLUE,
                                    ..default()
                                },
                            ),
                            TextSection::new(
                                " - ",
                                TextStyle {
                                    font_size: 60.0,
                                    color: TEXT_COLOR,
                                    ..default()
                                },
                            ),
                            TextSection::new(
                                "volume: Volume(7)",
                                TextStyle {
                                    font_size: 60.0,
                                    color: Color::GREEN,
                                    ..default()
                                },
                            ),
                        ])
                    );
                });
        });
    // });
}

@ickshonpe
Copy link
Contributor

ickshonpe commented Aug 31, 2023

This seems like a very good change. The only snag is root node styling, waiting for DioxusLabs/taffy#530 seems sensible.

I haven't checked all the examples yet but the ones I looked at seemed fine except for the problem with the margins in game_menu.

@nicoburns nicoburns added the S-Blocked This cannot move forward until something else changes label Sep 4, 2023
@nicoburns
Copy link
Contributor Author

DioxusLabs/taffy#530 has ended up being more complicated than anticipated (mostly because web treats root nodes as display: block - so we either need to implement that or some kind of switch for compatibility).

My suggestion is to re-add the implicit roots, but:

  • Create an implicit root per user-specified root (rather than having all user-specified roots under a single root node). This preserves the benefit provided by this PR of having user-specified root nodes layout entirely independently of each other).
  • Make the implicit root nodes display: grid; align-items: start; justify-items: start.

I think this will provide the most user-friendly environment for people to create their UIs in while not requiring us to wait on Taffy changes.

@nicoburns nicoburns force-pushed the no-implicit-ui-root-node branch 2 times, most recently from e4ef8e8 to 4637a38 Compare September 15, 2023 18:39
@nicoburns nicoburns changed the title Remove implicit root nodes from bevy_ui Have a separate implicit viewport node per root node + make viewport node Display::Grid Sep 15, 2023
@nicoburns nicoburns force-pushed the no-implicit-ui-root-node branch 3 times, most recently from 022636b to 7d75879 Compare September 15, 2023 21:41
@nicoburns
Copy link
Contributor Author

I have re-written this PR to use the above approach (#9637 (comment)) + updated the description to match the new approach. This seems to have fixed the examples. None of the following look related to this PR to me (they all just look like non-deterministic examples).

Screenshot 2023-09-15 at 23 01 32 Screenshot 2023-09-15 at 23 01 26 Screenshot 2023-09-15 at 23 01 21 Screenshot 2023-09-15 at 23 01 16

The only issue seems to be the game_menu example which does appear to be a regression from main. Although I am struggling to understand how this PR could be causing that, as it seems to not be related to the root node.

@nicoburns nicoburns requested review from ickshonpe and rparrett and removed request for ickshonpe September 15, 2023 22:21
@rparrett
Copy link
Contributor

None of the following look related to this PR to me (they all just look like non-deterministic examples).

Yeah. shadow_biases is deterministic if there is no mouse movement, but the diff shows identical UI. Some have inherent non-deterministic randomness and I think a few others just behave differently based on asset load timing.

@nicoburns
Copy link
Contributor Author

Yeah. shadow_biases is deterministic if there is no mouse movement, but the diff shows identical UI.

Yeah, I was trying to still use my computer while it ran, so a lot of them did have mouse movements! As you say, none of the diffs seem to be in UI elements (except font_atlas_debug, but that seems to be rendering random glyphs so it's never going to be consistent).

@ickshonpe
Copy link
Contributor

ickshonpe commented Sep 16, 2023

Yeah I have a similar problem with my latest borders reimplementation that causes the last word from blocks of text to vanish. I don't think we should let it block this PR anyway, I haven't looked at the latest changes closely yet but the direction seems good.

Had an idea about how to fix the text bugs, working on it now.

@ickshonpe
Copy link
Contributor

ickshonpe commented Sep 16, 2023

It's the layout rounding. I think I have a workaround, I'll put up a PR this evening.

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.

This is really good. Does exactly what we need with very minimal changes.

crates/bevy_ui/src/layout/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/layout/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/layout/mod.rs Show resolved Hide resolved
crates/bevy_ui/src/layout/mod.rs Show resolved Hide resolved
crates/bevy_ui/src/layout/mod.rs Show resolved Hide resolved
@ickshonpe
Copy link
Contributor

It's the layout rounding. I think I have a workaround, I'll put up a PR this evening.

Worked for my problems. but not the menu example here.

I'm convinced the problem with the menu is nothing to do with this PR anyway, it's an existing bug revealed by these changes.

@nicoburns nicoburns 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-Blocked This cannot move forward until something else changes labels Sep 18, 2023
@rparrett
Copy link
Contributor

rparrett commented Sep 18, 2023

This seems to break the multiple_windows example now with a runtime panic.

@nicoburns
Copy link
Contributor Author

This seems to break the multiple_windows example now with a runtime panic.

Good idea to check that. Fixed!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 19, 2023
Merged via the queue into bevyengine:main with commit b995827 Sep 19, 2023
21 checks passed
@nicoburns nicoburns deleted the no-implicit-ui-root-node branch September 19, 2023 22:52
@ickshonpe ickshonpe mentioned this pull request Sep 20, 2023
@james7132 james7132 mentioned this pull request Sep 20, 2023
@cart cart mentioned this pull request Oct 13, 2023
43 tasks
@nicoburns
Copy link
Contributor Author

nicoburns commented Oct 13, 2023

Before vs. After screenshot for two root nodes:
Screenshot 2023-10-13 at 11 16 16

rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
…node `Display::Grid` (bevyengine#9637)

# Objective

Make `bevy_ui` "root" nodes more intuitive to use/style by:
- Removing the implicit flexbox styling (such as stretch alignment) that
is applied to them, and replacing it with more intuitive CSS Grid
styling (notably with stretch alignment disabled in both axes).
- Making root nodes layout independently of each other. Instead of there
being a single implicit "viewport" node that all root nodes are children
of, there is now an implicit "viewport" node *per root node*. And layout
of each tree is computed separately.

## Solution

- Remove the global implicit viewport node, and instead create an
implicit viewport node for each user-specified root node.
- Keep track of both the user-specified root nodes and the implicit
viewport nodes in a separate `Vec`.
- Use the window's size as the `available_space` parameter to
`Taffy.compute_layout` rather than setting it on the implicit viewport
node (and set the viewport to `height: 100%; width: 100%` to make this
"just work").

---

## Changelog

- Bevy UI now lays out root nodes independently of each other in
separate layout contexts.
- The implicit viewport node (which contains each user-specified root
node) is now `Display::Grid` with `align_items` and `justify_items` both
set to `Start`.

## Migration Guide

- Bevy UI now lays out root nodes independently of each other in
separate layout contexts. If you were relying on your root nodes being
able to affect each other's layouts, then you may need to wrap them in a
single root node.
- The implicit viewport node (which contains each user-specified root
node) is now `Display::Grid` with `align_items` and `justify_items` both
set to `Start`. You may need to add `height: Val::Percent(100.)` to your
root nodes if you were previously relying on being implicitly set.
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-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.

4 participants