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

Upgrade to Taffy 0.4 #10690

Merged
merged 9 commits into from
Apr 30, 2024
Merged

Upgrade to Taffy 0.4 #10690

merged 9 commits into from
Apr 30, 2024

Conversation

nicoburns
Copy link
Contributor

@nicoburns nicoburns commented Nov 22, 2023

Objective

Todo

  • Fix rendering of text/images to account for padding/border on nodes (should size/position to content box rather than border box) In order get this into a mergeable state this PR instead zeroes out padding/border when syncing leaf node styles into Taffy to preserve the existing behaviour. Text and Images can't be given padding #6879 can be fixed in a followup PR.

Solution

  • Update the version of Taffy
  • Update code to work with the new version

Note: Taffy 0.4 has not yet been released. This PR is being created in advance of the release to ensure that there are no blockers to upgrading once the release occurs.


Changelog

  • Bevy now supports the Display::Block and Overflow::Hidden styles.

@nicoburns nicoburns added A-UI Graphical user interfaces, styles, layouts, and widgets S-Blocked This cannot move forward until something else changes labels Nov 22, 2023
@nicoburns nicoburns added this to the 0.13 milestone Nov 22, 2023
@nicoburns nicoburns marked this pull request as draft November 22, 2023 01:12
@nicoburns nicoburns changed the title WIP: Update to Taffy 0.4 WIP: Upgrade to Taffy 0.4 Nov 22, 2023
@alice-i-cecile alice-i-cecile added C-Dependencies A change to the crates that Bevy depends on C-Feature A new feature, making something new possible labels Nov 22, 2023
@alice-i-cecile alice-i-cecile removed this from the 0.13 milestone Jan 24, 2024
@robtfm robtfm mentioned this pull request Jan 29, 2024
@JMS55 JMS55 added this to the 0.13 milestone Feb 1, 2024
@alice-i-cecile alice-i-cecile modified the milestones: 0.13, 0.14 Feb 12, 2024
@nicoburns nicoburns force-pushed the taffy-0.4 branch 6 times, most recently from 8a924f4 to 3a7e8f3 Compare February 13, 2024 20:58
@nicoburns
Copy link
Contributor Author

nicoburns commented Feb 13, 2024

@ickshonpe This PR fixes Taffy reserving space for padding for text and image nodes, but the text/image position is not offset by the top/left padding when actually rendering. Do you know where that change would need to be made? Taffy 0.4 outputs computed padding and border values.

@alice-i-cecile alice-i-cecile removed the S-Blocked This cannot move forward until something else changes label Mar 24, 2024
@alice-i-cecile
Copy link
Member

This is no longer blocked, as taffy 0.4 has been released.

@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Mar 24, 2024
@BD103
Copy link
Member

BD103 commented Apr 12, 2024

We're getting some CI errors for the checks that run nightly, and they appear to be related to Taffy. Would updating to 0.4 fix this error?

error[E0277]: the trait bound `f32: From<f64>` is not satisfied
   --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/taffy-0.3.18/src/style_helpers.rs:58:31
    |
58  |     repeated_tracks.push(flex(1.0));
    |                          ---- ^^^ the trait `From<f64>` is not implemented for `f32`, which is required by `{float}: Into<f32>`
    |                          |
    |                          required by a bound introduced by this call
    |
    = help: the following other types implement trait `From<T>`:
              <f32 as From<bool>>
              <f32 as From<f16>>
              <f32 as From<i16>>
              <f32 as From<i8>>
              <f32 as From<u16>>
              <f32 as From<u8>>
    = note: required for `f64` to implement `Into<f32>`
note: required by a bound in `flex`
   --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/taffy-0.3.18/src/style_helpers.rs:100:12
    |
98  | pub fn flex<Input, Output>(flex_fraction: Input) -> Output
    |        ---- required by a bound in this function
99  | where
100 |     Input: Into<f32> + Copy,
    |            ^^^^^^^^^ required by this bound in `flex`

@nicoburns
Copy link
Contributor Author

Taffy 0.4 release is unlikely to fix this as that code is unchanged between versions. We'll likely need to do a patch release. Or for rustc to fix their inference regression.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Once there's docs for NodeMeasure this LGTM! Happy to see this finally make it in.

@alice-i-cecile alice-i-cecile 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 Apr 22, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 30, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 30, 2024
@alice-i-cecile
Copy link
Member

@nicoburns the CI failures look related: could you take a look at the failures reported during the failed merge if this latest attempt fails?

@nicoburns
Copy link
Contributor Author

Yep, just having lunch but will take a look after. Keen to get this merged so we can stop supporting Taffy 0.3!

auto-merge was automatically disabled April 30, 2024 01:05

Head branch was pushed to by a user without write access

@nicoburns nicoburns force-pushed the taffy-0.4 branch 2 times, most recently from d381d90 to db6390e Compare April 30, 2024 01:05
@nicoburns
Copy link
Contributor Author

@alice-i-cecile Passing checks now

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 30, 2024
Merged via the queue into bevyengine:main with commit 96b9d0a Apr 30, 2024
31 checks passed
@alice-i-cecile alice-i-cecile removed the M-Needs-Release-Note Work that should be called out in the blog due to impact label Jun 2, 2024
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 C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants