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

Fix image measure function to apply inherent aspect ratio to style sizes #13555

Merged
merged 2 commits into from
May 30, 2024

Conversation

nicoburns
Copy link
Contributor

@nicoburns nicoburns commented May 28, 2024

Objective

Solution

  • Taffy has been updated to:
    • Apply size styles to absolutely positioned children
    • Pass the node's Style through to the measure function
  • Bevy's image measure function has been updated to make use of this style information

Notes

  • This is currently using a git version of Taffy. If this is tested as fixing the issue then we can turn that into a Taffy 0.5 release (this would be the only change between Taffy 0.4 and Taffy 0.5 so upgrading is not expected to be an issue)
  • This implementation may not be completely correct. I would have preferred to extend Taffy's gentest infrastructure to handle images and used that to nail down the correct behaviour. But I don't have time for that atm so we'll have to iterate on this in future. This PR at least puts that under Bevy's control.

Testing


Changelog

No changelog should be required as it fixes a regression on main that was not present in bevy 0.13. The changelog for "Taffy upgrade" may want to be changed from 0.4 to 0.5 if this change gets merged.

@nicoburns
Copy link
Contributor Author

CC @bugsweeper @porkbrain

@nicoburns nicoburns added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 28, 2024
@alice-i-cecile
Copy link
Member

@bugsweeper can I get your review here as well?

@alice-i-cecile alice-i-cecile added this to the 0.14 milestone May 28, 2024
let aspect_ratio = s_aspect_ratio.unwrap_or_else(|| self.size.x / self.size.y);

// Apply aspect ratio
// If either width or height was determined at this point, then both are beyond this point.
Copy link
Member

Choose a reason for hiding this comment

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

not sure I understand what you mean by this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we get to this line we have width: Option<f32> and height: Option<f32>. Applying the aspect ratio means that if one dimension is Some and the other None then it will fill in the none value using the aspect ratio and the value in the other dimension.

Copy link
Contributor

@bugsweeper bugsweeper left a comment

Choose a reason for hiding this comment

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

This PR contains changes done according to one of previously approved in the issue ways. Looks solid. Examples work correctly. There is only one problem, in this PR bevy_ui depends on personal fork of taffy, it's the best what we can get from changing two separate but dependent crates at same time, but it's not suitable for release. Should we wait some time (two weeks?) for taffy new release?

@nicoburns
Copy link
Contributor Author

There is only one problem, in this PR bevy_ui depends on personal fork of taffy, it's the best what we can get from changing two separate but dependent crates at same time, but it's not suitable for release. Should we wait some time (two weeks?) for taffy new release?

Me (and Alice) are maintainers of Taffy. I/we can put out a release whenever one is needed, I just don't want to put out a release that doesn't actually fix the problem. The fix looks like it works for #13155 to me, but I was hoping that the proposed fix could get a little more testing, and confirmation from at least one other person that it works first.

@bugsweeper
Copy link
Contributor

Me (and Alice) are maintainers of Taffy

Verry well, I didn't know that.

crates/bevy_ui/Cargo.toml Outdated Show resolved Hide resolved
crates/bevy_ui/src/widget/image.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added S-Blocked This cannot move forward until something else changes S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 30, 2024
nicoburns and others added 2 commits May 30, 2024 19:23
Co-authored-by: François Mockers <francois.mockers@vleue.com>
@alice-i-cecile alice-i-cecile removed the S-Blocked This cannot move forward until something else changes label May 30, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 30, 2024
Merged via the queue into bevyengine:main with commit a3e60d3 May 30, 2024
31 checks passed
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-Bug An unexpected or incorrect behavior 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
5 participants