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

UI bottom margin isn't applied #7120

Closed
JulianGmp opened this issue Jan 7, 2023 · 14 comments
Closed

UI bottom margin isn't applied #7120

JulianGmp opened this issue Jan 7, 2023 · 14 comments
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior

Comments

@JulianGmp
Copy link

JulianGmp commented Jan 7, 2023

Bevy version

Tested with both the 0.9.1 release from crates.io and the current main branch (076e6f7)

[dependencies]
# bevy = { version = "0.9.1" }
bevy = { git = "https://github.com/bevyengine/bevy.git", branch = "main" }

[Optional] Relevant system information

$ cargo --version
cargo 1.66.0 (d65d197ad 2022-11-15)
$ rustup show
Default host: x86_64-unknown-linux-gnu
rustup home:  /home/julian/.rustup

stable-x86_64-unknown-linux-gnu (default)
rustc 1.66.0 (69f9c33d7 2022-12-12)
$ uname -a
Linux a15-arch202210 6.1.2-zen1-1-zen #1 ZEN SMP PREEMPT_DYNAMIC Sat, 31 Dec 2022 17:40:33 +0000 x86_64 GNU/Linux
$ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.33s
     Running `target/debug/bevy-test`
2023-01-07T13:30:30.928388Z  INFO winit::platform_impl::platform::x11::window: Guessed window scale factor: 1    
2023-01-07T13:30:30.956827Z  INFO bevy_render::renderer: AdapterInfo { name: "AMD Radeon Graphics (RADV RENOIR)", vendor: 4098, device: 5686, device_type: IntegratedGpu, driver: "radv", driver_info: "Mesa 22.3.2", backend: Vulkan }
2023-01-07T13:30:31.032928Z  INFO bevy_diagnostic::system_information_diagnostics_plugin::internal: SystemInfo { os: "Linux  Arch Linux", kernel: "6.1.2-zen1-1-zen", cpu: "AMD Ryzen 7 4700U with Radeon Graphics", core_count: "8", memory: "15.0 GiB" }

What you did

I attempted to have a UI root node that spans the entire screen, but with a slight margin on each side.

Considering the following main.rs:

use bevy::prelude::*;

fn main() {
    App::new()
        .insert_resource(ClearColor(Color::GRAY))
        .add_plugins(DefaultPlugins)
        .add_startup_system(setup)
        .run();
}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());

    let margin_val = Val::Percent(5.0);
    commands.spawn(NodeBundle {
        background_color: BackgroundColor(Color::ORANGE),
        style: Style {
            size: Size::new(Val::Percent(100.0), Val::Percent(100.0)),
            margin: UiRect {
                left: margin_val,
                right: margin_val,
                top: margin_val,
                bottom: margin_val,
            },
            ..default()
        },
        ..default()
    });
}

my Cargo.toml for reference

[package]
name = "bevy-test"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at
# https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
# bevy = { version = "0.9.1" }
bevy = { git = "https://github.com/bevyengine/bevy.git", branch = "main" }

# Enable some optimizations for dependencies (incl. Bevy),
# but not for our code:
[profile.dev.package."*"]
opt-level = 3

What went wrong

Setting the margin in the style properties works, but not for the bottom margin.
I tried using different configurations with pixel values too, but it seemed that the bottom margin wasn't applied at all.

Additional information

image
Relevant discussion on the discord server: https://discord.com/channels/691052431525675048/1060632681873932329

@JulianGmp JulianGmp added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jan 7, 2023
@mockersf
Copy link
Member

mockersf commented Jan 7, 2023

I think the issue is actually that there shouldn't be a margin on the right.

If you try this code in html:

<div style="background: orange; width: 100%; height: 100%; margin: 5%"></div>

you will have no margin on bottom and right.

The layout should reserve the margin on left/top, then take 100% of width/height for the orange part, then take 5% for the margins again, offscreen.

@mockersf mockersf added A-UI Graphical user interfaces, styles, layouts, and widgets and removed S-Needs-Triage This issue needs to be labelled labels Jan 7, 2023
@JulianGmp
Copy link
Author

I'm not entirely sure what you mean with that. The html example only has one value for the margin. But bevy has 4 seperate values for each side, so I would expect each of them to funciton seperately from each other.

I expanded my sample code a bit. I now use an ImageBundle to display a simple sprite and I added bevy-inspector-egui (0.16.2) so that I tinker with the parameters live:

2023-01-07_19-35-33.mp4

When adjusting the left and right margins, they behaved as I expected. But as you can see in the video, the top margin seems to offset the node, rather than "squishing" it like left and right margin do (style.size is still set to 100% for width and height).

In case anyone wants to run this example for themselves: bevy-test.zip

@mockersf
Copy link
Member

mockersf commented Jan 7, 2023

I'm not entirely sure what you mean with that. The html example only has one value for the margin. But bevy has 4 seperate values for each side, so I would expect each of them to funciton seperately from each other.

That's the same with CSS, when you use only one value it's interpreted as using that value for each margin. Bevy UI is mostly following CSS specs, if you ask for something to be 110% (100% + each 5% margin) it will be 110% and go over.

Most of the computations for layout is done by taffy, @alice-i-cecile is it something you've heard?

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 7, 2023

I don't recognize this: can you file a bug upstream and link this issue?

@JulianGmp
Copy link
Author

I'm not familiar with taffy but I'll see if I can recreate this issue there

@nicoburns
Copy link
Contributor

I suspect the reason the margin isn't overflowing for the horizontal axis is because it's a flexbox row, and flex-shrink is 1 by default, meaning the element will start at 100% width but then shrink to fit (suggested size along the main axis can be overriden in flexbox.

@JulianGmp
Copy link
Author

As an update: Due to private reasons I haven't found time to work on this. I still want to recreate this with taffy but I can't give any promises as to when that would happen.

@ickshonpe
Copy link
Contributor

ickshonpe commented Feb 2, 2023

nicoburns is right, it's not intuitive but this is the correct behaviour.
The lengths of the main-axis and the cross-axis are calculated in different ways.
In this case, the height should be set to Val::Auto instead of Val::Percent(100.):

 commands.spawn(NodeBundle {
      background_color: BackgroundColor(Color::ORANGE),
      style: Style {
          size: Size::new(Val::Percent(100.0), Val::Auto),
          margin: UiRect::All(Val::Percent(100.0)),
          ..default()
      },
      ..default()
  });

Why Val::Auto works and Val::Percent(100.) doesn't is clearer if you look at this example:

example_7120

use bevy::prelude::*;

fn main() {
    App::new()
        .insert_resource(ClearColor(Color::BLACK))
        .add_plugins(DefaultPlugins)
        .add_startup_system(setup)
        .run();
}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());
    commands.spawn(NodeBundle {
        background_color: BackgroundColor(Color::WHITE),
        style: Style {
            size: Size::new(Val::Px(100.), Val::Px(100.)),
            margin: UiRect::all(Val::Px(40.)),
            ..default()
        },
        ..default()
    }).with_children(|parent| {
        parent.spawn(NodeBundle {
            background_color: BackgroundColor(Color::RED),
            style: Style {
                flex_direction: FlexDirection::Column,
                size: Size::new(Val::Percent(100.), Val::Percent(100.)),
                margin: UiRect::all(Val::Px(10.)),
                ..default()
            },
            ..default()
        });
    });

    commands.spawn(NodeBundle {
        background_color: BackgroundColor(Color::WHITE),
        style: Style {
            flex_direction: FlexDirection::Column,
            size: Size::new(Val::Px(100.), Val::Px(100.)),
            margin: UiRect::all(Val::Px(40.)),
            ..default()
        },
        ..default()
    }).with_children(|parent| {
        parent.spawn(NodeBundle {
            background_color: BackgroundColor(Color::RED),
            style: Style {
                size: Size::new(Val::Percent(100.), Val::Percent(100.)),
                margin: UiRect::all(Val::Px(10.)),
                ..default()
            },
            ..default()
        });
    });
    
    commands.spawn(NodeBundle {
        background_color: BackgroundColor(Color::WHITE),
        style: Style {
            size: Size::new(Val::Px(100.), Val::Px(100.)),
            margin: UiRect::all(Val::Px(40.)),
            ..default()
        },
        ..default()
    }).with_children(|parent| {
        parent.spawn(NodeBundle {
            background_color: BackgroundColor(Color::RED),
            style: Style {
                size: Size::new(Val::Percent(100.), Val::Auto),
                margin: UiRect::all(Val::Px(10.)),
                ..default()
            },
            ..default()
        });
    });
}

With FlexDirection::Row, the main-axis is horizontal and the cross-axis is vertical.

With the height (cross-axis) set to Val::Percent(100.), the node takes 100% of the height of its parent node.
Not the height of the parent minus the sum of the vertical margins, as most people might expect.

With the height set to Val::Auto, the node takes the default behaviour AlignItems::Stretch, which sets the height to fill the available space as desired.

@ickshonpe
Copy link
Contributor

Thought about this some more, and there is a bug.
The CSS Flexbox width and height defaults are auto, whereas in Bevy their default is Size::Undefined.
This means that, unlike in CSS, if you elide a height or width value for a node it will be given zero length (unless it has an explicitly sized child node). This misleads people into falsely assuming that they have to explicitly set a value for both height and width all the time.

@nicoburns
Copy link
Contributor

Is there a difference between Auto and Size::Undefined for the width/height properties? I thought Undefined meant "unset so use the default value" and the default value for width/height is Auto. I would expect both width: Auto and width: Undefined to "be given zero length unless it has an explicitly sized child node" (although this size may then be altered by flex properties (e.g. flex-grow).

Taffy >= 0.2 has removed the Undefined variant of Dimension because it was the same as Auto and therefore pointless.

@ickshonpe
Copy link
Contributor

Seems to be:

use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_startup_system(setup)
        .run();
}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());

    commands.spawn(NodeBundle {
        style: Style {
            size: Size::new(Val::Px(100.0), Val::Auto),
            ..Default::default()
        },
        background_color: BackgroundColor(Color::RED),
        ..Default::default()
    });

    commands.spawn(NodeBundle {
        style: Style {
            size: Size::new(Val::Px(100.0), Val::Undefined),
            ..Default::default()
        },
        background_color: BackgroundColor(Color::BLUE),
        ..Default::default()
    });
}

Draws only a red bar.

@ickshonpe
Copy link
Contributor

It's still there?
https://github.com/DioxusLabs/taffy/blob/0.2.x/src/style.rs

and in https://github.com/bevyengine/bevy/blob/main/crates/bevy_ui/src/flex/convert.rs

pub fn from_val(scale_factor: f64, val: Val) -> taffy::style::Dimension {
    match val {
        Val::Auto => taffy::style::Dimension::Auto,
        Val::Percent(value) => taffy::style::Dimension::Percent(value / 100.0),
        Val::Px(value) => taffy::style::Dimension::Points((scale_factor * value as f64) as f32),
        Val::Undefined => taffy::style::Dimension::Undefined,
    }
}

@nicoburns
Copy link
Contributor

It's still there?

Ah, must be 0.3 that removes it. In any case, I think that versions of Taffy that have Undefined treat it the same Auto anyway.

@nicoburns
Copy link
Contributor

Draws only a red bar.

Hmm... perhaps there are some subtle differences.

bors bot pushed a commit that referenced this issue Feb 3, 2023
# Objective

In CSS Flexbox width and height are auto by default, whereas in Bevy their default is `Size::Undefined`.
This means that, unlike in CSS, if you elide a height or width value for a node it will be given zero length (unless it has an explicitly sized child node). This has misled users into falsely assuming that they have to explicitly set a value for both height and width all the time.

relevant issue: #7120

## Solution

Change the `Size` `width` and `height` default values to `Val::Auto`

## Changelog

* Changed the `Size` `width` and `height` default values to `Val::Auto`

## Migration Guide

The default values for `Size` `width` and `height` have been changed from `Val::Undefined` to `Val::Auto`.
It's unlikely to cause any issues with existing code.
bors bot pushed a commit that referenced this issue Feb 3, 2023
# Objective

In CSS Flexbox width and height are auto by default, whereas in Bevy their default is `Size::Undefined`.
This means that, unlike in CSS, if you elide a height or width value for a node it will be given zero length (unless it has an explicitly sized child node). This has misled users into falsely assuming that they have to explicitly set a value for both height and width all the time.

relevant issue: #7120

## Solution

Change the `Size` `width` and `height` default values to `Val::Auto`

## Changelog

* Changed the `Size` `width` and `height` default values to `Val::Auto`

## Migration Guide

The default values for `Size` `width` and `height` have been changed from `Val::Undefined` to `Val::Auto`.
It's unlikely to cause any issues with existing code.
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

No branches or pull requests

5 participants