Move char_style_indices out of the per-item ShapeOptions (#741)

LLM Contributions: Review.

The style indices are text-wide, whereas the options are per-item since
#727. Fixes the TODO from
#727 (comment).

Author: tomcur

parley/src/shape/mod.rs

@@ -173,7 +173,6 @@ pub(crate) fn shape_text<'a, B: Brush>(
                     font_size: item_style.font_size,
                     features: &style_features[item_style_index as usize],
                     variations: rcx.variations(item_style.font_variations).unwrap_or(&[]),
-                    char_style_indices,
                 },
             })
         })
@@ -184,6 +183,7 @@ pub(crate) fn shape_text<'a, B: Brush>(
     scx.shape_text(
         text,
         analysis,
+        char_style_indices,
         items,
         font_selector,
         &mut layout.data.shaped_text,

parley_engine/src/shape/atom.rs

@@ -858,10 +858,16 @@ mod tests {
                 language: None,
                 features: &[],
                 variations: &[],
-                char_style_indices: &char_style_indices,
             },
         }];
-        shaper.shape_text(text, &analysis, items, SingleFont(font), &mut shaped);
+        shaper.shape_text(
+            text,
+            &analysis,
+            &char_style_indices,
+            items,
+            SingleFont(font),
+            &mut shaped,
+        );
         shaped
     }
 

parley_engine/src/shape/shaped_text.rs

@@ -236,6 +236,7 @@ impl ShapedText {
         item: &Segment,
         options: &ShapeOptions<'_>,
         char_info: &[CharInfo],
+        char_style_indices: &[u16],
         font: &FontInstance,
         glyph_buffer: &harfrust::GlyphBuffer,
         normalized_coords: &[harfrust::NormalizedCoord],
@@ -318,7 +319,7 @@ impl ShapedText {
         for (((byte_offset, ch), info), style_index) in text[range.byte_range.clone()]
             .char_indices()
             .zip(&char_info[range.char_range.clone()])
-            .zip(&options.char_style_indices[range.char_range.clone()])
+            .zip(&char_style_indices[range.char_range.clone()])
         {
             self.characters.push(Character {
                 text_byte_start: (range.byte_range.start + byte_offset) as u32,
@@ -340,7 +341,6 @@ impl ShapedText {
                 scale_factor,
                 glyph_infos.iter(),
                 glyph_positions.iter(),
-                &options.char_style_indices[range.char_range.clone()],
                 &self.characters,
                 characters_start,
             );
@@ -351,7 +351,6 @@ impl ShapedText {
                 scale_factor,
                 glyph_infos.iter().rev(),
                 glyph_positions.iter().rev(),
-                &options.char_style_indices[range.char_range.clone()],
                 &self.characters,
                 characters_start,
             );
@@ -429,7 +428,6 @@ pub struct ShapedRun {
 /// * `glyph_infos` - `HarfRust` glyph information in logical order (i.e., reversed for RTL runs).
 /// * `glyph_positions` - `HarfRust` glyph positioning data in logical order (i.e., reversed for RTL
 ///   runs).
-/// * `char_style_indices` - The run's slice of per-character style indices, indexed by cluster ID.
 /// * `characters` must contain the shaped characters whose clusters we're now processing, starting at
 ///   index `characters_start`.
 /// * `characters_start` - See `characters`.
@@ -439,7 +437,6 @@ fn process_shaped_clusters<'a>(
     scale_factor: f32,
     glyph_infos: impl Iterator<Item = &'a harfrust::GlyphInfo>,
     glyph_positions: impl Iterator<Item = &'a harfrust::GlyphPosition>,
-    char_style_indices: &[u16],
     characters: &[Character],
     characters_start: usize,
 ) {
@@ -457,7 +454,6 @@ fn process_shaped_clusters<'a>(
     fn flush(
         cluster: &mut Cluster,
         char_end: usize,
-        style_index: u16,
         characters: &[Character],
         shaped_clusters: &mut Vec<ShapedCluster>,
     ) {
@@ -479,7 +475,7 @@ fn process_shaped_clusters<'a>(
 
         shaped_clusters.push(ShapedCluster {
             chars_range: (cluster.characters_start as u32, char_end as u32),
-            style_index,
+            style_index: first_character.style_index,
             flags: ShapedClusterFlags::new(glyph_len)
                 .with_grapheme_start(first_character.grapheme_start)
                 // TODO: fill with actual shaping data (`parley` currently just ignores this)
@@ -502,14 +498,7 @@ fn process_shaped_clusters<'a>(
     for (glyph_info, glyph_pos) in glyph_infos.zip(glyph_positions) {
         if glyph_info.cluster != cluster.id {
             let char_end = characters_start + glyph_info.cluster as usize;
-            let style_index = char_style_indices[cluster.id as usize];
-            flush(
-                &mut cluster,
-                char_end,
-                style_index,
-                characters,
-                shaped_clusters,
-            );
+            flush(&mut cluster, char_end, characters, shaped_clusters);
 
             cluster = Cluster {
                 id: glyph_info.cluster,
@@ -542,14 +531,7 @@ fn process_shaped_clusters<'a>(
     }
 
     // Flush the final cluster.
-    let style_index = char_style_indices[cluster.id as usize];
-    flush(
-        &mut cluster,
-        characters.len(),
-        style_index,
-        characters,
-        shaped_clusters,
-    );
+    flush(&mut cluster, characters.len(), characters, shaped_clusters);
 }
 
 #[cfg(test)]
@@ -621,10 +603,16 @@ mod tests {
                 language: None,
                 features: &[],
                 variations: &[],
-                char_style_indices: &char_style_indices,
             },
         }];
-        shaper.shape_text(text, &analysis, items, SingleFont(font), &mut shaped);
+        shaper.shape_text(
+            text,
+            &analysis,
+            &char_style_indices,
+            items,
+            SingleFont(font),
+            &mut shaped,
+        );
         shaped
     }
 
@@ -732,7 +720,6 @@ mod tests {
                     language: None,
                     features: &[],
                     variations: &[],
-                    char_style_indices: &char_style_indices,
                 },
             },
             Item {
@@ -742,11 +729,17 @@ mod tests {
                     language: None,
                     features: &[],
                     variations: &[],
-                    char_style_indices: &char_style_indices,
                 },
             },
         ];
-        shaper.shape_text(text, &analysis, items, SingleFont(font), &mut shaped);
+        shaper.shape_text(
+            text,
+            &analysis,
+            &char_style_indices,
+            items,
+            SingleFont(font),
+            &mut shaped,
+        );
 
         let grapheme_starts: Vec<bool> =
             shaped.characters.iter().map(|c| c.grapheme_start).collect();

parley_engine/src/shape/shaper.rs

@@ -34,11 +34,6 @@ pub struct ShapeOptions<'a> {
     pub features: &'a [FontFeature],
     /// The font variations that are constant over an item.
     pub variations: &'a [FontVariation],
-    /// The per-character style indices.
-    // TODO: rename to something like `user_data` (s.t. we don't assume it's a style per se).
-    // TODO: probably move this out of `ShapeOptions`, and supply it as a parameter on
-    // `Shaper::shape_text`.
-    pub char_style_indices: &'a [u16],
 }
 
 /// The font instance to shape an item with.
@@ -92,6 +87,10 @@ impl Shaper {
     /// split on properties like shaping-relevant style changes (e.g., font size) or properties like
     /// language.
     ///
+    /// `char_style_indices` holds per-character style indices; these are copied onto
+    /// [`Character`][super::Character`] and [`ShapedCluster`][super::ShapedCluster]. A shaped
+    /// cluster's style index is that of its logically first constituent character.
+    ///
     /// Characters that don't have a particular script have their script resolved based on
     /// surrounding context (see [`Segment::script`]).
     ///
@@ -106,6 +105,8 @@ impl Shaper {
         &mut self,
         text: &str,
         analysis: &Analysis,
+        // TODO: rename to something like `user_data` (s.t. we don't assume it's a style per se).
+        char_style_indices: &[u16],
         items: impl IntoIterator<Item = Item<'options>>,
         mut select_font: impl FontSelector,
         shaped_text: &mut ShapedText,
@@ -114,6 +115,12 @@ impl Shaper {
         shaped_text.reserve(text.len());
 
         let char_count = analysis.char_info().len();
+        debug_assert_eq!(
+            char_style_indices.len(),
+            char_count,
+            "The number of character style indices must be equal to the character count"
+        );
+
         let mut previous_item_end = 0;
         let mut itemizer = analysis.itemize(text);
 
@@ -142,6 +149,7 @@ impl Shaper {
                     &item.options,
                     &mut select_font,
                     analysis.char_info(),
+                    char_style_indices,
                     shaped_text,
                 )
                 .is_err()
@@ -182,6 +190,7 @@ fn shape_segment(
     options: &ShapeOptions<'_>,
     select_font: &mut impl FontSelector,
     char_info: &[CharInfo],
+    char_style_indices: &[u16],
     shaped_text: &mut ShapedText,
 ) -> Result<(), ()> {
     select_font.begin_segment(item, options);
@@ -193,7 +202,7 @@ fn shape_segment(
 
     // Only process current item
     let item_char_info = &char_info[char_range.start..char_range.end];
-    let item_char_style_indices = &options.char_style_indices[char_range.start..char_range.end];
+    let item_char_style_indices = &char_style_indices[char_range.start..char_range.end];
 
     if item_text.is_empty() {
         return Ok(()); // No clusters
@@ -378,6 +387,7 @@ fn shape_segment(
             item,
             options,
             char_info,
+            char_style_indices,
             &font,
             &glyph_buffer,
             harf_shaper.coords(),