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

UI: image with Absolute position don't render correctly #13155

Closed
mockersf opened this issue May 1, 2024 · 15 comments · Fixed by #13555
Closed

UI: image with Absolute position don't render correctly #13155

mockersf opened this issue May 1, 2024 · 15 comments · Fixed by #13555
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior C-Regression Functionality that used to work but no longer does. Add a test for this!
Milestone

Comments

@mockersf
Copy link
Member

mockersf commented May 1, 2024

Bevy version

main since #10690

What you did

Run example game_manu or overflow_debug

What went wrong

Images that have position_type: PositionType::Absolute in their style are not displayed correctly

Screenshot 2024-05-01 at 02 23 48
@mockersf mockersf added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets C-Regression Functionality that used to work but no longer does. Add a test for this! labels May 1, 2024
@mockersf
Copy link
Member Author

mockersf commented May 1, 2024

@nicoburns any idea?

@mockersf mockersf added this to the 0.14 milestone May 1, 2024
@nicoburns
Copy link
Contributor

Hmm... no that's weird. It looks like it's about the right size in the x axis but it's getting stretched by about a factor of 2 in the y axis :/

@bugsweeper
Copy link
Contributor

That because width is set, but height isn't. If you remove width from style, you'll see image big icons with correct proportions.

@bugsweeper
Copy link
Contributor

@mockersf I set height same as width and got
image
correct result. Is this change good enough solution, or someone should dig deeper and figure out why taffy changed behaviour?

@bugsweeper
Copy link
Contributor

For overflow_debug same thing: height is set, but width isn't. If add width: Val::Px(400.0),, then result
image
looks correct

@bugsweeper
Copy link
Contributor

bugsweeper commented May 8, 2024

But if this solution gets approve, then I think we should add some notes to Migration guide, because such things could be in bevy users code

@bugsweeper
Copy link
Contributor

Or better add some check/warning in bevy_ui::layout::convert::from_style(...) -> taffy::style::Style

@mockersf
Copy link
Member Author

mockersf commented May 8, 2024

Is this change good enough solution, or someone should dig deeper and figure out why taffy changed behaviour?

No, images should keep their ratio when you set only one of their dimension

@nicoburns
Copy link
Contributor

Yeah, this definitely ought to be fixed in Taffy and/or bevy_ui (wherever the bug is)

@bugsweeper
Copy link
Contributor

bugsweeper commented May 10, 2024

@mockersf The problem hides in taffy, because at some point size fills None by measured_size which is done componentwise. So there Size(Some(45), None) becomes Size(Some(45), Some(100)) which has no correct ratio. Looks like there is no issue in taffy for such case.

@nicoburns
Copy link
Contributor

I think the issue here is with the image measure function. It ought to read the image node's Style and use the size, min_size and max_size as part of it's computation. For most (all?) other "leaf" nodes this is automatically applied by Taffy, but that doesn't work for images because of their inherent aspect ratio.

@bugsweeper
Copy link
Contributor

bugsweeper commented May 14, 2024

I agree with your desire for universal approach, but IMO size is already width and height which we fill partially, and min_size and max_size is for clamping but not for ratio description, if there is width or height set only, image should be abble grow or reduce unlimitedly but proportionally. I read the contribution.md page of taffy, it says that behaviour must be similar as in chrome css render. That's why IMO we should make html example first (test?) for taffy and reproduce this issue. If chrome behaves as we expect, but taffy doesn't, then this is the basis for creating a well-formed issue in taffy project. Otherwise it should be local fix in bevy_ui, perhaps UiImage could autofill the width or height or better aspect_ratio in Style at some stage.

@bugsweeper
Copy link
Contributor

I didn't notice that bevy doesn't inform taffy about node types, that's why taffy can't make correct decision about size behaviour. I think Style::aspect_ratio is correct approach, I will make PR based on it.

@nicoburns
Copy link
Contributor

@alice-i-cecile Just wanted to call attention to this issue. It's a really not great regression which I believe it caused by the interaction between Taffy and Bevy's measure function. I don't think the fix should be too hard (either need to expose taffy's Style to the measure function or inherent aspect ratios to Taffy - the former is probably an easier quick fix), but it's a bit of an awkward one because I think it will require breaking changes in Taffy and then another Taffy upgrade in Bevy.

(but luckily Taffy doesn't have any other unreleased changes sitting around, so the upgrade should be quite straightforward).

What kind of release date is 0.14 targeting?

@alice-i-cecile
Copy link
Member

We're looking at 1-2 weeks for the release candidate. I agree that this is important to resolve.

github-merge-queue bot pushed a commit that referenced this issue May 30, 2024
…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 <alice.i.cecile@gmail.com>
Co-authored-by: François Mockers <francois.mockers@vleue.com>
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 C-Regression Functionality that used to work but no longer does. Add a test for this!
Projects
None yet
4 participants