UI: Image fills aspect_ratio of texture dimensions #13381
Conversation
|
@mockersf May I ask you for a review? |
alice-i-cecile
added
C-Bug
| @@ -107,6 +108,10 @@ pub fn update_image_content_size_system( | |||
| // multiply the image size by the scale factor to get the physical size | |||
| size: size.as_vec2() * combined_scale_factor, | |||
| })); | |||
| if let Some(mut style) = style { | |||
There was a problem hiding this comment.
A comment here would make this code easier to follow.
crates/bevy_ui/src/widget/image.rs
Outdated
| @@ -107,6 +108,10 @@ pub fn update_image_content_size_system( | |||
| // multiply the image size by the scale factor to get the physical size | |||
| size: size.as_vec2() * combined_scale_factor, | |||
| })); | |||
| if let Some(mut style) = style { | |||
| let Vec2 { x, y } = size.as_vec2(); | |||
| style.aspect_ratio = Some(x / y); | |||
There was a problem hiding this comment.
shouldn't this be the other way around? Using the style.aspect_ratio controlling image sizing? I mean, I had the idea that the Style is always a controlling component, never controlled by other components.
There was a problem hiding this comment.
The way the web does it is that the intrinsic aspect ratio is separate to the aspect ratio style.
There was a problem hiding this comment.
same, not a fan of changing the Style component. Also, if the user set a specific aspect_ratio, we shouldn't overwrite it
There was a problem hiding this comment.
The way the web does it is that the intrinsic aspect ratio is separate to the aspect ratio style.
I didn't find other place to set correct ratio, if you know such way, please, feel free to describe it, taffy knows nothing about type of nodes, the image should keep ratio, but div has no such restrictions.
if the user set a specific aspect_ratio, we shouldn't overwrite it
I agree that there shouldn't be overwriting, I will fix this.
There was a problem hiding this comment.
I didn't find other place to set correct ratio, if you know such way, please, feel free to describe it, taffy knows nothing about type of nodes
Taffy needs to change to fix this sensibly. Either Taffy needs to allow "measurable" items to communicate their inherent aspect ratio. Or Taffy needs to pass the styles in to the measure function to allow measurable items to apply it themselves.
There was a problem hiding this comment.
Either Taffy needs to allow "measurable" items to communicate their inherent aspect ratio. Or Taffy needs to pass the styles in to the measure function to allow measurable items to apply it themselves.
Looks like any of this changes is breaking change for Taffy. So we will not fix examples until Taffy becomes ready?
In the issue I proposed easy changes in the examples instead of bevy_ui, which will make examples looks good and has no side effects.
bugsweeper
force-pushed
the
missing-ratio-in-ui-image
branch
from
89dfc5e to
71f852e
Compare
alice-i-cecile
added
S-Needs-Review
|
Going with #13555. |
…zes (#13555) # Objective - Fixes #13155 - fixes #13517 - Supercedes #13381 - Requires DioxusLabs/taffy#661 ## 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 - I manually tested the game_menu_example (from #13155) - More testing is probably merited --- ## 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. --------- Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: François Mockers <[email protected]>
Objective
Solution
Testing
Run example
game_menuand
overflow_debug