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

Line Break doesn't clear trailing whitespace #12319

Open
TheDudeFromCI opened this issue Mar 5, 2024 · 8 comments
Open

Line Break doesn't clear trailing whitespace #12319

TheDudeFromCI opened this issue Mar 5, 2024 · 8 comments
Labels
A-Text Rendering and layout for characters A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior

Comments

@TheDudeFromCI
Copy link
Contributor

Bevy version

0.13.0

[Optional] Relevant system information

AdapterInfo { name: "NVIDIA GeForce GTX 1050 Ti", vendor: 4318, device: 7298, device_type: DiscreteGpu, driver: "NVIDIA", driver_info: "545.29.06", backend: Vulkan }
SystemInfo { os: "Linux 23.1.3 Manjaro Linux", kernel: "5.15.148-2-MANJARO", cpu: "Intel(R) Core(TM) i5-4690K CPU @ 3.50GHz", core_count: "4", memory: "31.3 GiB" }

What you did

Create a TextBundle node within a parent NodeBundle of a fixed size. The parent node centers the child node using align_items, align_content, justify_items, and justify_content. The text should contain several words and be set to line break on word boundry. The text is set to justify center.

What went wrong

When the line break occurs, the white space char is kept in the same position. This causes the renderer to include this char in the bounding box and thus shifting the center alignment of the text.

Additional information

This will cause the text to be off-center. The effect can clearly be seen with background colors enabled.

Screenshot_20240305_053132

As you can see in the image above, the space between "Streamline" and "UI" is kept in place, causing the text to be shifted by half a char width to the left.

@TheDudeFromCI TheDudeFromCI added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Mar 5, 2024
@TheDudeFromCI
Copy link
Contributor Author

For comparison, here is what the text looks like when manually replacing the space with a new line character. As you can see in this image, the text is properly centered.

Screenshot_20240305_054212

@rparrett rparrett added A-UI Graphical user interfaces, styles, layouts, and widgets A-Text Rendering and layout for characters and removed S-Needs-Triage This issue needs to be labelled labels Mar 5, 2024
@rparrett
Copy link
Contributor

rparrett commented Mar 5, 2024

I suspect the issue would be on Bevy's end rather than glyph-brush. We are probably not filtering glyphs without outlines when calculating text dimensions or something.

@TheDudeFromCI
Copy link
Contributor Author

I found the areas where this calculation is performed. The top-most function is where the bounding box calculation itself it done, adding the size of the character to the bounding box. These characters are not filtered by type, so all characters are counted.

/// Computes the minimal bounding rectangle for a block of text.
/// Ignores empty trailing lines.
pub(crate) fn compute_text_bounds<T>(
section_glyphs: &[SectionGlyph],
get_scaled_font: impl Fn(usize) -> PxScaleFont<T>,
) -> Rect
where
T: ab_glyph::Font,
{
let mut text_bounds = Rect {
min: Vec2::splat(f32::MAX),
max: Vec2::splat(f32::MIN),
};
for sg in section_glyphs {
let scaled_font = get_scaled_font(sg.section_index);
let glyph = &sg.glyph;
text_bounds = text_bounds.union(Rect {
min: Vec2::new(glyph.position.x, 0.),
max: Vec2::new(
glyph.position.x + scaled_font.h_advance(glyph.id),
glyph.position.y - scaled_font.descent(),
),
});
}
text_bounds
}

I imagine simply skipping invisible characters is a valid solution to this problem, right? Spaces between words would not affect the bounding box, while prefixing and trailing white space would be properly pruned.

The SectionGlyph struct provides a pointer to the TextSection and the byte index of the character in that text. Using text.as_bytes()[] and grabbing the encoded UTF-8 char out, I imagine it would be easy to test if it's one of a few specific white space characters and skip?

@rparrett
Copy link
Contributor

rparrett commented Mar 6, 2024

I imagine simply skipping invisible characters is a valid solution to this problem, right?

I am not sure anymore. Imagine the string " ".

Either way, I think the most reliable way of determining if a glyph is visible would be "did outline_glyph return Some for that glyph?"

I am starting to think that maybe the linebreaker should be removing these spaces.

@TheDudeFromCI
Copy link
Contributor Author

I agree. Filtering white spaces manually would also remove the leading spaces people would use for purposes such as indention.

The line breaker algorithm provided by glyph_brush has several other bugs as well, such as #10710.

@djeedai
Copy link
Contributor

djeedai commented Jun 24, 2024

I'm hitting the exact same issue in my custom text pipeline. The code linked is indeed the issue, and indeed the problem is that we only look at the advance without knowing if the character is visible (has some outline; outline_glyph() returned Some) so we will count the width of the blank space even if it's located at an edge. I'm not sure yet how to re-organize the code to have access to that info by the time we calculate the size.

My custom text renderer with right justify and center left anchor (blue cross); the small padding on the left is due to the fact the calculated text size accounts for the invisible space at the end of the first line (which is not even counted in the layouting as you can see, "as" and "can" are aligned, but nevertheless I use the same code as Bevy to calculate the width and it comes up too large):

Screenshot 2024-06-24 at 08 47 53

Here are all the combination of justify / anchor, where you can clearly spot the issue (in my own custom text renderer again, but the code of compute_text_bounds() is the same):

Screenshot 2024-06-24 at 08 51 53

You can note that when using left justified text we don't have the same problem. And when using center justifying we only have "half the problem". This is because glyph_brush_layout has some unexpected behavior where it conflates text justifying and alignment (if you pass left align, it also left justifies; same for center and right), and therefore Bevy only uses it to justify, and later recalculate the alignment after the fact (which is where compute_text_bounds() comes into play). I do the same above. So Bevy passes the text justifying as the horizontal alignment, which makes glyph_brush_layout return glyphs relative to a different point depending on that justifying (if you justify left, the crate returns positions relative to the left edge, and conversely if you justify right it returns positions relative to the right edge). So the blank space is accounted for on a different side depending on the justifying and alignment, which sometimes is invisible (left/left) and sometimes adds extra padding (left/right).

@rparrett
Copy link
Contributor

This seems to have been fixed by #10193. Could someone confirm?

@djeedai
Copy link
Contributor

djeedai commented Jul 14, 2024

I won't be able to for a while because I use ab_glyph directly, plus a few lines copied from Bevy. So I need to do my own port to Cosmic to confirm 🥲

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Text Rendering and layout for characters 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

3 participants