Make nested components fix depth first (still other problems to fix here)

Author: simoncozens

profile-universal/src/checks/nested_components.rs

@@ -1,9 +1,14 @@
 use std::collections::HashMap;
 
 use fontspector_checkapi::{fixfont, prelude::*, testfont, FileTypeConvert};
-use read_fonts::{tables::glyf::Glyph, TableProvider};
+use read_fonts::{
+    tables::{
+        glyf::{Glyf, Glyph},
+        loca::Loca,
+    },
+    TableProvider,
+};
 use skrifa::GlyphId;
-use write_fonts::tables::glyf::CompositeGlyph;
 
 use super::transformed_components::decompose_components_impl;
 
@@ -64,6 +69,19 @@ fn nested_components(f: &Testable, context: &Context) -> CheckFnResult {
     }
 }
 
+fn get_depth(glyph_id: GlyphId, loca: &Loca, glyf: &Glyf) -> u32 {
+    let mut depth = 0;
+    let glyph_entry = loca.get_glyf(glyph_id, glyf).ok().flatten();
+    if let Some(Glyph::Composite(composite)) = glyph_entry {
+        depth = 1 + composite
+            .components()
+            .map(|component| get_depth(component.glyph.into(), loca, glyf))
+            .max()
+            .unwrap_or(0)
+    }
+    depth
+}
+
 fn decompose_nested_components(t: &Testable) -> FixFnResult {
     let font = fixfont!(t);
     let loca = font
@@ -74,22 +92,16 @@ fn decompose_nested_components(t: &Testable) -> FixFnResult {
         .font()
         .glyf()
         .map_err(|_| "glyf table not found".to_string())?;
-    let composite_glyphs: HashMap<GlyphId, _> = font
-        .all_glyphs()
-        .filter_map(|glyphid| {
-            if let Some(Glyph::Composite(composite)) = loca.get_glyf(glyphid, &glyf).ok()? {
-                Some((glyphid, composite))
-            } else {
-                None
-            }
-        })
-        .collect();
+    let mut depths = HashMap::new();
+    for glyph in font.all_glyphs() {
+        depths.insert(glyph, get_depth(glyph, &loca, &glyf));
+    }
+    // Drop all with depth <2
+    depths.retain(|_, depth| *depth > 1);
+    // Sort by depth, descending
+    let mut sorted_glyphs: Vec<GlyphId> = depths.keys().copied().collect();
+    sorted_glyphs.sort_by_key(|&glyph| depths[&glyph]);
+    sorted_glyphs.reverse();
 
-    let has_nested_components = |composite: &CompositeGlyph| {
-        composite
-            .components()
-            .iter()
-            .any(|component| composite_glyphs.contains_key(&component.glyph.into()))
-    };
-    decompose_components_impl(t, has_nested_components)
+    decompose_components_impl(t, &sorted_glyphs)
 }

profile-universal/src/checks/transformed_components.rs

@@ -1,11 +1,13 @@
 use fontspector_checkapi::{fixfont, prelude::*, testfont, FileTypeConvert};
+use hashbrown::HashMap;
+use itertools::Itertools;
 use kurbo::Affine;
 use read_fonts::{
     tables::glyf::{Anchor, CurvePoint, Glyph, Transform},
     types::F2Dot14,
     FontData, TableProvider,
 };
-use skrifa::GlyphId16;
+use skrifa::GlyphId;
 use write_fonts::{
     from_obj::ToOwnedObj,
     tables::glyf::{
@@ -98,20 +100,40 @@ fn transformed_components(f: &Testable, context: &Context) -> CheckFnResult {
     }
 }
 
-fn bad_composite(c: &CompositeGlyph) -> bool {
-    c.components().iter().any(|component| {
-        !transform_is_linear(component.transform) || transform_is_semi_flipped(component.transform)
-    })
-}
-
 fn decompose_transformed_components(t: &Testable) -> FixFnResult {
-    decompose_components_impl(t, bad_composite)
+    let f = fixfont!(t);
+    let loca = f
+        .font()
+        .loca(None)
+        .map_err(|_| "loca table not found".to_string())?;
+    let glyf = f
+        .font()
+        .glyf()
+        .map_err(|_| "glyf table not found".to_string())?;
+    let bad_composites = f
+        .all_glyphs()
+        .filter_map(|gid| {
+            loca.get_glyf(gid, &glyf)
+                .ok()
+                .flatten()
+                .and_then(|glyph| match glyph {
+                    Glyph::Composite(composite)
+                        if composite.components().any(|component| {
+                            !transform_is_linear(component.transform)
+                                || transform_is_semi_flipped(component.transform)
+                        }) =>
+                    {
+                        Some(gid)
+                    }
+                    _ => None,
+                })
+        })
+        .collect::<Vec<_>>();
+
+    decompose_components_impl(t, &bad_composites)
 }
 
-pub(crate) fn decompose_components_impl(
-    t: &Testable,
-    bad_composite: impl Fn(&CompositeGlyph) -> bool,
-) -> FixFnResult {
+pub(crate) fn decompose_components_impl(t: &Testable, decompose_order: &[GlyphId]) -> FixFnResult {
     let f = fixfont!(t);
     let mut new_font = FontBuilder::new();
     let mut builder = GlyfLocaBuilder::new();
@@ -123,31 +145,36 @@ pub(crate) fn decompose_components_impl(
         .font()
         .glyf()
         .map_err(|_| "glyf table not found".to_string())?;
-    let all_glyphs: Vec<WriteGlyph> = f
+    let mut all_glyphs: HashMap<GlyphId, WriteGlyph> = f
         .all_glyphs()
         .map(|gid| {
-            loca.get_glyf(gid, &glyf).map(|option_glyph| {
-                option_glyph
-                    .map(|glyph| {
-                        let g: WriteGlyph = glyph.to_owned_obj(FontData::new(&[]));
-                        g
-                    })
-                    .unwrap_or(WriteGlyph::Empty)
-            })
+            loca.get_glyf(gid, &glyf)
+                .map(|option_glyph| {
+                    option_glyph
+                        .map(|glyph| {
+                            let g: WriteGlyph = glyph.to_owned_obj(FontData::new(&[]));
+                            g
+                        })
+                        .unwrap_or(WriteGlyph::Empty)
+                })
+                .map(|x| (gid, x))
         })
-        .collect::<Result<Vec<WriteGlyph>, _>>()
+        .collect::<Result<HashMap<GlyphId, WriteGlyph>, _>>()
         .map_err(|x| x.to_string())?;
-    for glyph in all_glyphs.iter() {
-        match glyph {
-            WriteGlyph::Composite(composite) if bad_composite(composite) => {
+    for glyph_id in decompose_order {
+        let current_glyph = all_glyphs.get(glyph_id).ok_or("glyph not found")?;
+        match current_glyph {
+            WriteGlyph::Composite(composite) => {
                 let new_glyph = decompose_glyph(composite, &all_glyphs)?;
-                builder.add_glyph(&new_glyph).map_err(|x| x.to_string())?;
-            }
-            WriteGlyph::Composite(_) | WriteGlyph::Empty | WriteGlyph::Simple(_) => {
-                builder.add_glyph(glyph).map_err(|x| x.to_string())?;
+                all_glyphs.insert(*glyph_id, new_glyph);
             }
+            WriteGlyph::Empty | WriteGlyph::Simple(_) => {}
         }
     }
+    for glyph_id in all_glyphs.keys().sorted_by(|a, b| a.cmp(b)) {
+        let glyph = all_glyphs.get(glyph_id).ok_or("glyph not found")?;
+        builder.add_glyph(glyph).map_err(|x| x.to_string())?;
+    }
     let (new_glyph, new_loca, _head_format) = builder.build();
     new_font.add_table(&new_glyph).map_err(|x| x.to_string())?;
     new_font.add_table(&new_loca).map_err(|x| x.to_string())?;
@@ -160,12 +187,12 @@ pub(crate) fn decompose_components_impl(
 
 fn decompose_glyph(
     composite: &CompositeGlyph,
-    glyphs: &[WriteGlyph],
+    glyphs: &HashMap<GlyphId, WriteGlyph>,
 ) -> Result<WriteGlyph, String> {
     let mut new_glyph = SimpleGlyph::default();
     for component in composite.components() {
         for (gid, affine) in flatten_component(glyphs, component)? {
-            let component_glyph = glyphs.get(gid.to_u16() as usize).ok_or("glyph not found")?;
+            let component_glyph = glyphs.get(&gid).ok_or("glyph not found")?;
             match component_glyph {
                 WriteGlyph::Simple(simple) => {
                     new_glyph
@@ -193,16 +220,16 @@ fn transform_contour(c: &Contour, affine: Affine) -> Contour {
 }
 
 fn flatten_component(
-    glyphs: &[WriteGlyph],
+    glyphs: &HashMap<GlyphId, WriteGlyph>,
     component: &Component,
-) -> Result<Vec<(GlyphId16, kurbo::Affine)>, String> {
+) -> Result<Vec<(GlyphId, kurbo::Affine)>, String> {
     let glyph = glyphs
-        .get(component.glyph.to_u16() as usize)
+        .get(&GlyphId::from(component.glyph))
         .ok_or("glyph not found")?;
     Ok(match glyph {
         WriteGlyph::Empty => vec![],
         WriteGlyph::Simple(_) => vec![(
-            component.glyph,
+            component.glyph.into(),
             to_kurbo_transform(&component.transform, &component.anchor),
         )],
         WriteGlyph::Composite(composite_glyph) => {