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

[stefee]

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]

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

[stefee]

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.

[cart]

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

[stefee]

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]

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]

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]

@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]

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

[stefee]

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]

@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]

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

[harrywincup]

@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? https://github.com/bevyengine/bevy/blob/a3c0740ca7dcd8538d759a785be3c5ada2374b53/crates/bevy_ui/src/node.rs#L95

[stefee]

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]

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