Remove custom rounding #16097

ickshonpe · 2024-10-25T09:12:04Z

Objective

Taffy added layout rounding a while ago but it had a couple of bugs and caused some problems with the fussy ab_glyph text implementation. So I disabled Taffy's builtin rounding and added some hacks ad hoc that fixed (some) of those issues. Since then though Taffy's rounding algorithm has improved while we've changed layout a lot and migrated to cosmic-text so those hacks don't help any more and in some cases cause significant problems.

Also our rounding implementation only rounds to the nearest logical pixel, whereas Taffy rounds to the nearest physical pixel meaning it's much more accurate with high dpi displays.

fixes #15197

Some examples of layout rounding errors visible in the UI examples

These errors are much more obvious at high scale factor, you might not see any problems at a scale factor of 1.

cargo run --example text_wrap_debug

The narrow horizontal and vertical lines are gaps in the layout caused by errors in the coordinate rounding.

cargo run --example text_debug

The two text blocks here are aligned right to the same boundary but in this screen shot you can see that the lower block is one pixel off to the left. Because the size of this text node changes between frames with the reported framerate the rounding errors cause it to jump left and right.

Solution

Remove all our custom rounding hacks and reenable Taffy's layout rounding.

The gaps in the text_wrap_debug example are gone:

This doesn't fix some of the gaps that occur between borders and content but they seem appear to be a rendering problem as they disappear with UiAntiAlias::Off set.

Testing

Run the examples as described above in the Objective section. With this PR the problems mentioned shouldn't appear.

Also added an example in a separate PR #16096 layout_rounding_debug for identifying these issues.

Migration Guide

UiSurface::get_layout now also returns the final sizes before rounding. Call .0 on the Ok result to get the previously returned taffy::Layout value.

nicoburns

Makes sense to me

…/n` to `1 - 1/n` and now it passes. `ui_rounding_test` was failing after the layout rounding changes but I don't think this is a problem. It only fails at an extremely low scale factor (below ~0.14) but scale factors below 1 aren't common or particularly useful. Even if the node sizes are inaccurate, at that scale a lot of the detail is lost anyway.

ickshonpe · 2024-10-25T10:37:24Z

ui_rounding_test was failing after the layout rounding changes but it only fails at extremely low scale factors (below ~0.14). At that scale factor with 7 logical pixels to 1 physical pixel not much is going to be visable anyway, so I don't think it's worth worrying about. I set the test to start at 1 - 1/n instead of 1/n scale factor and now it passes.

crates/bevy_ui/src/layout/mod.rs

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

alice-i-cecile · 2024-10-27T19:09:13Z

@ickshonpe CI is failing on a related test: can you take a look? I suspect the test itself is wrong.

ickshonpe · 2024-10-27T19:10:40Z

Yep I'll fix it now

…ith sf below 1.

BenjaminBrienen · 2024-10-27T19:30:08Z

crates/bevy_ui/src/layout/mod.rs

@@ -351,12 +351,10 @@ with UI components as a child of an entity without UI components, your UI layout

            absolute_location += layout_location;

-            let rounded_size = approx_round_layout_coords(absolute_location + layout_size)
-                - approx_round_layout_coords(absolute_location);
+            let rounded_size = (absolute_location + layout_size) - (absolute_location);


Parentheses are redundant

alice-i-cecile · 2024-10-27T20:24:46Z

@ickshonpe CI failure is pointing out a real, if minor issue :) Ping me once it's resolved!

…pdate_uinode_geometry_recursive`.

…/bevy into remove-custom-rounding

ickshonpe · 2024-10-27T21:02:02Z

Removing the unrounded size is a breaking change, but I'm okay if the migration guide is just "please tell us why you wanted this".

Couldn't decide what to say in the migration guide, so went back and worked out a really wretched method to retrieve the unrounded size from Taffy. I guess it's better this way though, there don't seem to be any problems anymore but it feels safer to stick with using the unrounded size for text.

ickshonpe · 2024-10-27T21:02:28Z

@ickshonpe CI failure is pointing out a real, if minor issue :) Ping me once it's resolved!

Fixed

crates/bevy_ui/src/layout/ui_surface.rs

alice-i-cecile

Small docs and/or quality request, then this LGTM :)

BenjaminBrienen

I noticed a small inconsistency with unwrap vs expect in the tests, but other than that, still good.

BenjaminBrienen · 2024-10-28T19:46:50Z

crates/bevy_ui/src/layout/mod.rs


            let layout = ui_surface
                .get_layout(ui_node_entity)
-                .expect("failed to get layout");
+                .expect("failed to get layout")


expect() here

BenjaminBrienen · 2024-10-28T19:47:00Z

crates/bevy_ui/src/layout/mod.rs

-        let ui_surface = world.resource::<UiSurface>();
-        let layout = ui_surface.get_layout(ui_entity).unwrap();
+        let mut ui_surface = world.resource_mut::<UiSurface>();
+        let layout = ui_surface.get_layout(ui_entity).unwrap().0;


unwrap() here

# Objective Taffy added layout rounding a while ago but it had a couple of bugs and caused some problems with the fussy `ab_glyph` text implementation. So I disabled Taffy's builtin rounding and added some hacks ad hoc that fixed (some) of those issues. Since then though Taffy's rounding algorithm has improved while we've changed layout a lot and migrated to `cosmic-text` so those hacks don't help any more and in some cases cause significant problems. Also our rounding implementation only rounds to the nearest logical pixel, whereas Taffy rounds to the nearest physical pixel meaning it's much more accurate with high dpi displays. fixes #15197 ## Some examples of layout rounding errors visible in the UI examples These errors are much more obvious at high scale factor, you might not see any problems at a scale factor of 1. `cargo run --example text_wrap_debug` <img width="1000" alt="text_debug_gaps" src="https://github.com/user-attachments/assets/5a584016-b8e2-487b-8842-f0f359077391"> The narrow horizontal and vertical lines are gaps in the layout caused by errors in the coordinate rounding. `cargo run --example text_debug` <img width="1000" alt="text_debug" src="https://github.com/user-attachments/assets/a4b37c02-a2fd-441c-a7bd-cd7a1a72e7dd"> The two text blocks here are aligned right to the same boundary but in this screen shot you can see that the lower block is one pixel off to the left. Because the size of this text node changes between frames with the reported framerate the rounding errors cause it to jump left and right. ## Solution Remove all our custom rounding hacks and reenable Taffy's layout rounding. The gaps in the `text_wrap_debug` example are gone: <img width="1000" alt="text_wrap_debug_fix" src="https://github.com/user-attachments/assets/92d2dd97-30c6-4ac8-99f1-6d65358995a7"> This doesn't fix some of the gaps that occur between borders and content but they seem appear to be a rendering problem as they disappear with `UiAntiAlias::Off` set. ## Testing Run the examples as described above in the `Objective` section. With this PR the problems mentioned shouldn't appear. Also added an example in a separate PR #16096 `layout_rounding_debug` for identifying these issues. ## Migration Guide `UiSurface::get_layout` now also returns the final sizes before rounding. Call `.0` on the `Ok` result to get the previously returned `taffy::Layout` value. --------- Co-authored-by: Rob Parrett <robparrett@gmail.com>

ickshonpe added 3 commits October 23, 2024 10:08

Enable taffy's builtin layout coords rounding

e2b1cb7

Removed the coordinate rounding from ui_layout_system.

275a06b

Merge branch 'main' into remove-custom-rounding

4567842

ickshonpe requested a review from nicoburns October 25, 2024 09:12

nicoburns approved these changes Oct 25, 2024

View reviewed changes

ickshonpe mentioned this pull request Oct 25, 2024

UI Layout Or Size Can Seem To Shift Unnecessarily Post-Creation #15826

Open

ickshonpe added A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 25, 2024

rparrett reviewed Oct 25, 2024

View reviewed changes

crates/bevy_ui/src/layout/mod.rs Show resolved Hide resolved

ickshonpe and others added 2 commits October 25, 2024 15:27

Update crates/bevy_ui/src/layout/mod.rs

536b784

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

Merge branch 'main' into remove-custom-rounding

22fb21f

alice-i-cecile added this to the 0.15 milestone Oct 27, 2024

alice-i-cecile approved these changes Oct 27, 2024

View reviewed changes

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 27, 2024

ickshonpe added 4 commits October 27, 2024 19:23

Modified ui_rounding_text so it passes, accuracy not so important w…

e48b4d8

…ith sf below 1.

Merge branch 'main' into remove-custom-rounding

2d1c9e5

Merge branch 'main' into remove-custom-rounding

e0c651a

Merge branch 'main' into remove-custom-rounding

b9985fb

BenjaminBrienen approved these changes Oct 27, 2024

View reviewed changes

ickshonpe added 2 commits October 27, 2024 19:38

Clean up, removed unrounded_size field from Node

5766f43

Removed too many brackets

7b96e5c

alice-i-cecile approved these changes Oct 27, 2024

View reviewed changes

alice-i-cecile enabled auto-merge October 27, 2024 19:45

alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Oct 27, 2024

BenjaminBrienen approved these changes Oct 27, 2024

View reviewed changes

Brought back unrounded sizes

9d80d38

auto-merge was automatically disabled October 27, 2024 20:48
Head branch was pushed to by a user without write access

ickshonpe added 3 commits October 27, 2024 20:49

Merge branch 'main' into remove-custom-rounding

321f3a6

Removed the no longer necessary absolute_location parameter from `u…

faf5965

…pdate_uinode_geometry_recursive`.

Merge branch 'remove-custom-rounding' of https://github.com/ickshonpe…

b635211

…/bevy into remove-custom-rounding

alice-i-cecile reviewed Oct 27, 2024

View reviewed changes

crates/bevy_ui/src/layout/ui_surface.rs Show resolved Hide resolved

alice-i-cecile requested changes Oct 27, 2024

View reviewed changes

ickshonpe added 2 commits October 28, 2024 17:06

Comments and a few renamings of local variables for clarity.

d58f037

Merge branch 'main' into remove-custom-rounding

f2d11a8

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 28, 2024

alice-i-cecile approved these changes Oct 28, 2024

View reviewed changes

alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Oct 28, 2024

BenjaminBrienen approved these changes Oct 28, 2024

View reviewed changes

Merge branch 'main' into remove-custom-rounding

cab2a2a

alice-i-cecile enabled auto-merge October 28, 2024 22:15

alice-i-cecile added this pull request to the merge queue Oct 28, 2024

Merged via the queue into bevyengine:main with commit 05d9068 Oct 28, 2024
29 checks passed

eero-lehtinen mentioned this pull request Nov 3, 2024

UI rounding is applied before UI scaling #14714

Closed

rparrett mentioned this pull request Nov 4, 2024

ui_scaling example is jittery #11207

Closed

DGriffin91 mentioned this pull request Nov 10, 2024

many_buttons performance regression #16318

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove custom rounding #16097

Remove custom rounding #16097

ickshonpe commented Oct 25, 2024 •

edited by alice-i-cecile

Loading

nicoburns left a comment

ickshonpe commented Oct 25, 2024 •

edited

Loading

alice-i-cecile commented Oct 27, 2024

ickshonpe commented Oct 27, 2024

BenjaminBrienen Oct 27, 2024

alice-i-cecile commented Oct 27, 2024

ickshonpe commented Oct 27, 2024 •

edited

Loading

ickshonpe commented Oct 27, 2024

alice-i-cecile left a comment

BenjaminBrienen left a comment

BenjaminBrienen Oct 28, 2024

BenjaminBrienen Oct 28, 2024

Remove custom rounding #16097

Remove custom rounding #16097

Conversation

ickshonpe commented Oct 25, 2024 • edited by alice-i-cecile Loading

Objective

Some examples of layout rounding errors visible in the UI examples

Solution

Testing

Migration Guide

nicoburns left a comment

Choose a reason for hiding this comment

ickshonpe commented Oct 25, 2024 • edited Loading

alice-i-cecile commented Oct 27, 2024

ickshonpe commented Oct 27, 2024

BenjaminBrienen Oct 27, 2024

Choose a reason for hiding this comment

alice-i-cecile commented Oct 27, 2024

ickshonpe commented Oct 27, 2024 • edited Loading

ickshonpe commented Oct 27, 2024

alice-i-cecile left a comment

Choose a reason for hiding this comment

BenjaminBrienen left a comment

Choose a reason for hiding this comment

BenjaminBrienen Oct 28, 2024

Choose a reason for hiding this comment

BenjaminBrienen Oct 28, 2024

Choose a reason for hiding this comment

ickshonpe commented Oct 25, 2024 •

edited by alice-i-cecile

Loading

ickshonpe commented Oct 25, 2024 •

edited

Loading

ickshonpe commented Oct 27, 2024 •

edited

Loading