`MeasureFunc` improvements #8402

ickshonpe · 2023-04-16T12:20:22Z

Objective

fixes #8516

Give CalculatedSize a more specific and intuitive name.
MeasureFuncs should only be updated when their CalculatedSize is modified by the systems managing their content.

For example, suppose that you have a UI displaying an image using an ImageNode. When the window is resized, the node's MeasureFunc will be updated even though the dimensions of the texture contained by the node are unchanged.
Fix the CalculatedSize API so that it no longer requires the extra boxing and the dyn_clone method.

Solution

Rename CalculatedSize to ContentSize
Only update MeasureFuncs on CalculatedSize changes.
Remove the dyn_clone method from Measure and move the Measure from the ContentSize component rather than cloning it.
Change the measure_func field of ContentSize to type Option<taffy::node::MeasureFunc>. Add a set method that wraps the given measure appropriately.

Changelog

Renamed CalculatedSize to ContentSize.
Replaced upsert_leaf with a function update_measure that only updates the node's MeasureFunc.
MeasureFuncs are only updated when the ContentSize changes and not when the layout changes.
Scale factor is no longer applied to the size values passed to the MeasureFunc.
Remove the ContentSize scaling in text_system.
The dyn_clone method has been removed from the Measure trait.
Measures are moved from the ContentSize component instead of cloning them.
Added set method to ContentSize that replaces the new function.

Migration Guide

CalculatedSize has been renamed to ContentSize.
The upsert_leaf function has been removed from UiSurface and replaced with update_measure which updates the MeasureFunc without node insertion.
The dyn_clone method has been removed from the Measure trait.
The new function of CalculatedSize has been replaced with the method set.

* Removed function `update_changed` from `flex_node_system` * Added a `Without<Calculated>` query filter so the two changed node queries in `flex_node_system` are mutually exclusive. * Moved the leaf node update loop into the else block of the update-all if statement.

…te-bug

* Replaced `upsert_leaf` with a function `update_measure` that only updates a nodes `MeasureFunc`. * `MeasureFunc`s are only updated when the `CalculatedSize` changes and not when the layout changes. * Scale factor is no longer applied to the size values passed to the measure func. * Remove the `CalculatedSize` scaling in `text_system`.

github-actions · 2023-04-16T17:03:43Z

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

nicoburns · 2023-04-17T16:47:07Z

@ickshonpe If Taffy is being passed scaled physical styles, then don't the measure_func's also need to be working in physical-sizes? Or are these values already scaled?

ickshonpe · 2023-04-17T20:04:15Z

@ickshonpe If Taffy is being passed scaled physical styles, then don't the measure_func's also need to be working in physical-sizes? Or are these values already scaled?

They are already scaled.

nicoburns

This change looks good to me. The only thing that confused me was the name of CalculatedSize, because (at least in the case of text) it does not actually contain a calculated size, but a MeasureFunc. I wonder if it ought to be renamed to MeasureFunction or IntrinsicSize or ContentSize or something like that?

ickshonpe · 2023-04-17T21:02:53Z

Yeah, the name CalculatedSize doesn't make sense anymore. I was going to change it to IntrinsicSize in the text-system-split PR but decided it had enough breaking changes already.

ContentSize might be good, I think users would it find more intuitive than InstrinsicSize or MeasureFunction probably.

crates/bevy_ui/src/layout/mod.rs

mockersf · 2023-04-18T12:24:31Z

there is a small style issue, a merge issue in crates/bevy_ui/src/widget/text.rs, and I would be for renaming. ContentSize sounds good 👍

…alculatedSize`

ickshonpe · 2023-04-18T13:40:52Z

very artificial benchmark:

cargo run --profile stress-test --features trace_tracy --example many_buttons recompute-layout

…ded a new function that wraps a `Measure` into a Taffy `Measurable`. Changed `UiSurface::set_measure` to move the boxed `Measurable` from the ContentSize component to the Taffy measure slotmap without boxing or cloning.

* cargo fmt --all * Removed change detection check from `set_measure`

This reverts commit 96903e5.

ickshonpe · 2023-04-20T17:35:53Z

Since a MeasureFunc doesn't ever need to be reupdated by ui_layout_system, the MeasureFunc can just be moved from the ContentSize component instead of cloning and that annoying dyn_clone method can be got rid of as well.

nicoburns · 2023-04-20T17:40:11Z

Since a MeasureFunc doesn't ever need to be reupdated by ui_layout_system, the MeasureFunc can just be moved from the ContentSize component instead of cloning and that annoying dyn_clone method can be got rid of as well.

How does that interact with change detection on the ContentSize component? Wouldn't moving out of the component change it?

ickshonpe · 2023-04-20T17:53:06Z

Since a MeasureFunc doesn't ever need to be reupdated by ui_layout_system, the MeasureFunc can just be moved from the ContentSize component instead of cloning and that annoying dyn_clone method can be got rid of as well.

How does that interact with change detection on the ContentSize component? Wouldn't moving out of the component change it?

ContentSize isn't queried for changes anymore, ui_layout_system just takes the Measure out of an option.

bypass_change_detection can always be used if it was needed for some reason.

* Removed the `new` function from `ContentSize`. * Changed the `measure_func` field of `ContentSize` to an `Option<taffy::node::MeasureFunc>`. This puts all the `MeasureFunc` wrapping code is in one place and allows us to use an unboxed function as a `MeasureFunc`. * Added `set` and `set_fn` methods to `ContentSize` to replace the `new` function. * Default zero-size measure uses an unboxed closure instead of a boxed `Measure`. * Removed the `FixedMeasure` type, which is no longer needed as an unboxed function is simpler and more efficient.

* Removed `set_fn`. * Returned `FixedMeasure`. * Fixed `default` implementation for `ContentSize`.

ickshonpe · 2023-04-30T14:34:51Z

Also fixes #8516, not sure why. Might be some problem in UiSurface not setting the needs measure or dirty flags correctly.

nicoburns · 2023-04-30T14:42:09Z

@ickshonpe Would you now consider this PR ready for review? I've been waiting for you to stop committing to it to re-review.

ickshonpe · 2023-04-30T15:04:03Z

Yes this is ready to go.

…

On Sun, 30 Apr 2023, 15:42 Nico Burns, ***@***.***> wrote: @ickshonpe <https://github.com/ickshonpe> Would you now consider this PR ready for review? I've been waiting for you to stop committing to it to re-review. — Reply to this email directly, view it on GitHub <#8402 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGVK3LSY4QFUOZ5RX2YSG3TXDZ24XANCNFSM6AAAAAAXACQJGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

nicoburns

This looks good to me. Also seems to work with a quick test locally.

ickshonpe added 5 commits March 30, 2023 15:17

changes:

c59149e

* Removed function `update_changed` from `flex_node_system` * Added a `Without<Calculated>` query filter so the two changed node queries in `flex_node_system` are mutually exclusive. * Moved the leaf node update loop into the else block of the update-all if statement.

fixed unused import

015dd74

Merge branch 'bevyengine:main' into flex-node-system-double-leaf-upda…

95f87a4

…te-bug

Merge branch 'main' into dont-update-measure-funcs-on-layout-updates

eb47e13

ickshonpe mentioned this pull request Apr 16, 2023

Custom MeasureFunc #7730

Closed

james7132 added A-UI Graphical user interfaces, styles, layouts, and widgets C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Apr 16, 2023

james7132 added the C-Performance A change motivated by improving speed, memory usage or compile times label Apr 16, 2023

nicoburns approved these changes Apr 17, 2023

View reviewed changes

Merge branch 'main' into dont-update-measure-funcs-on-layout-updates

8a511aa

mockersf reviewed Apr 18, 2023

View reviewed changes

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

ickshonpe added 4 commits April 18, 2023 13:27

cargo fmt

3236980

Removed left over commented section from merge

09687af

renamed CalculatedSize to ContentSize

38931e4

Changed comments and variables names to refer to ContentSize not `C…

71bd8aa

…alculatedSize`

mockersf approved these changes Apr 18, 2023

View reviewed changes

ickshonpe added 6 commits April 19, 2023 23:27

changes:

96903e5

* cargo fmt --all * Removed change detection check from `set_measure`

Revert "changes:"

6385799

This reverts commit 96903e5.

This reverts commit fb50589.

3849a09

This reverts commit 3849a09.

0f10753

This reverts commit 6385799.

a118920

ickshonpe added 2 commits April 21, 2023 08:40

Added doc comments for the functions in the layout module.

acd290d

ickshonpe changed the title ~~Only update MeasureFuncs on CalculatedSize changes~~ MeasureFunc improvements Apr 21, 2023

ickshonpe added 6 commits April 21, 2023 10:09

changes

47d8510

* Removed `set_fn`. * Returned `FixedMeasure`. * Fixed `default` implementation for `ContentSize`.

fix imports

4a23652

cargo fmt

00b515f

changed Node field name back to calculated_size

4142a78

Merge branch 'main' into dont-update-measure-funcs-on-layout-updates

859528b

cargo fmt

c6c62d2

nicoburns approved these changes May 1, 2023

View reviewed changes

nicoburns added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 1, 2023

nicoburns added this to the 0.11 milestone May 1, 2023

alice-i-cecile added this pull request to the merge queue May 1, 2023

Merged via the queue into bevyengine:main with commit ba532e4 May 1, 2023

This was referenced Jun 28, 2023

News: Release 0.11 (wrong branch ... see #657) bevyengine/bevy-website#656

Closed

News: Release 0.11 bevyengine/bevy-website#657

Merged

Selene-Amanita mentioned this pull request Jul 10, 2023

0.11 Migration: ImageBundle and TextBundle do not implement clone anymore bevyengine/bevy-website#699

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`MeasureFunc` improvements #8402

`MeasureFunc` improvements #8402

ickshonpe commented Apr 16, 2023 •

edited

Loading

github-actions bot commented Apr 16, 2023

nicoburns commented Apr 17, 2023

ickshonpe commented Apr 17, 2023

nicoburns left a comment

ickshonpe commented Apr 17, 2023

mockersf commented Apr 18, 2023 •

edited

Loading

ickshonpe commented Apr 18, 2023

ickshonpe commented Apr 20, 2023 •

edited

Loading

nicoburns commented Apr 20, 2023

ickshonpe commented Apr 20, 2023 •

edited

Loading

ickshonpe commented Apr 30, 2023

nicoburns commented Apr 30, 2023

ickshonpe commented Apr 30, 2023 via email

nicoburns left a comment

MeasureFunc improvements #8402

MeasureFunc improvements #8402

Conversation

ickshonpe commented Apr 16, 2023 • edited Loading

Objective

Solution

Changelog

Migration Guide

github-actions bot commented Apr 16, 2023

nicoburns commented Apr 17, 2023

ickshonpe commented Apr 17, 2023

nicoburns left a comment

Choose a reason for hiding this comment

ickshonpe commented Apr 17, 2023

mockersf commented Apr 18, 2023 • edited Loading

ickshonpe commented Apr 18, 2023

ickshonpe commented Apr 20, 2023 • edited Loading

nicoburns commented Apr 20, 2023

ickshonpe commented Apr 20, 2023 • edited Loading

ickshonpe commented Apr 30, 2023

nicoburns commented Apr 30, 2023

ickshonpe commented Apr 30, 2023 via email

nicoburns left a comment

Choose a reason for hiding this comment

`MeasureFunc` improvements #8402

`MeasureFunc` improvements #8402

ickshonpe commented Apr 16, 2023 •

edited

Loading

mockersf commented Apr 18, 2023 •

edited

Loading

ickshonpe commented Apr 20, 2023 •

edited

Loading

ickshonpe commented Apr 20, 2023 •

edited

Loading