diff --git a/fontir/src/glyph.rs b/fontir/src/glyph.rs index 054a625df..931285f02 100644 --- a/fontir/src/glyph.rs +++ b/fontir/src/glyph.rs @@ -254,7 +254,7 @@ fn flatten_non_export_components_for_glyph( context: &Context, glyph: &Glyph, ) -> Result { - let glyph = ensure_composite_defined_at_component_locations(context, glyph); + let glyph = ensure_composite_defined_at_component_locations(context, glyph)?; let mut builder = GlyphBuilder::from(glyph.clone()); builder.clear_components(); @@ -303,16 +303,40 @@ fn glyph_has_non_export_components(glyph: &Glyph, context: &Context) -> bool { .any(|name| !context.get_glyph(name.as_str()).emit_to_binary) } -fn ensure_composite_defined_at_component_locations(context: &Context, composite: &Glyph) -> Glyph { +fn ensure_composite_defined_at_component_locations( + context: &Context, + composite: &Glyph, +) -> Result { let mut glyph = composite.to_owned(); let child_locations = collect_component_locations_nested(context, &glyph); - for loc in child_locations { - if !glyph.sources().contains_key(&loc) { - let new_layer = instantiate_instance(&glyph, &loc, context).unwrap(); - glyph.sources_mut().insert(loc, new_layer); - } + batch_interpolate_missing(&mut glyph, child_locations.iter(), context)?; + Ok(glyph) +} + +/// Interpolate instances at all `locations` missing from `glyph`, in batch. +/// +/// All interpolations are computed from the glyph's *original* source set before +/// any new instances are inserted. This is critical for determinism: inserting +/// incrementally would mutate the VariationModel between iterations, and +/// non-deterministic HashMap iteration order would produce different results. +/// See . +fn batch_interpolate_missing<'a>( + glyph: &mut Glyph, + locations: impl IntoIterator, + context: &Context, +) -> Result<(), BadGlyph> { + let new_instances: Vec<_> = locations + .into_iter() + .filter(|loc| !glyph.sources().contains_key(*loc)) + .map(|loc| { + let instance = instantiate_instance(glyph, loc, context); + (loc.clone(), instance) + }) + .collect(); + for (loc, instance) in new_instances { + glyph.sources_mut().insert(loc, instance?); } - glyph + Ok(()) } fn collect_component_locations_nested( @@ -342,7 +366,7 @@ fn collect_component_locations_nested( /// /// fn convert_components_to_contours(context: &Context, original: &Glyph) -> Result<(), BadGlyph> { - let original = ensure_composite_defined_at_component_locations(context, original); + let original = ensure_composite_defined_at_component_locations(context, original)?; // Component until you can't component no more let mut frontier: VecDeque<_> = components(&original, Affine::IDENTITY); @@ -423,15 +447,7 @@ fn ensure_component_has_consistent_layers<'a>( } let mut component = component.to_owned(); - for loc in base.sources().keys() { - if component.sources().contains_key(loc) { - continue; - } - - let new_instance = get_or_instantiate_instance(&component, loc, context)?.into_owned(); - component.sources_mut().insert(loc.to_owned(), new_instance); - } - + batch_interpolate_missing(&mut component, base.sources().keys(), context)?; Ok(Cow::Owned(component)) } @@ -2297,4 +2313,271 @@ mod tests { "Glyph with pre-existing attaching anchor should be classified as Base" ); } + + /// Regression test for . + /// + /// `ensure_composite_defined_at_component_locations` must compute all + /// interpolations from the original source set, not incrementally mutate + /// the map. We verify the invariant: for each missing location, the result + /// matches an independent `instantiate_instance` call on the unmodified glyph. + /// + /// Derived from Georama's `uni03060300` (breve + grave combining mark). + /// The composite has 4 corner masters; its component `uni0306` has 5 + /// additional intermediate layers (4 on-axis + 1 interior), so 5 instances + /// must be interpolated. The interior point at (wght=0.5, wdth=0.4) is + /// critical: when edge intermediates are inserted first (incrementally), + /// they change the VariationModel's support regions, causing the interior + /// interpolation to produce a *mathematically different* result (up to + /// ~19 units off). With batch interpolation from the original 4-master + /// model, all results are deterministic. + #[test] + fn interpolated_instances_depend_only_on_original_sources() { + // Georama design-space locations (wght x wdth), normalized to [0, 1]. + // 4 corner masters + let m_00 = NormalizedLocation::for_pos(&[("wght", 0.0), ("wdth", 0.0)]); + let m_01 = NormalizedLocation::for_pos(&[("wght", 0.0), ("wdth", 1.0)]); + let m_10 = NormalizedLocation::for_pos(&[("wght", 1.0), ("wdth", 0.0)]); + let m_11 = NormalizedLocation::for_pos(&[("wght", 1.0), ("wdth", 1.0)]); + + // 5 intermediate locations from uni0306's brace layers: + // 4 on-axis (edges of the rectangle) + let i_0_04 = NormalizedLocation::for_pos(&[("wght", 0.0), ("wdth", 0.4)]); + let i_1_04 = NormalizedLocation::for_pos(&[("wght", 1.0), ("wdth", 0.4)]); + let i_05_0 = NormalizedLocation::for_pos(&[("wght", 0.5), ("wdth", 0.0)]); + let i_05_1 = NormalizedLocation::for_pos(&[("wght", 0.5), ("wdth", 1.0)]); + // 1 interior (both axes non-zero) — this is the trigger for the bug + let i_05_04 = NormalizedLocation::for_pos(&[("wght", 0.5), ("wdth", 0.4)]); + + // Composite uni03060300: components uni0306 + gravecomb.case. + // Real transforms from Georama source (4 corner masters only). + let composite = Glyph::new( + "uni03060300".into(), + true, + Default::default(), + [ + // (wght=0, wdth=0): identity + translate(24, -4) + ( + &m_00, + vec![ + Component::new("uni0306", Affine::IDENTITY), + Component::new("gravecomb.case", Affine::translate((24.0, -4.0))), + ], + ), + // (wght=0, wdth=1): identity + translate(173, 27) + ( + &m_01, + vec![ + Component::new("uni0306", Affine::IDENTITY), + Component::new("gravecomb.case", Affine::translate((173.0, 27.0))), + ], + ), + // (wght=1, wdth=0): identity + [0.9, 0, 0, 0.9, -21, 132] + ( + &m_10, + vec![ + Component::new("uni0306", Affine::IDENTITY), + Component::new( + "gravecomb.case", + Affine::new([0.9, 0.0, 0.0, 0.9, -21.0, 132.0]), + ), + ], + ), + // (wght=1, wdth=1): identity + [0.9, 0, 0, 0.9, 81, 142] + ( + &m_11, + vec![ + Component::new("uni0306", Affine::IDENTITY), + Component::new( + "gravecomb.case", + Affine::new([0.9, 0.0, 0.0, 0.9, 81.0, 142.0]), + ), + ], + ), + ] + .into_iter() + .map(|(loc, components)| { + ( + loc.clone(), + GlyphInstance { + components, + ..Default::default() + }, + ) + }) + .collect(), + ) + .unwrap(); + + // Component uni0306: simple glyph at 4 corners + 5 intermediates. + // Actual contour data doesn't affect the composite's interpolation; + // we just need the glyph to exist at the right locations so that + // collect_component_locations_nested returns all 9 locations. + let uni0306 = Glyph::new( + "uni0306".into(), + true, + Default::default(), + [ + &m_00, &m_01, &m_10, &m_11, &i_0_04, &i_1_04, &i_05_0, &i_05_1, &i_05_04, + ] + .into_iter() + .map(|loc| { + ( + loc.clone(), + GlyphInstance { + contours: vec![simple_square_path()], + ..Default::default() + }, + ) + }) + .collect(), + ) + .unwrap(); + + // Component gravecomb.case: only at the 4 corners (no intermediates). + let gravecomb_case = Glyph::new( + "gravecomb.case".into(), + true, + Default::default(), + [&m_00, &m_01, &m_10, &m_11] + .into_iter() + .map(|loc| { + ( + loc.clone(), + GlyphInstance { + contours: vec![simple_square_path()], + ..Default::default() + }, + ) + }) + .collect(), + ) + .unwrap(); + + // Context with the 4 corner master locations (the global model) + let context = test_context_with_locations(vec![ + m_00.clone(), + m_01.clone(), + m_10.clone(), + m_11.clone(), + ]); + context.glyphs.set(uni0306); + context.glyphs.set(gravecomb_case); + context.glyphs.set(composite.clone()); + + // Without the fix, the function incrementally inserts edge intermediates + // (in non-deterministic HashSet order), which changes the VariationModel's + // support regions before the interior point is interpolated. + // NOTE: this test may randomly PASS without the fix if the interior point is processed first + let result = ensure_composite_defined_at_component_locations(&context, &composite).unwrap(); + assert_eq!(result.sources().len(), 9); + for loc in [&i_0_04, &i_1_04, &i_05_0, &i_05_1, &i_05_04] { + let expected = instantiate_instance(&composite, loc, &context).unwrap(); + assert_eq!( + result.sources().get(loc).unwrap(), + &expected, + "instance at {loc:?} differs from independent interpolation on the original glyph" + ); + } + } + + /// Same bug mechanism as `interpolated_instances_depend_only_on_original_sources`, + /// but for `ensure_component_has_consistent_layers` which interpolates a + /// *component's contour coordinates* (not a composite's transforms). + /// + /// In `convert_components_to_contours`, after the composite gets all 9 + /// locations, each component is padded to match via + /// `ensure_component_has_consistent_layers`. For `gravecomb.case` (4 corner + /// sources), 5 intermediates must be interpolated — same edge+interior + /// layout, same non-determinism risk. + /// + /// Uses real Georama contour coordinates for `gravecomb.case`. + #[test] + fn component_consistent_layers_independent_of_insertion_order() { + // Same Georama locations as the composite test + let m_00 = NormalizedLocation::for_pos(&[("wght", 0.0), ("wdth", 0.0)]); + let m_01 = NormalizedLocation::for_pos(&[("wght", 0.0), ("wdth", 1.0)]); + let m_10 = NormalizedLocation::for_pos(&[("wght", 1.0), ("wdth", 0.0)]); + let m_11 = NormalizedLocation::for_pos(&[("wght", 1.0), ("wdth", 1.0)]); + let i_0_04 = NormalizedLocation::for_pos(&[("wght", 0.0), ("wdth", 0.4)]); + let i_1_04 = NormalizedLocation::for_pos(&[("wght", 1.0), ("wdth", 0.4)]); + let i_05_0 = NormalizedLocation::for_pos(&[("wght", 0.5), ("wdth", 0.0)]); + let i_05_1 = NormalizedLocation::for_pos(&[("wght", 0.5), ("wdth", 1.0)]); + let i_05_04 = NormalizedLocation::for_pos(&[("wght", 0.5), ("wdth", 0.4)]); + + // Real Georama contour data for gravecomb.case (one contour, 4 points). + fn gc_instance(points: [(f64, f64); 4]) -> GlyphInstance { + let mut path = BezPath::new(); + path.move_to(points[0]); + path.line_to(points[1]); + path.line_to(points[2]); + path.line_to(points[3]); + path.close_path(); + GlyphInstance { + contours: vec![path], + ..Default::default() + } + } + let component = Glyph::new( + "gravecomb.case".into(), + true, + Default::default(), + [ + ( + m_00.clone(), + gc_instance([(-156., 736.), (-138., 736.), (-200., 885.), (-221., 885.)]), + ), + ( + m_01.clone(), + gc_instance([(-644., 756.), (-615., 756.), (-837., 950.), (-867., 950.)]), + ), + ( + m_10.clone(), + gc_instance([(-285., 745.), (-144., 745.), (-194., 939.), (-377., 939.)]), + ), + ( + m_11.clone(), + gc_instance([(-694., 746.), (-465., 746.), (-570., 940.), (-864., 940.)]), + ), + ] + .into(), + ) + .unwrap(); + + // This glyph represents the composite after + // ensure_composite_defined_at_component_locations: it has all 9 + // locations. Contents don't matter, only the key set. + let composite = Glyph::new( + "composite".into(), + true, + Default::default(), + [ + &m_00, &m_01, &m_10, &m_11, &i_0_04, &i_1_04, &i_05_0, &i_05_1, &i_05_04, + ] + .into_iter() + .map(|loc| (loc.clone(), GlyphInstance::default())) + .collect(), + ) + .unwrap(); + + let context = test_context_with_locations(vec![ + m_00.clone(), + m_01.clone(), + m_10.clone(), + m_11.clone(), + ]); + + let result = ensure_component_has_consistent_layers(&composite, &component, &context) + .unwrap() + .into_owned(); + assert_eq!(result.sources().len(), 9); + + for loc in [&i_0_04, &i_1_04, &i_05_0, &i_05_1, &i_05_04] { + let expected = instantiate_instance(&component, loc, &context).unwrap(); + assert_eq!( + result.sources().get(loc).unwrap(), + &expected, + "instance at {loc:?} differs from independent interpolation on the original component" + ); + } + } }