From 0e44cc39c6b1d6fae352fbe162c2e836513f0b23 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Thu, 29 Feb 2024 09:29:29 -0500 Subject: [PATCH] [fea-rs] Remove ClassId, add gdef classes to output - ClassId was a pre-write-fonts enum standing in for the GlyphClassDef enum, which we can now juse use directly - passing any explicit GDEF classes through to the output feels like the simplest way to expose these to manual feature generation code, since that code needs to compile anyway to get the GSUB table. --- fea-rs/src/compile/compile_ctx.rs | 25 ++++++++++------ fea-rs/src/compile/lookups.rs | 23 ++++++++++----- fea-rs/src/compile/output.rs | 10 ++++++- fea-rs/src/compile/tables.rs | 2 +- fea-rs/src/compile/tables/gdef.rs | 49 +++++++++++++------------------ 5 files changed, 62 insertions(+), 47 deletions(-) diff --git a/fea-rs/src/compile/compile_ctx.rs b/fea-rs/src/compile/compile_ctx.rs index 930e26030..52de66a5e 100644 --- a/fea-rs/src/compile/compile_ctx.rs +++ b/fea-rs/src/compile/compile_ctx.rs @@ -13,7 +13,7 @@ use smol_str::SmolStr; use write_fonts::{ tables::{ self, - gdef::CaretValue, + gdef::{CaretValue, GlyphClassDef}, gpos::ValueFormat, layout::{ConditionFormat1, ConditionSet, FeatureVariations, LookupFlag}, variations::ivs_builder::{RemapVariationIndices, VariationStoreBuilder}, @@ -45,7 +45,7 @@ use super::{ }, metrics::{Anchor, DeviceOrDeltas, Metric, ValueRecord}, output::Compilation, - tables::{ClassId, ScriptRecord, Tables}, + tables::{GlyphClassDefExt, ScriptRecord, Tables}, tags, VariationInfo, }; @@ -240,6 +240,10 @@ impl<'a, F: FeatureProvider, V: VariationInfo> CompilationCtx<'a, F, V> { } } + let gdef_classes = self.tables.gdef.as_ref().and_then(|gdef| { + (!gdef.glyph_classes_were_inferred).then(|| gdef.glyph_classes.clone()) + }); + Ok(( Compilation { head: self.tables.head.as_ref().map(|raw| raw.build(None)), @@ -253,6 +257,7 @@ impl<'a, F: FeatureProvider, V: VariationInfo> CompilationCtx<'a, F, V> { gsub, gpos, opts: self.opts.clone(), + gdef_classes, }, self.errors.clone(), )) @@ -294,6 +299,7 @@ impl<'a, F: FeatureProvider, V: VariationInfo> CompilationCtx<'a, F, V> { let mut gdef = self.tables.gdef.take().unwrap_or_default(); // infer glyph classes, if they were not declared explicitly if gdef.glyph_classes.is_empty() { + gdef.glyph_classes_were_inferred = true; self.lookups.infer_glyph_classes(|glyph, class_id| { gdef.glyph_classes.insert(glyph, class_id); }); @@ -303,7 +309,7 @@ impl<'a, F: FeatureProvider, V: VariationInfo> CompilationCtx<'a, F, V> { .flat_map(|class| class.members.iter().map(|(cls, _)| cls.iter())) .flatten() { - gdef.glyph_classes.insert(glyph, ClassId::Mark); + gdef.glyph_classes.insert(glyph, GlyphClassDef::Mark); } } @@ -1691,10 +1697,10 @@ impl<'a, F: FeatureProvider, V: VariationInfo> CompilationCtx<'a, F, V> { typed::GdefTableItem::ClassDef(rule) => { for (class, id) in [ - (rule.base_glyphs(), ClassId::Base), - (rule.ligature_glyphs(), ClassId::Ligature), - (rule.mark_glyphs(), ClassId::Mark), - (rule.component_glyphs(), ClassId::Component), + (rule.base_glyphs(), GlyphClassDef::Base), + (rule.ligature_glyphs(), GlyphClassDef::Ligature), + (rule.mark_glyphs(), GlyphClassDef::Mark), + (rule.component_glyphs(), GlyphClassDef::Component), ] { let Some(class) = class else { continue; @@ -1702,8 +1708,9 @@ impl<'a, F: FeatureProvider, V: VariationInfo> CompilationCtx<'a, F, V> { if let Err((bad_glyph, old_class)) = gdef.add_glyph_class(self.resolve_glyph_class(&class), id) { - let bad_glyph_name = self.reverse_glyph_map.get(&bad_glyph).unwrap(); - self.error(class.range(), format!("class includes glyph '{bad_glyph_name}', already in class {old_class}")); + let bad_name = self.reverse_glyph_map.get(&bad_glyph).unwrap(); + let class_name = old_class.display(); + self.error(class.range(), format!("class includes glyph '{bad_name}', already in class {class_name}")); } } } diff --git a/fea-rs/src/compile/lookups.rs b/fea-rs/src/compile/lookups.rs index f684767cf..e5f029f45 100644 --- a/fea-rs/src/compile/lookups.rs +++ b/fea-rs/src/compile/lookups.rs @@ -14,6 +14,7 @@ use smol_str::SmolStr; use write_fonts::{ tables::{ + gdef::GlyphClassDef, gpos::{self as write_gpos}, gsub as write_gsub, layout::{ @@ -33,7 +34,7 @@ use crate::{ Kind, Opts, }; -use super::{features::AllFeatures, metrics::Anchor, tables::ClassId, tags}; +use super::{features::AllFeatures, metrics::Anchor, tags}; use contextual::{ ContextualLookupBuilder, PosChainContextBuilder, PosContextBuilder, ReverseChainBuilder, @@ -506,19 +507,27 @@ impl AllLookups { ))); } - pub(crate) fn infer_glyph_classes(&self, mut f: impl FnMut(GlyphId, ClassId)) { + pub(crate) fn infer_glyph_classes(&self, mut f: impl FnMut(GlyphId, GlyphClassDef)) { for lookup in &self.gpos { match lookup { PositionLookup::MarkToBase(lookup) => { for subtable in &lookup.subtables { - subtable.base_glyphs().for_each(|k| f(k, ClassId::Base)); - subtable.mark_glyphs().for_each(|k| f(k, ClassId::Mark)); + subtable + .base_glyphs() + .for_each(|k| f(k, GlyphClassDef::Base)); + subtable + .mark_glyphs() + .for_each(|k| f(k, GlyphClassDef::Mark)); } } PositionLookup::MarkToLig(lookup) => { for subtable in &lookup.subtables { - subtable.lig_glyphs().for_each(|k| f(k, ClassId::Ligature)); - subtable.mark_glyphs().for_each(|k| f(k, ClassId::Mark)); + subtable + .lig_glyphs() + .for_each(|k| f(k, GlyphClassDef::Ligature)); + subtable + .mark_glyphs() + .for_each(|k| f(k, GlyphClassDef::Mark)); } } PositionLookup::MarkToMark(lookup) => { @@ -526,7 +535,7 @@ impl AllLookups { subtable .mark1_glyphs() .chain(subtable.mark2_glyphs()) - .for_each(|k| f(k, ClassId::Mark)); + .for_each(|k| f(k, GlyphClassDef::Mark)); } } _ => (), diff --git a/fea-rs/src/compile/output.rs b/fea-rs/src/compile/output.rs index 8e54d8c5f..9549c71bf 100644 --- a/fea-rs/src/compile/output.rs +++ b/fea-rs/src/compile/output.rs @@ -1,7 +1,10 @@ //! The result of a compilation +use std::collections::HashMap; + use write_fonts::{ - tables::{self as wtables, maxp::Maxp}, + tables::{self as wtables, gdef::GlyphClassDef, maxp::Maxp}, + types::GlyphId, BuilderError, FontBuilder, }; @@ -42,6 +45,11 @@ pub struct Compilation { pub gsub: Option, /// The `GPOS` table, if one was generated pub gpos: Option, + /// Any *explicit* gdef classes declared in the FEA. + /// + /// This is provided so that the user can reference them if they are going + /// to manually generate kerning or markpos lookups. + pub gdef_classes: Option>, } impl Compilation { diff --git a/fea-rs/src/compile/tables.rs b/fea-rs/src/compile/tables.rs index 29c955a84..dd6958654 100644 --- a/fea-rs/src/compile/tables.rs +++ b/fea-rs/src/compile/tables.rs @@ -27,7 +27,7 @@ mod os2; mod stat; pub(crate) use base::{BaseBuilder, ScriptRecord}; -pub(crate) use gdef::{ClassId, GdefBuilder}; +pub(crate) use gdef::{GdefBuilder, GlyphClassDefExt}; pub(crate) use name::{NameBuilder, NameSpec}; pub(crate) use os2::{CodePageRange, Os2Builder}; pub(crate) use stat::{AxisLocation, AxisRecord, AxisValue, StatBuilder, StatFallbackName}; diff --git a/fea-rs/src/compile/tables/gdef.rs b/fea-rs/src/compile/tables/gdef.rs index 1c204c87e..6550c3709 100644 --- a/fea-rs/src/compile/tables/gdef.rs +++ b/fea-rs/src/compile/tables/gdef.rs @@ -7,6 +7,7 @@ //! [gdef-spec]: http://adobe-type-tools.github.io/afdko/OpenTypeFeatureFileSpecification.html#9b-gdef-table use std::collections::{BTreeMap, BTreeSet, HashMap}; +use std::fmt::Display; use write_fonts::types::GlyphId; @@ -24,7 +25,12 @@ use crate::common::{GlyphClass, GlyphSet}; /// Data collected from a GDEF block. #[derive(Clone, Debug, Default)] pub struct GdefBuilder { - pub glyph_classes: HashMap, + pub glyph_classes: HashMap, + /// if `true`, then glyph classes were not declared explicitly. + /// + /// we track this because it is an important distinction when using the + /// glyph classes for manually generated kern/markpos lookups + pub glyph_classes_were_inferred: bool, pub attach: BTreeMap>, pub ligature_pos: BTreeMap>, pub mark_attach_class: BTreeMap, @@ -32,26 +38,6 @@ pub struct GdefBuilder { pub var_store: Option, } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -#[repr(u16)] -pub enum ClassId { - Base = 1, - Ligature = 2, - Mark = 3, - Component = 4, -} - -impl From for GlyphClassDef { - fn from(src: ClassId) -> GlyphClassDef { - match src { - ClassId::Base => GlyphClassDef::Base, - ClassId::Ligature => GlyphClassDef::Ligature, - ClassId::Mark => GlyphClassDef::Mark, - ClassId::Component => GlyphClassDef::Component, - } - } -} - impl GdefBuilder { pub fn build(&self) -> (tables::gdef::Gdef, Option) { let mut table = tables::gdef::Gdef::new( @@ -128,8 +114,8 @@ impl GdefBuilder { // technically should be a GlyphSet, but we're storing it as individual // glyphs and this is private API, so :shrug: glyphs: GlyphClass, - class: ClassId, - ) -> Result<(), (GlyphId, ClassId)> { + class: GlyphClassDef, + ) -> Result<(), (GlyphId, GlyphClassDef)> { for glyph in glyphs.iter() { if let Some(prev_class) = self.glyph_classes.insert(glyph, class) { if prev_class != class { @@ -150,13 +136,18 @@ impl GdefBuilder { } } -impl std::fmt::Display for ClassId { - fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { +pub(crate) trait GlyphClassDefExt { + fn display(&self) -> impl Display; +} + +impl GlyphClassDefExt for GlyphClassDef { + fn display(&self) -> impl Display { match self { - ClassId::Base => write!(f, "Base"), - ClassId::Ligature => write!(f, "Ligature"), - ClassId::Mark => write!(f, "Mark"), - ClassId::Component => write!(f, "Component"), + GlyphClassDef::Base => "Base", + GlyphClassDef::Ligature => "Ligature", + GlyphClassDef::Mark => "Mark", + GlyphClassDef::Component => "Component", + _ => "Invalid", } } }