bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
35.19k stars 3.47k forks source link

Line Break doesn't clear trailing whitespace #12319

Open TheDudeFromCI opened 6 months ago

TheDudeFromCI commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.

https://github.com/bevyengine/bevy/blob/4cd53cc7e12fb8e4d612fc02b634828cda97af82/crates/bevy_text/src/glyph_brush.rs#L207-L234

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 commented 6 months ago

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 commented 6 months ago

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 https://github.com/bevyengine/bevy/issues/10710.

djeedai commented 2 months ago

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 commented 2 months ago

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

djeedai commented 2 months ago

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 🥲