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

Allow constraining Text in percent of window size #1493

Closed
wants to merge 1 commit into from

Conversation

NiklasEi
Copy link
Member

@NiklasEi NiklasEi commented Feb 21, 2021

resolves #1490

I based the extended constraint calculation on the previous code. But I find the previous and also the current implementation a bit confusing. It does not behave like I would expect it to. Could somebody explain to me why we take min and max constraints but only look at max if it is given? There doesn't seem to be any logic for the text box to be somewhere in between min and max based on e.g. screen size or other content on the screen.

That said, the corresponding issue should be fixed with this PR. If someone else would like to play around with it, I created a minimal Bevy App for testing based on the issue: https://github.com/NiklasEi/bevy_text_wrap_fix_1490

@NiklasEi NiklasEi changed the title check for text constraints given in percent Allow constraining Text in percent of window size Feb 21, 2021
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets labels Feb 21, 2021
@tigregalis
Copy link
Contributor

I think it's an oversight / incomplete implementation. I think the intent is that it works like width, min-width, max-width in css.

Also, Percent should be relative to its container, which may not necessarily be the window.

Now you could rename Percent to ViewPercent (similar to vh and vw) and it would be right.

The challenge is that Text can also be not a UI element though. So not all of these concepts would apply.

@NiklasEi
Copy link
Member Author

I think Text is always UI. Text2D is a different story, but also not relevant for the code changes I did.

Good point with window not always being the container! I will look into that :)

@tigregalis
Copy link
Contributor

tigregalis commented Feb 22, 2021

Ah, sorry, you're right.

With respect to min_size, size and max_size, I believe the correct approach is to independently calculate each of these in terms of actual pixels. Then you take calculated_pixel_size, and clamp it between calculated_pixel_min_size and calculated_pixel_max_size:

/// Defines how min_size, size, and max_size affects the bounds of a text
/// block.
pub fn text_constraint(
    min_size: Val,
    size: Val,
    max_size: Val,
    scale_factor: f64,
    constraint: f32, // parent node's constraint
) -> f32 {
    let calculated_min_size = match min_size {
        Val::Undefined | Val::Auto => 0.0,
        Val::Px(sz) => scale_value(sz, scale_factor),
        Val::Percent(sz) => scale_value((sz / 100.) * constraint, scale_factor),
    };
    let calculated_max_size = match max_size {
        Val::Undefined | Val::Auto => f32::MAX,
        Val::Px(sz) => scale_value(sz, scale_factor),
        Val::Percent(sz) => scale_value((sz / 100.) * constraint, scale_factor),
    };
    let calculated_size = match size {
        Val::Undefined | Val::Auto => f32::MAX,
        Val::Px(sz) => scale_value(sz, scale_factor),
        Val::Percent(sz) => scale_value((sz / 100.) * constraint, scale_factor),
    };
    // min-biased clamp - seems to be how css works: https://jsfiddle.net/hz2ra8sk/
    if calculated_size <= calculated_min_size {
        calculated_min_size
    } else if calculated_size >= calculated_max_size {
        calculated_max_size
    } else {
        calculated_size
    }
}

@NiklasEi NiklasEi marked this pull request as draft May 23, 2021 08:24
@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@DJMcNab DJMcNab removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Aug 7, 2021
@alice-i-cecile
Copy link
Member

Closing in favor of #5502.

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 this pull request may close these issues.

Text wrapping should work with a percent width
5 participants