From 24d2f1996b9c967351b3be5e39bab31558c81dd2 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Thu, 28 Nov 2024 15:56:40 -0500 Subject: [PATCH 1/4] Add GlyphInstance::path_elements This will be useful for debugging in fontir. --- fontbe/src/glyphs.rs | 17 +---------------- fontir/src/ir.rs | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/fontbe/src/glyphs.rs b/fontbe/src/glyphs.rs index 902fd10b..dfe45a83 100644 --- a/fontbe/src/glyphs.rs +++ b/fontbe/src/glyphs.rs @@ -615,12 +615,7 @@ impl CheckedGlyph { let path_els: HashSet = glyph .sources() .values() - .map(|s| { - s.contours - .iter() - .map(|c| c.elements().iter().map(path_el_type).collect::()) - .collect() - }) + .map(|g| g.path_elements()) .collect(); if path_els.len() > 1 { warn!( @@ -719,16 +714,6 @@ impl CheckedGlyph { } } -fn path_el_type(el: &PathEl) -> &'static str { - match el { - PathEl::MoveTo(..) => "M", - PathEl::LineTo(..) => "L", - PathEl::QuadTo(..) => "Q", - PathEl::CurveTo(..) => "C", - PathEl::ClosePath => "Z", - } -} - fn affine_for(component: &Component) -> Affine { let glyf::Anchor::Offset { x: dx, y: dy } = component.anchor else { panic!("Only offset anchor is supported"); diff --git a/fontir/src/ir.rs b/fontir/src/ir.rs index 63295149..b29b7440 100644 --- a/fontir/src/ir.rs +++ b/fontir/src/ir.rs @@ -1792,6 +1792,28 @@ pub struct GlyphInstance { pub components: Vec, } +impl GlyphInstance { + /// Returns the concatenation of the element types for each outline. + /// + /// These are 'M' for moveto, 'L' for lineto, 'Q' for quadto, 'C' for + /// curveto and 'Z' for closepath. + pub fn path_elements(&self) -> String { + fn path_el_type(el: &PathEl) -> &'static str { + match el { + PathEl::MoveTo(..) => "M", + PathEl::LineTo(..) => "L", + PathEl::QuadTo(..) => "Q", + PathEl::CurveTo(..) => "C", + PathEl::ClosePath => "Z", + } + } + self.contours + .iter() + .flat_map(|c| c.elements().iter().map(path_el_type)) + .collect() + } +} + /// A single glyph component, reference to another glyph. #[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] pub struct Component { From f49c3fbc37f1e0d7b1ec790447b673315a16d075 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Thu, 28 Nov 2024 17:38:39 -0500 Subject: [PATCH 2/4] Ensure glyph normalization is deterministic When converting components to contours we need to make sure we consider dependencies between glyphs. --- fontir/src/glyph.rs | 191 +++++++++++++++++++++++++++++++++++++++----- fontir/src/ir.rs | 10 +++ 2 files changed, 183 insertions(+), 18 deletions(-) diff --git a/fontir/src/glyph.rs b/fontir/src/glyph.rs index 394676e0..19149f24 100644 --- a/fontir/src/glyph.rs +++ b/fontir/src/glyph.rs @@ -3,7 +3,10 @@ //! Notably includes splitting glyphs with contours and components into one new glyph with //! the contours and one updated glyph with no contours that references the new gyph as a component. -use std::collections::{HashMap, HashSet, VecDeque}; +use std::{ + collections::{HashMap, HashSet, VecDeque}, + sync::Arc, +}; use fontdrasil::{ coords::NormalizedLocation, @@ -168,6 +171,101 @@ fn components( .collect() } +// Operations performed on glyphs with mixed contours/components +#[derive(Clone, Copy, PartialEq, Eq)] +enum GlyphOp { + // convert comonents in this mixed glyph into contours + ConvertToContour, + // move contours in this mixed glyph into components + MoveContoursToComponent, +} + +/// Fix glyphs with mixed components/contours. +/// +/// If there are component relationships between these glyphs the leaves have +/// to be handled first. +fn resolve_inconsistencies( + context: &Context, + todo: Vec<(GlyphOp, Arc)>, + order: &mut GlyphOrder, +) -> Result<(), BadGlyph> { + let items = todo + .into_iter() + .map(|(op, glyph)| (glyph.name.clone(), (op, glyph))) + .collect::>(); + + for name in component_ordering_within_glyphs(items.values().map(|(_, g)| g.clone())) { + let (op, glyph) = items.get(&name).unwrap(); + match op { + GlyphOp::ConvertToContour => convert_components_to_contours(context, glyph)?, + GlyphOp::MoveContoursToComponent => { + move_contours_to_new_component(context, order, glyph)? + } + } + } + Ok(()) +} + +// given a sequence of glyphs, return an ordering such that any glyph that +// is a component of another glyph in the input set occurs before it. +// +// this has a funny signature so I can unit test it. +fn component_ordering_within_glyphs( + glyphs: impl Iterator>, +) -> impl Iterator { + // first figure out our order: + let items = glyphs + .map(|glyph| (glyph.name.clone(), glyph)) + .collect::>(); + + // depth of components within the todo list only. + let mut depths = HashMap::new(); + let mut depth_work_queue = Vec::new(); + + // an initial pass to find if any glyphs here use any others as components + for (name, glyph) in &items { + if glyph.component_names().any(|name| items.contains_key(name)) { + depth_work_queue.push(name.clone()); + } else { + depths.insert(name.clone(), 0); + } + } + + let mut progress = depth_work_queue.len(); + while progress > 0 { + progress = depth_work_queue.len(); + depth_work_queue.retain(|next| { + let glyph = items.get(next).unwrap(); + let deepest_component = glyph + .component_names() + .filter(|comp| items.contains_key(*comp)) + .map(|comp| depths.get(comp).copied()) + .try_fold(0, |acc, e| e.map(|e| acc.max(e))); + match deepest_component { + Some(depth) => { + depths.insert(next.clone(), depth + 1); + false + } + None => true, + } + }); + progress -= depth_work_queue.len(); + } + + if !depth_work_queue.is_empty() { + log::warn!("component cycle involving glyphs {depth_work_queue:?}"); + // keep going for now, we'll error out later + depths.extend(depth_work_queue.into_iter().map(|g| (g, i32::MAX))); + } + + let mut work_order = depths + .into_iter() + .map(|(name, depth)| (depth, name)) + .collect::>(); + work_order.sort(); + work_order.into_iter().map(|(_, name)| name) +} + /// Convert a glyph with contours and components to a contour-only, aka simple, glyph /// /// At time of writing we only support this if every instance uses the same set of components. @@ -423,32 +521,34 @@ impl Work for GlyphOrderWork { } } - // Glyphs with paths and components, and glyphs whose component 2x2 transforms vary over designspace - // are not directly supported in fonts. To resolve we must do one of: + // Glyphs with paths and components, and glyphs whose component 2x2 + // transforms vary over designspace are not directly supported in fonts. + // To resolve we must do one of: // 1) need to push their paths to a new glyph that is a component // 2) collapse such glyphs into a simple (contour-only) glyph - // fontmake (Python) prefers option 2. - for glyph_name in new_glyph_order.clone().iter() { + // fontmake (Python) prefers option 2. + let mut todo = Vec::new(); + for glyph_name in new_glyph_order.iter() { let glyph = original_glyphs.get(glyph_name).unwrap(); - let inconsistent_components = !glyph.has_consistent_components(); - if inconsistent_components || has_components_and_contours(glyph) { - if inconsistent_components { + if !glyph.has_consistent_components() { + debug!( + "Coalescing '{glyph_name}' into a simple glyph because \ + component 2x2s vary across the designspace" + ); + todo.push((GlyphOp::ConvertToContour, glyph.clone())); + } else if has_components_and_contours(glyph) { + if context.flags.contains(Flags::PREFER_SIMPLE_GLYPHS) { + todo.push((GlyphOp::ConvertToContour, glyph.clone())); debug!( - "Coalescing'{0}' into a simple glyph because component 2x2s vary across the designspace", - glyph.name + "Coalescing '{glyph_name} into a simple glyph; it has \ + contours and components and prefer simple glyphs is set", ); - convert_components_to_contours(context, glyph)?; - } else if context.flags.contains(Flags::PREFER_SIMPLE_GLYPHS) { - debug!( - "Coalescing '{0}' into a simple glyph because it has contours and components and prefer simple glyphs is set", - glyph.name - ); - convert_components_to_contours(context, glyph)?; } else { - move_contours_to_new_component(context, &mut new_glyph_order, glyph)?; + todo.push((GlyphOp::MoveContoursToComponent, glyph.clone())); } } } + resolve_inconsistencies(context, todo, &mut new_glyph_order)?; drop(original_glyphs); // lets not accidentally use that from here on apply_optional_transformations(context, &new_glyph_order)?; @@ -982,4 +1082,59 @@ mod tests { // infected the deep_component and caused it to be decomposed. assert_is_flattened_component(&context, test_data.deep_component.name); } + + #[derive(Default)] + struct GlyphOrderBuilder(Vec>); + + impl GlyphOrderBuilder { + fn add_glyph(&mut self, name: &str, components: [&str; N]) { + let instance = GlyphInstance { + components: components + .into_iter() + .map(|name| Component { + base: name.into(), + transform: Default::default(), + }) + .collect(), + ..Default::default() + }; + let loc = NormalizedLocation::for_pos(&[("axis", 0.0)]); + let glyph = Glyph::new( + name.into(), + true, + Default::default(), + HashMap::from([(loc, instance)]), + ) + .unwrap(); + self.0.push(Arc::new(glyph)) + } + } + + #[test] + fn component_sorting() { + let mut builder = GlyphOrderBuilder::default(); + builder.add_glyph("a", ["b", "c"]); + builder.add_glyph("b", ["z"]); + builder.add_glyph("c", ["d"]); + builder.add_glyph("d", ["x", "y"]); + builder.add_glyph("e", ["z"]); + + let sorted = component_ordering_within_glyphs(builder.0.into_iter()).collect::>(); + + assert_eq!(sorted, ["b", "d", "e", "c", "a"]); + } + + // all we care about here is that we don't deadlock + #[test] + fn component_sorting_with_cycle() { + let mut builder = GlyphOrderBuilder::default(); + builder.add_glyph("a", ["z"]); + builder.add_glyph("b", ["c"]); + builder.add_glyph("c", ["d"]); + builder.add_glyph("z", ["a"]); // a cycle + // + let sorted = component_ordering_within_glyphs(builder.0.into_iter()).collect::>(); + + assert_eq!(sorted, ["c", "b", "a", "z"]); + } } diff --git a/fontir/src/ir.rs b/fontir/src/ir.rs index b29b7440..5274aec3 100644 --- a/fontir/src/ir.rs +++ b/fontir/src/ir.rs @@ -1548,6 +1548,16 @@ impl Glyph { self.sources.get_mut(loc) } + /// Iterate over the names of all components in all instances. + /// + /// This will return duplicates if multiple instances have identical + /// components (which is normal) + pub(crate) fn component_names(&self) -> impl Iterator { + self.sources + .values() + .flat_map(|inst| inst.components.iter().map(|comp| &comp.base)) + } + /// Does the Glyph use the same components, (name, 2x2 transform), for all instances? /// /// The (glyphname, 2x2 transform) pair is considered for uniqueness. Note that From 9a1e107a6857b07aaaa5d812cc4c1adb9ea187fb Mon Sep 17 00:00:00 2001 From: rsheeter Date: Mon, 2 Dec 2024 15:12:43 -0800 Subject: [PATCH 3/4] Take a swing at a simpler implementation of processing in a safe order --- fontir/src/glyph.rs | 164 +++++++++----------------------------------- 1 file changed, 33 insertions(+), 131 deletions(-) diff --git a/fontir/src/glyph.rs b/fontir/src/glyph.rs index 19149f24..f2caa5bf 100644 --- a/fontir/src/glyph.rs +++ b/fontir/src/glyph.rs @@ -172,7 +172,7 @@ fn components( } // Operations performed on glyphs with mixed contours/components -#[derive(Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] enum GlyphOp { // convert comonents in this mixed glyph into contours ConvertToContour, @@ -181,89 +181,46 @@ enum GlyphOp { } /// Fix glyphs with mixed components/contours. -/// -/// If there are component relationships between these glyphs the leaves have -/// to be handled first. +/// +/// We presume component cycles are checked elsewhere and do not check for them here fn resolve_inconsistencies( context: &Context, - todo: Vec<(GlyphOp, Arc)>, + mut todo: VecDeque<(GlyphOp, Arc)>, order: &mut GlyphOrder, ) -> Result<(), BadGlyph> { - let items = todo - .into_iter() - .map(|(op, glyph)| (glyph.name.clone(), (op, glyph))) - .collect::>(); + let mut pending = todo + .iter() + .map(|(_, g)| g.name.clone()) + .collect::>(); + let mut curr_components = Vec::with_capacity(8); // avoid inner reallocs + 'next_todo: while let Some((op, glyph)) = todo.pop_front() { + // Only fix curr if nothing else that needs fixing is reachable + curr_components.clear(); + curr_components.extend(glyph.component_names().cloned()); + while let Some(component_name) = curr_components.pop() { + if pending.contains(&component_name) { + // We can't got yet + todo.push_back((op, glyph)); + continue 'next_todo; + } + let glyph = context.glyphs.get(&WorkId::Glyph(component_name)); + curr_components.extend(glyph.component_names().cloned()); + } - for name in component_ordering_within_glyphs(items.values().map(|(_, g)| g.clone())) { - let (op, glyph) = items.get(&name).unwrap(); + // Our component graph doesn't reach any pending component, we are a go! + debug!("{op:?} {}", glyph.name); match op { - GlyphOp::ConvertToContour => convert_components_to_contours(context, glyph)?, + GlyphOp::ConvertToContour => convert_components_to_contours(context, &glyph)?, GlyphOp::MoveContoursToComponent => { - move_contours_to_new_component(context, order, glyph)? + move_contours_to_new_component(context, order, &glyph)? } } - } - Ok(()) -} - -// given a sequence of glyphs, return an ordering such that any glyph that -// is a component of another glyph in the input set occurs before it. -// -// this has a funny signature so I can unit test it. -fn component_ordering_within_glyphs( - glyphs: impl Iterator>, -) -> impl Iterator { - // first figure out our order: - let items = glyphs - .map(|glyph| (glyph.name.clone(), glyph)) - .collect::>(); - - // depth of components within the todo list only. - let mut depths = HashMap::new(); - let mut depth_work_queue = Vec::new(); - - // an initial pass to find if any glyphs here use any others as components - for (name, glyph) in &items { - if glyph.component_names().any(|name| items.contains_key(name)) { - depth_work_queue.push(name.clone()); - } else { - depths.insert(name.clone(), 0); - } - } - - let mut progress = depth_work_queue.len(); - while progress > 0 { - progress = depth_work_queue.len(); - depth_work_queue.retain(|next| { - let glyph = items.get(next).unwrap(); - let deepest_component = glyph - .component_names() - .filter(|comp| items.contains_key(*comp)) - .map(|comp| depths.get(comp).copied()) - .try_fold(0, |acc, e| e.map(|e| acc.max(e))); - match deepest_component { - Some(depth) => { - depths.insert(next.clone(), depth + 1); - false - } - None => true, - } - }); - progress -= depth_work_queue.len(); - } - if !depth_work_queue.is_empty() { - log::warn!("component cycle involving glyphs {depth_work_queue:?}"); - // keep going for now, we'll error out later - depths.extend(depth_work_queue.into_iter().map(|g| (g, i32::MAX))); + // I ain't a-gonna pend no more + pending.remove(&glyph.name); } - let mut work_order = depths - .into_iter() - .map(|(name, depth)| (depth, name)) - .collect::>(); - work_order.sort(); - work_order.into_iter().map(|(_, name)| name) + Ok(()) } /// Convert a glyph with contours and components to a contour-only, aka simple, glyph @@ -527,7 +484,7 @@ impl Work for GlyphOrderWork { // 1) need to push their paths to a new glyph that is a component // 2) collapse such glyphs into a simple (contour-only) glyph // fontmake (Python) prefers option 2. - let mut todo = Vec::new(); + let mut todo = VecDeque::new(); for glyph_name in new_glyph_order.iter() { let glyph = original_glyphs.get(glyph_name).unwrap(); if !glyph.has_consistent_components() { @@ -535,16 +492,16 @@ impl Work for GlyphOrderWork { "Coalescing '{glyph_name}' into a simple glyph because \ component 2x2s vary across the designspace" ); - todo.push((GlyphOp::ConvertToContour, glyph.clone())); + todo.push_back((GlyphOp::ConvertToContour, glyph.clone())); } else if has_components_and_contours(glyph) { if context.flags.contains(Flags::PREFER_SIMPLE_GLYPHS) { - todo.push((GlyphOp::ConvertToContour, glyph.clone())); + todo.push_back((GlyphOp::ConvertToContour, glyph.clone())); debug!( "Coalescing '{glyph_name} into a simple glyph; it has \ contours and components and prefer simple glyphs is set", ); } else { - todo.push((GlyphOp::MoveContoursToComponent, glyph.clone())); + todo.push_back((GlyphOp::MoveContoursToComponent, glyph.clone())); } } } @@ -1082,59 +1039,4 @@ mod tests { // infected the deep_component and caused it to be decomposed. assert_is_flattened_component(&context, test_data.deep_component.name); } - - #[derive(Default)] - struct GlyphOrderBuilder(Vec>); - - impl GlyphOrderBuilder { - fn add_glyph(&mut self, name: &str, components: [&str; N]) { - let instance = GlyphInstance { - components: components - .into_iter() - .map(|name| Component { - base: name.into(), - transform: Default::default(), - }) - .collect(), - ..Default::default() - }; - let loc = NormalizedLocation::for_pos(&[("axis", 0.0)]); - let glyph = Glyph::new( - name.into(), - true, - Default::default(), - HashMap::from([(loc, instance)]), - ) - .unwrap(); - self.0.push(Arc::new(glyph)) - } - } - - #[test] - fn component_sorting() { - let mut builder = GlyphOrderBuilder::default(); - builder.add_glyph("a", ["b", "c"]); - builder.add_glyph("b", ["z"]); - builder.add_glyph("c", ["d"]); - builder.add_glyph("d", ["x", "y"]); - builder.add_glyph("e", ["z"]); - - let sorted = component_ordering_within_glyphs(builder.0.into_iter()).collect::>(); - - assert_eq!(sorted, ["b", "d", "e", "c", "a"]); - } - - // all we care about here is that we don't deadlock - #[test] - fn component_sorting_with_cycle() { - let mut builder = GlyphOrderBuilder::default(); - builder.add_glyph("a", ["z"]); - builder.add_glyph("b", ["c"]); - builder.add_glyph("c", ["d"]); - builder.add_glyph("z", ["a"]); // a cycle - // - let sorted = component_ordering_within_glyphs(builder.0.into_iter()).collect::>(); - - assert_eq!(sorted, ["c", "b", "a", "z"]); - } } From 9ddafb4cd79296425ffd5b73288321155d4ec97b Mon Sep 17 00:00:00 2001 From: rsheeter Date: Mon, 2 Dec 2024 16:15:47 -0800 Subject: [PATCH 4/4] Add back one of the tests --- fontir/src/glyph.rs | 87 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 79 insertions(+), 8 deletions(-) diff --git a/fontir/src/glyph.rs b/fontir/src/glyph.rs index f2caa5bf..f8a88da7 100644 --- a/fontir/src/glyph.rs +++ b/fontir/src/glyph.rs @@ -181,12 +181,13 @@ enum GlyphOp { } /// Fix glyphs with mixed components/contours. -/// +/// /// We presume component cycles are checked elsewhere and do not check for them here fn resolve_inconsistencies( context: &Context, mut todo: VecDeque<(GlyphOp, Arc)>, order: &mut GlyphOrder, + mut apply_fix: impl FnMut(GlyphOp, &Glyph, &mut GlyphOrder) -> Result<(), BadGlyph>, ) -> Result<(), BadGlyph> { let mut pending = todo .iter() @@ -209,12 +210,7 @@ fn resolve_inconsistencies( // Our component graph doesn't reach any pending component, we are a go! debug!("{op:?} {}", glyph.name); - match op { - GlyphOp::ConvertToContour => convert_components_to_contours(context, &glyph)?, - GlyphOp::MoveContoursToComponent => { - move_contours_to_new_component(context, order, &glyph)? - } - } + apply_fix(op, &glyph, order)?; // I ain't a-gonna pend no more pending.remove(&glyph.name); @@ -505,7 +501,17 @@ impl Work for GlyphOrderWork { } } } - resolve_inconsistencies(context, todo, &mut new_glyph_order)?; + resolve_inconsistencies( + context, + todo, + &mut new_glyph_order, + |op, glyph, order| match op { + GlyphOp::ConvertToContour => convert_components_to_contours(context, glyph), + GlyphOp::MoveContoursToComponent => { + move_contours_to_new_component(context, order, glyph) + } + }, + )?; drop(original_glyphs); // lets not accidentally use that from here on apply_optional_transformations(context, &new_glyph_order)?; @@ -1039,4 +1045,69 @@ mod tests { // infected the deep_component and caused it to be decomposed. assert_is_flattened_component(&context, test_data.deep_component.name); } + + #[derive(Default)] + struct GlyphOrderBuilder(Vec>); + + impl GlyphOrderBuilder { + fn add_glyph(&mut self, name: &str, components: [&str; N]) { + let instance = GlyphInstance { + components: components + .into_iter() + .map(|name| Component { + base: name.into(), + transform: Default::default(), + }) + .collect(), + ..Default::default() + }; + let loc = NormalizedLocation::for_pos(&[("axis", 0.0)]); + let glyph = Glyph::new( + name.into(), + true, + Default::default(), + HashMap::from([(loc, instance)]), + ) + .unwrap(); + self.0.push(Arc::new(glyph)) + } + } + + #[test] + fn component_sorting() { + let mut builder = GlyphOrderBuilder::default(); + builder.add_glyph("a", ["b", "c"]); + builder.add_glyph("b", ["z"]); + builder.add_glyph("c", ["d"]); + builder.add_glyph("d", ["x", "y"]); + builder.add_glyph("e", ["z"]); + + let mut order: GlyphOrder = builder.0.iter().map(|g| g.name.clone()).collect(); + let context = test_context(); + for glyph in builder.0.iter() { + context.glyphs.set((**glyph).clone()); + for component_name in glyph.component_names() { + if order.contains(component_name) { + continue; + } + context.glyphs.set(contour_glyph(component_name.as_str())); + order.insert(component_name.clone()); + } + } + let todo = builder + .0 + .into_iter() + .map(|g| (GlyphOp::ConvertToContour, g)) + .collect(); + + let mut fix_order = Vec::new(); + + resolve_inconsistencies(&context, todo, &mut order, |_op, glyph, _order| { + fix_order.push(glyph.name.clone()); + Ok(()) + }) + .unwrap(); + + assert_eq!(fix_order, ["b", "d", "e", "c", "a"]); + } }