Cleanup some bevy_text pipeline.rs

nicopap · nicopap · commit 6ee2bc5851b2 · 2023-07-11T12:02:44.000+02:00
Objective
------

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

Solution
------

Remove the cruft.

- `&amp;mut self` argument was unused by `TextPipeline::create_text_measure`, so we replace it with a constructor `TextMeasureInfo::from_text`.
- We also pass a `&amp;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&lt;Font&gt;. 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&lt;T&gt;` into `Box&lt;[T]&gt;` and `String` into `Box&lt;str&gt;`

The `ResMut&lt;TextPipeline&gt;` argument to  `measure_text_system` doesn't exist anymore. If you were calling this system manually, you should remove the argument.
diff --git a/crates/bevy_text/src/pipeline.rs b/crates/bevy_text/src/pipeline.rs
@@ -1,4 +1,4 @@
-use ab_glyph::{PxScale, ScaleFont};
+use ab_glyph::{Font as AbFont, PxScale, ScaleFont};
 use bevy_asset::{Assets, Handle, HandleId};
 use bevy_ecs::component::Component;
 use bevy_ecs::system::Resource;
@@ -7,11 +7,12 @@ use bevy_render::texture::Image;
 use bevy_sprite::TextureAtlas;
 use bevy_utils::HashMap;
 
-use glyph_brush_layout::{FontId, GlyphPositioner, SectionGeometry, SectionText};
+use glyph_brush_layout::{FontId, GlyphPositioner, SectionGeometry, SectionText, ToSectionText};
 
 use crate::{
     error::TextError, glyph_brush::GlyphBrush, scale_value, BreakLineOn, Font, FontAtlasSet,
-    FontAtlasWarning, PositionedGlyph, TextAlignment, TextSection, TextSettings, YAxisOrientation,
+    FontAtlasWarning, PositionedGlyph, Text, TextAlignment, TextSection, TextSettings,
+    YAxisOrientation,
 };
 
 #[derive(Default, Resource)]
@@ -117,116 +118,93 @@ impl TextPipeline {
 
         Ok(TextLayoutInfo { glyphs, size })
     }
+}
 
-    pub fn create_text_measure(
-        &mut self,
+#[derive(Debug, Clone)]
+pub struct TextMeasureSection {
+    pub text: Box<str>,
+    pub scale: f32,
+    pub font_id: FontId,
+}
+
+#[derive(Debug, Clone, Default)]
+pub struct TextMeasureInfo {
+    pub fonts: Box<[ab_glyph::FontArc]>,
+    pub sections: Box<[TextMeasureSection]>,
+    pub text_alignment: TextAlignment,
+    pub linebreak_behavior: glyph_brush_layout::BuiltInLineBreaker,
+    pub min: Vec2,
+    pub max: Vec2,
+}
+
+impl TextMeasureInfo {
+    pub fn from_text(
+        text: &Text,
         fonts: &Assets<Font>,
-        sections: &[TextSection],
         scale_factor: f64,
-        text_alignment: TextAlignment,
-        linebreak_behaviour: BreakLineOn,
     ) -> Result<TextMeasureInfo, TextError> {
-        let mut auto_fonts = Vec::with_capacity(sections.len());
-        let mut scaled_fonts = Vec::with_capacity(sections.len());
-        let sections = sections
+        let sections = &text.sections;
+        for section in sections {
+            if !fonts.contains(&section.style.font) {
+                return Err(TextError::NoSuchFont);
+            }
+        }
+        let (auto_fonts, sections) = sections
             .iter()
             .enumerate()
             .map(|(i, section)| {
-                let font = fonts
-                    .get(&section.style.font)
-                    .ok_or(TextError::NoSuchFont)?;
-                let font_size = scale_value(section.style.font_size, scale_factor);
-                auto_fonts.push(font.font.clone());
-                let px_scale_font = ab_glyph::Font::into_scaled(font.font.clone(), font_size);
-                scaled_fonts.push(px_scale_font);
-
-                let section = TextMeasureSection {
-                    font_id: FontId(i),
-                    scale: PxScale::from(font_size),
-                    text: section.value.clone(),
-                };
-
-                Ok(section)
+                // SAFETY: we exited early earlier in this function if
+                // one of the fonts was missing.
+                let font = unsafe { fonts.get(&section.style.font).unwrap_unchecked() };
+                (
+                    font.font.clone(),
+                    TextMeasureSection {
+                        font_id: FontId(i),
+                        scale: scale_value(section.style.font_size, scale_factor),
+                        text: section.value.clone().into_boxed_str(),
+                    },
+                )
             })
-            .collect::<Result<Vec<_>, _>>()?;
+            .unzip();
 
-        Ok(TextMeasureInfo::new(
+        Ok(Self::new(
             auto_fonts,
-            scaled_fonts,
             sections,
-            text_alignment,
-            linebreak_behaviour.into(),
+            text.alignment,
+            text.linebreak_behavior.into(),
         ))
     }
-}
-
-#[derive(Debug, Clone)]
-pub struct TextMeasureSection {
-    pub text: String,
-    pub scale: PxScale,
-    pub font_id: FontId,
-}
-
-#[derive(Debug, Clone)]
-pub struct TextMeasureInfo {
-    pub fonts: Vec<ab_glyph::FontArc>,
-    pub scaled_fonts: Vec<ab_glyph::PxScaleFont<ab_glyph::FontArc>>,
-    pub sections: Vec<TextMeasureSection>,
-    pub text_alignment: TextAlignment,
-    pub linebreak_behaviour: glyph_brush_layout::BuiltInLineBreaker,
-    pub min_width_content_size: Vec2,
-    pub max_width_content_size: Vec2,
-}
-
-impl TextMeasureInfo {
     fn new(
         fonts: Vec<ab_glyph::FontArc>,
-        scaled_fonts: Vec<ab_glyph::PxScaleFont<ab_glyph::FontArc>>,
         sections: Vec<TextMeasureSection>,
         text_alignment: TextAlignment,
-        linebreak_behaviour: glyph_brush_layout::BuiltInLineBreaker,
+        linebreak_behavior: glyph_brush_layout::BuiltInLineBreaker,
     ) -> Self {
         let mut info = Self {
-            fonts,
-            scaled_fonts,
-            sections,
+            fonts: fonts.into_boxed_slice(),
+            sections: sections.into_boxed_slice(),
             text_alignment,
-            linebreak_behaviour,
-            min_width_content_size: Vec2::ZERO,
-            max_width_content_size: Vec2::ZERO,
+            linebreak_behavior,
+            min: Vec2::ZERO,
+            max: Vec2::ZERO,
         };
 
-        let section_texts = info.prepare_section_texts();
-        let min =
-            info.compute_size_from_section_texts(&section_texts, Vec2::new(0.0, f32::INFINITY));
-        let max = info.compute_size_from_section_texts(
-            &section_texts,
-            Vec2::new(f32::INFINITY, f32::INFINITY),
-        );
-        info.min_width_content_size = min;
-        info.max_width_content_size = max;
+        let min = info.compute_size(Vec2::new(0.0, f32::INFINITY));
+        let max = info.compute_size(Vec2::INFINITY);
+        info.min = min;
+        info.max = max;
         info
     }
 
-    fn prepare_section_texts(&self) -> Vec<SectionText> {
-        self.sections
-            .iter()
-            .map(|section| SectionText {
-                font_id: section.font_id,
-                scale: section.scale,
-                text: &section.text,
-            })
-            .collect::<Vec<_>>()
-    }
-
-    fn compute_size_from_section_texts(&self, sections: &[SectionText], bounds: Vec2) -> Vec2 {
+    pub fn compute_size(&self, bounds: Vec2) -> Vec2 {
+        let sections = &self.sections;
         let geom = SectionGeometry {
             bounds: (bounds.x, bounds.y),
             ..Default::default()
         };
         let section_glyphs = glyph_brush_layout::Layout::default()
             .h_align(self.text_alignment.into())
-            .line_breaker(self.linebreak_behaviour)
+            .line_breaker(self.linebreak_behavior)
             .calculate_glyphs(&self.fonts, &geom, sections);
 
         let mut min_x: f32 = std::f32::MAX;
@@ -235,7 +213,10 @@ impl TextMeasureInfo {
         let mut max_y: f32 = std::f32::MIN;
 
         for sg in section_glyphs {
-            let scaled_font = &self.scaled_fonts[sg.section_index];
+            let font = &self.fonts[sg.section_index];
+            let font_size = self.sections[sg.section_index].scale;
+            let scaled_font = font.into_scaled(font_size);
+
             let glyph = &sg.glyph;
             // The fonts use a coordinate system increasing upwards so ascent is a positive value
             // and descent is negative, but Bevy UI uses a downwards increasing coordinate system,
@@ -248,9 +229,14 @@ impl TextMeasureInfo {
 
         Vec2::new(max_x - min_x, max_y - min_y)
     }
-
-    pub fn compute_size(&self, bounds: Vec2) -> Vec2 {
-        let sections = self.prepare_section_texts();
-        self.compute_size_from_section_texts(&sections, bounds)
+}
+impl ToSectionText for TextMeasureSection {
+    #[inline(always)]
+    fn to_section_text(&self) -> SectionText<'_> {
+        SectionText {
+            text: &self.text,
+            scale: PxScale::from(self.scale),
+            font_id: self.font_id,
+        }
     }
 }
diff --git a/crates/bevy_text/src/text.rs b/crates/bevy_text/src/text.rs
@@ -140,11 +140,12 @@ impl TextSection {
 }
 
 /// Describes horizontal alignment preference for positioning & bounds.
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Reflect, Serialize, Deserialize)]
+#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Hash, Reflect, Serialize, Deserialize)]
 #[reflect(Serialize, Deserialize)]
 pub enum TextAlignment {
     /// Leftmost character is immediately to the right of the render position.<br/>
     /// Bounds start from the render position and advance rightwards.
+    #[default]
     Left,
     /// Leftmost & rightmost characters are equidistant to the render position.<br/>
     /// Bounds start from the render position and advance equally left & right.
diff --git a/crates/bevy_ui/src/widget/text.rs b/crates/bevy_ui/src/widget/text.rs
@@ -53,20 +53,17 @@ impl Measure for TextMeasure {
         _available_height: AvailableSpace,
     ) -> Vec2 {
         let x = width.unwrap_or_else(|| match available_width {
-            AvailableSpace::Definite(x) => x.clamp(
-                self.info.min_width_content_size.x,
-                self.info.max_width_content_size.x,
-            ),
-            AvailableSpace::MinContent => self.info.min_width_content_size.x,
-            AvailableSpace::MaxContent => self.info.max_width_content_size.x,
+            AvailableSpace::Definite(x) => x.clamp(self.info.min.x, self.info.max.x),
+            AvailableSpace::MinContent => self.info.min.x,
+            AvailableSpace::MaxContent => self.info.max.x,
         });
 
         height
             .map_or_else(
                 || match available_width {
                     AvailableSpace::Definite(_) => self.info.compute_size(Vec2::new(x, f32::MAX)),
-                    AvailableSpace::MinContent => Vec2::new(x, self.info.min_width_content_size.y),
-                    AvailableSpace::MaxContent => Vec2::new(x, self.info.max_width_content_size.y),
+                    AvailableSpace::MinContent => Vec2::new(x, self.info.min.y),
+                    AvailableSpace::MaxContent => Vec2::new(x, self.info.max.y),
                 },
                 |y| Vec2::new(x, y),
             )
@@ -77,24 +74,15 @@ impl Measure for TextMeasure {
 #[inline]
 fn create_text_measure(
     fonts: &Assets<Font>,
-    text_pipeline: &mut TextPipeline,
     scale_factor: f64,
     text: Ref<Text>,
     mut content_size: Mut<ContentSize>,
     mut text_flags: Mut<TextFlags>,
 ) {
-    match text_pipeline.create_text_measure(
-        fonts,
-        &text.sections,
-        scale_factor,
-        text.alignment,
-        text.linebreak_behavior,
-    ) {
+    match TextMeasureInfo::from_text(&text, fonts, scale_factor) {
         Ok(measure) => {
             if text.linebreak_behavior == BreakLineOn::NoWrap {
-                content_size.set(FixedMeasure {
-                    size: measure.max_width_content_size,
-                });
+                content_size.set(FixedMeasure { size: measure.max });
             } else {
                 content_size.set(TextMeasure { info: measure });
             }
@@ -120,7 +108,6 @@ pub fn measure_text_system(
     fonts: Res<Assets<Font>>,
     windows: Query<&Window, With<PrimaryWindow>>,
     ui_scale: Res<UiScale>,
-    mut text_pipeline: ResMut<TextPipeline>,
     mut text_query: Query<(Ref<Text>, &mut ContentSize, &mut TextFlags), With<Node>>,
 ) {
     let window_scale_factor = windows
@@ -135,29 +122,15 @@ pub fn measure_text_system(
         // scale factor unchanged, only create new measure funcs for modified text
         for (text, content_size, text_flags) in text_query.iter_mut() {
             if text.is_changed() || text_flags.needs_new_measure_func {
-                create_text_measure(
-                    &fonts,
-                    &mut text_pipeline,
-                    scale_factor,
-                    text,
-                    content_size,
-                    text_flags,
-                );
+                create_text_measure(&fonts, scale_factor, text, content_size, text_flags);
             }
         }
     } else {
         // scale factor changed, create new measure funcs for all text
         *last_scale_factor = scale_factor;
 
         for (text, content_size, text_flags) in text_query.iter_mut() {
-            create_text_measure(
-                &fonts,
-                &mut text_pipeline,
-                scale_factor,
-                text,
-                content_size,
-                text_flags,
-            );
+            create_text_measure(&fonts, scale_factor, text, content_size, text_flags);
         }
     }
 }
