From a5a2f69c901112bff8242600c4456bbb89218a73 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Thu, 21 Mar 2024 17:10:24 -0400 Subject: [PATCH] [kerning] Delay decomposing kern rules This reworks how we do kerning in order to preserve information about whether rules included a glyph class until we are ready to do the final compilation. Preserving this information lets us match the behaviour of FEA in ordering these rules, where we put (glyph, glyph) first, followed by (glyph, class), (class, glyph) and finally (class, class). --- fea-rs/src/common/glyph_class.rs | 3 + fontbe/src/features/kern.rs | 168 ++++++++++---------- fontbe/src/orchestration.rs | 253 +++++++++++++++++++------------ 3 files changed, 244 insertions(+), 180 deletions(-) diff --git a/fea-rs/src/common/glyph_class.rs b/fea-rs/src/common/glyph_class.rs index 3ceef3f12..5c6eeb8e6 100644 --- a/fea-rs/src/common/glyph_class.rs +++ b/fea-rs/src/common/glyph_class.rs @@ -60,6 +60,9 @@ impl GlyphClass { } impl GlyphSet { + /// The empty glyph set + pub const EMPTY: Self = GlyphSet(Vec::new()); + /// Iterate over the glyphs in this class pub fn iter(&self) -> impl Iterator + '_ { self.0.iter().copied() diff --git a/fontbe/src/features/kern.rs b/fontbe/src/features/kern.rs index 0aaf198db..30e37ea93 100644 --- a/fontbe/src/features/kern.rs +++ b/fontbe/src/features/kern.rs @@ -20,7 +20,9 @@ use fontdrasil::{ types::GlyphName, }; use fontir::{ - ir::{Glyph, KernGroup, KernPair, KernParticipant, KerningGroups, KerningInstance}, + ir::{ + Glyph, KernGroup, KernPair as IrKernPair, KernParticipant, KerningGroups, KerningInstance, + }, orchestration::WorkId as FeWorkId, }; use icu_properties::BidiClass; @@ -40,7 +42,7 @@ use crate::{ }, orchestration::{ AllKerningPairs, AnyWorkId, BeWork, Context, FeaRsKerns, KernAdjustments, KernFragment, - PairPosEntry, WorkId, + KernPair, KernSide, WorkId, }, }; @@ -156,7 +158,7 @@ impl Work for GatherIrKerningWork { align_kerning(&ir_groups, &mut kern_by_pos); // Use a BTreeMap because it seems the order we process pairs matters. Maybe we should sort instead...? - let mut adjustments: BTreeMap = Default::default(); + let mut adjustments: BTreeMap = Default::default(); // We want to add items to locations in the same order as the group locations // so start with group locations and then find the matching kerning. @@ -259,7 +261,7 @@ fn align_kerning( // fn lookup_kerning_value( - pair: &KernPair, + pair: &IrKernPair, kerning: &KerningInstance, side1_glyphs: &HashMap<&GlyphName, &KernGroup>, side2_glyphs: &HashMap<&GlyphName, &KernGroup>, @@ -315,8 +317,7 @@ impl Work for KerningFragmentWork { fn exec(&self, context: &Context) -> Result<(), Error> { let static_metadata = context.ir.static_metadata.get(); - let arc_glyph_order = context.ir.glyph_order.get(); - let glyph_order = arc_glyph_order.as_ref(); + let glyph_order = context.ir.glyph_order.get(); let kerning = context.all_kerning_pairs.get(); let start = self.segment * KERNS_PER_BLOCK; let end = (start + KERNS_PER_BLOCK).min(kerning.adjustments.len()); @@ -325,50 +326,27 @@ impl Work for KerningFragmentWork { // now for each kerning entry, directly add a rule to a builder: let our_kerns = &kerning.adjustments[start..end]; - let mut kerns = Vec::new(); - for ((left, right), values) in our_kerns { + let mut kerns = Vec::with_capacity(our_kerns.len()); + for ((side1, side2), values) in our_kerns { let (default_value, deltas) = resolve_variable_metric(&static_metadata, values.iter()) .map_err(|error| Error::KernDeltaError { - pair: (left.clone(), right.clone()), + pair: (side1.clone(), side2.clone()), error, })?; - let mut x_adv_record = ValueRecordBuilder::new().with_x_advance(default_value); + let mut value = ValueRecordBuilder::new().with_x_advance(default_value); // only encode deltas if they aren't all zeros if deltas.iter().any(|v| v.1 != 0) { - x_adv_record = x_adv_record.with_x_advance_device(deltas); - } - - match (left, right) { - // these unwraps are all fine because we've already validated the input - (KernParticipant::Glyph(left), KernParticipant::Glyph(right)) => { - let (left, right) = ( - glyph_order.glyph_id(left).unwrap(), - glyph_order.glyph_id(right).unwrap(), - ); - kerns.push(PairPosEntry::Pair(left, x_adv_record.clone(), right)); - } - (KernParticipant::Group(left), KernParticipant::Group(right)) => { - let left = kerning.get_group(left).unwrap().clone(); - let right = kerning.get_group(right).unwrap().clone(); - kerns.push(PairPosEntry::Class(left, x_adv_record.clone(), right)); - } - // if groups are mixed with glyphs then we enumerate the group - (KernParticipant::Glyph(left), KernParticipant::Group(right)) => { - let gid0 = glyph_order.glyph_id(left).unwrap(); - let right = kerning.get_group(right).unwrap().clone(); - for gid1 in right.iter() { - kerns.push(PairPosEntry::Pair(gid0, x_adv_record.clone(), gid1)); - } - } - (KernParticipant::Group(left), KernParticipant::Glyph(right)) => { - let left = kerning.get_group(left).unwrap().clone(); - let gid1 = glyph_order.glyph_id(right).unwrap(); - for gid0 in left.iter() { - kerns.push(PairPosEntry::Pair(gid0, x_adv_record.clone(), gid1)); - } - } + value = value.with_x_advance_device(deltas); } + // groups and glyphs have already been validated + let side1 = KernSide::from_ir_side(side1, &glyph_order, &kerning.groups).unwrap(); + let side2 = KernSide::from_ir_side(side2, &glyph_order, &kerning.groups).unwrap(); + kerns.push(KernPair { + side1, + side2, + value, + }) } context.kern_fragments.set(KernFragment { @@ -453,10 +431,11 @@ impl KerningGatherWork { let gdef = compilation.gdef_classes; - let pairs = fragments + let mut pairs = fragments .iter() .flat_map(|frag| frag.kerns.iter()) .collect::>(); + pairs.sort(); let known_scripts = guess_font_scripts(ast, &glyphs); let mark_glyphs = glyphs @@ -665,7 +644,7 @@ impl KernSplitContext { // fn make_lookups( &self, - pairs: &[&PairPosEntry], + pairs: &[&KernPair], ) -> BTreeMap, Vec>> { if !self.opts.ignore_marks { let pairs = pairs.iter().map(|x| Cow::Borrowed(*x)).collect::>(); @@ -693,7 +672,7 @@ impl KernSplitContext { // fn make_split_script_kern_lookups( &self, - pairs: &[Cow], + pairs: &[Cow], are_marks: bool, ) -> BTreeMap, Vec>> { let mut lookups_by_script = BTreeMap::new(); @@ -712,8 +691,9 @@ impl KernSplitContext { && bidi_buf.contains(&BidiClass::RightToLeft) { log::warn!( - "skipping kern pair with ambigous direction: {}", - pair.display_glyphs() + "skipping kern pair with ambigous direction: {}/{}", + pair.side1, + pair.side2, ); continue; } @@ -765,8 +745,8 @@ impl KernSplitContext { // fn split_kerns( &self, - pairs: &[Cow], - ) -> HashMap, Vec> { + pairs: &[Cow], + ) -> HashMap, Vec> { let mut kerning_per_script = HashMap::new(); for pair in pairs { for (scripts, pair) in self.partition_by_script(pair) { @@ -788,8 +768,8 @@ impl KernSplitContext { // fn partition_by_script<'b>( &self, - pair: &'b PairPosEntry, - ) -> impl Iterator, PairPosEntry)> + 'b { + pair: &'b KernPair, + ) -> impl Iterator, KernPair)> + 'b { //TODO: we could definitely make a reusable context and save all this //reallocation let mut resolved_scripts = HashMap::new(); @@ -876,8 +856,8 @@ impl KernSplitContext { /// fn split_base_and_mark_pairs<'b>( &self, - pairs: &[&'b PairPosEntry], - ) -> (Vec>, Vec>) { + pairs: &[&'b KernPair], + ) -> (Vec>, Vec>) { enum GlyphSetContent { Empty, BasesOnly, @@ -886,11 +866,11 @@ impl KernSplitContext { } // little helper to tell us what's in a glyphset - fn classify_glyphset_contents( - glyph_set: &GlyphSet, + fn classify_kernside_contents( + side: &KernSide, marks: &HashMap, ) -> GlyphSetContent { - glyph_set.iter().fold(GlyphSetContent::Empty, |val, gid| { + side.iter().fold(GlyphSetContent::Empty, |val, gid| { match (val, marks.contains_key(&gid)) { (GlyphSetContent::Empty, true) => GlyphSetContent::MarksOnly, (GlyphSetContent::Empty, false) => GlyphSetContent::BasesOnly, @@ -903,11 +883,20 @@ impl KernSplitContext { /// split a glyphset into (bases, marks) fn split_glyphs( - glyphs: &GlyphSet, + glyphs: &KernSide, marks: &HashMap, - ) -> (GlyphSet, GlyphSet) { - let (x, y): (Vec<_>, Vec<_>) = glyphs.iter().partition(|gid| !marks.contains_key(gid)); - (x.into(), y.into()) + ) -> (KernSide, KernSide) { + match glyphs { + KernSide::Glyph(gid) if !marks.contains_key(gid) => { + (KernSide::Glyph(*gid), KernSide::empty()) + } + KernSide::Glyph(gid) => (KernSide::empty(), KernSide::Glyph(*gid)), + KernSide::Group(glyphs) => { + let (x, y): (Vec<_>, Vec<_>) = + glyphs.iter().partition(|gid| !marks.contains_key(gid)); + (KernSide::Group(x.into()), KernSide::Group(y.into())) + } + } } if self.mark_glyphs.is_empty() { @@ -920,19 +909,19 @@ impl KernSplitContext { let (mut base_pairs, mut mark_pairs) = (Vec::new(), Vec::new()); for pair in pairs { - match pair { - PairPosEntry::Pair(side1, _, side2) + match (&pair.side1, &pair.side2) { + (KernSide::Glyph(side1), KernSide::Glyph(side2)) if !self.mark_glyphs.contains_key(side1) && !self.mark_glyphs.contains_key(side2) => { base_pairs.push(Cow::Borrowed(*pair)) } - PairPosEntry::Pair(..) => mark_pairs.push(Cow::Borrowed(*pair)), + (KernSide::Glyph(_), KernSide::Glyph(_)) => mark_pairs.push(Cow::Borrowed(*pair)), // handle the case where all are marks or bases, first: - PairPosEntry::Class(side1, value, side2) => { - let side1_cls = classify_glyphset_contents(side1, &self.mark_glyphs); - let side2_cls = classify_glyphset_contents(side2, &self.mark_glyphs); + (side1, side2) => { + let side1_cls = classify_kernside_contents(side1, &self.mark_glyphs); + let side2_cls = classify_kernside_contents(side2, &self.mark_glyphs); match (side1_cls, side2_cls) { (GlyphSetContent::Empty, _) | (_, GlyphSetContent::Empty) => continue, @@ -946,16 +935,16 @@ impl KernSplitContext { mark_pairs.push(Cow::Borrowed(*pair)); continue; } - _ => { + (GlyphSetContent::Mixed, _) | (_, GlyphSetContent::Mixed) => { let (side1_bases, side1_marks) = split_glyphs(side1, &self.mark_glyphs); let (side2_bases, side2_marks) = split_glyphs(side2, &self.mark_glyphs); if !side1_bases.is_empty() && !side2_bases.is_empty() { - base_pairs.push(Cow::Owned(PairPosEntry::Class( - side1_bases.clone(), - value.clone(), - side2_bases.clone(), - ))); + base_pairs.push(Cow::Owned(KernPair { + side1: side1_bases.clone(), + value: pair.value.clone(), + side2: side2_bases.clone(), + })); } // these various combos all go in the marks group for (side1, side2) in [ @@ -964,11 +953,11 @@ impl KernSplitContext { (&side1_marks, &side2_marks), ] { if !side1.is_empty() && !side2.is_empty() { - mark_pairs.push(Cow::Owned(PairPosEntry::Class( - side1.clone(), - value.clone(), - side2.clone(), - ))); + mark_pairs.push(Cow::Owned(KernPair { + side1: side1.clone(), + value: pair.value.clone(), + side2: side2.clone(), + })); } } } @@ -1037,8 +1026,8 @@ fn get_script_language_systems(ast: &ParseTree) -> HashMap fn merge_scripts( - kerning_per_script: HashMap, Vec>, -) -> HashMap, Vec> { + kerning_per_script: HashMap, Vec>, +) -> HashMap, Vec> { let mut sets = kerning_per_script.keys().cloned().collect::>(); let mut buf = Vec::with_capacity(sets.len()); let mut did_merge = true; @@ -1094,18 +1083,23 @@ mod tests { } impl MockCharMap { - fn make_rule(&self, left: char, right: char, val: i16) -> PairPosEntry { - PairPosEntry::Pair( - self.get(left), - ValueRecordBuilder::new().with_x_advance(val), - self.get(right), - ) + fn make_rule(&self, left: char, right: char, val: i16) -> KernPair { + KernPair { + side1: KernSide::Glyph(self.get(left)), + side2: KernSide::Glyph(self.get(right)), + value: ValueRecordBuilder::new().with_x_advance(val), + } } - fn make_class_rule(&self, left: &[char], right: &[char], val: i16) -> PairPosEntry { + fn make_class_rule(&self, left: &[char], right: &[char], val: i16) -> KernPair { let left = left.iter().map(|c| self.get(*c)).collect(); let right = right.iter().map(|c| self.get(*c)).collect(); - PairPosEntry::Class(left, ValueRecordBuilder::new().with_x_advance(val), right) + + KernPair { + side1: KernSide::Group(left), + side2: KernSide::Group(right), + value: ValueRecordBuilder::new().with_x_advance(val), + } } fn get(&self, c: char) -> GlyphId { diff --git a/fontbe/src/orchestration.rs b/fontbe/src/orchestration.rs index 0412f3a0e..e441c132a 100644 --- a/fontbe/src/orchestration.rs +++ b/fontbe/src/orchestration.rs @@ -22,7 +22,7 @@ use fontdrasil::{ types::GlyphName, }; use fontir::{ - ir::{Anchor, KernGroup, KernPair}, + ir::{Anchor, GlyphOrder, KernGroup, KernPair as IrKernPair, KernParticipant}, orchestration::{ Context as FeContext, ContextItem, ContextMap, Flags, IdAware, Persistable, PersistentStorage, WorkId as FeWorkIdentifier, @@ -411,13 +411,7 @@ pub type KernAdjustments = BTreeMap>; pub struct AllKerningPairs { /// A mapping from named kern groups to the appropriate set of glyphs pub groups: BTreeMap, - pub adjustments: Vec<(KernPair, KernAdjustments)>, -} - -impl AllKerningPairs { - pub(crate) fn get_group(&self, group: &KernGroup) -> Option<&GlyphSet> { - self.groups.get(group) - } + pub adjustments: Vec<(IrKernPair, KernAdjustments)>, } impl Persistable for AllKerningPairs { @@ -430,32 +424,112 @@ impl Persistable for AllKerningPairs { } } -#[derive(Clone, Serialize, Deserialize, PartialEq, PartialOrd, Eq, Ord)] -pub enum PairPosEntry { - Pair(GlyphId, ValueRecordBuilder, GlyphId), - Class(GlyphSet, ValueRecordBuilder, GlyphSet), +/// One side of a kerning pair, represented as glyph ids +/// +/// This parallels the [`KernParticipant`] IR type, with glyph names resolved +/// to GIDs. +#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, PartialOrd, Eq, Ord)] +pub(crate) enum KernSide { + /// A specific glyph + Glyph(GlyphId), + /// A group of glyphs + Group(GlyphSet), +} + +/// A resolved user kern rule +/// +/// This parallels the [`ir::KernPair`](IrKernPair), preserving the same structure, +/// but using glyph ids instead of glyph names and a finalized value record +/// instead of raw per-location positioning. +#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, PartialOrd, Eq, Ord)] +pub(crate) struct KernPair { + pub(crate) side1: KernSide, + pub(crate) side2: KernSide, + pub(crate) value: ValueRecordBuilder, +} + +impl KernSide { + pub(crate) const fn empty() -> Self { + Self::Group(GlyphSet::EMPTY) + } + + pub(crate) fn is_empty(&self) -> bool { + matches!(self, KernSide::Group(items) if items.is_empty()) + } + + /// Convert from IR (which uses glyph names) to our representation (using ids) + pub(crate) fn from_ir_side( + ir: &KernParticipant, + glyphs: &GlyphOrder, + groups: &BTreeMap, + ) -> Option { + match ir { + KernParticipant::Glyph(name) => glyphs.glyph_id(name).map(Self::Glyph), + KernParticipant::Group(name) => groups.get(name).cloned().map(Self::Group), + } + } + + pub(crate) fn iter(&self) -> impl Iterator + '_ { + let (first, second) = match self { + Self::Glyph(gid) => (Some(*gid), None), + Self::Group(group) => (None, Some(group)), + }; + + first + .into_iter() + .chain(second.into_iter().flat_map(|set| set.iter())) + } +} + +impl Display for KernSide { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + KernSide::Glyph(gid) => write!(f, "{gid}"), + KernSide::Group(group) => { + let mut first = true; + write!(f, "[")?; + for gid in group.iter() { + if !first { + write!(f, ", ")?; + } + write!(f, "{gid}")?; + first = false; + } + write!(f, "]") + } + } + } } -impl PairPosEntry { +impl KernPair { /// if a rule is right-to-left, we need to set both x-advance AND x-position /// /// see /// for further details. The tl;dr is that the lookup itself does not have /// any knowledge of writing direction. pub(crate) fn make_rtl_compatible(&mut self) { - let record = match self { - PairPosEntry::Pair(_, record, _) => record, - PairPosEntry::Class(_, record, _) => record, - }; - record.make_rtl_compatible(); + self.value.make_rtl_compatible() } + pub(crate) fn add_to(self, builder: &mut PairPosBuilder) { - match self { - PairPosEntry::Pair(gid0, rec0, gid1) => { - builder.insert_pair(gid0, rec0, gid1, Default::default()) + match (self.side1, self.side2) { + // these unwraps are all fine because we've already validated the input + (KernSide::Glyph(side1), KernSide::Glyph(side2)) => { + builder.insert_pair(side1, self.value, side2, Default::default()); + } + (KernSide::Group(side1), KernSide::Group(side2)) => { + builder.insert_classes(side1, self.value, side2, Default::default()); } - PairPosEntry::Class(set0, rec0, set1) => { - builder.insert_classes(set0, rec0, set1, Default::default()) + // if groups are mixed with glyphs then we enumerate the group + (KernSide::Glyph(side1), KernSide::Group(side2)) => { + for side2 in side2.iter() { + builder.insert_pair(side1, self.value.clone(), side2, Default::default()); + } + } + (KernSide::Group(side1), KernSide::Glyph(side2)) => { + for side1 in side1.iter() { + builder.insert_pair(side1, self.value.clone(), side2, Default::default()); + } } } } @@ -469,83 +543,35 @@ impl PairPosEntry { /// a helper used when splitting kerns based on script direction pub(crate) fn with_new_glyphs(&self, side1: GlyphSet, side2: GlyphSet) -> Self { - let value = match self { - PairPosEntry::Pair(_, value, _) => value, - PairPosEntry::Class(_, value, _) => value, + // for each of our sides, we want to keep groups groups and glyphs glyphs + let (side1, side2) = match (&self.side1, &self.side2) { + (KernSide::Glyph(_), KernSide::Glyph(_)) => (self.side1.clone(), self.side2.clone()), + (KernSide::Glyph(_), KernSide::Group(_)) => { + assert_eq!(side1.len(), 1); + (self.side1.clone(), KernSide::Group(side2)) + } + (KernSide::Group(_), KernSide::Glyph(_)) => { + assert_eq!(side2.len(), 1); + (KernSide::Group(side1), self.side2.clone()) + } + _ => { + assert!(!side1.is_empty() && !side2.is_empty()); + (KernSide::Group(side1), KernSide::Group(side2)) + } }; - - if side1.len() == 1 && side2.len() == 1 { - Self::Pair( - side1.iter().next().unwrap(), - value.clone(), - side2.iter().next().unwrap(), - ) - } else { - Self::Class(side1, value.clone(), side2) + KernPair { + side1, + side2, + value: self.value.clone(), } } pub(crate) fn first_glyphs(&self) -> impl Iterator + '_ { - let (first, second) = match self { - PairPosEntry::Pair(gid0, ..) => (Some(*gid0), None), - PairPosEntry::Class(set0, ..) => (None, Some(set0)), - }; - - first - .into_iter() - .chain(second.into_iter().flat_map(|set| set.iter())) + self.side1.iter() } pub(crate) fn second_glyphs(&self) -> impl Iterator + '_ { - let (first, second) = match self { - PairPosEntry::Pair(_, _, gid1) => (Some(*gid1), None), - PairPosEntry::Class(_, _, set1) => (None, Some(set1)), - }; - - first - .into_iter() - .chain(second.into_iter().flat_map(|set| set.iter())) - } - - /// A helper for pretty-printing the rule's glyph or glyphs - pub(crate) fn display_glyphs(&self) -> impl Display + '_ { - enum DisplayGlyphs<'a> { - Pair(GlyphId, GlyphId), - Class(&'a GlyphSet, &'a GlyphSet), - } - - impl Display for DisplayGlyphs<'_> { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - DisplayGlyphs::Pair(one, two) => write!(f, "{one}/{two}"), - DisplayGlyphs::Class(one, two) => { - let mut first = true; - for gid in one.iter() { - if !first { - write!(f, ", ")?; - } - write!(f, "{gid}")?; - first = false; - } - write!(f, "/")?; - first = true; - for gid in two.iter() { - if !first { - write!(f, ", ")?; - } - write!(f, "{gid}")?; - first = false; - } - Ok(()) - } - } - } - } - - match self { - PairPosEntry::Pair(one, _, two) => DisplayGlyphs::Pair(*one, *two), - PairPosEntry::Class(one, _, two) => DisplayGlyphs::Class(one, two), - } + self.side2.iter() } } @@ -555,7 +581,7 @@ impl PairPosEntry { #[derive(Default, Clone, Serialize, Deserialize, PartialEq)] pub struct KernFragment { pub(crate) segment: usize, - pub(crate) kerns: Vec, + pub(crate) kerns: Vec, } impl IdAware for KernFragment { @@ -940,4 +966,45 @@ mod tests { .to_deltas(&[Tag::new(b"wght")]); assert!(deltas.is_empty(), "{deltas:?}"); } + + // kerning needs to be sort such that (glyph, glyph) comes first, then + // (glyph, class), (class, glyph) and (class, class). This matches the + // behaviour of the FEA syntax, and ensures that more specific rules are + // applied first + #[test] + fn kern_pair_sort_order() { + let glyph = KernSide::Glyph(GlyphId::new(5)); + let class_ = KernSide::Group([1, 2, 3, 4].into_iter().map(GlyphId::new).collect()); + let value = ValueRecordBuilder::new().with_x_advance(420); + let glyph_glyph = KernPair { + side1: glyph.clone(), + side2: glyph.clone(), + value: value.clone(), + }; + + let glyph_class = KernPair { + side1: glyph.clone(), + side2: class_.clone(), + value: value.clone(), + }; + + let class_glyph = KernPair { + side1: class_.clone(), + side2: glyph.clone(), + value: value.clone(), + }; + + let class_class = KernPair { + side1: class_.clone(), + side2: class_.clone(), + value: value.clone(), + }; + + let mut unsorted = [&class_class, &glyph_class, &glyph_glyph, &class_glyph]; + unsorted.sort(); + assert_eq!( + unsorted, + [&glyph_glyph, &glyph_class, &class_glyph, &class_class] + ); + } }