Conversation
crates/bevy_ui/src/ui_node.rs
Outdated
| #[derive(Component, Debug, Clone, Reflect)] | ||
| #[reflect(Component, Default)] | ||
| pub struct ScrollPosition { | ||
| /// How far across the node is scrolled (0 = not scrolled / scrolled to right) |
There was a problem hiding this comment.
These docs should clearly describe the units.
crates/bevy_ui/src/ui_node.rs
Outdated
| } | ||
| } | ||
|
|
||
| /// The scroll position on the node |
There was a problem hiding this comment.
A bit more context about how this is used would be nice.
examples/ui/scroll.rs
Outdated
| @@ -0,0 +1,388 @@ | |||
| //! This example illustrates scrolling in Bevy UI. | |||
| //! | |||
alice-i-cecile
left a comment
There was a problem hiding this comment.
Really pleased to see this adopted, and this is looking good. Just a few small suggestions.
examples/ui/scroll.rs
Outdated
| ) { | ||
| for mouse_wheel_event in mouse_wheel_events.read() { | ||
| let (mut dx, mut dy) = match mouse_wheel_event.unit { | ||
| MouseScrollUnit::Line => (mouse_wheel_event.x * 20., mouse_wheel_event.y * 20.), |
There was a problem hiding this comment.
Shouldn't this 20 scale factor be pulled out into a constant and used for the font size?
7380ef2 to
611059e
Compare
…le added, remove unused Scroll system set
611059e to
d9db2af
Compare
There was a problem hiding this comment.
Agree that a one-frame delay is completely unacceptable. I'm worried that the changes here introduce too much unnecessary extra complexity to ui_layout_system though and I really don't like having to query for Style again in the update_uinode_geometry_recursive function.
I've not gone through the PR in detail though, maybe there's no other way. I'll try and find time to take a longer look this weekend or on Monday.
43bec95 to
2565183
Compare
Cool, we are aligned. I've greatly reduced the complexity thanks to @nicoburns. Regarding |
crates/bevy_ui/src/layout/mod.rs
Outdated
| Vec2::new( | ||
| if style.overflow.x == OverflowAxis::Scroll { | ||
| scroll_pos.offset_x | ||
| } else { | ||
| 0.0 | ||
| }, | ||
| if style.overflow.y == OverflowAxis::Scroll { | ||
| scroll_pos.offset_y | ||
| } else { | ||
| 0.0 | ||
| }, | ||
| ) |
There was a problem hiding this comment.
This logic seems correct and necessary to me, but perhaps is_scrollable in each axis could be copied somewhere else ahead of time if accessing Style here is undesirable.
alice-i-cecile
left a comment
There was a problem hiding this comment.
Much simpler. Good recommendations @nicoburns!
|
Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1693 if you'd like to help out. |

Objective
Overflow::Scroll?) #8074Overflow::Scroll) inbevy_ui#8104Solution
Adapted from #8104 and affords the same benefits.
Additions
Omissions
bevy_ui. Users should updateScrollPositiondirectly.Implementation
Adds a new
ScrollPositioncomponent. Updating this component on aNodewith an overflow axis set toOverflowAxis::Scrollwill reposition its children by that amount when calculating node transforms. As before, no impact on the underlying Taffy layout.Calculating this correctly is trickier than it was in #8104 due to
"Update scrolling on relayout".Background
When
ScrollPositionis updated directly by the user, it can be trivially handled in-engine by adding the parent's scroll position to the final location of each child node. However, other layout actions may result in a situation whereScrollPositionneeds to be updated. Consider a 1000 pixel tall vertically scrolling list of 100 elements, each 100 pixels tall. Scrolled to the bottom, theScrollPosition.offset_yis 9000, just enough to display the last element in the list. When removing an element from that list, the new desiredScrollPosition.offset_yis 8900, but, critically, that is not known until after the sizes and positions of the children of the scrollable node are resolved.All user scrolling code today handles this by delaying the resolution by one frame. One notable disadvantage of this is the inability to support
WinitSettings::desktop_app(), since there would need to be an input AFTER the layout change that caused the scroll position to update for the results of the scroll position update to render visually.I propose the alternative in this PR, which allows for same-frame resolution of scrolling layout.
Resolution
When recursively iterating the children of a node, each child now returns a
Vec2representing the location of their own bottom right corner. Then,[[0,0, [x,y]]represents a bounding box containing the scrollable area filled by that child. Scrollable parents aggregate those areas into the bounding box of all children, then consider that result againstScrollPositionto ensure its validity.In the event that resolution of the layout of the children invalidates the
ScrollPosition(e.g. scrolled further than there were children to scroll to), all children of that node must be recursively repositioned. The position of each child must change as a result of the change in scroll position.Therefore, this implementation takes care to only spend the cost of the "second layout pass" when a specific node actually had a
ScrollPositionforcibly updated by the layout of its children.Luckily, this is exactly what Taffy's
Layout::content_sizedoes. So we use it here to avoid this "second pass" entirely!Testing
Examples in
ui/scroll.rs. There may be more complex node/style interactions that were unconsidered.Showcase
Alternatives
bevy_uidoesn't support scrolling.bevy_uiimplements scrolling with a one-frame delay on reactions to layout changes.