From 4c42e3c0ad18fd94b0514f1f9449e67625a331db Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Fri, 6 Sep 2024 17:54:02 +0100 Subject: [PATCH 1/3] move optional glyph transformations to helper fn to ease testing --- fontir/src/glyph.rs | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/fontir/src/glyph.rs b/fontir/src/glyph.rs index 7edca020..78cbd2a4 100644 --- a/fontir/src/glyph.rs +++ b/fontir/src/glyph.rs @@ -298,6 +298,32 @@ fn flatten_glyph(context: &Context, glyph: &Glyph) -> Result<(), BadGlyph> { Ok(()) } +fn apply_optional_transformations( + context: &Context, + new_glyph_order: &GlyphOrder, +) -> Result<(), BadGlyph> { + 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)?; + } + } + } + + Ok(()) +} + fn ensure_notdef_exists_and_is_gid_0( context: &Context, glyph_order: &mut GlyphOrder, @@ -398,24 +424,7 @@ impl Work 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 From 894f312ee33b1a9fe48004ad6fb3a1d1b30ad68c Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Fri, 6 Sep 2024 18:09:47 +0100 Subject: [PATCH 2/3] test we do the right thing when both --flatten-components and --decompose-transformed-components flags are set currently we do not thus this test fails (we should run them in inverse order) --- fontir/src/glyph.rs | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/fontir/src/glyph.rs b/fontir/src/glyph.rs index 78cbd2a4..ae83a10f 100644 --- a/fontir/src/glyph.rs +++ b/fontir/src/glyph.rs @@ -533,6 +533,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 { @@ -880,6 +888,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() { @@ -914,4 +930,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); + } } From 15a8ade1acfeba87021fe49fca489a274626d54a Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Fri, 6 Sep 2024 18:25:10 +0100 Subject: [PATCH 3/3] decompose transformed components before flattening nested components Fixes #947 The failing test from the previous commit should now pass --- fontir/src/glyph.rs | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/fontir/src/glyph.rs b/fontir/src/glyph.rs index ae83a10f..eca1cd46 100644 --- a/fontir/src/glyph.rs +++ b/fontir/src/glyph.rs @@ -298,22 +298,25 @@ 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, - new_glyph_order: &GlyphOrder, + glyph_order: &GlyphOrder, ) -> Result<(), BadGlyph> { - 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 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 new_glyph_order.iter() { + 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)?; @@ -321,6 +324,13 @@ fn apply_optional_transformations( } } + 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(()) }