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

Cleanup some bevy_text pipeline.rs #9111

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Jul 11, 2023

Objective

  • bevy_text/src/pipeline.rs had some crufty code.

Solution

Remove the cruft.

  • &mut self argument was unused by TextPipeline::create_text_measure, so we replace it with a constructor TextMeasureInfo::from_text.
  • We also pass a &Text to from_text since there is no reason to split the struct before passing it as argument.
  • from_text also checks beforehand that every Font exist in the Assets. This allows rust to skip the drop code on the Vecs we create in the method, since there is no early exit.
  • We also remove the scaled_fonts field on TextMeasureInfo. This avoids an additional allocation. We can re-use the font on fonts instead in compute_size. Building a ScaledFont seems fairly cheap, when looking at the ab_glyph internals.
  • We also implement ToSectionText on TextMeasureSection, this let us skip creating a whole new Vec each time we call compute_size.
  • This let us remove compute_size_from_section_text, since its only purpose was to not have to allocate the Vec we just made redundant.
  • Make some immutabe Vec<T> into Box<[T]> and String into Box<str>
  • {min,max}_width_content_size fields of TextMeasureInfo have name width in them, yet the contain information on both width and height.
  • TextMeasureInfo::linebreak_behaviour -> linebreak_behavior

Migration Guide

  • The ResMut<TextPipeline> argument to measure_text_system doesn't exist anymore. If you were calling this system manually, you should remove the argument.
  • The {min,max}_width_content_size fields of TextMeasureInfo are renamed to min and max respectively
  • Other changes to TextMeasureInfo may also break your code if you were manually building it. Please consider using the new TextMeasureInfo::from_text to build one instead.
  • TextPipeline::create_text_measure has been removed in favor of TextMeasureInfo::from_text

@nicopap nicopap added 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 Jul 11, 2023
@ickshonpe
Copy link
Contributor

ickshonpe commented Jul 12, 2023

Seem like good changes. Just getting #7779 working and fixing all the bugs took so much effort, I didn't want to work on it any more and some of the code I left was really ugly and embarrassing, and I'm relieved it's getting fixed now.

I've been doing some work on TextPipeline too, fixing some bugs, but there shouldn't be much conflict.
I'll do a proper review after I've finished my PRs.

Objective
------

- `bevy_text/src/pipeline.rs` had some crufty code.

Solution
------

Remove the cruft.

- `&mut self` argument was unused by `TextPipeline::create_text_measure`, so we replace it with a constructor `TextMeasureInfo::from_text`.
- We also pass a `&Text` to `from_text` since there is no reason to split the struct before passing it as argument.
- from_text also checks beforehand that every Font exist in the Assets<Font>. This allows rust to skip the drop code on the Vecs we create in the method, since there is no early exit.
- We also remove the scaled_fonts field on `TextMeasureInfo`. This avoids an additional allocation. We can re-use the font on `fonts` instead in `compute_size`. Building a `ScaledFont` seems fairly cheap, when looking at the ab_glyph internals.
- We also implement ToSectionText on TextMeasureSection, this let us skip creating a whole new Vec each time we call compute_size.
- This let us remove compute_size_from_section_text, since its only purpose was to not have to allocate the Vec we just made redundant.
- Make some immutabe `Vec<T>` into `Box<[T]>` and `String` into `Box<str>`

The `ResMut<TextPipeline>` argument to  `measure_text_system` doesn't exist anymore. If you were calling this system manually, you should remove the argument.
@mockersf mockersf 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 Aug 20, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 28, 2023
Merged via the queue into bevyengine:main with commit afcb1fe Aug 28, 2023
1 check passed
Comment on lines +119 to +120
pub min: Vec2,
pub max: Vec2,
Copy link
Contributor

@ickshonpe ickshonpe Aug 28, 2023

Choose a reason for hiding this comment

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

Ah just noticed this from the changelog, it's not a mistake that these were called min_content_width and max_content_width. The word width here isn't referring to the result but the constraint the size value is based on. min_content_width is the space (horizontal and vertical) that the text will occupy given a zero width constraint (in which case the text wraps at every breakpoint) and max_content_width is the space the text will occupy given an infinite width constraint (in which case the text next never wraps).

Edit: didn't mean to start a review just to leave a comment 😓

@nicopap nicopap deleted the static_measure_func branch August 30, 2023 13:37
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
## Objective

- `bevy_text/src/pipeline.rs` had some crufty code.

## Solution

Remove the cruft.

- `&mut self` argument was unused by
`TextPipeline::create_text_measure`, so we replace it with a constructor
`TextMeasureInfo::from_text`.
- We also pass a `&Text` to `from_text` since there is no reason to
split the struct before passing it as argument.
- from_text also checks beforehand that every Font exist in the
Assets<Font>. This allows rust to skip the drop code on the Vecs we
create in the method, since there is no early exit.
- We also remove the scaled_fonts field on `TextMeasureInfo`. This
avoids an additional allocation. We can re-use the font on `fonts`
instead in `compute_size`. Building a `ScaledFont` seems fairly cheap,
when looking at the ab_glyph internals.
- We also implement ToSectionText on TextMeasureSection, this let us
skip creating a whole new Vec each time we call compute_size.
- This let us remove compute_size_from_section_text, since its only
purpose was to not have to allocate the Vec we just made redundant.
- Make some immutabe `Vec<T>` into `Box<[T]>` and `String` into
`Box<str>`
- `{min,max}_width_content_size` fields of `TextMeasureInfo` have name
`width` in them, yet the contain information on both width and height.
- `TextMeasureInfo::linebreak_behaviour`  -> `linebreak_behavior`

## Migration Guide

- The `ResMut<TextPipeline>` argument to `measure_text_system` doesn't
exist anymore. If you were calling this system manually, you should
remove the argument.
- The `{min,max}_width_content_size` fields of `TextMeasureInfo` are
renamed to `min` and `max` respectively
- Other changes to `TextMeasureInfo` may also break your code if you
were manually building it. Please consider using the new
`TextMeasureInfo::from_text` to build one instead.
- `TextPipeline::create_text_measure` has been removed in favor of
`TextMeasureInfo::from_text`
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-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
Development

Successfully merging this pull request may close these issues.

5 participants