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 vertical layout inaccurate for TextNode #7488

Closed
dvogel opened this issue Feb 3, 2023 · 9 comments · Fixed by DioxusLabs/taffy#348
Closed

UI vertical layout inaccurate for TextNode #7488

dvogel opened this issue Feb 3, 2023 · 9 comments · Fixed by DioxusLabs/taffy#348
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior

Comments

@dvogel
Copy link

dvogel commented Feb 3, 2023

Bevy version

Bevy v0.9.1
Rustc 1.66.1
Linux, debian/bookworm

AdapterInfo { name: "AMD Radeon RX 560 Series (RADV POLARIS11)", vendor: 4098, device: 26623, device_type: DiscreteGpu, driver: "radv", driver_info: "Mesa 22.3.3", backend: Vulkan }

What you did

The following code (also available with assets in this repro) is meant to render a text box from the top of the screen to the bottom of the screen. However it renders part of the text box underneath the window titlebar and leaves a gap between the bottom of the text box and the window border (screenshots below).

use bevy::{
    prelude::*,
    ui::{BackgroundColor, Val},
};

pub fn inject_ui_entities(asset_server: Res<AssetServer>, mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());
    commands.spawn((
        TextBundle {
            text: Text::from_section(
                "Hello world!",
                TextStyle {
                    font: asset_server.load("fonts/FiraSans-Regular.ttf"),
                    font_size: 30.0,
                    color: Color::WHITE,
                },
            ),
            style: Style {
                position: UiRect {
                    left: Val::Auto,
                    right: Val::Auto,
                    top: Val::Px(0.0),
                    bottom: Val::Percent(1.0),
                },
                ..default()
            },
            transform: Transform::from_xyz(0.0, 0.0, 0.0),
            ..default()
        },
        BackgroundColor(Color::rgba(164.0 / 256.0, 116.0 / 256.0, 73.0 / 256.0, 0.5)),
    ));
}

fn main() {
    App::new()
        .add_plugins(DefaultPlugins.set(WindowPlugin {
            window: WindowDescriptor {
                title: String::from("UI Gap Repro"),
                width: 1200.0,
                height: 600.0,
                ..Default::default()
            },
            exit_on_all_closed: true,
            ..default()
        }))
        .add_system(bevy::window::close_on_esc)
        .add_startup_system(inject_ui_entities)
        .run();
}

What went wrong

With two different window dimensions, you can see that the error is proportional to the height of the window:

Screenshot from 2023-02-03 08-27-14

Screenshot from 2023-02-03 08-26-58

Additional information

Other information that can be used to further reproduce or isolate the problem.
This commonly includes:

  • I tried to reproduce this with just a NodeBundle but it didn't render at all (likely an error or misunderstanding on my part).
@dvogel dvogel added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Feb 3, 2023
@rparrett rparrett added A-Windowing Platform-agnostic interface layer to run your app in A-UI Graphical user interfaces, styles, layouts, and widgets and removed S-Needs-Triage This issue needs to be labelled labels Feb 3, 2023
@rparrett
Copy link
Contributor

rparrett commented Feb 3, 2023

Thanks for the repro.

I can also reproduce with

bevy_render::renderer: AdapterInfo { name: "Apple M1 Max", vendor: 0, device: 0, device_type: IntegratedGpu, driver: "", driver_info: "", backend: Metal }
bevy_diagnostic::system_information_diagnostics_plugin::internal: SystemInfo { os: "MacOS 13.1 ", kernel: "22.2.0", cpu: "Apple M1 Max", core_count: "10", memory: "64.0 GiB" }

and with bevy's latest main branch, and with scale_factor_override at 1.0.

@rparrett
Copy link
Contributor

rparrett commented Feb 3, 2023

This feels buggy, but I also have no idea what the expected behavior would be with both a top and bottom position set.

To create a node spanning the window vertically, you can give it a position and size:

style: Style {
    position: UiRect {
        left: Val::Px(0.0),
        top: Val::Px(0.0),
        ..default()
    },
    size: Size {
        width: Val::Auto,
        height: Val::Percent(100.0),
    },
    ..default()
},

@tim-blackbird
Copy link
Contributor

I believe position corresponds to inset in CSS.

@nicoburns
Copy link
Contributor

@dvogel Can you try setting position_type: PositionType::Absolute on your node? Automatically computing height based on position like that only works for absolutely positioned nodes.

@nicoburns
Copy link
Contributor

@rparrett

but I also have no idea what the expected behavior would be with both a top and bottom position set.

For an absolutely positioned element

Assuming that height is not set (or set to Auto), then the behaviour should indeed be the intuitive one:

  • The top of the element should be at position 0.0
  • The bottom should be at window_height - (1% of window_height)
  • The height of the element should be computed automatically by the distance between those two points

If height is set, then bottom is ignored and the size/position is determined by top and height.

I believe that this layout would work as intended if it was absolutely positioned, and this is probably the best fix for the end user. Alternatively if they want to stick to relative positioning, then a 1% bottom margin could be used instead of bottom position.

For a relatively positioned element

The behaviour should be:

  • The height should be determined ignoring top and bottom entirely (in this case stretching to 100% height)
  • The element's "static position" is determined using it's size/alignment (still ignoring top/bottom)
  • top and bottom then offset the position relative to the static position
  • If top and bottom are both set then top takes precedence and bottom is ignored.

However, there seems to be a bug in Taffy that applies both top and bottom to relatively positioned elements. Thus the node in this example is first being offset by 0px in a downwards direction (which has no effect) and then also being offset by 1% in an upwards direction (causing the result we're seeing).

Confirmed against Taffy's main branch using the following test snippet:

<div id="test-root" style="height: 100px; width: 100px;">
  <div style="width: 40px;top: 0px;bottom:10%"></div>
</div>

@ickshonpe
Copy link
Contributor

ickshonpe commented Feb 3, 2023

With PositionType::Absolute it works correctly in main.
In Bevy 9.1 there is a bug in the text node size calculations, #6825
The brown background color shouldn't be filling the entire window. It should only be as wide as the text.

@alice-i-cecile
Copy link
Member

Wait, cross-repo and even cross-org "closes" works? I guess since I have permissions? Anyways, this isn't actually fixed until we release taffy 0.3 and then migrate Bevy to it.

@dvogel
Copy link
Author

dvogel commented Feb 3, 2023

I can confirm that the absolutely positioned variant with an explicit size field does work as I expected it to:

pub fn inject_ui_entities(asset_server: Res<AssetServer>, mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());
    commands.spawn((
        TextBundle {
            text: Text::from_section(
                "Hello world!",
                TextStyle {
                    font: asset_server.load("fonts/FiraSans-Regular.ttf"),
                    font_size: 30.0,
                    color: Color::WHITE,
                },
            ),
            style: Style {
                position_type: PositionType::Absolute,
                size: Size {
                    width: Val::Auto,
                    height: Val::Percent(100.0),
                },
                ..default()
            },
            transform: Transform::from_xyz(0.0, 0.0, 0.0),
            ..default()
        },
        BackgroundColor(Color::rgba(164.0 / 256.0, 116.0 / 256.0, 73.0 / 256.0, 0.5)),
    ));
}

I feel a little silly here as my use of Val::Percent(1.0) was intended to mean 100% as if the valid range was [0.0, 1.0] rather than [0.0, 100.0]. Nevertheless this appears to have led to ancillary improvements so bravo! 😄

@nicoburns
Copy link
Contributor

I feel a little silly here as my use of Val::Percent(1.0) was intended to mean 100% as if the valid range was [0.0, 1.0] rather than [0.0, 100.0].

That actually is how it works in the underlying Taffy library. So it's understandable that you'd be confused. This probably ought to be documented better!

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 A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior
Projects
None yet
6 participants