Skip to content

Commit

Permalink
[marks] Convert to GIDs earlier
Browse files Browse the repository at this point in the history 
This is more efficient, and better matches what we're doing in kern
which will let us more easily share code.
  • Loading branch information
@cmyr
cmyr committed Feb 21, 2025
1 parent 191e276 commit df05d78
Showing 1 changed file with 65 additions and 77 deletions.
142 changes: 65 additions & 77 deletions fontbe/src/features/marks.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ use crate::{
use fontir::{
ir::{self, Anchor, AnchorKind, GlyphAnchors, GlyphOrder, StaticMetadata},
orchestration::WorkId as FeWorkId,
variations::DeltaError,
};

use super::DFLT_SCRIPT;
Expand All @@ -45,14 +46,14 @@ type GroupName = SmolStr;

struct MarkLookupBuilder<'a> {
// extracted from public.openTypeCatgories/GlyphData.xml or FEA
gdef_classes: BTreeMap<GlyphName, GlyphClassDef>,
gdef_classes: HashMap<GlyphId16, GlyphClassDef>,
// pruned, only the anchors we are using
anchor_lists: BTreeMap<GlyphName, Vec<&'a ir::Anchor>>,
anchor_lists: BTreeMap<GlyphId16, Vec<&'a ir::Anchor>>,
glyph_order: &'a GlyphOrder,
static_metadata: &'a StaticMetadata,
// we don't currently use this, because just adding lookups to all scripts works?
_fea_scripts: HashSet<Tag>,
mark_glyphs: BTreeSet<GlyphName>,
mark_glyphs: BTreeSet<GlyphId16>,
lig_carets: BTreeMap<GlyphId16, Vec<CaretValue>>,
}

Expand All @@ -66,24 +67,20 @@ enum BaseOrLigAnchors<T> {
/// The bases and marks in a particular group, e.g. "top" or "bottom"
#[derive(Default, Debug, Clone, PartialEq)]
struct MarkGroup<'a> {
bases: Vec<(GlyphName, BaseOrLigAnchors<&'a ir::Anchor>)>,
marks: Vec<(GlyphName, &'a ir::Anchor)>,
bases: Vec<(GlyphId16, BaseOrLigAnchors<&'a ir::Anchor>)>,
marks: Vec<(GlyphId16, &'a ir::Anchor)>,
// if `true`, we will make a mark filter set from the marks in this group
// (only true for mkmk)
filter_glyphs: bool,
}

impl MarkGroup<'_> {
fn make_filter_glyph_set(&self, glyph_order: &GlyphOrder) -> Option<GlyphSet> {
fn make_filter_glyph_set(&self) -> Option<GlyphSet> {
self.filter_glyphs.then(|| {
self.marks
.iter()
.map(|(name, _)| glyph_order.glyph_id(name).unwrap())
.chain(
self.bases
.iter()
.map(|(name, _)| glyph_order.glyph_id(name).unwrap()),
)
.map(|(gid, _)| *gid)
.chain(self.bases.iter().map(|(gid, _)| *gid))
.collect()
})
}
Expand Down Expand Up @@ -148,13 +145,7 @@ impl<'a> MarkLookupBuilder<'a> {
static_metadata: &'a StaticMetadata,
ast: &FeaFirstPassOutput,
) -> Result<Self, Error> {
// the initial version of this code used names instead of GIDs, and
// so we keep that up for now by converting from gids to names, here
let gdef_classes: BTreeMap<_, _> =
super::get_gdef_classes(static_metadata, ast, glyph_order)
.into_iter()
.filter_map(|(k, v)| glyph_order.glyph_name(k.into()).cloned().map(|k| (k, v)))
.collect();
let gdef_classes = super::get_gdef_classes(static_metadata, ast, glyph_order);
let fea_scripts = get_fea_scripts(&ast.ast);
let lig_carets = get_ligature_carets(glyph_order, static_metadata, &anchors)?;
// first we want to narrow our input down to only anchors that are participating.
Expand All @@ -177,8 +168,9 @@ impl<'a> MarkLookupBuilder<'a> {
if !glyph_order.contains(&anchors.glyph_name) {
continue;
}
let gid = glyph_order.glyph_id(&anchors.glyph_name).unwrap();
// skip glyphs that are not mark/lig/base, if we have any defined categories
if !include.is_empty() && !include.contains(&anchors.glyph_name) {
if !include.is_empty() && !include.contains(&gid) {
continue;
}
for anchor in &anchors.anchors {
Expand All @@ -198,10 +190,7 @@ impl<'a> MarkLookupBuilder<'a> {
| AnchorKind::Caret(_)
| AnchorKind::VCaret(_) => (),
}
pruned
.entry(anchors.glyph_name.clone())
.or_insert(Vec::new())
.push(anchor);
pruned.entry(gid).or_insert(Vec::new()).push(anchor);
}
}

Expand Down Expand Up @@ -262,22 +251,21 @@ impl<'a> MarkLookupBuilder<'a> {
.filter(|(_, group)| !(group.bases.is_empty() || group.marks.is_empty()))
.map(|(group_name, group)| {
let mut builder = T::default();
let filter_set = group.make_filter_glyph_set(self.glyph_order);
let filter_set = group.make_filter_glyph_set();
let mut flags = LookupFlag::empty();
if filter_set.is_some() {
flags |= LookupFlag::USE_MARK_FILTERING_SET;
}
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_once(anchor, self.static_metadata, &mark_name)?;
builder.add_mark(gid, &group_name, anchor);
for (mark_gid, anchor) in &group.marks {
let anchor = resolve_anchor_once(anchor, self.static_metadata)
.map_err(|e| self.convert_delta_error(e, *mark_gid))?;
builder.add_mark(*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 (mark_gid, anchor) in group.bases {
let anchor = resolve_anchor(anchor, self.static_metadata)
.map_err(|e| self.convert_delta_error(e, mark_gid))?;
builder.add_base(mark_gid, &group_name, anchor);
}
Ok(PendingLookup::new(vec![builder], flags, filter_set))
})
Expand All @@ -286,11 +274,11 @@ impl<'a> MarkLookupBuilder<'a> {

fn make_mark_to_base_groups(&self) -> BTreeMap<GroupName, MarkGroup<'a>> {
let mut groups = BTreeMap::<_, MarkGroup>::new();
for (glyph_name, anchors) in &self.anchor_lists {
let is_mark = self.mark_glyphs.contains(glyph_name);
for (gid, anchors) in &self.anchor_lists {
let is_mark = self.mark_glyphs.contains(gid);
// if we have explicit gdef classes and this is not an expilcit base
let is_not_base = !self.gdef_classes.is_empty()
&& (self.gdef_classes.get(glyph_name)) != Some(&GlyphClassDef::Base);
&& (self.gdef_classes.get(gid)) != Some(&GlyphClassDef::Base);

let treat_as_base = !(is_mark | is_not_base);
for anchor in anchors {
Expand All @@ -299,12 +287,12 @@ impl<'a> MarkLookupBuilder<'a> {
.entry(group.clone())
.or_default()
.bases
.push((glyph_name.clone(), BaseOrLigAnchors::Base(anchor))),
.push((*gid, BaseOrLigAnchors::Base(anchor))),
ir::AnchorKind::Mark(group) if is_mark => groups
.entry(group.clone())
.or_default()
.marks
.push((glyph_name.clone(), anchor)),
.push((*gid, anchor)),
_ => continue,
}
}
Expand Down Expand Up @@ -332,8 +320,8 @@ impl<'a> MarkLookupBuilder<'a> {
// then iterate again, looking for glyphs that we have identified as marks,
// but which also have participating base anchors
let mut result = BTreeMap::<GroupName, MarkGroup>::new();
for (glyph, glyph_anchors) in &self.anchor_lists {
if !mark_glyphs.contains(glyph) {
for (gid, glyph_anchors) in &self.anchor_lists {
if !mark_glyphs.contains(gid) {
continue;
}

Expand All @@ -343,9 +331,7 @@ impl<'a> MarkLookupBuilder<'a> {
if mark_anchors.contains(group_name) {
let group = result.entry(group_name.clone()).or_default();
group.filter_glyphs = true;
group
.bases
.push((glyph.clone(), BaseOrLigAnchors::Base(anchor)))
group.bases.push((*gid, BaseOrLigAnchors::Base(anchor)))
}
}
}
Expand All @@ -358,7 +344,7 @@ impl<'a> MarkLookupBuilder<'a> {
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))
group.marks.push((*mark, anchor))
}
}
}
Expand All @@ -375,10 +361,10 @@ impl<'a> MarkLookupBuilder<'a> {

// first do a pass to build up the ligature anchors and track the set
// of mark classes used
for (glyph_name, anchors) in &self.anchor_lists {
for (gid, anchors) in &self.anchor_lists {
// skip anything that is definitely not a ligature glyph
let might_be_liga = self.gdef_classes.is_empty()
|| (self.gdef_classes.get(glyph_name) == Some(&GlyphClassDef::Ligature));
|| (self.gdef_classes.get(gid) == Some(&GlyphClassDef::Ligature));

if !might_be_liga {
continue;
Expand All @@ -405,13 +391,13 @@ impl<'a> MarkLookupBuilder<'a> {
.entry(group_name)
.or_default()
.bases
.push((glyph_name.clone(), BaseOrLigAnchors::Ligature(anchors)));
.push((*gid, BaseOrLigAnchors::Ligature(anchors)));
}
}

// then we do another pass to add the marks in the used classes
for (glyph_name, anchors) in &self.anchor_lists {
if !self.mark_glyphs.contains(glyph_name) {
for (gid, anchors) in &self.anchor_lists {
if !self.mark_glyphs.contains(gid) {
continue;
}
for anchor in anchors {
Expand All @@ -421,7 +407,7 @@ impl<'a> MarkLookupBuilder<'a> {
.entry(group.to_owned())
.or_default()
.marks
.push((glyph_name.clone(), anchor));
.push((*gid, anchor));
}
}
}
Expand All @@ -436,16 +422,16 @@ impl<'a> MarkLookupBuilder<'a> {
let mut affected_glyphs = BTreeSet::new();
let mut exits = BTreeMap::new();

for (glyph_name, anchors) in &self.anchor_lists {
for (gid, anchors) in &self.anchor_lists {
for anchor in anchors {
match anchor.kind {
AnchorKind::CursiveEntry => {
entries.insert(glyph_name, anchor);
affected_glyphs.insert(glyph_name);
entries.insert(*gid, anchor);
affected_glyphs.insert(*gid);
}
AnchorKind::CursiveExit => {
exits.insert(glyph_name, anchor);
affected_glyphs.insert(glyph_name);
exits.insert(*gid, anchor);
affected_glyphs.insert(*gid);
}
_ => {}
}
Expand All @@ -454,22 +440,28 @@ impl<'a> MarkLookupBuilder<'a> {
if affected_glyphs.is_empty() {
return Ok(vec![]);
}
for glyph_name in affected_glyphs {
let gid = self.glyph_order.glyph_id(glyph_name).unwrap();
for gid in affected_glyphs {
let entry_anchor = entries
.get(glyph_name)
.map(|anchor| resolve_anchor_once(anchor, self.static_metadata, glyph_name))
.transpose()?;
.get(&gid)
.map(|anchor| resolve_anchor_once(anchor, self.static_metadata))
.transpose()
.map_err(|e| self.convert_delta_error(e, gid))?;
let exit_anchor = exits
.get(glyph_name)
.map(|anchor| resolve_anchor_once(anchor, self.static_metadata, glyph_name))
.transpose()?;
.get(&gid)
.map(|anchor| resolve_anchor_once(anchor, self.static_metadata))
.transpose()
.map_err(|e| self.convert_delta_error(e, gid))?;
builder.insert(gid, entry_anchor, exit_anchor);
}
// In the future we might to do an LTR/RTL split, but for now return a
// vector with one element.
Ok(vec![PendingLookup::new(vec![builder], flags, None)])
}

fn convert_delta_error(&self, err: DeltaError, gid: GlyphId16) -> Error {
let name = self.glyph_order.glyph_name(gid.into()).cloned().unwrap();
Error::AnchorDeltaError(name, err)
}
}

impl Work<Context, AnyWorkId, Error> for MarkWork {
Expand Down Expand Up @@ -512,9 +504,9 @@ impl Work<Context, AnyWorkId, Error> for MarkWork {
// in py this is set during _groupMarkGlyphsByAnchor; we try to match that logic
// https://github.com/googlefonts/ufo2ft/blob/8e9e6eb66/Lib/ufo2ft/featureWriters/markFeatureWriter.py#L412
fn find_mark_glyphs(
anchors: &BTreeMap<GlyphName, Vec<&Anchor>>,
gdef_classes: &BTreeMap<GlyphName, GlyphClassDef>,
) -> BTreeSet<GlyphName> {
anchors: &BTreeMap<GlyphId16, Vec<&Anchor>>,
gdef_classes: &HashMap<GlyphId16, GlyphClassDef>,
) -> BTreeSet<GlyphId16> {
anchors
.iter()
.filter(|(name, anchors)| {
Expand Down Expand Up @@ -543,16 +535,15 @@ fn get_fea_scripts(ast: &ParseTree) -> HashSet<Tag> {
fn resolve_anchor(
anchor: BaseOrLigAnchors<&ir::Anchor>,
static_metadata: &StaticMetadata,
glyph_name: &GlyphName,
) -> Result<BaseOrLigAnchors<FeaAnchor>, Error> {
) -> Result<BaseOrLigAnchors<FeaAnchor>, DeltaError> {
match anchor {
BaseOrLigAnchors::Base(anchor) => {
resolve_anchor_once(anchor, static_metadata, glyph_name).map(BaseOrLigAnchors::Base)
resolve_anchor_once(anchor, static_metadata).map(BaseOrLigAnchors::Base)
}
BaseOrLigAnchors::Ligature(anchors) => anchors
.into_iter()
.map(|a| {
a.map(|a| resolve_anchor_once(a, static_metadata, glyph_name))
a.map(|a| resolve_anchor_once(a, static_metadata))
.transpose()
})
.collect::<Result<_, _>>()
Expand All @@ -563,8 +554,7 @@ fn resolve_anchor(
fn resolve_anchor_once(
anchor: &ir::Anchor,
static_metadata: &StaticMetadata,
glyph_name: &GlyphName, // just used for error reporting
) -> Result<FeaAnchor, Error> {
) -> Result<FeaAnchor, DeltaError> {
let (x_values, y_values): (Vec<_>, Vec<_>) = anchor
.positions
.iter()
Expand All @@ -583,13 +573,11 @@ fn resolve_anchor_once(
// clippy complains about the seemingly no-op identity map:
// https://rust-lang.github.io/rust-clippy/master/index.html#/map_identity
x_values.iter().map(|item| (&item.0, &item.1)),
)
.map_err(|err| Error::AnchorDeltaError(glyph_name.to_owned(), err))?;
)?;
let (y_default, y_deltas) = crate::features::resolve_variable_metric(
static_metadata,
y_values.iter().map(|item| (&item.0, &item.1)),
)
.map_err(|err| Error::AnchorDeltaError(glyph_name.to_owned(), err))?;
)?;

let mut anchor = FeaAnchor::new(x_default, y_default);
if x_deltas.iter().any(|v| v.1 != 0) {
Expand Down

0 comments on commit df05d78

Please sign in to comment.