Store both the rounded and unrounded node size in Node #9923

ickshonpe · 2023-09-25T22:39:23Z

Objective

Text bounds are computed by the layout algorithm using the text's measurefunc so that text will only wrap after it's used the maximum amount of available horizontal space.

When the layout size is returned the layout coordinates are rounded and this sometimes results in the final size of the Node not matching the size computed with the measurefunc. This means that the text may no longer fit the horizontal available space and instead wrap onto a new line. However, no glyphs will be generated for this new line because no vertical space for the extra line was allocated.

fixes #9874

Solution

Store both the rounded and unrounded node sizes in Node.

Rounding is used to eliminate pixel-wide gaps between nodes that should be touching edge to edge, but this isn't necessary for text nodes as they don't have solid edges.

Changelog

Added the rounded_size: Vec2 field to Node.
text_system uses the unrounded node size when computing a text layout.

…that given whatever the constraints are text will only wrap after it's used the maximum amount of available horizontal space. However then when the layout size is returned, before the Node size is set the layout coordinates are rounded, sometimes changing the size of the Node. This means that the text may no longer fit the horizontal available space and wrap onto a new line. But since the layout algorithm hasn't alloted any vertical space for this new line, the text on this new line dissappears. The commit adds a flag `no_rounding` to `ContentSize` that tells the layout algorithm not to round the size of the node, avoiding this problem. Rounding is used eliminate pixel wide gaps between nodes that should be touching edge to edge, but this isn't necessary for text nodes as they don't have solid edges.

Renamed the `no_rounding` field to `rounding` and changed its type to `SizeRounding`.

nicoburns

This makes sense to me. I think the general principle is:

Rounding (pixel-grid alignment) should never affect layout, and should be for rendering only.

For text, I suspect that might mean passing the unrounded value into text layout, and then clipping to the rounded value when rendering.

ickshonpe · 2023-09-26T21:43:22Z

This makes sense to me. I think the general principle is:

Rounding (pixel-grid alignment) should never affect layout, and should be for rendering only.

For text, I suspect that might mean passing the unrounded value into text layout, and then clipping to the rounded value when rendering.

Yeah it needs a lot of thinking about. I also considered just adding both the rounded and unrounded sizes to Node. I was feeling before that it would be a bit redundant and a waste of memory, but maybe it's the better solution.

nicoburns · 2023-09-26T21:46:10Z

In theory, you shouldn't need to perform text layout again once Taffy's layout has completed. But I'm guessing our current text infrastructure doesn't allow us to reuse the result? We ended up having to store both in Taffy, because otherwise we can't cache results from previous layout runs (as they might round differently if their parents lay out differently and they end up in a different absolute position within the viewport).

rparrett · 2023-09-27T15:17:31Z

I tested every example at scale factor 2., and this seems to fix text layout problems with quite a few of them and doesn't seem to introduce any new problems.

The ones that had differences were

display_and_visibility
overflow
text_wrap_debug
overflow_debug
ui
text_debug

For other scale factors, I only tested the examples listed above.

At scale factor 4./3., this seems to fix some text wrapping issues, but also introduces a 1px gap in text_wrap_debug.

Seemingly the same story at 1.0 -- text layout is better, but 1px gap in text_wrap_debug.

Instead, store the both the unrounded and rounded sizes in `Node`.

ickshonpe · 2023-09-27T17:01:13Z

Switched approach to storing both the rounded and unrounded node sizes in Node.

ickshonpe · 2023-09-27T17:04:14Z

I tested every example at scale factor 2., and this seems to fix text layout problems with quite a few of them and doesn't seem to introduce any new problems.

The ones that had differences were
display_and_visibility
overflow
text_wrap_debug
overflow_debug
ui
text_debug
For other scale factors, I only tested the examples listed above.

At scale factor 4./3., this seems to fix some text wrapping issues, but also introduces a 1px gap in text_wrap_debug.

Seemingly the same story at 1.0 -- text layout is better, but 1px gap in text_wrap_debug.

I think the gaps should be gone now with the new approach. Text can use the unrounded bounds for layout calculations but the size of any node containing text will remain rounded, so there shouldn't be any gaps in the layout. How does it look for you now?

examples/ui/text_wrap_debug.rs

Co-authored-by: Rob Parrett <robparrett@gmail.com>

rparrett

Love how simple this version is.

Did some more testing using my collection of screenshot diffing scripts vs the base commit

All examples at SF 2
UI examples at SF 1, 4/3, 5/3

Looks good to me. Minor changes in text wrapping behavior that seem more correct, and seemingly no unintended consequences.

# Objective Text bounds are computed by the layout algorithm using the text's measurefunc so that text will only wrap after it's used the maximum amount of available horizontal space. When the layout size is returned the layout coordinates are rounded and this sometimes results in the final size of the Node not matching the size computed with the measurefunc. This means that the text may no longer fit the horizontal available space and instead wrap onto a new line. However, no glyphs will be generated for this new line because no vertical space for the extra line was allocated. fixes bevyengine#9874 ## Solution Store both the rounded and unrounded node sizes in `Node`. Rounding is used to eliminate pixel-wide gaps between nodes that should be touching edge to edge, but this isn't necessary for text nodes as they don't have solid edges. ## Changelog * Added the `rounded_size: Vec2` field to `Node`. * `text_system` uses the unrounded node size when computing a text layout. --------- Co-authored-by: Rob Parrett <robparrett@gmail.com>

ickshonpe changed the title ~~Make rounding optional for nodes with a ContentSize.~~ Make rounding optional for nodes with a ContentSize Sep 25, 2023

ickshonpe changed the title ~~Make rounding optional for nodes with a ContentSize~~ Optional rounding for nodes with a ContentSize Sep 25, 2023

ickshonpe added 4 commits September 26, 2023 01:11

cargo fmt --all

8996409

added doc comments for new field

01dcd8e

Added a SizeRounding enum with Enabled and Disabled variants.

20c760b

Renamed the `no_rounding` field to `rounding` and changed its type to `SizeRounding`.

cargo fmt

0da99d8

nicoburns approved these changes Sep 26, 2023

View reviewed changes

nicoburns added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets labels Sep 26, 2023

Removed changes that made layout optional rounding.

d085f62

Instead, store the both the unrounded and rounded sizes in `Node`.

ickshonpe changed the title ~~Optional rounding for nodes with a ContentSize~~ Store both the rounded and unrounded node size in Node Sep 27, 2023

cargo fmt

e287641

rparrett reviewed Sep 28, 2023

View reviewed changes

examples/ui/text_wrap_debug.rs Outdated Show resolved Hide resolved

ickshonpe mentioned this pull request Sep 28, 2023

Mixing Text and non-Text content in UIs #9944

Open

Update examples/ui/text_wrap_debug.rs

3d766f7

Co-authored-by: Rob Parrett <robparrett@gmail.com>

rparrett approved these changes Sep 28, 2023

View reviewed changes

nicoburns added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 28, 2023

mockersf approved these changes Sep 28, 2023

View reviewed changes

mockersf added this pull request to the merge queue Sep 28, 2023

Merged via the queue into bevyengine:main with commit edba496 Sep 28, 2023
22 checks passed

cart mentioned this pull request Oct 13, 2023

News: Release 0.12 bevyengine/bevy-website#754

Merged

43 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store both the rounded and unrounded node size in Node #9923

Store both the rounded and unrounded node size in Node #9923

ickshonpe commented Sep 25, 2023 •

edited

Loading

nicoburns left a comment

ickshonpe commented Sep 26, 2023 •

edited

Loading

nicoburns commented Sep 26, 2023

rparrett commented Sep 27, 2023 •

edited

Loading

ickshonpe commented Sep 27, 2023

ickshonpe commented Sep 27, 2023 •

edited

Loading

rparrett left a comment •

edited

Loading

Store both the rounded and unrounded node size in Node #9923

Store both the rounded and unrounded node size in Node #9923

Conversation

ickshonpe commented Sep 25, 2023 • edited Loading

Objective

Solution

Changelog

nicoburns left a comment

Choose a reason for hiding this comment

ickshonpe commented Sep 26, 2023 • edited Loading

nicoburns commented Sep 26, 2023

rparrett commented Sep 27, 2023 • edited Loading

ickshonpe commented Sep 27, 2023

ickshonpe commented Sep 27, 2023 • edited Loading

rparrett left a comment • edited Loading

Choose a reason for hiding this comment

ickshonpe commented Sep 25, 2023 •

edited

Loading

ickshonpe commented Sep 26, 2023 •

edited

Loading

rparrett commented Sep 27, 2023 •

edited

Loading

ickshonpe commented Sep 27, 2023 •

edited

Loading

rparrett left a comment •

edited

Loading