-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Cleanup some bevy_text pipeline.rs #9111
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
Conversation
|
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. |
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.
6ee2bc5 to
c446236
Compare
| pub min: Vec2, | ||
| pub max: Vec2, |
There was a problem hiding this comment.
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 😓
## 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`
Objective
bevy_text/src/pipeline.rshad some crufty code.Solution
Remove the cruft.
&mut selfargument was unused byTextPipeline::create_text_measure, so we replace it with a constructorTextMeasureInfo::from_text.&Texttofrom_textsince there is no reason to split the struct before passing it as argument.TextMeasureInfo. This avoids an additional allocation. We can re-use the font onfontsinstead incompute_size. Building aScaledFontseems fairly cheap, when looking at the ab_glyph internals.Vec<T>intoBox<[T]>andStringintoBox<str>{min,max}_width_content_sizefields ofTextMeasureInfohave namewidthin them, yet the contain information on both width and height.TextMeasureInfo::linebreak_behaviour->linebreak_behaviorMigration Guide
ResMut<TextPipeline>argument tomeasure_text_systemdoesn't exist anymore. If you were calling this system manually, you should remove the argument.{min,max}_width_content_sizefields ofTextMeasureInfoare renamed tominandmaxrespectivelyTextMeasureInfomay also break your code if you were manually building it. Please consider using the newTextMeasureInfo::from_textto build one instead.TextPipeline::create_text_measurehas been removed in favor ofTextMeasureInfo::from_text