From 52270d865f2da486375e3492c0977171b1023194 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Tue, 2 Apr 2024 13:34:03 -0400 Subject: [PATCH] [marks] Move marks logic into a context struct This patch just matches the existing behaviour, but it will be improved to match the behaviour of fontmake. --- fontbe/src/error.rs | 1 + fontbe/src/features/marks.rs | 360 +++++++++++++++++++++-------------- fontbe/src/orchestration.rs | 8 +- fontir/src/ir.rs | 10 + 4 files changed, 226 insertions(+), 153 deletions(-) diff --git a/fontbe/src/error.rs b/fontbe/src/error.rs index 541b7b85..0e34b691 100644 --- a/fontbe/src/error.rs +++ b/fontbe/src/error.rs @@ -79,6 +79,7 @@ pub enum Error { #[error("No glyph class '{0}'")] MissingGlyphClass(GlyphName), #[error("Mark glyph '{glyph}' in conflicting classes '{old_class}' and '{new_class}'")] + //FIXME: this can be deleted eventually, we will manually ensure classes are disjoint PreviouslyAssignedMarkClass { old_class: SmolStr, new_class: SmolStr, diff --git a/fontbe/src/features/marks.rs b/fontbe/src/features/marks.rs index f9d8556f..f35b5fd2 100644 --- a/fontbe/src/features/marks.rs +++ b/fontbe/src/features/marks.rs @@ -1,11 +1,8 @@ //! Generates a [FeaRsMarks] datastructure to be fed to fea-rs -use std::{ - collections::{BTreeMap, BTreeSet}, - sync::Arc, -}; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; -use fea_rs::compile::{MarkToBaseBuilder, MarkToMarkBuilder, PreviouslyAssignedClass}; +use fea_rs::compile::{MarkToBaseBuilder, MarkToMarkBuilder}; use fontdrasil::{ orchestration::{Access, AccessBuilder, Work}, types::GlyphName, @@ -13,13 +10,17 @@ use fontdrasil::{ use ordered_float::OrderedFloat; use smol_str::SmolStr; +use write_fonts::{ + tables::gdef::GlyphClassDef, + types::{GlyphId, Tag}, +}; use crate::{ error::Error, - orchestration::{AnyWorkId, BeWork, Context, FeaRsMarks, MarkGroup, WorkId}, + orchestration::{AnyWorkId, BeWork, Context, FeaRsMarks, WorkId}, }; use fontir::{ - ir::{AnchorKind, GlyphAnchors, GlyphOrder, StaticMetadata}, + ir::{self, AnchorKind, GlyphAnchors, GlyphOrder, StaticMetadata}, orchestration::WorkId as FeWorkId, }; @@ -33,94 +34,225 @@ pub fn create_mark_work() -> Box { /// The canonical name shared for a given mark/base pair, e.g. `top` for `top`/`_top` type MarkGroupName = SmolStr; -fn create_mark_to_base_groups( - anchors: &BTreeMap>, - glyph_order: &GlyphOrder, -) -> BTreeMap { - let mut groups: BTreeMap = Default::default(); - for (glyph_name, glyph_anchors) in anchors.iter() { - // We assume the anchor list to be small - // considering only glyphs with anchors, - // - glyphs with *only* base anchors are bases - // - glyphs with *any* mark anchor are marks - // We ignore glyphs missing from the glyph order (e.g. no-export glyphs) - if !glyph_order.contains(glyph_name) { - continue; - } - for anchor in glyph_anchors.anchors.iter() { - match &anchor.kind { - fontir::ir::AnchorKind::Base(group) => groups - .entry(group.clone()) - .or_default() - .bases - .push((glyph_name.clone(), anchor.clone())), - fontir::ir::AnchorKind::Mark(group) => groups - .entry(group.clone()) - .or_default() - .marks - .push((glyph_name.clone(), anchor.clone())), - _ => continue, - } - } +#[allow(dead_code)] // a few currently unused fields +struct MarkLookupBuilder<'a> { + gdef_classes: Option>, + // pruned, only the anchors we are using + anchors: BTreeMap>, + glyph_order: &'a GlyphOrder, + static_metadata: &'a StaticMetadata, + fea_scripts: HashSet, +} + +/// The bases and marks in a particular group, e.g. "top" or "bottom" +#[derive(Default, Clone, PartialEq)] +struct MarkGroup<'a> { + bases: Vec<(GlyphName, &'a ir::Anchor)>, + marks: Vec<(GlyphName, &'a ir::Anchor)>, +} + +// a trait to abstract over two very similar builders +trait MarkAttachmentBuilder: Default { + fn add_mark(&mut self, gid: GlyphId, group: &MarkGroupName, anchor: fea_rs::compile::Anchor); + fn add_base(&mut self, gid: GlyphId, group: &MarkGroupName, anchor: fea_rs::compile::Anchor); +} + +impl MarkAttachmentBuilder for MarkToBaseBuilder { + fn add_mark(&mut self, gid: GlyphId, group: &MarkGroupName, anchor: fea_rs::compile::Anchor) { + //FIXME: precheck groups to ensure no overlap + let _ = self.insert_mark(gid, group.clone(), anchor); + } + + fn add_base(&mut self, gid: GlyphId, group: &MarkGroupName, anchor: fea_rs::compile::Anchor) { + self.insert_base(gid, group, anchor) } - groups } -fn create_mark_to_mark_groups( - anchors: &BTreeMap>, - glyph_order: &GlyphOrder, -) -> BTreeMap { - // first find the set of glyphs that are marks, i.e. have any mark attachment point. - let (mark_glyphs, mark_anchors): (BTreeSet<_>, BTreeSet<_>) = anchors - .iter() - .filter(|(name, anchors)| glyph_order.contains(name) && anchors.contains_marks()) - .flat_map(|(name, anchors)| { - anchors.anchors.iter().filter_map(|anchor| { - if let AnchorKind::Mark(group) = &anchor.kind { - Some((name.clone(), group.clone())) - } else { - None +impl MarkAttachmentBuilder for MarkToMarkBuilder { + fn add_mark(&mut self, gid: GlyphId, group: &MarkGroupName, anchor: fea_rs::compile::Anchor) { + //FIXME: precheck groups to ensure no overlap + let _ = self.insert_mark1(gid, group.clone(), anchor); + } + + fn add_base(&mut self, gid: GlyphId, group: &MarkGroupName, anchor: fea_rs::compile::Anchor) { + self.insert_mark2(gid, group, anchor) + } +} + +impl<'a> MarkLookupBuilder<'a> { + fn new( + anchors: Vec<&'a GlyphAnchors>, + glyph_order: &'a GlyphOrder, + gdef_classes: Option>, + static_metadata: &'a StaticMetadata, + ) -> Self { + // first we want to narrow our input down to only anchors that are participating. + let mut pruned = BTreeMap::new(); + let mut base_names = HashSet::new(); + let mut mark_names = HashSet::new(); + for anchors in anchors { + // skip anchors for non-export glyphs + if !glyph_order.contains(&anchors.glyph_name) { + continue; + } + for anchor in &anchors.anchors { + match &anchor.kind { + AnchorKind::Base(group) => { + base_names.insert(group); + } + AnchorKind::Mark(group) => { + mark_names.insert(group); + } + //TODO: ligatures + // skip non base/mark anchors + _ => continue, } - }) - }) - .unzip(); + pruned + .entry(anchors.glyph_name.clone()) + .or_insert(Vec::new()) + .push(anchor); + } + } - // then iterate again, looking for glyphs that we have identified as marks, - // but which also have participating base anchors - let mut result = BTreeMap::::new(); - for (glyph, glyph_anchors) in anchors { - if !mark_glyphs.contains(glyph) { - continue; + let used_groups = base_names.intersection(&mark_names).collect::>(); + // we don't care about marks with no corresponding bases, & vice-versa see: + // + pruned.retain(|_, anchors| { + anchors.retain(|anchor| { + anchor + .mark_group_name() + .map(|group| used_groups.contains(&group)) + .unwrap_or(false) + }); + !anchors.is_empty() + }); + Self { + gdef_classes, + anchors: pruned, + glyph_order, + fea_scripts: Default::default(), + static_metadata, } + } + + fn build(&self) -> Result { + let mark_base_groups = self.make_mark_to_base_groups(); + let mark_mark_groups = self.make_mark_to_mark_groups(); + + let mark_base = self.make_lookups::(mark_base_groups)?; + let mark_mark = self.make_lookups::(mark_mark_groups)?; + Ok(FeaRsMarks { + glyphmap: self.glyph_order.iter().cloned().collect(), + mark_base, + mark_mark, + }) + } + + fn make_lookups( + &self, + groups: BTreeMap, + ) -> Result, Error> { + groups + .into_iter() + .map(|(group_name, group)| { + assert!( + !group.bases.is_empty() && !group.marks.is_empty(), + "prechecked" + ); + + let mut builder = T::default(); + for (mark_name, anchor) in group.marks { + // we already filtered to only things in glyph order + let gid = self.glyph_order.glyph_id(&mark_name).unwrap(); + let anchor = resolve_anchor(anchor, self.static_metadata, &mark_name)?; + builder.add_mark(gid, &group_name, anchor); + } + + for (base_name, anchor) in group.bases { + let gid = self.glyph_order.glyph_id(&base_name).unwrap(); + let anchor = resolve_anchor(anchor, self.static_metadata, &base_name)?; + builder.add_base(gid, &group_name, anchor); + } - for anchor in &glyph_anchors.anchors { - if let AnchorKind::Base(group) = &anchor.kind { - // only if this anchor is a base, AND we have a mark in the same group - if mark_anchors.contains(group) { - assert_eq!(glyph, &glyph_anchors.glyph_name); - result + Ok(builder) + }) + .collect() + } + + fn make_mark_to_base_groups(&self) -> BTreeMap> { + let mut groups = BTreeMap::<_, MarkGroup>::new(); + for (glyph_name, anchors) in &self.anchors { + for anchor in anchors { + match &anchor.kind { + fontir::ir::AnchorKind::Base(group) => groups .entry(group.clone()) .or_default() .bases - .push((glyph.clone(), anchor.clone())) + .push((glyph_name.clone(), anchor)), + fontir::ir::AnchorKind::Mark(group) => groups + .entry(group.clone()) + .or_default() + .marks + .push((glyph_name.clone(), anchor)), + _ => continue, } } } + groups } - // then add the anchors for the mark glyphs, if they exist - for mark in mark_glyphs { - let anchors = anchors.get(&mark).unwrap(); - for anchor in &anchors.anchors { - if let AnchorKind::Mark(group) = &anchor.kind { - // only add mark glyph if there is at least one base - if let Some(group) = result.get_mut(group) { - group.marks.push((mark.clone(), anchor.clone())) + fn make_mark_to_mark_groups(&self) -> BTreeMap> { + // first find the set of glyphs that are marks, i.e. have any mark attachment point. + let (mark_glyphs, mark_anchors): (BTreeSet<_>, BTreeSet<_>) = self + .anchors + .iter() + .flat_map(|(name, anchors)| { + anchors.iter().filter_map(|anchor| { + if let AnchorKind::Mark(group) = &anchor.kind { + Some((name.clone(), group.clone())) + } else { + None + } + }) + }) + .unzip(); + + // then iterate again, looking for glyphs that we have identified as marks, + // but which also have participating base anchors + let mut result = BTreeMap::::new(); + for (glyph, glyph_anchors) in &self.anchors { + if !mark_glyphs.contains(glyph) { + continue; + } + + for anchor in glyph_anchors { + if let AnchorKind::Base(group) = &anchor.kind { + // only if this anchor is a base, AND we have a mark in the same group + if mark_anchors.contains(group) { + result + .entry(group.clone()) + .or_default() + .bases + .push((glyph.clone(), anchor)) + } } } } + + // then add the anchors for the mark glyphs, if they exist + for mark in mark_glyphs { + let anchors = self.anchors.get(&mark).unwrap(); + for anchor in anchors { + if let AnchorKind::Mark(group) = &anchor.kind { + // only add mark glyph if there is at least one base + if let Some(group) = result.get_mut(group) { + group.marks.push((mark.clone(), anchor)) + } + } + } + } + result } - result } impl Work for MarkWork { @@ -141,70 +273,14 @@ impl Work for MarkWork { let static_metadata = context.ir.static_metadata.get(); let glyph_order = context.ir.glyph_order.get(); let raw_anchors = context.ir.anchors.all(); - let gid = |name| { - glyph_order - .glyph_id(name) - .ok_or_else(|| Error::MissingGlyphId(name.clone())) - }; - - let mut anchors = BTreeMap::new(); - for (work_id, arc_glyph_anchors) in raw_anchors { - let FeWorkId::Anchor(glyph_name) = work_id else { - return Err(Error::ExpectedAnchor(work_id.clone())); - }; - anchors.insert(glyph_name.clone(), arc_glyph_anchors.clone()); - } - let groups = create_mark_to_base_groups(&anchors, &glyph_order); + let anchors = raw_anchors + .iter() + .map(|(_, anchors)| anchors.as_ref()) + .collect::>(); - let mut all_marks = FeaRsMarks { - glyphmap: glyph_order.iter().cloned().collect(), - ..Default::default() - }; - - for (group_name, group) in groups.iter() { - // if we have bases *and* marks produce mark to base - if !group.bases.is_empty() && !group.marks.is_empty() { - let mut mark_base = MarkToBaseBuilder::default(); - - for (mark_name, mark_anchor) in group.marks.iter() { - let gid = gid(mark_name)?; - let anchor = resolve_anchor(mark_anchor, &static_metadata, mark_name)?; - mark_base - .insert_mark(gid, group_name.clone(), anchor) - .map_err(|err| map_prev_class_error(err, group_name, mark_name))?; - } - - for (base_name, base_anchor) in group.bases.iter() { - let gid = gid(base_name)?; - let anchor = resolve_anchor(base_anchor, &static_metadata, base_name)?; - mark_base.insert_base(gid, group_name, anchor); - } - - all_marks.mark_base.push(mark_base); - } - } - - let mkmk_groups = create_mark_to_mark_groups(&anchors, &glyph_order); - for (group_name, MarkGroup { bases, marks }) in &mkmk_groups { - if bases.is_empty() || marks.is_empty() { - continue; - } - - let mut mark_mark = MarkToMarkBuilder::default(); - for (mark_name, mark_anchor) in marks { - let anchor = resolve_anchor(mark_anchor, &static_metadata, mark_name)?; - mark_mark - .insert_mark1(gid(mark_name)?, group_name.clone(), anchor) - .map_err(|err| map_prev_class_error(err, group_name, mark_name))?; - } - - for (base_name, base_anchor) in bases { - let anchor = resolve_anchor(base_anchor, &static_metadata, base_name)?; - mark_mark.insert_mark2(gid(base_name)?, group_name, anchor); - } - all_marks.mark_mark.push(mark_mark); - } + let ctx = MarkLookupBuilder::new(anchors, &glyph_order, None, &static_metadata); + let all_marks = ctx.build()?; context.fea_rs_marks.set(all_marks); @@ -212,14 +288,6 @@ impl Work for MarkWork { } } -fn map_prev_class_error(err: PreviouslyAssignedClass, class: &SmolStr, glyph: &GlyphName) -> Error { - Error::PreviouslyAssignedMarkClass { - old_class: err.class, - new_class: class.to_owned(), - glyph: glyph.to_owned(), - } -} - fn resolve_anchor( anchor: &fontir::ir::Anchor, static_metadata: &StaticMetadata, diff --git a/fontbe/src/orchestration.rs b/fontbe/src/orchestration.rs index f40be02b..af81bcd7 100644 --- a/fontbe/src/orchestration.rs +++ b/fontbe/src/orchestration.rs @@ -22,7 +22,7 @@ use fontdrasil::{ types::GlyphName, }; use fontir::{ - ir::{self, Anchor, GlyphOrder, KernGroup}, + ir::{self, GlyphOrder, KernGroup}, orchestration::{ Context as FeContext, ContextItem, ContextMap, Flags, IdAware, Persistable, PersistentStorage, WorkId as FeWorkIdentifier, @@ -318,12 +318,6 @@ impl TupleBuilder { } } -#[derive(Default, Clone, Serialize, Deserialize, PartialEq)] -pub(crate) struct MarkGroup { - pub(crate) bases: Vec<(GlyphName, Anchor)>, - pub(crate) marks: Vec<(GlyphName, Anchor)>, -} - /// Marks, ready to feed to fea-rs in the form it expects #[derive(Default, Clone, Serialize, Deserialize, PartialEq)] pub struct FeaRsMarks { diff --git a/fontir/src/ir.rs b/fontir/src/ir.rs index f1f65127..b5a73423 100644 --- a/fontir/src/ir.rs +++ b/fontir/src/ir.rs @@ -1078,6 +1078,16 @@ impl Anchor { .map(|(_, p)| *p) .unwrap() } + + /// If this is a mark, base, or ligature anchor, return the group name + pub fn mark_group_name(&self) -> Option<&SmolStr> { + match &self.kind { + AnchorKind::Base(group_name) + | AnchorKind::Mark(group_name) + | AnchorKind::Ligature { group_name, .. } => Some(group_name), + _ => None, + } + } } /// A type for building glyph anchors, reused by different backends