Skip to content
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

Update taffy requirement from 0.3.10 to 0.4.0 #12298

Closed
wants to merge 2 commits into from

Conversation

Kanabenki
Copy link
Contributor

Objective

Solution

  • Fix the renamed import paths and renamed types.
  • Remove the tests asserts using needs_measure since it has been removed.
  • Replace use of the LayoutTree trait with the corresponding traits it has been split into.
  • Replace the use of the remove taffy MeasureFunc with a Box<dyn<Measure>> as a node context, equivalent to MeasureFunc::Boxed.

Changelog

Changed

  • The Taffy dependency has been updated to 0.4.0. If you are directly using taffy types re-exported from bevy_ui, you may need to change some import paths. (see the corresponding release notes)
  • UiSurface::try_update_measure takes a Box<dyn Measure> instead of the now removed taffy MeasureFunc.

Migration Guide

  • If you were directly using UiSurface::try_update_measure, you should implement the bevy provided Measure trait on your measurement struct instead of using the removed MeasureFunc::Boxed with the similar taffy provided Measurable trait.

Updates the requirements on [taffy](https://github.com/DioxusLabs/taffy) to permit the latest version.
- [Release notes](https://github.com/DioxusLabs/taffy/releases)
- [Changelog](https://github.com/DioxusLabs/taffy/blob/main/RELEASES.md)
- [Commits](DioxusLabs/taffy@v0.3.10...v0.4.0)

---
updated-dependencies:
- dependency-name: taffy
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
@@ -133,6 +134,7 @@ pub fn from_style(context: &LayoutContext, style: &Style) -> taffy::style::Style
.collect::<Vec<_>>(),
grid_row: style.grid_row.into(),
grid_column: style.grid_column.into(),
..default()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some new overflow and scrollbar_width properties. Should they be replicated on bevy_ui::Style and if so, should that be done in this PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please to both.

}

type UiTaffyTree = TaffyTree<Box<dyn Measure>>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the measurement/node context type is user-defined, should we use a enum with one variant for each of FixedMeasure, ImageMeasure... internal to bevy_ui plus a Box<dyn Measure> variant, to avoid doing dynamic dispatch for most use cases?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

available_space.height,
)
})
.unwrap_or(Vec2::ZERO);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should layouting an empty leaf node with no size log a warning/error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like a warn_once! for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say probably not. Such a node could get it's size from it's parent (e.g. by stretch alignment in a flexbox stack or to a grid cell)

@Kanabenki Kanabenki added C-Dependencies A change to the crates that Bevy depends on A-UI Graphical user interfaces, styles, layouts, and widgets labels Mar 4, 2024
@james7132 james7132 added this to the 0.14 milestone Mar 4, 2024
@james7132 james7132 requested a review from alice-i-cecile March 4, 2024 21:24
@alice-i-cecile alice-i-cecile requested a review from nicoburns March 4, 2024 22:16
@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 4, 2024
@nicoburns
Copy link
Contributor

nicoburns commented Mar 4, 2024

Will do a proper review later, but note that I also have a PR for this: #10690 The only thing blocking that PR is:

  • Fix rendering of text/images to account for padding/border on nodes (should size/position to content box rather than border box)

This is required because Taffy 0.4 reserves space for padding/borders on leaf nodes but currently the content (text, image, etc) is still rendering at the top-left corner of the larger box rather than being offset by the border and padding widths.

@Kanabenki
Copy link
Contributor Author

Will do a proper review later, but note that I also have a PR for this: #10690

Closing in favor of this one since it seems a straight upgrade over the changes I did here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Dependencies A change to the crates that Bevy depends on M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants