From b9d674fe15181764eb0e624605555276f8017ec4 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Wed, 2 Oct 2024 18:49:51 -0400 Subject: [PATCH 1/2] [kerning] Add test that alignment is deterministic --- fontbe/src/features/kern.rs | 40 +++++++++++++++++++++++++++++++++++++ fontir/src/ir.rs | 12 +++++++++++ 2 files changed, 52 insertions(+) diff --git a/fontbe/src/features/kern.rs b/fontbe/src/features/kern.rs index c98f6b8b5..ed3f30461 100644 --- a/fontbe/src/features/kern.rs +++ b/fontbe/src/features/kern.rs @@ -1282,4 +1282,44 @@ mod tests { ((LookupFlag::IGNORE_MARKS, 2), (LookupFlag::empty(), 1)), ); } + + // we had a bug where we were updating the kerning values in place, which + // meant the order in which we handled pairs could influence the results + #[test] + fn alignment_determinism() { + let g1 = GlyphName::new("a"); + let g2 = GlyphName::new("b"); + let side1 = ir::KernGroup::Side1("aa".into()); + let side2 = ir::KernGroup::Side2("bb".into()); + let side1_glyphs = HashMap::from([(&g1, &side1)]); + let side2_glyphs = HashMap::from([(&g2, &side2)]); + + let glyph_glyph: ir::KernPair = (g1.clone().into(), g2.clone().into()); + let glyph_group: ir::KernPair = (g1.clone().into(), side2.clone().into()); + let group_glyph: ir::KernPair = (side1.clone().into(), g2.clone().into()); + let group_group: ir::KernPair = (side1.clone().into(), side2.clone().into()); + + let all_pairs = HashSet::from([ + glyph_glyph.clone(), + glyph_group.clone(), + group_glyph.clone(), + group_group.clone(), + ]); + let mut kerns = BTreeMap::new(); + kerns.insert(group_group.clone(), OrderedFloat::from(-70.)); + kerns.insert(group_glyph.clone(), 10.0.into()); + // explanation: + // we need to align glyph_glyph and glyph_group. + // - if we do glyph_group first, we will use the group_group value of + // -70, and then when we do glyph_glyph we will use this value, since + // glyph_group is preferred to group_glyph + // - but if we do glyph_glyph first, we will use the value from + // group_glyph, which is set. + + // run a few times because triggering depended on hashmap iteration order + for _ in 0..20 { + align_instance(&all_pairs, &mut kerns, &side1_glyphs, &side2_glyphs); + assert_eq!(kerns.get(&glyph_glyph).map(|x| x.0), Some(10.0f32)); + } + } } diff --git a/fontir/src/ir.rs b/fontir/src/ir.rs index b2c98eebe..90ec49a81 100644 --- a/fontir/src/ir.rs +++ b/fontir/src/ir.rs @@ -324,6 +324,18 @@ impl KernSide { } } +impl From for KernSide { + fn from(src: GlyphName) -> KernSide { + KernSide::Glyph(src) + } +} + +impl From for KernSide { + fn from(src: KernGroup) -> KernSide { + KernSide::Group(src) + } +} + impl StaticMetadata { // TODO: we could consider a builder or something for this? #[allow(clippy::too_many_arguments)] From ca111ea4250a615f0945efd28349925d4d302eda Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Wed, 2 Oct 2024 13:16:56 -0400 Subject: [PATCH 2/2] [kerning] Ensure alignment is deterministic We were modifying the instance inside the loop, which meant that in some cases the final result would depend on the order in which we processed the pairs. --- fontbe/src/features/kern.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/fontbe/src/features/kern.rs b/fontbe/src/features/kern.rs index ed3f30461..6d3e0ef56 100644 --- a/fontbe/src/features/kern.rs +++ b/fontbe/src/features/kern.rs @@ -213,6 +213,9 @@ impl Work for GatherIrKerningWork { /// missing pairs are filled in via the UFO kerning value lookup algorithm: /// /// +/// +/// in pythonland this happens in ufo2ft, here: +/// fn align_kerning( groups: &KerningGroups, instances: &mut HashMap, @@ -253,13 +256,17 @@ fn align_instance( side1_glyphs: &HashMap<&GlyphName, &KernGroup>, side2_glyphs: &HashMap<&GlyphName, &KernGroup>, ) { - let missing_pairs = all_pairs - .iter() - .filter(|pair| !instance.contains_key(pair)) - .collect::>(); - - for pair in missing_pairs { + let mut buf = Vec::new(); + // iterate the pairs that are not present in this instance + for pair in all_pairs.iter().filter(|pair| !instance.contains_key(pair)) { let value = lookup_kerning_value(pair, instance, side1_glyphs, side2_glyphs); + + // accumulate any additions and add at the end, otherwise newly added + // additions could influence the calculation of subsequent values + buf.push((pair, value)); + } + // when done all pairs, add them to the instance + for (pair, value) in buf { instance.insert(pair.to_owned(), value); } }