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

blend_modes example: fix label position #8454

Merged
merged 3 commits into from
Jun 26, 2023

Conversation

mockersf
Copy link
Member

Objective

  • Labels are not correctly placed

Screenshot 2023-04-22 at 00 12 54

Solution

  • Set a width in the UI so that text doesn't try to wrap

Screenshot 2023-04-22 at 00 13 04

@mockersf mockersf added the C-Examples An addition or correction to our examples label Apr 21, 2023
@cart
Copy link
Member

cart commented Apr 26, 2023

Is this working around some sort of regression? I think what-would-be-single-line text wrapping is undesirable / buggy. People shouldn't need to anticipate the width of their text labels to avoid broken wrapping.

@mockersf mockersf force-pushed the blend-modes-example-label-position branch from 876b829 to 5e11e2b Compare April 26, 2023 21:16
@mockersf
Copy link
Member Author

This is a change since #7779... I'm not sure if it's a normal case as the text is full of \n.

@ickshonpe what do you think?

Also, the example is now "broken" on main as I switched it to the default font but it uses non ascii characters, I added back the font in this PR

@ickshonpe
Copy link
Contributor

This is a change since #7779... I'm not sure if it's a normal case as the text is full of \n.

@ickshonpe what do you think?

Also, the example is now "broken" on main as I switched it to the default font but it uses non ascii characters, I added back the font in this PR

There were a whole bunch of cases where lines would get wrapped incorrectly because of rounding problems, I thought I'd caught them all though. Maybe it's because this example is using absolute positioning which I didn't do as much testing with, but I'm not sure why that would make a difference. I'll run a few tests to see what is happening internally.

@ickshonpe
Copy link
Contributor

ickshonpe commented Apr 28, 2023

Yep with absolute positioning it's using the minimum content width. There isn't any need to calculate a size from the text, instead set the right style value of the node wrapping the text to any value other than Auto. The right position will be ignored as left position values override right when the text direction is left-to-right, This sets the node to fill the available horizontal space.

Minimal reproduction:

absolute_positioning_text

use bevy::prelude::*;

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

fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());
    for (i, (position_type, message, right)) in [
        (
            PositionType::Relative,
            "Relative Positioning\nRelative Positioning",
            Val::Auto,
        ),
        (
            PositionType::Absolute,
            "Absolute Positioning\nAbsolute Positioning",
            Val::Auto,
        ),
        (
            PositionType::Absolute,
            "Absolute Positioning\nAbsolute Positioning",
            Val::Px(0.),
        ),
    ]
    .into_iter()
    .enumerate()
    {
        let x = i as f32 * 150.;
        commands
            .spawn(NodeBundle {
                style: Style {
                    position_type: PositionType::Absolute,
                    top: Val::Px(10.),
                    left: Val::Px(10. + x),
                    right,
                    ..default()
                },
                background_color: Color::MAROON.into(),
                ..default()
            })
            .with_children(|builder| {
                builder.spawn(
                    TextBundle::from_section(message, TextStyle::default())
                        .with_style(Style {
                            position_type,
                            ..default()
                        })
                        .with_background_color(Color::DARK_GREEN),
                );
            });
    }
}

https://github.com/ickshonpe/text_wrap_blend_bug

@ickshonpe
Copy link
Contributor

ickshonpe commented Apr 28, 2023

I'm not sure that this is a bug exactly, even though it deviates from the HTML + CSS behaviour for text. More like a not-yet-implemented feature.

It's not really related to #7779 either. It's just that before #7779, Bevy always used the max-content width for text regardless of all constraints except local width and height Px values.

@cart
Copy link
Member

cart commented Apr 28, 2023

It's not really related to #7779 either. It's just that before #7779, Bevy always used the max-content width for text regardless of all constraints except local width and height Px values.

Gotcha: seems like the "missing feature" is that text should not have a minimum width of zero when its parent container doesn't have an explicit width (and is absolutely positioned). Instead the parent container should expand with the size of the text.

image

@ickshonpe
Copy link
Contributor

ickshonpe commented Apr 29, 2023

Maybe I was wrong and this is a bug, experimented some more and found some other cases where the calculated available space seems obviously incorrect.

Here is a simple one:

use bevy::prelude::*;
use bevy::ui::AvailableSpace;
use bevy::ui::CalculatedSize;
use bevy::ui::Measure;

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

#[derive(Clone)]
pub struct TestMeasure;

impl Measure for TestMeasure {
    fn measure(
        &self,
        _: Option<f32>,
        _: Option<f32>,
        _: AvailableSpace,
        _: AvailableSpace,
    ) -> Vec2 {
        Vec2::new(500., 0.)
    }

    fn dyn_clone(&self) -> Box<dyn Measure> {
        Box::new(self.clone())
    }
}


fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());
        commands.spawn((NodeBundle {
            style: Style {
                max_size: Size::width(Val::Px(0.)),
                ..Default::default()
            },
            background_color: Color::GREEN.into(),
            ..default()
        },
        CalculatedSize { measure: Box::new(TestMeasure) }
    ));
    commands.spawn(NodeBundle {
        style: Style {
            size: Size::width(Val::Percent(100.)),
            ..Default::default()
        },
        background_color: Color::RED.into(),
        ..Default::default()
    });
}

Capture-size

The green NodeBundle with the calculated size has a max-width constraint of 0., so it takes up no space in the layout as expected. But the red NodeBundle doesn't grow to fill 100% of the width of the window and leaves an empty space.

@mockersf mockersf force-pushed the blend-modes-example-label-position branch from 5e11e2b to 7038aa0 Compare June 26, 2023 17:30
@mockersf
Copy link
Member Author

now using the no wrap feature of text from #8947

@mockersf mockersf added this to the 0.11 milestone Jun 26, 2023
@cart cart added this pull request to the merge queue Jun 26, 2023
Merged via the queue into bevyengine:main with commit 8fa94a0 Jun 26, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Examples An addition or correction to our examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants