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

[Merged by Bors] - text aspect ratio bug fix #6825

Closed
wants to merge 8 commits into from

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Dec 2, 2022

Objective

Bevy UI uses a MeasureFunc that preserves the aspect ratio of text, not just images. This means that the extent of flex-items containing text may be calculated incorrectly depending on the ratio of the text size compared to the size of its containing node.

Fixes #6748
Related to #6724

with Bevy 0.9:

Capture_cols_0 9

with this PR (accurately matching the behavior of Flexbox):

Capture_fixed

Solution

Only perform the aspect ratio calculations if the uinode contains an image.

Changelog

  • Added a field preserve_aspect_ratio to CalculatedSize
  • The MeasureFunc only preserves the aspect ratio when preserve_aspect_ratio is true.
  • update_image_calculated_size_system sets preserve_aspect_ratio to true for nodes with images.

Bevy UI was using the MeasureFunc that preserves aspect ratio for both images and text.
This meant that the cross-axis of flex items containing text would be calculated incorrectly sometimes
depending on how the aspect ratio of the text compared to the size of the containing node.

changes:
    * Added parameter preserve_aspect_ratio to the upsert_leaf function.
    * The MeasureFunc only preserves the aspect ratio when preserve_aspect_ratio is true.
    * flex_node_system queries for UiImage before it calls upsert_leaf
    and sets preserve_aspect_ratio appropriately.
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets labels Dec 3, 2022
crates/bevy_ui/src/flex/mod.rs Outdated Show resolved Hide resolved
…aspect_ratio field to CalculatedSize which is

set to true for image nodes by update_image_calculated_size_system.
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.

That is a dramatically better solution, nice work.

@Weibye Weibye 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 Dec 16, 2022
@mockersf
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Dec 20, 2022
## Objective 

Bevy UI uses a `MeasureFunc` that preserves the aspect ratio of text, not just images. This means that the extent of flex-items containing text may be calculated incorrectly depending on the ratio of the text size compared to the size of its containing node.

Fixes #6748 
Related to #6724

with Bevy 0.9:

![Capture_cols_0 9](https://user-images.githubusercontent.com/27962798/205435999-386d3400-fe9b-475a-aab1-18e61c4c074f.PNG)

with this PR (accurately matching the behavior of Flexbox):

![Capture_fixed](https://user-images.githubusercontent.com/27962798/205436005-6bafbcc2-cd87-4eb7-b5c6-9dbcb30fc795.PNG)

## Solution
Only perform the aspect ratio calculations if the uinode contains an image.

## Changelog
* Added a field `preserve_aspect_ratio` to `CalculatedSize`
* The `MeasureFunc` only preserves the aspect ratio when `preserve_aspect_ratio` is true.
* `update_image_calculated_size_system` sets `preserve_aspect_ratio` to true for nodes with images.
@bors
Copy link
Contributor

bors bot commented Dec 20, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Dec 20, 2022
## Objective 

Bevy UI uses a `MeasureFunc` that preserves the aspect ratio of text, not just images. This means that the extent of flex-items containing text may be calculated incorrectly depending on the ratio of the text size compared to the size of its containing node.

Fixes #6748 
Related to #6724

with Bevy 0.9:

![Capture_cols_0 9](https://user-images.githubusercontent.com/27962798/205435999-386d3400-fe9b-475a-aab1-18e61c4c074f.PNG)

with this PR (accurately matching the behavior of Flexbox):

![Capture_fixed](https://user-images.githubusercontent.com/27962798/205436005-6bafbcc2-cd87-4eb7-b5c6-9dbcb30fc795.PNG)

## Solution
Only perform the aspect ratio calculations if the uinode contains an image.

## Changelog
* Added a field `preserve_aspect_ratio` to `CalculatedSize`
* The `MeasureFunc` only preserves the aspect ratio when `preserve_aspect_ratio` is true.
* `update_image_calculated_size_system` sets `preserve_aspect_ratio` to true for nodes with images.
@bors
Copy link
Contributor

bors bot commented Dec 20, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Dec 20, 2022
## Objective 

Bevy UI uses a `MeasureFunc` that preserves the aspect ratio of text, not just images. This means that the extent of flex-items containing text may be calculated incorrectly depending on the ratio of the text size compared to the size of its containing node.

Fixes #6748 
Related to #6724

with Bevy 0.9:

![Capture_cols_0 9](https://user-images.githubusercontent.com/27962798/205435999-386d3400-fe9b-475a-aab1-18e61c4c074f.PNG)

with this PR (accurately matching the behavior of Flexbox):

![Capture_fixed](https://user-images.githubusercontent.com/27962798/205436005-6bafbcc2-cd87-4eb7-b5c6-9dbcb30fc795.PNG)

## Solution
Only perform the aspect ratio calculations if the uinode contains an image.

## Changelog
* Added a field `preserve_aspect_ratio` to `CalculatedSize`
* The `MeasureFunc` only preserves the aspect ratio when `preserve_aspect_ratio` is true.
* `update_image_calculated_size_system` sets `preserve_aspect_ratio` to true for nodes with images.
@bors
Copy link
Contributor

bors bot commented Dec 20, 2022

Build failed:

@james7132
Copy link
Member

bors retry

@bors
Copy link
Contributor

bors bot commented Dec 20, 2022

Canceled.

@james7132
Copy link
Member

bors retry

bors bot pushed a commit that referenced this pull request Dec 20, 2022
## Objective 

Bevy UI uses a `MeasureFunc` that preserves the aspect ratio of text, not just images. This means that the extent of flex-items containing text may be calculated incorrectly depending on the ratio of the text size compared to the size of its containing node.

Fixes #6748 
Related to #6724

with Bevy 0.9:

![Capture_cols_0 9](https://user-images.githubusercontent.com/27962798/205435999-386d3400-fe9b-475a-aab1-18e61c4c074f.PNG)

with this PR (accurately matching the behavior of Flexbox):

![Capture_fixed](https://user-images.githubusercontent.com/27962798/205436005-6bafbcc2-cd87-4eb7-b5c6-9dbcb30fc795.PNG)

## Solution
Only perform the aspect ratio calculations if the uinode contains an image.

## Changelog
* Added a field `preserve_aspect_ratio` to `CalculatedSize`
* The `MeasureFunc` only preserves the aspect ratio when `preserve_aspect_ratio` is true.
* `update_image_calculated_size_system` sets `preserve_aspect_ratio` to true for nodes with images.
@bors bors bot changed the title text aspect ratio bug fix [Merged by Bors] - text aspect ratio bug fix Dec 20, 2022
@bors bors bot closed this Dec 20, 2022
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
## Objective 

Bevy UI uses a `MeasureFunc` that preserves the aspect ratio of text, not just images. This means that the extent of flex-items containing text may be calculated incorrectly depending on the ratio of the text size compared to the size of its containing node.

Fixes bevyengine#6748 
Related to bevyengine#6724

with Bevy 0.9:

![Capture_cols_0 9](https://user-images.githubusercontent.com/27962798/205435999-386d3400-fe9b-475a-aab1-18e61c4c074f.PNG)

with this PR (accurately matching the behavior of Flexbox):

![Capture_fixed](https://user-images.githubusercontent.com/27962798/205436005-6bafbcc2-cd87-4eb7-b5c6-9dbcb30fc795.PNG)

## Solution
Only perform the aspect ratio calculations if the uinode contains an image.

## Changelog
* Added a field `preserve_aspect_ratio` to `CalculatedSize`
* The `MeasureFunc` only preserves the aspect ratio when `preserve_aspect_ratio` is true.
* `update_image_calculated_size_system` sets `preserve_aspect_ratio` to true for nodes with images.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
## Objective 

Bevy UI uses a `MeasureFunc` that preserves the aspect ratio of text, not just images. This means that the extent of flex-items containing text may be calculated incorrectly depending on the ratio of the text size compared to the size of its containing node.

Fixes bevyengine#6748 
Related to bevyengine#6724

with Bevy 0.9:

![Capture_cols_0 9](https://user-images.githubusercontent.com/27962798/205435999-386d3400-fe9b-475a-aab1-18e61c4c074f.PNG)

with this PR (accurately matching the behavior of Flexbox):

![Capture_fixed](https://user-images.githubusercontent.com/27962798/205436005-6bafbcc2-cd87-4eb7-b5c6-9dbcb30fc795.PNG)

## Solution
Only perform the aspect ratio calculations if the uinode contains an image.

## Changelog
* Added a field `preserve_aspect_ratio` to `CalculatedSize`
* The `MeasureFunc` only preserves the aspect ratio when `preserve_aspect_ratio` is true.
* `update_image_calculated_size_system` sets `preserve_aspect_ratio` to true for nodes with images.
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
Status: Done
Development

Successfully merging this pull request may close these issues.

MeasureFunc Text aspect ratio bug
5 participants