-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MeasureFunc
improvements
#8402
MeasureFunc
improvements
#8402
Conversation
* 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`.
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? |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Yeah, the name
|
there is a small style issue, a merge issue in |
…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.
This reverts commit 96903e5.
Since a |
How does that interact with change detection on the |
|
* 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.
MeasureFunc
s on CalculatedSize
changesMeasureFunc
improvements
Also fixes #8516, not sure why. Might be some problem in |
@ickshonpe Would you now consider this PR ready for review? I've been waiting for you to stop committing to it to re-review. |
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:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Also seems to work with a quick test locally.
Objective
fixes #8516
Give
CalculatedSize
a more specific and intuitive name.MeasureFunc
s should only be updated when theirCalculatedSize
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'sMeasureFunc
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 thedyn_clone
method.Solution
Rename
CalculatedSize
toContentSize
Only update
MeasureFunc
s onCalculatedSize
changes.Remove the
dyn_clone
method fromMeasure
and move theMeasure
from theContentSize
component rather than cloning it.Change the measure_func field of
ContentSize
to typeOption<taffy::node::MeasureFunc>
. Add aset
method that wraps the given measure appropriately.Changelog
CalculatedSize
toContentSize
.upsert_leaf
with a functionupdate_measure
that only updates the node'sMeasureFunc
.MeasureFunc
s are only updated when theContentSize
changes and not when the layout changes.MeasureFunc
.ContentSize
scaling intext_system
.dyn_clone
method has been removed from theMeasure
trait.Measure
s are moved from theContentSize
component instead of cloning them.set
method toContentSize
that replaces thenew
function.Migration Guide
CalculatedSize
has been renamed toContentSize
.upsert_leaf
function has been removed fromUiSurface
and replaced withupdate_measure
which updates theMeasureFunc
without node insertion.dyn_clone
method has been removed from theMeasure
trait.CalculatedSize
has been replaced with the methodset
.