bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Computed border thickness

Open ickshonpe opened this issue 2 years ago • 6 comments

Objective

Precompute UI node's border thickness and store it in a component, instead of calculating it in extracted_uinode_borders.

Reasons:

  • Border values should be computed by the layout algorithm not calculated in the render module. extracted_uinode_borders has to query for the Style and Parent components, and retrieve the UiScale and Window resources to resolve the border values. It should only be responsible for extracting the data and queueing the border geometry for rendering.
  • Border values have to be recomputed every frame (and in the extraction function) rather than only on UI layout updates.
  • We can't easily support alternative UI layout algorithms which calculate border thickness differently if the calculations are performed in the extraction function.
  • It's extremely undesirable to have a mini layout algorithm running in extraction to compute border thickness. As we add more features to Bevy UI and Taffy the border thickness calculations are only going to get more and more complicated too.

Solution

Add a new component ComputedBorderThickness that contains the thickness of the border on each edge in logical pixels. Precompute border thickness in ui_layout_system.

Ideally, border thickness should be returned by Taffy and accessed via the UiSurface::get_layout method.


Changelog

  • New component ComputedBorderThickness.
  • Added ComputedBorderThickness to NodeBundle and ButtonBundle.
  • Border thickness is precomputed by ui_layout_system and stored in the ComputedBorderThickness component instead of being extract_uinode_borders calculating the values.

Migration Guide

ickshonpe avatar Sep 12 '23 20:09 ickshonpe

Can we pick up this difference in our stress tests?

alice-i-cecile avatar Sep 12 '23 21:09 alice-i-cecile

Not really, in main atm UI benchmarks aren't very meaningful because the rendering is so slow and in ui_layout_system we always update all the layout geometry every frame even when there are no changes.

ickshonpe avatar Sep 12 '23 21:09 ickshonpe

I should have been clearer as well, the point of this PR isn't to improve performance. It's about the undesirability of having a mini layout algorithm running in extraction to compute border thickness. As we add more features to Bevy UI and Taffy calculating the border thickness values is only going to get more and more complicated too.

So imo this PR is a big improvement even if it results in a minor performance regression (though there shouldn't be as this change comes from my third-party borders crate for 0.10 where the pre-computed version is faster).

ickshonpe avatar Sep 14 '23 08:09 ickshonpe

I think that having distinct components for each different layout output value is probably untenable. It would be more ergonomic to have the computed values for border thickness, border radius, outline thickness, node size values and node position together in one ComputedLayout component. Then we can add helper methods for picking and intersections etc that work correctly with border radius.

It doesn't need to be considered until we've added outline and border radius support though.

ickshonpe avatar Sep 18 '23 08:09 ickshonpe

Agreed that a single ComputedLayout component is likely the way to go here. FYI the only reason Taffy isn't exposing these values is concerns around bloating memory usage. If we think these are important, then they should probably be added to Taffy's Layout.

nicoburns avatar Sep 18 '23 13:09 nicoburns

@ickshonpe once this is merge-unconflicted I'll merge this in for you :)

alice-i-cecile avatar Jan 04 '24 21:01 alice-i-cecile

This is now exposed by Taffy. Just need to copy it out from there if you want it accessible from the ECS.

nicoburns avatar May 21 '24 22:05 nicoburns

Ahoy Alice, I looked through this and I have to say I don't see the benefit of adding it. It's an extra component and per-frame calculation for something that is 99.9% useless (as in: not used for anything actively). The border size is also calculated by taffy so it could make sense to wire that out into the Node, but again, very niche use cases. In my case it was for aligning the dropdown panels to their buttons, which is only needed when you open one.

That said, what I find lacking is a way to anchor Nodes to each other in various ways, i.e I don't care how wide the border is, I want my absolute-positioned element to line up with it on the outside, border to border which is not possible at the moment since absolutely positioned elements line up on the inside of the border of their containers. CC @Nico Burns: anchoring would also help with rounding issues that could leak pixel-wide gaps, though I understand it's not a flex-box thing

alice-i-cecile avatar May 22 '24 19:05 alice-i-cecile