UI leaf node styling and scrollbars#19344
UI leaf node styling and scrollbars#19344andrewzhurov wants to merge 12 commits intobevyengine:mainfrom
Conversation
Content now is rather lengthy, so it's put in a sclobbale container. typo
As presently the meant-to-be-scrollable list displays in full on a not-that-large screen. tweak scroll
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
…; scrollbars! switch from internal import
a6a3dd4 to
e976b54
Compare
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
Tuffy provides us content size that is stripped from border scroll padding sizes - available_width available_height. We need to figure out whether it's a hard constraint or not. If size styles are Auto - image need not scale to fit if it's less than available_size. So rendered size is min of image size and available size. If size is explicitly set via styles, then we scale image accordingly. This is similar to CSS's object-fit: contain; Which may not be expected as default, as I think of it.
ickshonpe
left a comment
There was a problem hiding this comment.
I haven't had a look at this properly yet and it might be better than I think.
IMO I'm probably against this though, I think it's much simpler to just disallow styling on leaf nodes containing content completely. If users want text or an image with borders and padding they can just wrap them in an intermediate parent Node.
Maybe if we could have some sort of common interface for content that handles recalculating the coordinates that's invisable to the update, clipping, interaction and extraction logics for each content type it might be okay, but that's still a lot of effort and extra complexity that seems unnecessary.
| if let Some(mut calculated_clip) = maybe_calculated_clip { | ||
| if let Some(inherited_clip) = maybe_inherited_clip { | ||
| // Replace the previous calculated clip with the inherited clipping rect | ||
| match (maybe_inherited_clip, maybe_calculated_clip) { | ||
| (Some(inherited_clip), None) => { | ||
| commands.entity(entity).try_insert(CalculatedClip { | ||
| clip: inherited_clip, | ||
| }); | ||
| } | ||
| (Some(inherited_clip), Some(mut calculated_clip)) => { | ||
| if calculated_clip.clip != inherited_clip { | ||
| *calculated_clip = CalculatedClip { | ||
| clip: inherited_clip, | ||
| }; | ||
| } | ||
| } else { | ||
| // No inherited clipping rect, remove the component | ||
| } | ||
| (None, Some(_)) => { | ||
| commands.entity(entity).remove::<CalculatedClip>(); | ||
| } | ||
| } else if let Some(inherited_clip) = maybe_inherited_clip { | ||
| // No previous calculated clip, add a new CalculatedClip component with the inherited clipping rect | ||
| commands.entity(entity).try_insert(CalculatedClip { | ||
| clip: inherited_clip, | ||
| }); | ||
| } | ||
| _ => {} | ||
| }; |
There was a problem hiding this comment.
Just switches from nested ifs to a match
Yeah, that approach seems appealing.
Although that boils down to "add content_inset to content's tf". I modified the borders example to see how it'll look with Screencast.From.2025-05-24.14-13-30.mp4What seems broken:
But that would be relatively easy to fix, I guess. |
|
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
|
I don't have the knowledge to review this PR, but I'm excited that we can finally add layout styling to text nodes! |
|
@alice-i-cecile This PR is partly in conflict with the core scrollbar PR #19803. This PR provides automatic scrollbars with limited customization options. The core scrollbar PR #19803 isn't automatic, but (like other headless widgets) allows the user to fully customize appearance and placement of the scrollbar, which is important to satisfy the visual novelty requirement of games. While I think that some of the issues addressed by this PR might be valid, it's trying to solve too many problems at once; at the very least it should be split up. |
|
I agree with that analysis :) @andrewzhurov, are you open to that? |
|
I like the idea of Scrollbars as Entities. As the other way of having scrollbars only at render-time (as in this PR):
It is the way browsers have it, as scrollbars are not DOM nodes, but that's not an excuse.:) So happy to remove them altogether from this PR, given we want the leaf nodes styling from it. (whether to have automatic scrollbars via |
|
Sounds good. It might be easier to remake the leaf node styling as a separate PR (merge conflicts and a messy discussion history), but I'll leave that call to you. If you change this PR, please update the description and title! |
Objective
Enable layout and rendering of padding and border on UI leaf nodes.
As presently they're being ignored.
Fixes #6879
Fixes #11557
Fixes #17300 (duplicate of the above)
Scale-to-fit leaf node images.
As padding and border on these nodes "eat" available for image space.
Adds layout and render of scrollbars.
Solution
measure_fns make use of Taffy's available_size, which comes with borders, scrollbars and padding stripped out, so available_size for pure content.
known_sizes are disregarded as they include those (so it's a size of border box).
Previously they've been fine to use, as leaf nodes meant to have border_box = pure content size.
Also, images follow CSS's object-fit: contain; and that's not a default behavior in CSS. Default is to stretch. So perhaps that's something to change?
Cache it as ComputedContentClip (as content's clip, and not inherited from parent clip is now necessary for rendering UI leaf nodes)
Scrollbar sizes are
consts. (as CSS does not allow for Px val customization of scrollbar sizes, only coarse "auto" "thin" viascrollbar-width, so Px vals are not exposed to be configurable e.g., via a component)ScrollbarColorcomponent, required fromNodecan be used to specify track and thumb colors for node's scrollbars.ScrollPositionsdoes not work onNodewithflex_directionset toRowReverse#17129 atm.For rendering of a scale-to-fit image I've explored two approaches:
But then I discovered the caveat, when clip cuts some part of an image (e.g., only top part of image is seen when scrolling), GLSL's UVs are not modified, so whole image texture is rendered on top-half rect, resulting in a "squashed" look.
That's the way it's done in PR. Images get clipped correctly (incl. flipped ones, as can be seen in
bordersexample)Eyes wanted on performance of the approach.
Testing
I performed visual testing on
bordersscrollandgame_menuexamples.Ubuntu 25.04, X11
Showcase
Before / After
Screencast.From.2025-05-23.13-06-47.mp4
Screencast.From.2025-05-23.13-13-02.mp4
Can be run with
cargo run --example borderson commits:24cc495 (Extend borders example with text and images)
e976b54 (UI: add support for padding...)