-
-
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
Fix image measure function to apply inherent aspect ratio to style sizes #13555
Fix image measure function to apply inherent aspect ratio to style sizes #13555
Conversation
10fb479
to
fdd8543
Compare
@bugsweeper can I get your review here as well? |
crates/bevy_ui/src/widget/image.rs
Outdated
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. |
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.
not sure I understand what you mean by this comment
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.
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.
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 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?
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. |
Verry well, I didn't know that. |
c0409c0
to
8f0d375
Compare
Upgrade to Taffy to 0.5
Co-authored-by: François Mockers <[email protected]>
8f0d375
to
0a4232a
Compare
Objective
Solution
Style
through to the measure functionNotes
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.