Growing UI nodes Fix #8931
Growing UI nodes Fix #8931
Conversation
Taffy's `round_layout` uses a rounding function that rounds away from zero, so x bounds from -100.5 to 99.5 (width 100) will be rounded to -101 to 101 (width 101), gaining a pixel. This commit disables Taffy's builtin rounding with a function that round halfs up rather than rounding them towards zero.
Taffy's builtin rounding is disabled and rounding is now performed by `ui_layout_system`. Instead of rounding the values of the internal layout tree as Taffy's builtin rounding does, we leave those values unmodified and only the values stored in the components are rounded. This requires that we walk the tree for the uinode geometry update rather than iterating through a query. Because the component values are regenerated each update, that should mean that UI updates are idempotent now and make the growing node behaviour seen in issue bevyengine#8911 impossible. Added two helper functions `round_ties_up` and `round_layout_coordinates`.
alice-i-cecile
added
C-Bug
* Added comments explaining the behaviour of the `round_ties_up` function. * Changed the first predicate to use `<=` instead of `<`. This doesn't change the function's behaviour (since `(0.).ceil() == 0.`) but it feels like it makes it a little easier to undertand.
There was a problem hiding this comment.
This fix looks reasonable to me. I can't reproduce this issue locally in Bevy (example in #8911 doesn't exhibit the growing behaviour for me) so I have been unable to test this, but having looked into DioxusLabs/taffy#502 (which I can reproduce), I suspect this PR might fix the issue even if it was using .round() from std rather than round_ties_up.
The key change would be the input and output of the rounding process would no longer be the same location. In which case I believe you'd still end up with the width of the node increasing by 1px during rounding, but it would only do so once and then remain that size.
round_ties_up would still be a reasonable change to make though.
Oh yes just realised, the example for #8911 is flawed. This was something deliberate that I forgot to fix when I made the issue. I have monitors with different scale factors which makes it trivial to detect UI scaling bugs by moving the window from one screen to the other. The example adds a left margin: and my main monitor has a scale factor of
That's correct. At first, I thought this was only a rounding issue but after I went through the calculations step by step it was clear that Taffy's
Yep, I thought |
|
if this bug is fixed in Taffy with DioxusLabs/taffy#501, does it need to also be fixed in Bevy? |
Even if this is fixed in Taffy before 0.11 is out, it still seems to be a bit more efficient to do the rounding ourselves instead of going over all the layout nodes twice, and I think we'll probably need to switch updates to do a depth-first walk of the layout tree for multiple UI layout support anyway. |
`stack` module: * This module has been deleted. * `ui_stack_system` is gone, `ui_layout_system` updates `UiStack`. `UiSystem`: * `Stack` variant removed. `UiStack` resource: * Moved to `ui_node` module. * Updated by `ui_layout_system`. `ZIndex` * The `ZIndex` component is now a new type struct wrapping an i32. The value of `ZIndex` only controls the nodes local z ordering relative to other children with the same parent. * `ZIndex::Global` is no longer supported. `ui_layout_sytem`: * Instead of iterating through a query, the uinode rects (implementation copied from bevyengine#8931) are updated during a depth first walk of the layout tree. * `UiStack` is updated along with the uinodes during the layout tree walk.
Objective
fixes #8911, #7712
Solution
Rounding was added to Taffy which fixed issue #7712.
The implementation uses the f32
roundmethod which rounds ties (fractional part is a half) away from zero. Issue #8911 occurs when a node's min and max bounds on either axis are "ties" and zero is between them. Then the bounds are rounded away from each other, and the node grows by a pixel. This alone shouldn't cause the node to expand continuously, but I think there is some interaction with the way Taffy recomputes a layout from its cached data that I didn't identify.This PR fixes #8911 by first disabling Taffy's internal rounding and using an alternative rounding function that rounds ties up.
Then, instead of rounding the values of the internal layout tree as Taffy's built-in rounding does, we leave those values unmodified and only the values stored in the components are rounded. This requires walking the tree for the UI node geometry update rather than iterating through a query.
Because the component values are regenerated each update, that should mean that UI updates are idempotent (ish) now and make the growing node behaviour seen in issue #8911 impossible.
I expected a performance regression, but it's an improvement on main:
I guess it makes sense to do the rounding together with the node size and position updates.
Changelog
bevy_ui::layout:Taffy's built-in rounding is disabled and rounding is now performed by
ui_layout_system.Instead of rounding the values of the internal layout tree as Taffy's built-in rounding does, we leave those values unmodified and only the values stored in the components are rounded. This requires walking the tree for the UI node geometry update rather than iterating through a query. Because the component values are regenerated each update, that should mean that UI updates are idempotent now and make the growing node behaviour seen in issue Taffy rounding bug can cause UI nodes to expand each update #8911 impossible.
Added two helper functions
round_ties_upandround_layout_coordinates.