Merge pull request #947 from googlefonts/decompose-before-flattening

must decompose transformed components before flattening deep nested components

fontir/src/glyph.rs

@@ -298,6 +298,42 @@ fn flatten_glyph(context: &Context, glyph: &Glyph) -> Result<(), BadGlyph> {
     Ok(())
 }
 
+/// Run some optional transformations on the glyphs listed.
+///
+/// This includes decomposing components with non-identity 2x2 transforms
+/// and flattening nested composite glyphs so that they all have depth 1
+/// (no components that reference components).
+fn apply_optional_transformations(
+    context: &Context,
+    glyph_order: &GlyphOrder,
+) -> Result<(), BadGlyph> {
+    // If both --flatten-components and --decompose-transformed-components flags
+    // are set, we want to decompose any transformed components first and *then*
+    // flatten the rest. That's how fontmake (ufo2ft) does, and also tends to
+    // keep more components, which usually means smaller glyf size.
+    // https://github.com/googlefonts/fontc/issues/929
+    if context
+        .flags
+        .contains(Flags::DECOMPOSE_TRANSFORMED_COMPONENTS)
+    {
+        for glyph_name in glyph_order.iter() {
+            let glyph = context.glyphs.get(&WorkId::Glyph(glyph_name.clone()));
+            if glyph.has_nonidentity_2x2() {
+                convert_components_to_contours(context, &glyph)?;
+            }
+        }
+    }
+
+    if context.flags.contains(Flags::FLATTEN_COMPONENTS) {
+        for glyph_name in glyph_order.iter() {
+            let glyph = context.glyphs.get(&WorkId::Glyph(glyph_name.clone()));
+            flatten_glyph(context, &glyph)?;
+        }
+    }
+
+    Ok(())
+}
+
 fn ensure_notdef_exists_and_is_gid_0(
     context: &Context,
     glyph_order: &mut GlyphOrder,
@@ -398,24 +434,7 @@ impl Work<Context, WorkId, Error> for GlyphOrderWork {
         }
         drop(original_glyphs); // lets not accidentally use that from here on
 
-        if context.flags.contains(Flags::FLATTEN_COMPONENTS) {
-            for glyph_name in new_glyph_order.iter() {
-                let glyph = context.glyphs.get(&WorkId::Glyph(glyph_name.clone()));
-                flatten_glyph(context, &glyph)?;
-            }
-        }
-
-        if context
-            .flags
-            .contains(Flags::DECOMPOSE_TRANSFORMED_COMPONENTS)
-        {
-            for glyph_name in new_glyph_order.iter() {
-                let glyph = context.glyphs.get(&WorkId::Glyph(glyph_name.clone()));
-                if glyph.has_nonidentity_2x2() {
-                    convert_components_to_contours(context, &glyph)?;
-                }
-            }
-        }
+        apply_optional_transformations(context, &new_glyph_order)?;
 
         // Resolve component references to glyphs that are not retained by conversion to contours
         // Glyphs have to have consistent components at this point so it's safe to just check the default
@@ -524,6 +543,14 @@ mod tests {
             context.glyphs.set(self.shallow_component.clone());
             context.glyphs.set(self.deep_component.clone());
         }
+
+        fn glyph_order(&self) -> GlyphOrder {
+            let mut order = GlyphOrder::new();
+            order.insert(self.simple_glyph.name.clone());
+            order.insert(self.shallow_component.name.clone());
+            order.insert(self.deep_component.name.clone());
+            order
+        }
     }
 
     fn deep_component() -> DeepComponent {
@@ -871,6 +898,14 @@ mod tests {
         assert!(glyph.has_consistent_components());
     }
 
+    fn assert_is_simple_glyph(context: &Context, glyph_name: GlyphName) {
+        let glyph = context.glyphs.get(&WorkId::Glyph(glyph_name));
+        assert!(glyph
+            .sources()
+            .values()
+            .all(|inst| !inst.contours.is_empty() && inst.components.is_empty()));
+    }
+
     fn assert_is_flattened_component(context: &Context, glyph_name: GlyphName) {
         let glyph = context.glyphs.get(&WorkId::Glyph(glyph_name));
         for (loc, inst) in glyph.sources().iter() {
@@ -905,4 +940,29 @@ mod tests {
         flatten_glyph(&context, &test_data.deep_component).unwrap();
         assert_is_flattened_component(&context, test_data.deep_component.name);
     }
+
+    #[test]
+    fn decompose_transformed_and_flatten_components() {
+        // when both flags are set, the flattening should happen last, after
+        // the decomposition of the transformed components
+        // https://github.com/googlefonts/fontc/issues/929
+        let test_data = deep_component();
+        let mut context = test_context();
+        context
+            .flags
+            .set(Flags::DECOMPOSE_TRANSFORMED_COMPONENTS, true);
+        context.flags.set(Flags::FLATTEN_COMPONENTS, true);
+        test_data.write_to(&context);
+
+        apply_optional_transformations(&context, &test_data.glyph_order()).unwrap();
+
+        // the shallow_component had a non-identity 2x2 transform so it was
+        // converted to a simple glyph
+        assert_is_simple_glyph(&context, test_data.shallow_component.name);
+        // the deep_component glyph should still be a composite but no longer nested;
+        // if flattening happened first, it would have been converted to a simple glyph,
+        // because the non-id 2x2 transform of the shallow_component would have
+        // infected the deep_component and caused it to be decomposed.
+        assert_is_flattened_component(&context, test_data.deep_component.name);
+    }
 }