alexheretic / glyph-brush

Fast GPU cached text rendering
Apache License 2.0
673 stars 52 forks source link

layout: spaces are positioned incorrectly when soft-breaking right or center aligned text #167

Closed rparrett closed 8 months ago

rparrett commented 8 months ago

Testing 13722c038594fcd768b06a3ff1ca65f382dc598a.

With the right aligned string " H\nHH" constrained in x so that soft-breaking happens, something like this is returned:

  |__    // Vertical bars are a line at x=0.
 H|      // Underscores are spaces
HH|

Where something like this is expected:

__|      // Spaces should be placed right-to-left, like the rest of the text
 H|
HH|

To reproduce:

use glyph_brush_layout::{ab_glyph::*, *};

fn main() {
    let dejavu = FontRef::try_from_slice(include_bytes!("../../fonts/DejaVuSans.ttf")).unwrap();
    let fonts = &[dejavu];

    let text = "  H\nHH";

    let glyphs = Layout::default()
        .h_align(HorizontalAlign::Right)
        .calculate_glyphs(
            fonts,
            &SectionGeometry {
                screen_position: (0.0, 0.0),
                bounds: (0.0, f32::MAX), // force soft-breaking
            },
            &[SectionText {
                text,
                scale: PxScale::from(15.5),
                font_id: FontId(0),
            }],
        );

    for glyph in glyphs {
        let character = &text[glyph.byte_index..glyph.byte_index + 1];
        println!(
            "{:?} @ {},{}",
            character,
            glyph.glyph.position.x.round(),
            glyph.glyph.position.y.round(),
        );
    }
}

This outputs

" " @ 0,12
" " @ 4,12
"H" @ -10,28
"H" @ -20,43
"H" @ -10,43

But I would expect something like

" " @ -20,12
" " @ -16,12
"H" @ -10,28
"H" @ -20,43
"H" @ -10,43

This is causing an interesting layout bug over in bevy: https://github.com/bevyengine/bevy/issues/9395 (more discussion in a workaround PR here: https://github.com/bevyengine/bevy/pull/9415)

alexheretic commented 8 months ago

I think this behaviour occurs because word "trailing whitespace" is ignored for the purposes wrapping. This is because that trailing whitespace is unwanted when soft-wrapping (as it would either bump the word leftwards or the following word rightwards on the next line).

Take the simple example "foo bar" right-aligned. Having the foo word trailing space in the layout looks wrong, we want to ignore it.

# keeping trailing spaces (wrong)
foo |
 bar|
# ignore trailing spaces (correct)
 foo|
 bar|

How this is currently implemented is the line glyphs are shifted left to right-align them ignoring the width of trailing whitespace, but the space glyphs themselves are preserved that's why they fall to the right of the x-bound.

# keeping trailing spaces (wrong)
foo_|
 bar|
# ignore trailing spaces (correct)
 foo|_
 bar|

Multiple spaces are handled the same way, e.g. "foo bar":

 foo|___
 bar|

In your example all lines are a single "word", the first is just 2 trailing spaces. There is currently no special handling for that case.

Hard breaks are handled differently, e.g. "foo \nbar":

foo___|
   bar|

Also see #130 previous discussion.

What behaviour would you expect in these cases?

I think the soft-wrapping behaviour itself is correct. One potential improvement would be to drop the trailing space glyphs instead of preserving them out of bounds. Wdyt?

rparrett commented 8 months ago

That all makes sense. I'm actually not totally clear on why we're having to use individual glyph positions to calculate bounds, but maybe we don't have to do that.

One potential improvement would be to drop the trailing space glyphs instead of preserving them out of bounds.

I think that would be totally fine and good, assuming the first line is special. (e.g. " " would still have glyphs)

I'll try to get the other folks who were investigating this issue looped in here, I'm very new to this code. @tigregalis @ickshonpe

alexheretic commented 8 months ago

That's another potential change, having a all whitespace line/word behave differently from trailing whitespace at the end of a word.

I'm actually not totally clear on why we're having to use individual glyph positions to calculate bounds

Yes, also consider that you have SectionGeometry::bounds for the layout already. There are other cases where glyphs may actually escape these bounds, like a small (or zero) width bound and a larger single word. It is up to the renderer to decide how to handle this, cutting off the glyph as it goes oob or just drawing it oob.

Note that bounds are provided in GlyphVertex for vertex generation, so you can guarantee that glyphs don't render oob but cutting them, see opengl example. At this point whitespace chars won't generate a vertex anyway as they are invisible and don't result in one.

If rendering is enforcing the bounds this way you can calculate the "min layout box" (which is what I assume is happening here?) using the glyph bounds as before but clamping to the layout bounds. This would handle right-aligned trailing spaces and large single words.

alexheretic commented 8 months ago

One potential improvement would be to drop the trailing space glyphs instead of preserving them out of bounds.

I've implemented that in #168, however I could imagine this causing issues for some

Take a text editor use case. Previously the layout provide a position for all spaces, this could be used to render the cursor (like in https://github.com/alexheretic/glyph-brush/issues/130#issuecomment-781477924). By omitting the glyphs this would be much more painful. Not being able to reliably have these whitespace chars as glyphs could be an unwelcome change here.

This adds some doubt that this change is correct. Alternatives

tigregalis commented 8 months ago

For context we're attempting to calculate the range of widths of a box of text, for example, as part of a flexbox layout. Text is content-sized, so the UI layout engine needs the minimum size and the maximum size.

Reading through this issue and comparing to browser behaviour I've changed my mind. The behaviour of glyph_brush_layout was correct... the problem is that when we're calculating layout in bevy and needing the min-content-width-size, min-content-width is in fact larger than max-content-width.

Max-width scenario: image

Min-width scenario (size of the text is wider than the visible box, hence the scroll): image image

rparrett commented 8 months ago

I'll close this up since it seems that there's consensus that this is normal behavior. Thanks for the help!