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.

Sign up for GitHub

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

Jump to bottom

TextLayoutInfo::size should hold the drawn size of the text, and not a scaled value. #7794

Merged
merged 15 commits into from
Sep 11, 2023
Merged

TextLayoutInfo::size should hold the drawn size of the text, and not a scaled value. #7794

merged 15 commits into from
Sep 11, 2023

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Feb 23, 2023 •
edited
Loading

Objective

TextLayoutInfo::size isn't the drawn size of the text, but a scaled value. This is fragile, counter-intuitive and makes it awkward to retrieve the correct value.

Solution

Multiply TextLayoutInfo::size by the reciprocal of the window's scale factor after generating the text layout in update_text2d_layout and bevy_ui::widget::text_system.

fixes: #7787

Changelog

  • Multiply TextLayoutInfo::size by the reciprocal of the scale factor after text computation to reflect the actual size of the text as drawn.
  • Reorder the operations in extract_text2d_sprite to apply the alignment offset before the scale factor scaling.

Migration Guide

The size value of TextLayoutInfo is stored in logical pixels and has been renamed to logical_size. There is no longer any need to divide by the window's scale factor to get the logical size.

@ickshonpe
changes:
954b44b 
* multiply `text_layout_info.size` by the reciprocal of the scale factor after generating text2d's text layout so it relects the actual size of the text.
* reorder the operations in `extract_2d_sprite` so that the
alignment offset is applied before the scale factor scaling.
@ickshonpe ickshonpe mentioned this pull request Feb 23, 2023
Closed
@ickshonpe ickshonpe changed the title Make it so that TextLayoutInfo::size holds the actual size of the text, and not a scaled value. Make it so that TextLayoutInfo::size holds the actual size of the text for Text2d, and not a scaled value. Feb 23, 2023
@ickshonpe ickshonpe changed the title Make it so that TextLayoutInfo::size holds the actual size of the text for Text2d, and not a scaled value. Make it so that TextLayoutInfo::size holds of the text as drawn, and not a scaled value. Feb 23, 2023
@ickshonpe ickshonpe changed the title Make it so that TextLayoutInfo::size holds of the text as drawn, and not a scaled value. Make it so that TextLayoutInfo::size holds the drawn size of the text, and not a scaled value. Feb 23, 2023
@ickshonpe
Copy link
Contributor Author

I didn't make the same change in the UI, as it's much less useful and calculated_size already contains the unscaled size.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Feb 23, 2023
@ickshonpe ickshonpe changed the title Make it so that TextLayoutInfo::size holds the drawn size of the text, and not a scaled value. TextLayoutInfo::size should hold the drawn size of the text, and not a scaled value. Feb 23, 2023
@ickshonpe
Copy link
Contributor Author

I didn't make the same change in the UI, as it's much less useful and calculated_size already contains the unscaled size.

This no longer true about CalculatedSize, and it's very confusing for it to use different coordinate systems depending on context.

@alice-i-cecile
Copy link
Member

@ickshonpe are you happy with the state of this, or would you like to make additional changes in this PR?

@ickshonpe
Copy link
Contributor Author

@ickshonpe are you happy with the state of this, or would you like to make additional changes in this PR?

I'm just fixing it now.

@ickshonpe
Copy link
Contributor Author

@alice-i-cecile should all be correct now.

nicoburns
nicoburns approved these changes Apr 21, 2023
View reviewed changes
Copy link
Contributor

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

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

I like the name change. Have checked and it text still seems to be rendering correctly with this PR :)

@rparrett
Copy link
Contributor

rparrett commented Sep 8, 2023

Looks like this got two approvals but was forgotten about. Making a note here check on this PR after #9676 is resolved.

@alice-i-cecile alice-i-cecile 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 Sep 8, 2023
@alice-i-cecile
Copy link
Member

@ickshonpe want to rebase this for us?

@ickshonpe
Copy link
Contributor Author

@ickshonpe want to rebase this for us?

Yep having a look now.

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Sep 9, 2023 •
edited
Loading

Rebased and seems to be working correctly.

Since Text2d isn't working atm I merged this and #9708 at https://github.com/ickshonpe/bevy/tree/text2d-visibility-fix-logical-text-layout-info

@alice-i-cecile alice-i-cecile added this to the 0.12 milestone Sep 9, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 11, 2023
Merged via the queue into bevyengine:main with commit 9d9750b Sep 11, 2023
21 checks passed
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@ickshonpe
TextLayoutInfo::size should hold the drawn size of the text, and no…
0d69939 
…t a scaled value. (bevyengine#7794)

# Objective

`TextLayoutInfo::size` isn't the drawn size of the text, but a scaled
value. This is fragile, counter-intuitive and makes it awkward to
retrieve the correct value.

## Solution

Multiply `TextLayoutInfo::size` by the reciprocal of the window's scale
factor after generating the text layout in `update_text2d_layout` and
`bevy_ui::widget::text_system`.

---

fixes: bevyengine#7787

## Changelog

* Multiply `TextLayoutInfo::size` by the reciprocal of the scale factor
after text computation to reflect the actual size of the text as drawn.
* Reorder the operations in `extract_text2d_sprite` to apply the
alignment offset before the scale factor scaling.

## Migration Guide

The `size` value of `TextLayoutInfo` is stored in logical pixels and has
been renamed to `logical_size`. There is no longer any need to divide by
the window's scale factor to get the logical size.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@nicoburns nicoburns nicoburns approved these changes

Assignees
No one assigned
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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
Milestone
0.12
Development

Successfully merging this pull request may close these issues.

Size in TextLayoutInfo does not correspond to actual Text2d dimensions with non-one scale factor
5 participants
@ickshonpe @alice-i-cecile @rparrett @nicoburns @mockersf