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

Node size computed incorrectly(?) on cross axis for AlignItems::Stretch #291

Closed
stefee opened this issue Aug 22, 2020 · 14 comments · Fixed by #304
Closed

Node size computed incorrectly(?) on cross axis for AlignItems::Stretch #291

stefee opened this issue Aug 22, 2020 · 14 comments · Fixed by #304
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior

Comments

@stefee
Copy link
Contributor

stefee commented Aug 22, 2020

Here is a CodeSandbox showing how flex-basis and align-items: stretch works: https://codesandbox.io/s/stoic-turing-tdyz7?file=/index.html

Note that by default align-items is stretch in browsers and also in the Stretch library which Bevy UI is based on.

I am trying to reproduce the above CodeSandbox using Bevy UI: https://github.com/stefee/bevy/commit/33bdca418f1944ed05daf9e8c1e96217106e32ff

use bevy::prelude::*;

fn main() {
    env_logger::init();

    App::build()
        .add_resource(bevy::window::WindowDescriptor {
            width: 1920,
            height: 1080,
            title: "Bevy UI Storybook Prototype".to_string(),
            ..Default::default()
        })
        .add_resource(bevy::render::pass::ClearColor(Color::rgb(1.0, 1.0, 1.0)))
        .add_default_plugins()
        .add_startup_system(setup.system())
        .run();
}

fn setup(
    mut commands: Commands,
    _asset_server: Res<AssetServer>,
    mut materials: ResMut<Assets<ColorMaterial>>,
) {
    commands
        // ui camera
        .spawn(UiCameraComponents::default())
        .spawn(NodeComponents {
            style: Style {
                size: Size::new(Val::Px(200.0), Val::Px(200.0)),
                align_items: AlignItems::Stretch,
                ..Default::default()
            },
            material: materials.add(Color::rgb(1.0, 0.0, 1.0).into()),
            ..Default::default()
        })
        .with_children(|parent| {
            parent
                .spawn(NodeComponents {
                    style: Style {
                        flex_basis: Val::Percent(50.0),
                        ..Default::default()
                    },
                    material: materials.add(Color::rgb(1.0, 0.0, 0.0).into()),
                    ..Default::default()
                })
                .spawn(NodeComponents {
                    style: Style {
                        flex_basis: Val::Percent(50.0),
                        ..Default::default()
                    },
                    material: materials.add(Color::rgb(0.0, 1.0, 0.0).into()),
                    ..Default::default()
                });
        });
}

The above code results in a purple square – the parent node is visible but not the child nodes, because the child nodes have a height of 0.

Height in this case corresponds to the "cross axis" in the flex model, and the child node should have cross axis size of 100% when AlignItems::Stretch is used. (See Guide to Flexbox).

Workaround

The current workaround is to specify a size as well as (or instead of) flex_basis.

                .spawn(NodeComponents {
                    style: Style {
                        size: Size::new(Val::Percent(100.0), Val::Percent(100.0)),
                        flex_basis: Val::Percent(50.0),
                        ..Default::default()
                    },
                    material: materials.add(Color::rgb(1.0, 0.0, 0.0).into()),
                    ..Default::default()
                })

The problem with this is that width and height do not respect flex direction, so if you were to change flex direction (e.g. from row to column) you would need to also swap the width and height properties.

Also, if you were to change the parent to use something other than AlignItems::Stretch you would then need to update the child sizes as well to get the desired effect.

Update 24/08/18

@harrywincup found a better workaround of using Val::Auto for the child node size.

                .spawn(NodeComponents {
                    style: Style {
                        size: Size::new(Val::Auto, Val::Auto),
                        flex_basis: Val::Percent(50.0),
                        ..Default::default()
                    },
                    material: materials.add(Color::rgb(1.0, 0.0, 0.0).into()),
                    ..Default::default()
                })

This has the same result as the above workaround, but with the advantage that flex direction is respected and AlignItems behaviour works as expected.

I've opened a PR to make Val::Auto the default value for style.size. 👉 #304

@karroffel karroffel added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets labels Aug 22, 2020
@karroffel
Copy link
Contributor

(no clue if it's a bug or working as expected, label can always be removed later 😅 )

@stefee
Copy link
Contributor Author

stefee commented Aug 22, 2020

I can try to get a repro of this using Stretch directly, and if the same issue happens with Stretch we can raise this with them. Maybe this is expected behaviour.

@stefee stefee changed the title Node size computed incorrectly(?) when using flex_basis instead of width & height Node size computed incorrectly(?) on cross-axis when using flex_basis instead of width & height Aug 22, 2020
@stefee stefee changed the title Node size computed incorrectly(?) on cross-axis when using flex_basis instead of width & height Node size computed incorrectly(?) on cross axis when using flex_basis instead of width & height Aug 22, 2020
@cart
Copy link
Member

cart commented Aug 22, 2020

Sounds like a plan to me. Thanks for the thoroughness here!

@stefee
Copy link
Contributor Author

stefee commented Aug 22, 2020

We can also check whether AlignItems::Stretch is broken in all cases or only on the root node.

Edit: I just checked and it is the same behaviour regardless of whether the parent node is the root node or nested under the root.

@stefee stefee changed the title Node size computed incorrectly(?) on cross axis when using flex_basis instead of width & height Node size computed incorrectly(?) on cross axis for AlignItems::Stretch Aug 22, 2020
@stefee
Copy link
Contributor Author

stefee commented Aug 23, 2020

I have not been able to reproduce this problem using Stretch directly: https://gist.github.com/stefee/b01f1f7097bcfc2de6727742231d4199

Stretch computes the child node height correctly:

[src/main.rs:48] stretch.layout(first_child).unwrap() = Layout {
    order: 0,
    size: Size {
        width: 100.0,
        height: 200.0,
    },
    location: Point {
        x: 100.0,
        y: 0.0,
    },
}

So this does actually appear to be a Bevy UI issue.

@stefee
Copy link
Contributor Author

stefee commented Aug 23, 2020

I also just confirmed that the Bevy UI behaviour is the same when the parent has Px dimensions or Percent dimensions to make sure that is not the cause.

@harrywincup
Copy link

@stefee It seems that if you set each of the Flex Items' size to Size::new(Val::Auto, Val::Auto), then it behaves as expected too. Would you argue that the default size values should be auto rather than undefined?

@stefee
Copy link
Contributor Author

stefee commented Aug 23, 2020

@harrywincup This might be it... Bevy UI should default size to the same as whatever Stretch lib defaults to?

@stefee
Copy link
Contributor Author

stefee commented Aug 23, 2020

OK, so I just tried setting the child style to the following in my Stretch gist:

    Style {
        size: Size{
            width: Dimension::Undefined,
            height: Dimension::Undefined,
        },
        flex_basis: Dimension::Percent(50.0),
        ..Default::default()
    }

And this resulted in the following test output:

running 1 test
test tests::correct_layout ... FAILED

failures:

---- tests::correct_layout stdout ----
thread 'tests::correct_layout' panicked at 'assertion failed: `(left == right)`
  left: `Size { width: 100.0, height: 0.0 }`,
 right: `Size { width: 100.0, height: 200.0 }`', src/main.rs:72:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::correct_layout

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

I think we might have found the bug @harrywincup!

@harrywincup
Copy link

@stefee Well i've got no idea what i'm doing because i'm a total Rust newbie, but it sounds like you're making progress!

These lines in Stretch also seem relevant, as it appears to resolve any FlexItem's size to the parent node's dimensions:

https://github.com/vislyhq/stretch/blob/6879b9a1cf3c244430bbf8b88bf205c614d72562/src/geometry.rs#L156-L160

impl Size<style::Dimension> {
    pub(crate) fn resolve(&self, parent: Size<Number>) -> Size<Number> {
        Size { width: self.width.resolve(parent.width), height: self.height.resolve(parent.height) }
    }
}

https://github.com/vislyhq/stretch/blob/6879b9a1cf3c244430bbf8b88bf205c614d72562/src/algo.rs#L227
size: child_style.size.resolve(node_inner_size),

I don't know what the suggestion is to fix, but it definitely seems like Bevy should be doing something similar?

@stefee
Copy link
Contributor Author

stefee commented Aug 23, 2020

@harrywincup
Bevy UI uses Stretch under the hood, so we should be covered as long as we give it the correct inputs, right?

@harrywincup
Copy link

@stefee Yeah I think so. I suppose the question then is what the correct input is? Are you thinking that the default here should be Val::Auto, or is there a smarter way for Bevy/Stretch to convert it in a Flex condition?

size: Default::default(),

@stefee
Copy link
Contributor Author

stefee commented Aug 23, 2020

So I think ideally we should do something like this in node.rs

impl Default for Size<Val> {
    fn default() -> Self {
        Self { width: Val::Auto, height: Val::Auto }
    }
}

This matches Stretch default:

https://github.com/vislyhq/stretch/blob/6879b9a1cf3c244430bbf8b88bf205c614d72562/src/style.rs#L247

But this doesn't currently work because of these errors:

error[E0119]: conflicting implementations of trait `std::default::Default` for type `bevy_math::Size<node::Val>`:
  --> crates/bevy_ui/src/node.rs:24:1
   |
24 | impl Default for Size<Val> {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: conflicting implementation in crate `bevy_math`:
           - impl<T> std::default::Default for bevy_math::Size<T>
             where T: std::default::Default;

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
  --> crates/bevy_ui/src/node.rs:24:1
   |
24 | impl Default for Size<Val> {
   | ^^^^^^^^^^^^^^^^^---------
   | |                |
   | |                `bevy_math::Size` is not defined in the current crate
   | impl doesn't use only types from inside the current crate
   |
   = note: define and implement a trait or new type instead

error: aborting due to 2 previous errors

@stefee
Copy link
Contributor Author

stefee commented Aug 24, 2020

I updated the issue description with @harrywincup's solution.

@cart cart closed this as completed in #304 Aug 25, 2020
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-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants