From b5e31922b5e362be09efae7b7ac0c22d40ac5c55 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Thu, 23 Nov 2023 12:36:31 -0500 Subject: [PATCH 1/5] [fea-rs] Add variable-native ValueRecord and Anchor These types store their deltas inline, which means that we don't need to have access to an ItemVariationStore in orderto create them. This, in turn, lets us move all of the concern over deltas to the end of the compile pass, and also lets us provide a nicer feature writer API. --- fea-rs/src/common.rs | 5 +- fea-rs/src/compile.rs | 1 + fea-rs/src/compile/compile_ctx.rs | 150 ++++++------- fea-rs/src/compile/feature_writer.rs | 3 +- fea-rs/src/compile/lookups.rs | 162 +++++--------- fea-rs/src/compile/lookups/contextual.rs | 24 +- fea-rs/src/compile/lookups/gpos_builders.rs | 223 ++++++++----------- fea-rs/src/compile/lookups/gsub_builders.rs | 10 +- fea-rs/src/compile/tables.rs | 10 - fea-rs/src/compile/valuerecordext.rs | 229 ++++++++++++++------ 10 files changed, 401 insertions(+), 416 deletions(-) diff --git a/fea-rs/src/common.rs b/fea-rs/src/common.rs index 4a55b354c..b17255518 100644 --- a/fea-rs/src/common.rs +++ b/fea-rs/src/common.rs @@ -3,7 +3,6 @@ use std::fmt::{Display, Formatter}; use smol_str::SmolStr; -use write_fonts::tables::gpos::AnchorTable; pub use write_fonts::types::GlyphId; mod glyph_class; @@ -14,6 +13,8 @@ pub(crate) use glyph_class::GlyphClass; pub use glyph_class::GlyphSet; pub use glyph_map::GlyphMap; +use crate::compile::Anchor; + /// A glyph name pub type GlyphName = SmolStr; @@ -41,7 +42,7 @@ pub enum GlyphIdent { #[derive(Clone, Debug, Default)] pub(crate) struct MarkClass { - pub(crate) members: Vec<(GlyphClass, Option)>, + pub(crate) members: Vec<(GlyphClass, Option)>, } impl> From for GlyphIdent { diff --git a/fea-rs/src/compile.rs b/fea-rs/src/compile.rs index cd1477b12..49dfe1f68 100644 --- a/fea-rs/src/compile.rs +++ b/fea-rs/src/compile.rs @@ -21,6 +21,7 @@ pub use lookups::{ }; pub use opts::Opts; pub use output::Compilation; +pub use valuerecordext::{Anchor, ValueRecord}; pub use variations::{AxisLocation, VariationInfo}; #[cfg(any(test, feature = "test", feature = "cli"))] diff --git a/fea-rs/src/compile/compile_ctx.rs b/fea-rs/src/compile/compile_ctx.rs index 430f52e10..f4289ce23 100644 --- a/fea-rs/src/compile/compile_ctx.rs +++ b/fea-rs/src/compile/compile_ctx.rs @@ -14,10 +14,9 @@ use write_fonts::{ tables::{ self, gdef::CaretValue, - gpos::{AnchorTable, ValueFormat, ValueRecord}, - layout::{ - ConditionFormat1, ConditionSet, FeatureVariations, LookupFlag, PendingVariationIndex, - }, + gpos::ValueFormat, + layout::{ConditionFormat1, ConditionSet, FeatureVariations, LookupFlag}, + variations::ivs_builder::{RemapVariationIndices, VariationStoreBuilder}, }, types::{F2Dot14, NameId, Tag}, }; @@ -47,7 +46,7 @@ use super::{ output::Compilation, tables::{ClassId, ScriptRecord, Tables}, tags, - valuerecordext::ValueRecordExt, + valuerecordext::{Anchor, DeviceOrDeltas, Metric, ValueRecord}, VariationInfo, }; @@ -88,7 +87,7 @@ pub struct CompilationCtx<'a> { script: Option, glyph_class_defs: HashMap, mark_classes: HashMap, - anchor_defs: HashMap, + anchor_defs: HashMap, value_record_defs: HashMap, conditionset_defs: ConditionSetMap, mark_attach_class_id: HashMap, @@ -184,18 +183,22 @@ impl<'a> CompilationCtx<'a> { .as_ref() .map(|raw| raw.build(&mut name_builder)); + // the var store builder is required so that variable metrics/anchors + // in the GPOS table can be collected into an ItemVariationStore + let mut ivs = VariationStoreBuilder::new(); + + let (mut gsub, mut gpos) = self.lookups.build(&self.features, &mut ivs); + if !ivs.is_empty() { + self.tables + .gdef + .get_or_insert_with(Default::default) + .var_store = Some(ivs); + } + let (gdef, key_map) = match self.tables.gdef.as_ref().map(|raw| raw.build()) { Some((gdef, key_map)) => (Some(gdef), key_map), None => (None, None), }; - // all VariationIndex tables (in value records and anchors) currently have - // temporary indices; now that we've built the ItemVariationStore we need - // to go and update them all. - if let Some(key_map) = key_map { - self.lookups.update_variation_index_tables(&key_map); - } - - let (mut gsub, mut gpos) = self.lookups.build(&self.features); let feature_params = self.features.build_feature_params(&mut name_builder); @@ -212,6 +215,12 @@ impl<'a> CompilationCtx<'a> { } } if let Some(gpos) = gpos.as_mut() { + if let Some(key_map) = key_map { + // all VariationIndex tables (in value records and anchors) + // currently have temporary indices; now that we've built the + // ItemVariationStore we need to go and update them all. + gpos.remap_variation_indices(&key_map); + } if let Some(variations) = gpos.feature_variations.as_mut() { sort_feature_variations(variations, |condset| { self.conditionset_defs.sort_order(condset) @@ -1084,15 +1093,17 @@ impl<'a> CompilationCtx<'a> { } if let Some(adv) = record.advance() { - let (adv, var_idx) = self.resolve_metric(&adv); + let adv = self.resolve_metric(&adv); if self.vertical_feature.in_eligible_vertical_feature() { - return ValueRecord::new() - .with_y_advance(adv) - .with_device(var_idx, ValueFormat::Y_ADVANCE_DEVICE); + return ValueRecord { + y_advance: Some(adv), + ..Default::default() + }; } else { - return ValueRecord::new() - .with_x_advance(adv) - .with_device(var_idx, ValueFormat::X_ADVANCE_DEVICE); + return ValueRecord { + x_advance: Some(adv), + ..Default::default() + }; } } @@ -1101,20 +1112,16 @@ impl<'a> CompilationCtx<'a> { return Default::default(); }; - let (x_place, x_place_var) = self.resolve_metric(&x_place); - let (y_place, y_place_var) = self.resolve_metric(&y_place); - let (x_adv, x_adv_var) = self.resolve_metric(&x_adv); - let (y_adv, y_adv_var) = self.resolve_metric(&y_adv); - - let result = ValueRecord::new() - .with_x_placement(x_place) - .with_y_placement(y_place) - .with_x_advance(x_adv) - .with_y_advance(y_adv) - .with_device(x_place_var, ValueFormat::X_PLACEMENT_DEVICE) - .with_device(y_place_var, ValueFormat::Y_PLACEMENT_DEVICE) - .with_device(x_adv_var, ValueFormat::X_ADVANCE_DEVICE) - .with_device(y_adv_var, ValueFormat::Y_ADVANCE_DEVICE); + let x_placement = self.resolve_metric(&x_place); + let y_placement = self.resolve_metric(&y_place); + let x_advance = self.resolve_metric(&x_adv); + let y_advance = self.resolve_metric(&y_adv); + let mut result = ValueRecord { + x_placement: Some(x_placement), + y_placement: Some(y_placement), + x_advance: Some(x_advance), + y_advance: Some(y_advance), + }; if let Some([x_place_dev, y_place_dev, x_adv_dev, y_adv_dev]) = record.device() { // if we have an explicit device we must not also be variable @@ -1127,32 +1134,29 @@ impl<'a> CompilationCtx<'a> { ), "checked during parsing" ); - return result - .with_device(x_place_dev.compile(), ValueFormat::X_PLACEMENT_DEVICE) - .with_device(y_place_dev.compile(), ValueFormat::Y_PLACEMENT_DEVICE) - .with_device(x_adv_dev.compile(), ValueFormat::X_ADVANCE_DEVICE) - .with_device(y_adv_dev.compile(), ValueFormat::Y_ADVANCE_DEVICE); + result.x_advance.as_mut().unwrap().device_or_deltas = x_adv_dev.compile().into(); + result.y_advance.as_mut().unwrap().device_or_deltas = y_adv_dev.compile().into(); + result.x_placement.as_mut().unwrap().device_or_deltas = x_place_dev.compile().into(); + result.y_placement.as_mut().unwrap().device_or_deltas = y_place_dev.compile().into(); } result } - fn resolve_metric(&mut self, metric: &typed::Metric) -> (i16, Option) { + //fn resolve_metric(&mut self, metric: &typed::Metric) -> (i16, Option) { + fn resolve_metric(&mut self, metric: &typed::Metric) -> Metric { match metric { - typed::Metric::Scalar(value) => (value.parse_signed(), None), + typed::Metric::Scalar(value) => value.parse_signed().into(), typed::Metric::Variable(variable) => self.resolve_variable_metric(variable), } } - fn resolve_variable_metric( - &mut self, - metric: &typed::VariableMetric, - ) -> (i16, Option) { + fn resolve_variable_metric(&mut self, metric: &typed::VariableMetric) -> Metric { let Some(var_info) = self.variation_info else { self.error( metric.range(), "variable metric only valid when compiling variable font", ); - return (0, None); + return Default::default(); }; let mut locations = HashMap::new(); @@ -1176,14 +1180,13 @@ impl<'a> CompilationCtx<'a> { locations.insert(pos, metric_loc.value().parse_signed()); } match var_info.resolve_variable_metric(&locations) { - Ok((default, deltas)) => { - let temp_idx = self.tables.var_store().add_deltas(deltas); - let var_idx = PendingVariationIndex::new(temp_idx); - (default, Some(var_idx)) - } + Ok((default, deltas)) => Metric { + default, + device_or_deltas: DeviceOrDeltas::Deltas(deltas), + }, Err(e) => { self.error(metric.range(), format!("failed to compute deltas: '{e}'")); - (0, None) + Default::default() } } } @@ -1797,15 +1800,15 @@ impl<'a> CompilationCtx<'a> { let anchor_block = anchor_def.anchor(); let name = anchor_def.name(); let anchor = match self.resolve_anchor(&anchor_block) { - Some(a @ AnchorTable::Format1(_) | a @ AnchorTable::Format2(_)) => a, - Some(_) => { + Some(a) if !a.x.has_device_or_deltas() && !a.y.has_device_or_deltas() => a, + _ => { return self.error( anchor_block.range(), "named anchor definition can only be in format A or B", ) } - None => return, }; + if let Some(_prev) = self .anchor_defs .insert(name.text.clone(), (anchor, anchor_def.range().start)) @@ -1814,7 +1817,7 @@ impl<'a> CompilationCtx<'a> { } } - fn resolve_anchor(&mut self, item: &typed::Anchor) -> Option { + fn resolve_anchor(&mut self, item: &typed::Anchor) -> Option { if item.null().is_some() { return None; } @@ -1834,33 +1837,24 @@ impl<'a> CompilationCtx<'a> { return None; }; - let (x, x_var) = self.resolve_metric(&x); - let (y, y_var) = self.resolve_metric(&y); - - if x_var.is_some() || y_var.is_some() { - return Some(AnchorTable::format_3( - x, - y, - x_var.map(Into::into), - y_var.map(Into::into), - )); - } + let x = self.resolve_metric(&x); + let y = self.resolve_metric(&y); + let mut anchor = Anchor { + x, + y, + contourpoint: None, + }; if let Some(point) = item.contourpoint() { match point.parse_unsigned() { - Some(point) => Some(AnchorTable::format_2(x, y, point)), + Some(point) => anchor.contourpoint = Some(point), None => panic!("negative contourpoint, go fix your parser"), } - } else if let Some((x_coord, y_coord)) = item.devices() { - Some(AnchorTable::format_3( - x, - y, - x_coord.compile().map(Into::into), - y_coord.compile().map(Into::into), - )) - } else { - Some(AnchorTable::format_1(x, y)) + } else if let Some((x_dev, y_dev)) = item.devices() { + anchor.x.device_or_deltas = x_dev.compile().into(); + anchor.y.device_or_deltas = y_dev.compile().into(); } + Some(anchor) } fn define_condition_set(&mut self, node: typed::ConditionSet) { diff --git a/fea-rs/src/compile/feature_writer.rs b/fea-rs/src/compile/feature_writer.rs index 579322bb1..8a2858e32 100644 --- a/fea-rs/src/compile/feature_writer.rs +++ b/fea-rs/src/compile/feature_writer.rs @@ -101,8 +101,7 @@ impl<'a> FeatureBuilder<'a> { &mut self, deltas: Vec<(VariationRegion, T)>, ) -> PendingVariationIndex { - let delta_set_id = self.tables.var_store().add_deltas(deltas); - PendingVariationIndex { delta_set_id } + todo!("remove me!") } /// Add lookups to every default language system. diff --git a/fea-rs/src/compile/lookups.rs b/fea-rs/src/compile/lookups.rs index b34b092de..1a92396dd 100644 --- a/fea-rs/src/compile/lookups.rs +++ b/fea-rs/src/compile/lookups.rs @@ -14,26 +14,26 @@ use smol_str::SmolStr; use write_fonts::{ tables::{ - gpos::{self as write_gpos, AnchorTable, ValueRecord}, + gpos::{self as write_gpos}, gsub as write_gsub, layout::{ - ConditionSet as RawConditionSet, DeviceOrVariationIndex, Feature, FeatureList, - FeatureRecord, FeatureTableSubstitution, FeatureTableSubstitutionRecord, - FeatureVariationRecord, FeatureVariations, LangSys, LangSysRecord, Lookup as RawLookup, - LookupFlag, LookupList, Script, ScriptList, ScriptRecord, + ConditionSet as RawConditionSet, Feature, FeatureList, FeatureRecord, + FeatureTableSubstitution, FeatureTableSubstitutionRecord, FeatureVariationRecord, + FeatureVariations, LangSys, LangSysRecord, Lookup as RawLookup, LookupFlag, LookupList, + Script, ScriptList, ScriptRecord, }, - variations::ivs_builder::VariationIndexRemapping, + variations::ivs_builder::VariationStoreBuilder, }, types::Tag, }; use crate::{ common::{GlyphId, GlyphOrClass, GlyphSet}, - compile::lookups::contextual::ChainOrNot, + compile::{lookups::contextual::ChainOrNot, valuerecordext::ValueRecord}, Kind, }; -use super::{features::AllFeatures, tables::ClassId, tags}; +use super::{features::AllFeatures, tables::ClassId, tags, valuerecordext::Anchor}; use contextual::{ ContextualLookupBuilder, PosChainContextBuilder, PosContextBuilder, ReverseChainBuilder, @@ -49,9 +49,12 @@ use gsub_builders::{ }; pub(crate) use helpers::ClassDefBuilder2; -pub trait Builder { +// A simple trait for building lookups +// This exists because we use it to implement `LookupBuilder` +pub(crate) trait Builder { type Output; - fn build(self) -> Self::Output; + // the var_store is only used in GPOS, but we pass it everywhere + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output; } pub(crate) type FilterSetId = u16; @@ -183,14 +186,6 @@ pub(crate) struct PosSubBuilder { variations: HashMap>>, } -/// A trait for position lookups which might contain VariationIndex tables -/// -/// These tables contain indicies that are not known until compilaiton time, -/// so we need to go and update them all then. -pub(crate) trait VariationIndexContainingLookup { - fn remap_variation_indices(&mut self, key_map: &VariationIndexRemapping); -} - impl LookupBuilder { fn new(flags: LookupFlag, mark_set: Option) -> Self { LookupBuilder { @@ -226,14 +221,6 @@ impl LookupBuilder { } } -impl LookupBuilder { - fn update_variation_index_tables(&mut self, key_map: &VariationIndexRemapping) { - for table in &mut self.subtables { - table.remap_variation_indices(key_map); - } - } -} - impl LookupBuilder { /// A helper method for converting from (say) ContextBuilder to PosContextBuilder fn convert>(self) -> LookupBuilder { @@ -263,18 +250,6 @@ impl PositionLookup { PositionLookup::ChainedContextual(lookup) => lookup.force_subtable_break(), } } - - fn update_variation_index_tables(&mut self, key_map: &VariationIndexRemapping) { - match self { - PositionLookup::Single(lookup) => lookup.update_variation_index_tables(key_map), - PositionLookup::Pair(lookup) => lookup.update_variation_index_tables(key_map), - PositionLookup::Cursive(lookup) => lookup.update_variation_index_tables(key_map), - PositionLookup::MarkToBase(lookup) => lookup.update_variation_index_tables(key_map), - PositionLookup::MarkToLig(lookup) => lookup.update_variation_index_tables(key_map), - PositionLookup::MarkToMark(lookup) => lookup.update_variation_index_tables(key_map), - PositionLookup::Contextual(_) | PositionLookup::ChainedContextual(_) => (), - } - } } impl SubstitutionLookup { @@ -298,11 +273,11 @@ where { type Output = RawLookup; - fn build(self) -> Self::Output { + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output { let subtables = self .subtables .into_iter() - .flat_map(|b| b.build().into_iter()) + .flat_map(|b| b.build(var_store).into_iter()) .collect(); RawLookup::new(self.flags, subtables, self.mark_set.unwrap_or_default()) } @@ -311,25 +286,31 @@ where impl Builder for PositionLookup { type Output = write_gpos::PositionLookup; - fn build(self) -> Self::Output { + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output { match self { - PositionLookup::Single(lookup) => write_gpos::PositionLookup::Single(lookup.build()), - PositionLookup::Pair(lookup) => write_gpos::PositionLookup::Pair(lookup.build()), - PositionLookup::Cursive(lookup) => write_gpos::PositionLookup::Cursive(lookup.build()), + PositionLookup::Single(lookup) => { + write_gpos::PositionLookup::Single(lookup.build(var_store)) + } + PositionLookup::Pair(lookup) => { + write_gpos::PositionLookup::Pair(lookup.build(var_store)) + } + PositionLookup::Cursive(lookup) => { + write_gpos::PositionLookup::Cursive(lookup.build(var_store)) + } PositionLookup::MarkToBase(lookup) => { - write_gpos::PositionLookup::MarkToBase(lookup.build()) + write_gpos::PositionLookup::MarkToBase(lookup.build(var_store)) } PositionLookup::MarkToLig(lookup) => { - write_gpos::PositionLookup::MarkToLig(lookup.build()) + write_gpos::PositionLookup::MarkToLig(lookup.build(var_store)) } PositionLookup::MarkToMark(lookup) => { - write_gpos::PositionLookup::MarkToMark(lookup.build()) + write_gpos::PositionLookup::MarkToMark(lookup.build(var_store)) } PositionLookup::Contextual(lookup) => { - write_gpos::PositionLookup::Contextual(lookup.build().into_concrete()) + write_gpos::PositionLookup::Contextual(lookup.build(var_store).into_concrete()) } PositionLookup::ChainedContextual(lookup) => { - write_gpos::PositionLookup::ChainContextual(lookup.build().into_concrete()) + write_gpos::PositionLookup::ChainContextual(lookup.build(var_store).into_concrete()) } } } @@ -338,28 +319,30 @@ impl Builder for PositionLookup { impl Builder for SubstitutionLookup { type Output = write_gsub::SubstitutionLookup; - fn build(self) -> Self::Output { + fn build(self, _var_store: &mut VariationStoreBuilder) -> Self::Output { match self { SubstitutionLookup::Single(lookup) => { - write_gsub::SubstitutionLookup::Single(lookup.build()) + write_gsub::SubstitutionLookup::Single(lookup.build(_var_store)) } SubstitutionLookup::Multiple(lookup) => { - write_gsub::SubstitutionLookup::Multiple(lookup.build()) + write_gsub::SubstitutionLookup::Multiple(lookup.build(_var_store)) } SubstitutionLookup::Alternate(lookup) => { - write_gsub::SubstitutionLookup::Alternate(lookup.build()) + write_gsub::SubstitutionLookup::Alternate(lookup.build(_var_store)) } SubstitutionLookup::Ligature(lookup) => { - write_gsub::SubstitutionLookup::Ligature(lookup.build()) + write_gsub::SubstitutionLookup::Ligature(lookup.build(_var_store)) } SubstitutionLookup::Contextual(lookup) => { - write_gsub::SubstitutionLookup::Contextual(lookup.build().into_concrete()) + write_gsub::SubstitutionLookup::Contextual(lookup.build(_var_store).into_concrete()) } SubstitutionLookup::ChainedContextual(lookup) => { - write_gsub::SubstitutionLookup::ChainContextual(lookup.build().into_concrete()) + write_gsub::SubstitutionLookup::ChainContextual( + lookup.build(_var_store).into_concrete(), + ) } SubstitutionLookup::Reverse(lookup) => { - write_gsub::SubstitutionLookup::Reverse(lookup.build()) + write_gsub::SubstitutionLookup::Reverse(lookup.build(_var_store)) } } } @@ -630,12 +613,6 @@ impl AllLookups { lookup_ids } - pub(crate) fn update_variation_index_tables(&mut self, key_map: &VariationIndexRemapping) { - for lookup in self.gpos.iter_mut() { - lookup.update_variation_index_tables(key_map); - } - } - /// Returns a map that must be used to remap the ids in any features where /// they were used. pub(crate) fn merge_external_lookups( @@ -653,6 +630,7 @@ impl AllLookups { pub(crate) fn build( &self, features: &AllFeatures, + var_store: &mut VariationStoreBuilder, ) -> (Option, Option) { let mut gpos_builder = PosSubBuilder::new(self.gpos.clone()); let mut gsub_builder = PosSubBuilder::new(self.gsub.clone()); @@ -702,7 +680,7 @@ impl AllLookups { } } - (gsub_builder.build(), gpos_builder.build()) + (gsub_builder.build(var_store), gpos_builder.build(var_store)) } } @@ -869,8 +847,8 @@ impl SomeLookup { pub(crate) fn add_gpos_type_3( &mut self, id: GlyphId, - entry: Option, - exit: Option, + entry: Option, + exit: Option, ) { if let SomeLookup::GposLookup(PositionLookup::Cursive(table)) = self { let subtable = table.last_mut().unwrap(); @@ -1050,6 +1028,7 @@ where #[allow(clippy::type_complexity)] // i love my big dumb tuple fn build_raw( self, + var_store: &mut VariationStoreBuilder, ) -> Option<( LookupList, ScriptList, @@ -1084,7 +1063,11 @@ where }) .collect::>(); - let lookups = self.lookups.into_iter().map(|x| x.build()).collect(); + let lookups = self + .lookups + .into_iter() + .map(|x| x.build(var_store)) + .collect(); let variations = if self.variations.is_empty() { None @@ -1126,8 +1109,8 @@ where impl Builder for PosSubBuilder { type Output = Option; - fn build(self) -> Self::Output { - self.build_raw() + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output { + self.build_raw(var_store) .map(|(lookups, scripts, features, variations)| { let mut gpos = write_gpos::Gpos::new(scripts, features, lookups); gpos.feature_variations = variations.into(); @@ -1139,8 +1122,8 @@ impl Builder for PosSubBuilder { impl Builder for PosSubBuilder { type Output = Option; - fn build(self) -> Self::Output { - self.build_raw() + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output { + self.build_raw(var_store) .map(|(lookups, scripts, features, variations)| { let mut gsub = write_gsub::Gsub::new(scripts, features, lookups); gsub.feature_variations = variations.into(); @@ -1163,43 +1146,6 @@ impl FeatureKey { } } -impl VariationIndexContainingLookup for ValueRecord { - fn remap_variation_indices(&mut self, key_map: &VariationIndexRemapping) { - for table in [ - self.x_placement_device.as_mut(), - self.y_placement_device.as_mut(), - self.x_advance_device.as_mut(), - self.y_advance_device.as_mut(), - ] - .into_iter() - .flatten() - { - table.remap_variation_indices(key_map) - } - } -} - -impl VariationIndexContainingLookup for DeviceOrVariationIndex { - fn remap_variation_indices(&mut self, key_map: &VariationIndexRemapping) { - if let DeviceOrVariationIndex::PendingVariationIndex(table) = self { - *self = key_map.get(table.delta_set_id).unwrap().into(); - } - } -} - -impl VariationIndexContainingLookup for AnchorTable { - fn remap_variation_indices(&mut self, key_map: &VariationIndexRemapping) { - if let AnchorTable::Format3(table) = self { - table - .x_device - .as_mut() - .into_iter() - .chain(table.y_device.as_mut()) - .for_each(|x| x.remap_variation_indices(key_map)) - } - } -} - fn is_gpos_rule(kind: Kind) -> bool { matches!( kind, diff --git a/fea-rs/src/compile/lookups/contextual.rs b/fea-rs/src/compile/lookups/contextual.rs index 743c744fe..f4066ccd9 100644 --- a/fea-rs/src/compile/lookups/contextual.rs +++ b/fea-rs/src/compile/lookups/contextual.rs @@ -7,17 +7,17 @@ use std::{ use write_fonts::{ tables::{ - gpos::ValueRecord, gsub as write_gsub, gsub::ReverseChainSingleSubstFormat1, layout::{self as write_layout, CoverageTableBuilder, LookupFlag}, + variations::ivs_builder::VariationStoreBuilder, }, types::GlyphId, validate::Validate, FontWrite, }; -use crate::common::GlyphOrClass; +use crate::{common::GlyphOrClass, compile::valuerecordext::ValueRecord}; use super::{ Builder, ClassDefBuilder2, FilterSetId, LookupBuilder, LookupId, PositionLookup, @@ -440,21 +440,25 @@ impl ContextRule { impl Builder for PosContextBuilder { type Output = Vec; - fn build(self) -> Self::Output { - self.0.build(true) + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output { + self.0.build(Some(var_store)) } } impl Builder for SubContextBuilder { type Output = Vec; - fn build(self) -> Self::Output { - self.0.build(false) + fn build(self, _var_store: &mut VariationStoreBuilder) -> Self::Output { + self.0.build(None) } } impl ContextBuilder { - fn build(self, in_gpos: bool) -> Vec { + fn build( + self, + var_store: Option<&mut VariationStoreBuilder>, + ) -> Vec { + let in_gpos = var_store.is_some(); assert!(self.rules.iter().all(|rule| !rule.is_chain_rule())); let format_1 = self.build_format_1(in_gpos); //NOTE: I'm skipping format_2 because it seems consistently larger @@ -674,7 +678,7 @@ impl SubChainContextBuilder { impl Builder for PosChainContextBuilder { type Output = Vec; - fn build(self) -> Self::Output { + fn build(self, _: &mut VariationStoreBuilder) -> Self::Output { self.0.build(true) } } @@ -682,7 +686,7 @@ impl Builder for PosChainContextBuilder { impl Builder for SubChainContextBuilder { type Output = Vec; - fn build(self) -> Self::Output { + fn build(self, _: &mut VariationStoreBuilder) -> Self::Output { self.0.build(false) } } @@ -733,7 +737,7 @@ impl ReverseChainBuilder { impl Builder for ReverseChainBuilder { type Output = Vec; - fn build(self) -> Self::Output { + fn build(self, _: &mut VariationStoreBuilder) -> Self::Output { self.rules .into_iter() .map(|rule| { diff --git a/fea-rs/src/compile/lookups/gpos_builders.rs b/fea-rs/src/compile/lookups/gpos_builders.rs index 36347d96f..34c84998b 100644 --- a/fea-rs/src/compile/lookups/gpos_builders.rs +++ b/fea-rs/src/compile/lookups/gpos_builders.rs @@ -5,16 +5,19 @@ use std::collections::{BTreeMap, HashMap}; use smol_str::SmolStr; use write_fonts::{ tables::{ - gpos::{self as write_gpos, AnchorTable, MarkRecord, ValueFormat, ValueRecord}, + gpos::{self as write_gpos, MarkRecord, ValueFormat, ValueRecord as RawValueRecord}, layout::CoverageTable, - variations::ivs_builder::VariationIndexRemapping, + variations::ivs_builder::VariationStoreBuilder, }, types::GlyphId, }; -use crate::common::GlyphSet; +use crate::{ + common::GlyphSet, + compile::valuerecordext::{Anchor, ValueRecord}, +}; -use super::{Builder, ClassDefBuilder2, VariationIndexContainingLookup}; +use super::{Builder, ClassDefBuilder2}; #[derive(Clone, Debug, Default)] pub struct SinglePosBuilder { @@ -35,19 +38,11 @@ impl SinglePosBuilder { } } -impl VariationIndexContainingLookup for SinglePosBuilder { - fn remap_variation_indices(&mut self, key_map: &VariationIndexRemapping) { - self.items - .values_mut() - .for_each(|x| x.remap_variation_indices(key_map)) - } -} - impl Builder for SinglePosBuilder { type Output = Vec; - fn build(self) -> Self::Output { - fn build_subtable(items: BTreeMap) -> write_gpos::SinglePos { + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output { + fn build_subtable(items: BTreeMap) -> write_gpos::SinglePos { let first = *items.values().next().unwrap(); let use_format_1 = first.format().is_empty() || items.values().all(|val| val == &first); let coverage: CoverageTable = items.keys().copied().collect(); @@ -58,21 +53,26 @@ impl Builder for SinglePosBuilder { } } const NEW_SUBTABLE_COST: usize = 10; + let items = self + .items + .into_iter() + .map(|(glyph, anchor)| (glyph, anchor.build(var_store))) + .collect::>(); // list of sets of glyph ids which will end up in their own subtables let mut subtables = Vec::new(); - let mut group_by_record: HashMap<&ValueRecord, BTreeMap> = + let mut group_by_record: HashMap<&RawValueRecord, BTreeMap> = Default::default(); // first group by specific record; glyphs that share a record can use // the more efficient format-1 subtable type - for (gid, value) in &self.items { + for (gid, value) in &items { group_by_record .entry(value) .or_default() .insert(*gid, value); } - let mut group_by_format: HashMap> = + let mut group_by_format: HashMap> = Default::default(); for (value, glyphs) in group_by_record { // if this saves us size, use format 1 @@ -144,23 +144,6 @@ impl Default for ClassPairPosSubtable { #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] struct ClassPairPosBuilder(BTreeMap<(ValueFormat, ValueFormat), Vec>); -impl VariationIndexContainingLookup for PairPosBuilder { - fn remap_variation_indices(&mut self, key_map: &VariationIndexRemapping) { - self.pairs - .0 - .values_mut() - .flat_map(|x| x.values_mut()) - .chain(self.classes.0.values_mut().flat_map(|x| { - x.iter_mut() - .flat_map(|x| x.items.values_mut().flat_map(|x| x.values_mut())) - })) - .for_each(|v| { - v.0.remap_variation_indices(key_map); - v.1.remap_variation_indices(key_map); - }); - } -} - impl ClassPairPosBuilder { fn insert( &mut self, @@ -249,9 +232,9 @@ impl PairPosBuilder { impl Builder for PairPosBuilder { type Output = Vec; - fn build(self) -> Self::Output { - let mut out = self.pairs.build(); - out.extend(self.classes.build()); + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output { + let mut out = self.pairs.build(var_store); + out.extend(self.classes.build(var_store)); out } } @@ -259,7 +242,7 @@ impl Builder for PairPosBuilder { impl Builder for GlyphPairPosBuilder { type Output = Vec; - fn build(self) -> Self::Output { + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output { let mut split_by_format = BTreeMap::<_, BTreeMap<_, Vec<_>>>::default(); for (g1, map) in self.0 { for (g2, (v1, v2)) in map { @@ -268,7 +251,11 @@ impl Builder for GlyphPairPosBuilder { .or_default() .entry(g1) .or_default() - .push(write_gpos::PairValueRecord::new(g2, v1, v2)); + .push(write_gpos::PairValueRecord::new( + g2, + v1.build(var_store), + v2.build(var_store), + )); } } @@ -286,18 +273,20 @@ impl Builder for GlyphPairPosBuilder { impl Builder for ClassPairPosBuilder { type Output = Vec; - fn build(self) -> Self::Output { - self.0 - .into_values() - .flat_map(|subs| subs.into_iter().map(Builder::build)) - .collect() + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output { + // not an iterator because we have funky lifetime issues + let mut result = Vec::new(); + for sub in self.0.into_values().flat_map(|subs| subs.into_iter()) { + result.push(sub.build(var_store)); + } + result } } impl Builder for ClassPairPosSubtable { type Output = write_gpos::PairPos; - fn build(self) -> Self::Output { + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output { assert!(!self.items.is_empty(), "filter before here"); // we have a set of classes/values with a single valueformat @@ -309,8 +298,8 @@ impl Builder for ClassPairPosSubtable { .and_then(|val| val.values().next()) .map(|(v1, v2)| { write_gpos::Class2Record::new( - ValueRecord::new().with_explicit_value_format(v1.format()), - ValueRecord::new().with_explicit_value_format(v2.format()), + RawValueRecord::new().with_explicit_value_format(v1.format()), + RawValueRecord::new().with_explicit_value_format(v2.format()), ) }) .unwrap(); @@ -326,7 +315,8 @@ impl Builder for ClassPairPosSubtable { let mut records = vec![empty_record.clone(); class2map.len() + 1]; for (class, (v1, v2)) in stuff { let idx = class2map.get(&class).unwrap(); - records[*idx as usize] = write_gpos::Class2Record::new(v1.clone(), v2.clone()); + records[*idx as usize] = + write_gpos::Class2Record::new(v1.build(var_store), v2.build(var_store)); } out[*idx as usize] = write_gpos::Class1Record::new(records); } @@ -336,42 +326,32 @@ impl Builder for ClassPairPosSubtable { #[derive(Clone, Debug, Default)] pub struct CursivePosBuilder { - items: BTreeMap, -} - -impl VariationIndexContainingLookup for CursivePosBuilder { - fn remap_variation_indices(&mut self, key_map: &VariationIndexRemapping) { - self.items - .values_mut() - .flat_map(|record| { - record - .entry_anchor - .as_mut() - .into_iter() - .chain(record.exit_anchor.as_mut()) - }) - .for_each(|anchor| anchor.remap_variation_indices(key_map)) - } + // (entry, exit) + items: BTreeMap, Option)>, } impl CursivePosBuilder { - pub fn insert( - &mut self, - glyph: GlyphId, - entry: Option, - exit: Option, - ) { - let record = write_gpos::EntryExitRecord::new(entry, exit); - self.items.insert(glyph, record); + pub fn insert(&mut self, glyph: GlyphId, entry: Option, exit: Option) { + self.items.insert(glyph, (entry, exit)); } } impl Builder for CursivePosBuilder { type Output = Vec; - fn build(self) -> Self::Output { + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output { let coverage = self.items.keys().copied().collect(); - let records = self.items.into_values().collect(); + let records = self + .items + .into_values() + .map(|(entry, exit)| { + write_gpos::EntryExitRecord::new( + entry.map(|x| x.build(var_store)), + exit.map(|x| x.build(var_store)), + ) + }) + .collect(); + //let record = write_gpos::EntryExitRecord::new(entry, exit); vec![write_gpos::CursivePosFormat1::new(coverage, records)] } } @@ -379,19 +359,12 @@ impl Builder for CursivePosBuilder { // shared between several tables #[derive(Clone, Debug, Default)] struct MarkList { - glyphs: BTreeMap, + // (class id, anchor) + glyphs: BTreeMap, // map class names to their idx for this table classes: HashMap, } -impl VariationIndexContainingLookup for MarkList { - fn remap_variation_indices(&mut self, key_map: &VariationIndexRemapping) { - self.glyphs - .values_mut() - .for_each(|mark| mark.mark_anchor.remap_variation_indices(key_map)) - } -} - impl MarkList { /// If this glyph is already part of another class, return the previous class name /// @@ -400,19 +373,20 @@ impl MarkList { &mut self, glyph: GlyphId, class: SmolStr, - anchor: AnchorTable, + anchor: Anchor, ) -> Result { let next_id = self.classes.len().try_into().unwrap(); let id = *self.classes.entry(class).or_insert(next_id); if let Some(prev) = self .glyphs - .insert(glyph, MarkRecord::new(id, anchor)) - .filter(|prev| prev.mark_class != id) + //.insert(glyph, MarkRecord::new(id, anchor)) + .insert(glyph, (id, anchor)) + .filter(|prev| prev.0 != id) { let class = self .classes .iter() - .find_map(|(name, idx)| (*idx == prev.mark_class).then(|| name.clone())) + .find_map(|(name, idx)| (*idx == prev.0).then(|| name.clone())) .unwrap(); return Err(PreviouslyAssignedClass { @@ -438,9 +412,14 @@ impl MarkList { impl Builder for MarkList { type Output = (CoverageTable, write_gpos::MarkArray); - fn build(self) -> Self::Output { + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output { let coverage = self.glyphs().collect(); - let array = write_gpos::MarkArray::new(self.glyphs.into_values().collect()); + let array = write_gpos::MarkArray::new( + self.glyphs + .into_values() + .map(|(class, anchor)| MarkRecord::new(class, anchor.build(var_store))) + .collect(), + ); (coverage, array) } } @@ -449,17 +428,7 @@ impl Builder for MarkList { #[derive(Clone, Debug, Default)] pub struct MarkToBaseBuilder { marks: MarkList, - bases: BTreeMap>, -} - -impl VariationIndexContainingLookup for MarkToBaseBuilder { - fn remap_variation_indices(&mut self, key_map: &VariationIndexRemapping) { - self.marks.remap_variation_indices(key_map); - self.bases - .values_mut() - .flat_map(|vals| vals.iter_mut().map(|(_, anchor)| anchor)) - .for_each(|anchor| anchor.remap_variation_indices(key_map)) - } + bases: BTreeMap>, } /// An error indicating a given glyph has been assigned to multiple mark classes @@ -480,13 +449,13 @@ impl MarkToBaseBuilder { &mut self, glyph: GlyphId, class: SmolStr, - anchor: AnchorTable, + anchor: Anchor, ) -> Result { self.marks.insert(glyph, class, anchor) } /// Insert a new base glyph. - pub fn insert_base(&mut self, glyph: GlyphId, class: &SmolStr, anchor: AnchorTable) { + pub fn insert_base(&mut self, glyph: GlyphId, class: &SmolStr, anchor: Anchor) { let class = self.marks.get_class(class); self.bases.entry(glyph).or_default().push((class, anchor)) } @@ -505,18 +474,18 @@ impl MarkToBaseBuilder { impl Builder for MarkToBaseBuilder { type Output = Vec; - fn build(self) -> Self::Output { + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output { let MarkToBaseBuilder { marks, bases } = self; let n_classes = marks.classes.len(); - let (mark_coverage, mark_array) = marks.build(); + let (mark_coverage, mark_array) = marks.build(var_store); let base_coverage = bases.keys().copied().collect(); let base_records = bases .into_values() .map(|anchors| { let mut anchor_offsets = vec![None; n_classes]; for (class, anchor) in anchors { - anchor_offsets[class as usize] = Some(anchor); + anchor_offsets[class as usize] = Some(anchor.build(var_store)); } write_gpos::BaseRecord::new(anchor_offsets) }) @@ -534,7 +503,7 @@ impl Builder for MarkToBaseBuilder { #[derive(Clone, Debug, Default)] pub struct MarkToLigBuilder { marks: MarkList, - ligatures: BTreeMap>>, + ligatures: BTreeMap>>, } impl MarkToLigBuilder { @@ -542,12 +511,12 @@ impl MarkToLigBuilder { &mut self, glyph: GlyphId, class: SmolStr, - anchor: AnchorTable, + anchor: Anchor, ) -> Result { self.marks.insert(glyph, class, anchor) } - pub fn add_lig(&mut self, glyph: GlyphId, components: Vec>) { + pub fn add_lig(&mut self, glyph: GlyphId, components: Vec>) { self.ligatures.insert(glyph, components); } @@ -560,20 +529,10 @@ impl MarkToLigBuilder { } } -impl VariationIndexContainingLookup for MarkToLigBuilder { - fn remap_variation_indices(&mut self, key_map: &VariationIndexRemapping) { - self.marks.remap_variation_indices(key_map); - self.ligatures - .values_mut() - .flat_map(|vals| vals.iter_mut().flat_map(|tree| tree.values_mut())) - .for_each(|anchor| anchor.remap_variation_indices(key_map)) - } -} - impl Builder for MarkToLigBuilder { type Output = Vec; - fn build(self) -> Self::Output { + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output { let MarkToLigBuilder { marks, ligatures } = self; let n_classes = marks.classes.len(); @@ -591,7 +550,7 @@ impl Builder for MarkToLigBuilder { let mut anchor_offsets = vec![None; n_classes]; for (class, anchor) in anchors { let class_idx = marks.get_class(&class); - anchor_offsets[class_idx as usize] = Some(anchor); + anchor_offsets[class_idx as usize] = Some(anchor.build(var_store)); } write_gpos::ComponentRecord::new(anchor_offsets) }) @@ -600,7 +559,7 @@ impl Builder for MarkToLigBuilder { }) .collect(); let ligature_array = write_gpos::LigatureArray::new(ligature_array); - let (mark_coverage, mark_array) = marks.build(); + let (mark_coverage, mark_array) = marks.build(var_store); vec![write_gpos::MarkLigPosFormat1::new( mark_coverage, ligature_coverage, @@ -614,7 +573,7 @@ impl Builder for MarkToLigBuilder { #[derive(Clone, Debug, Default)] pub struct MarkToMarkBuilder { attaching_marks: MarkList, - base_marks: BTreeMap>, + base_marks: BTreeMap>, } impl MarkToMarkBuilder { @@ -626,13 +585,13 @@ impl MarkToMarkBuilder { &mut self, glyph: GlyphId, class: SmolStr, - anchor: AnchorTable, + anchor: Anchor, ) -> Result { self.attaching_marks.insert(glyph, class, anchor) } /// Insert a new mark2 (base) glyph - pub fn insert_mark2(&mut self, glyph: GlyphId, class: &SmolStr, anchor: AnchorTable) { + pub fn insert_mark2(&mut self, glyph: GlyphId, class: &SmolStr, anchor: Anchor) { let id = self.attaching_marks.get_class(class); self.base_marks.entry(glyph).or_default().push((id, anchor)) } @@ -648,34 +607,24 @@ impl MarkToMarkBuilder { } } -impl VariationIndexContainingLookup for MarkToMarkBuilder { - fn remap_variation_indices(&mut self, key_map: &VariationIndexRemapping) { - self.attaching_marks.remap_variation_indices(key_map); - self.base_marks - .values_mut() - .flat_map(|vals| vals.iter_mut().map(|(_, anchor)| anchor)) - .for_each(|anchor| anchor.remap_variation_indices(key_map)) - } -} - impl Builder for MarkToMarkBuilder { type Output = Vec; - fn build(self) -> Self::Output { + fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output { let MarkToMarkBuilder { attaching_marks, base_marks, } = self; let n_classes = attaching_marks.classes.len(); - let (mark_coverage, mark_array) = attaching_marks.build(); + let (mark_coverage, mark_array) = attaching_marks.build(var_store); let mark2_coverage = base_marks.keys().copied().collect(); let mark2_records = base_marks .into_values() .map(|anchors| { let mut anchor_offsets = vec![None; n_classes]; for (class, anchor) in anchors { - anchor_offsets[class as usize] = Some(anchor); + anchor_offsets[class as usize] = Some(anchor.build(var_store)); } write_gpos::Mark2Record::new(anchor_offsets) }) diff --git a/fea-rs/src/compile/lookups/gsub_builders.rs b/fea-rs/src/compile/lookups/gsub_builders.rs index 853b1b8d7..3fd839c53 100644 --- a/fea-rs/src/compile/lookups/gsub_builders.rs +++ b/fea-rs/src/compile/lookups/gsub_builders.rs @@ -3,7 +3,7 @@ use std::{collections::BTreeMap, convert::TryFrom}; use write_fonts::{ - tables::gsub as write_gsub, + tables::{gsub as write_gsub, variations::ivs_builder::VariationStoreBuilder}, types::{FixedSize, GlyphId}, }; @@ -59,7 +59,7 @@ impl SingleSubBuilder { impl Builder for SingleSubBuilder { type Output = Vec; - fn build(self) -> Self::Output { + fn build(self, _: &mut VariationStoreBuilder) -> Self::Output { const COST_OF_EXTRA_SUB1F1_SUBTABLE: usize = 2 + // extra offset 2 + 2 + 2 + // format1 table itself 2 + 2; // extra coverage table @@ -147,7 +147,7 @@ pub struct MultipleSubBuilder { impl Builder for MultipleSubBuilder { type Output = Vec; - fn build(self) -> Self::Output { + fn build(self, _: &mut VariationStoreBuilder) -> Self::Output { let coverage = self.items.keys().copied().collect(); let seq_tables = self .items @@ -193,7 +193,7 @@ impl AlternateSubBuilder { impl Builder for AlternateSubBuilder { type Output = Vec; - fn build(self) -> Self::Output { + fn build(self, _: &mut VariationStoreBuilder) -> Self::Output { let coverage = self.items.keys().copied().collect(); let seq_tables = self .items @@ -230,7 +230,7 @@ impl LigatureSubBuilder { impl Builder for LigatureSubBuilder { type Output = Vec; - fn build(self) -> Self::Output { + fn build(self, _: &mut VariationStoreBuilder) -> Self::Output { let coverage = self.items.keys().copied().collect(); let lig_sets = self .items diff --git a/fea-rs/src/compile/tables.rs b/fea-rs/src/compile/tables.rs index a9b5854db..29c955a84 100644 --- a/fea-rs/src/compile/tables.rs +++ b/fea-rs/src/compile/tables.rs @@ -57,16 +57,6 @@ pub(crate) struct VmtxBuilder { pub advances_y: Vec<(GlyphId, i16)>, } -impl Tables { - // convenience method to access the varstore, creating it if it doesn't exist - pub(crate) fn var_store(&mut self) -> &mut VariationStoreBuilder { - self.gdef - .get_or_insert_with(Default::default) - .var_store - .get_or_insert_with(Default::default) - } -} - // this is the value used in python fonttools when writing this table const DATE_2011_12_13_H11_M22_S33: LongDateTime = LongDateTime::new(1323780153); diff --git a/fea-rs/src/compile/valuerecordext.rs b/fea-rs/src/compile/valuerecordext.rs index 6c13e6649..56419f651 100644 --- a/fea-rs/src/compile/valuerecordext.rs +++ b/fea-rs/src/compile/valuerecordext.rs @@ -1,43 +1,87 @@ //! Extra helper methods on ValueRecord use write_fonts::tables::{ - gpos::{ValueFormat, ValueRecord}, - layout::DeviceOrVariationIndex, + gpos::{AnchorTable, ValueFormat}, + layout::{Device, DeviceOrVariationIndex, PendingVariationIndex}, + variations::{ivs_builder::VariationStoreBuilder, VariationRegion}, }; -pub(crate) trait ValueRecordExt { - fn clear_zeros(self) -> Self; - fn for_pair_pos(self, in_vert_feature: bool) -> Self; - fn is_all_zeros(&self) -> bool; - /// Set a device for the field indicated by the provided format - fn with_device>( - self, - val: Option, - field: ValueFormat, - ) -> Self; +/// A value record, possibly containing raw deltas or device tables +#[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub struct ValueRecord { + /// The x advance, plus a possible device table or set of deltas + pub x_advance: Option, + /// The y advance, plus a possible device table or set of deltas + pub y_advance: Option, + /// The x placement, plus a possible device table or set of deltas + pub x_placement: Option, + /// The y placement, plus a possible device table or set of deltas + pub y_placement: Option, } -impl ValueRecordExt for ValueRecord { - fn clear_zeros(mut self) -> Self { - if self.x_placement == Some(0) && self.x_placement_device.is_none() { - self.x_placement = None; - } - - if self.y_placement == Some(0) && self.y_placement_device.is_none() { - self.y_placement = None; - } +/// An Anchor table, possibly containing deltas or a devices +#[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub struct Anchor { + /// The x coordinate, plus a possible device table or set of deltas + pub x: Metric, + /// The y coordinate, plus a possible device table or set of deltas + pub y: Metric, + /// The countourpoint, in a format 2 anchor. + /// + /// This is a rarely used format. + pub contourpoint: Option, +} - if self.x_advance == Some(0) && self.x_advance_device.is_none() { - self.x_advance = None; - } +#[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub enum DeviceOrDeltas { + Device(Device), + Deltas(Vec<(VariationRegion, i16)>), + #[default] + None, +} - if self.y_advance == Some(0) && self.y_advance_device.is_none() { - self.y_advance = None; - } +/// A metric with optional device or variation information +#[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub struct Metric { + pub default: i16, + pub device_or_deltas: DeviceOrDeltas, +} +impl ValueRecord { + pub(crate) fn clear_zeros(mut self) -> Self { + self.x_advance = self.x_advance.filter(|m| !m.is_zero()); + self.y_advance = self.y_advance.filter(|m| !m.is_zero()); + self.x_placement = self.x_placement.filter(|m| !m.is_zero()); + self.y_placement = self.y_placement.filter(|m| !m.is_zero()); self } + pub(crate) fn format(&self) -> ValueFormat { + const EMPTY: ValueFormat = ValueFormat::empty(); + use ValueFormat as VF; + + let get_flags = |field: &Option, def_flag, dev_flag| { + let field = field.as_ref(); + let def_flag = if field.is_some() { def_flag } else { EMPTY }; + let dev_flag = field + .and_then(|fld| (!fld.device_or_deltas.is_none()).then_some(dev_flag)) + .unwrap_or(EMPTY); + (def_flag, dev_flag) + }; + + let (x_adv, x_adv_dev) = get_flags(&self.x_advance, VF::X_ADVANCE, VF::X_ADVANCE_DEVICE); + let (y_adv, y_adv_dev) = get_flags(&self.y_advance, VF::Y_ADVANCE, VF::Y_ADVANCE_DEVICE); + let (x_place, x_place_dev) = + get_flags(&self.x_placement, VF::X_PLACEMENT, VF::X_PLACEMENT_DEVICE); + let (y_place, y_place_dev) = + get_flags(&self.y_placement, VF::Y_PLACEMENT, VF::Y_PLACEMENT_DEVICE); + x_adv | y_adv | x_place | y_place | x_adv_dev | y_adv_dev | x_place_dev | y_place_dev + } + /// `true` if we are not null, but our set values are all 0 fn is_all_zeros(&self) -> bool { let device_mask = ValueFormat::X_PLACEMENT_DEVICE @@ -50,12 +94,14 @@ impl ValueRecordExt for ValueRecord { return false; } let all_values = [ - self.x_placement, - self.y_placement, - self.x_advance, - self.y_advance, + &self.x_placement, + &self.y_placement, + &self.x_advance, + &self.y_advance, ]; - all_values.iter().all(|v| v.unwrap_or_default() == 0) + all_values + .iter() + .all(|v| v.as_ref().map(|v| v.is_zero()).unwrap_or(true)) } // Modify this value record for the special requirements of pairpos lookups @@ -63,50 +109,105 @@ impl ValueRecordExt for ValueRecord { // In pair pos tables, if a value record is all zeros (but not null) then // we interpret it as a having a single zero advance in the x/y direction, // depending on context. - fn for_pair_pos(self, in_vert_feature: bool) -> Self { + pub(crate) fn for_pair_pos(self, in_vert_feature: bool) -> Self { if !self.is_all_zeros() { return self.clear_zeros(); } let mut out = self.clear_zeros(); if in_vert_feature { - out.y_advance = Some(0); + out.y_advance = Some(0.into()); } else { - out.x_advance = Some(0); + out.x_advance = Some(0.into()); } out } - fn with_device>( - mut self, - val: Option, - field: ValueFormat, - ) -> Self { - let val = val.map(Into::into); - match field { - ValueFormat::X_PLACEMENT_DEVICE => self.x_placement_device = val.into(), - ValueFormat::Y_PLACEMENT_DEVICE => self.y_placement_device = val.into(), - ValueFormat::X_ADVANCE_DEVICE => self.x_advance_device = val.into(), - ValueFormat::Y_ADVANCE_DEVICE => self.y_advance_device = val.into(), - other => panic!("how did '{other:?}' get in here?"), - }; - self + pub(crate) fn build( + self, + var_store: &mut VariationStoreBuilder, + ) -> write_fonts::tables::gpos::ValueRecord { + let mut result = write_fonts::tables::gpos::ValueRecord::new(); + result.x_advance = self.x_advance.as_ref().map(|val| val.default); + result.y_advance = self.y_advance.as_ref().map(|val| val.default); + result.x_placement = self.x_placement.as_ref().map(|val| val.default); + result.y_placement = self.y_placement.as_ref().map(|val| val.default); + result.x_advance_device = self + .x_advance + .and_then(|val| val.device_or_deltas.build(var_store)) + .into(); + result.y_advance_device = self + .y_advance + .and_then(|val| val.device_or_deltas.build(var_store)) + .into(); + result.x_placement_device = self + .x_placement + .and_then(|val| val.device_or_deltas.build(var_store)) + .into(); + result.y_placement_device = self + .y_placement + .and_then(|val| val.device_or_deltas.build(var_store)) + .into(); + + result + } +} + +impl Anchor { + pub(crate) fn build(self, var_store: &mut VariationStoreBuilder) -> AnchorTable { + let x = self.x.default; + let y = self.y.default; + let x_dev = self.x.device_or_deltas.build(var_store); + let y_dev = self.y.device_or_deltas.build(var_store); + if x_dev.is_some() || y_dev.is_some() { + AnchorTable::format_3(x, y, x_dev, y_dev) + } else if let Some(point) = self.contourpoint { + AnchorTable::format_2(x, y, point) + } else { + AnchorTable::format_1(x, y) + } + } +} + +impl Metric { + fn is_zero(&self) -> bool { + self.default == 0 && !self.has_device_or_deltas() + } + + pub fn has_device_or_deltas(&self) -> bool { + !self.device_or_deltas.is_none() + } +} + +impl DeviceOrDeltas { + fn is_none(&self) -> bool { + *self == DeviceOrDeltas::None + } + + fn build(self, var_store: &mut VariationStoreBuilder) -> Option { + match self { + DeviceOrDeltas::Device(dev) => Some(DeviceOrVariationIndex::Device(dev)), + DeviceOrDeltas::Deltas(deltas) => { + let temp_id = var_store.add_deltas(deltas); + Some(DeviceOrVariationIndex::PendingVariationIndex( + PendingVariationIndex::new(temp_id), + )) + } + DeviceOrDeltas::None => None, + } + } +} + +impl From for Metric { + fn from(src: i16) -> Metric { + Metric { + default: src, + device_or_deltas: DeviceOrDeltas::None, + } } } -#[cfg(test)] -mod tests { - use write_fonts::tables::layout::VariationIndex; - - use super::*; - - #[test] - fn leave_zero_if_device_exists() { - let record = ValueRecord::new().with_x_advance(0).clear_zeros(); - assert!(record.x_advance.is_none()); - let record = ValueRecord::new() - .with_x_advance(0) - .with_x_advance_device(VariationIndex::new(420, 0xbeef)) - .clear_zeros(); - assert_eq!(record.x_advance, Some(0)); +impl From> for DeviceOrDeltas { + fn from(src: Option) -> DeviceOrDeltas { + src.map(DeviceOrDeltas::Device).unwrap_or_default() } } From 8803e874008c8f94c1f4cc13b664d6d1a777fe91 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Thu, 23 Nov 2023 12:40:54 -0500 Subject: [PATCH 2/5] [fea-rs] Rename valuerecordext -> metrics --- fea-rs/src/compile.rs | 4 ++-- fea-rs/src/compile/compile_ctx.rs | 5 ++--- fea-rs/src/compile/lookups.rs | 4 ++-- fea-rs/src/compile/lookups/contextual.rs | 2 +- fea-rs/src/compile/lookups/gpos_builders.rs | 3 +-- fea-rs/src/compile/{valuerecordext.rs => metrics.rs} | 3 ++- 6 files changed, 10 insertions(+), 11 deletions(-) rename fea-rs/src/compile/{valuerecordext.rs => metrics.rs} (98%) diff --git a/fea-rs/src/compile.rs b/fea-rs/src/compile.rs index 49dfe1f68..1134f80cc 100644 --- a/fea-rs/src/compile.rs +++ b/fea-rs/src/compile.rs @@ -19,9 +19,9 @@ pub use lookups::{ FeatureKey, LookupId, MarkToBaseBuilder, MarkToMarkBuilder, PairPosBuilder, PreviouslyAssignedClass, }; +pub use metrics::{Anchor, ValueRecord}; pub use opts::Opts; pub use output::Compilation; -pub use valuerecordext::{Anchor, ValueRecord}; pub use variations::{AxisLocation, VariationInfo}; #[cfg(any(test, feature = "test", feature = "cli"))] @@ -35,12 +35,12 @@ mod features; mod glyph_range; mod language_system; mod lookups; +mod metrics; mod opts; mod output; mod tables; mod tags; mod validate; -mod valuerecordext; mod variations; /// Run the validation pass, returning any diagnostics. diff --git a/fea-rs/src/compile/compile_ctx.rs b/fea-rs/src/compile/compile_ctx.rs index f4289ce23..847fbed8f 100644 --- a/fea-rs/src/compile/compile_ctx.rs +++ b/fea-rs/src/compile/compile_ctx.rs @@ -43,11 +43,10 @@ use super::{ lookups::{ AllLookups, FilterSetId, LookupFlagInfo, LookupId, PreviouslyAssignedClass, SomeLookup, }, + metrics::{Anchor, DeviceOrDeltas, Metric, ValueRecord}, output::Compilation, tables::{ClassId, ScriptRecord, Tables}, - tags, - valuerecordext::{Anchor, DeviceOrDeltas, Metric, ValueRecord}, - VariationInfo, + tags, VariationInfo, }; /// Context that manages state for a compilation. diff --git a/fea-rs/src/compile/lookups.rs b/fea-rs/src/compile/lookups.rs index 1a92396dd..4e6c6d356 100644 --- a/fea-rs/src/compile/lookups.rs +++ b/fea-rs/src/compile/lookups.rs @@ -29,11 +29,11 @@ use write_fonts::{ use crate::{ common::{GlyphId, GlyphOrClass, GlyphSet}, - compile::{lookups::contextual::ChainOrNot, valuerecordext::ValueRecord}, + compile::{lookups::contextual::ChainOrNot, metrics::ValueRecord}, Kind, }; -use super::{features::AllFeatures, tables::ClassId, tags, valuerecordext::Anchor}; +use super::{features::AllFeatures, metrics::Anchor, tables::ClassId, tags}; use contextual::{ ContextualLookupBuilder, PosChainContextBuilder, PosContextBuilder, ReverseChainBuilder, diff --git a/fea-rs/src/compile/lookups/contextual.rs b/fea-rs/src/compile/lookups/contextual.rs index f4066ccd9..d8e8b7b0a 100644 --- a/fea-rs/src/compile/lookups/contextual.rs +++ b/fea-rs/src/compile/lookups/contextual.rs @@ -17,7 +17,7 @@ use write_fonts::{ FontWrite, }; -use crate::{common::GlyphOrClass, compile::valuerecordext::ValueRecord}; +use crate::{common::GlyphOrClass, compile::metrics::ValueRecord}; use super::{ Builder, ClassDefBuilder2, FilterSetId, LookupBuilder, LookupId, PositionLookup, diff --git a/fea-rs/src/compile/lookups/gpos_builders.rs b/fea-rs/src/compile/lookups/gpos_builders.rs index 34c84998b..814820f2d 100644 --- a/fea-rs/src/compile/lookups/gpos_builders.rs +++ b/fea-rs/src/compile/lookups/gpos_builders.rs @@ -14,7 +14,7 @@ use write_fonts::{ use crate::{ common::GlyphSet, - compile::valuerecordext::{Anchor, ValueRecord}, + compile::metrics::{Anchor, ValueRecord}, }; use super::{Builder, ClassDefBuilder2}; @@ -351,7 +351,6 @@ impl Builder for CursivePosBuilder { ) }) .collect(); - //let record = write_gpos::EntryExitRecord::new(entry, exit); vec![write_gpos::CursivePosFormat1::new(coverage, records)] } } diff --git a/fea-rs/src/compile/valuerecordext.rs b/fea-rs/src/compile/metrics.rs similarity index 98% rename from fea-rs/src/compile/valuerecordext.rs rename to fea-rs/src/compile/metrics.rs index 56419f651..d3bb2854b 100644 --- a/fea-rs/src/compile/valuerecordext.rs +++ b/fea-rs/src/compile/metrics.rs @@ -1,4 +1,4 @@ -//! Extra helper methods on ValueRecord +//! Variable-first metrics, ValueRecords & Anchors use write_fonts::tables::{ gpos::{AnchorTable, ValueFormat}, @@ -34,6 +34,7 @@ pub struct Anchor { pub contourpoint: Option, } +/// Either a `Device` table or a set of deltas #[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub enum DeviceOrDeltas { From 3d1de1a3e8a590f84f3418c24f51ee7a91588b3c Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Thu, 23 Nov 2023 13:30:15 -0500 Subject: [PATCH 3/5] [fea-rs] Add ValueRecord and Metric constructors This also tweaks the visibility of some fields. --- fea-rs/src/compile/metrics.rs | 144 ++++++++++++++++++++++++++++++++-- 1 file changed, 136 insertions(+), 8 deletions(-) diff --git a/fea-rs/src/compile/metrics.rs b/fea-rs/src/compile/metrics.rs index d3bb2854b..5f79c8874 100644 --- a/fea-rs/src/compile/metrics.rs +++ b/fea-rs/src/compile/metrics.rs @@ -11,13 +11,13 @@ use write_fonts::tables::{ #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub struct ValueRecord { /// The x advance, plus a possible device table or set of deltas - pub x_advance: Option, + pub(crate) x_advance: Option, /// The y advance, plus a possible device table or set of deltas - pub y_advance: Option, + pub(crate) y_advance: Option, /// The x placement, plus a possible device table or set of deltas - pub x_placement: Option, + pub(crate) x_placement: Option, /// The y placement, plus a possible device table or set of deltas - pub y_placement: Option, + pub(crate) y_placement: Option, } /// An Anchor table, possibly containing deltas or a devices @@ -25,18 +25,19 @@ pub struct ValueRecord { #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub struct Anchor { /// The x coordinate, plus a possible device table or set of deltas - pub x: Metric, + pub(crate) x: Metric, /// The y coordinate, plus a possible device table or set of deltas - pub y: Metric, + pub(crate) y: Metric, /// The countourpoint, in a format 2 anchor. /// /// This is a rarely used format. - pub contourpoint: Option, + pub(crate) contourpoint: Option, } /// Either a `Device` table or a set of deltas #[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +#[allow(missing_docs)] pub enum DeviceOrDeltas { Device(Device), Deltas(Vec<(VariationRegion, i16)>), @@ -47,12 +48,88 @@ pub enum DeviceOrDeltas { /// A metric with optional device or variation information #[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] -pub struct Metric { +pub(crate) struct Metric { + /// The value at the default location pub default: i16, + /// An optional device table or delta set pub device_or_deltas: DeviceOrDeltas, } impl ValueRecord { + /// Create a new all-zeros ValueRecord + pub fn new() -> Self { + Default::default() + } + + // these methods just match the existing builder methods on `ValueRecord` + /// Builder style method to set the default x_placement value + pub fn with_x_placement(mut self, val: i16) -> Self { + self.x_placement + .get_or_insert_with(Default::default) + .default = val; + self + } + + /// Builder style method to set the default y_placement value + pub fn with_y_placement(mut self, val: i16) -> Self { + self.y_placement + .get_or_insert_with(Default::default) + .default = val; + self + } + + /// Builder style method to set the default x_placement value + pub fn with_x_advance(mut self, val: i16) -> Self { + self.x_advance.get_or_insert_with(Default::default).default = val; + self + } + + /// Builder style method to set the default y_placement value + pub fn with_y_advance(mut self, val: i16) -> Self { + self.y_advance.get_or_insert_with(Default::default).default = val; + self + } + + /// Builder style method to set the device or deltas for x_placement + /// + /// The argument can be a `Device` table or a `Vec<(VariationRegion, i16)>` + pub fn with_x_placement_device(mut self, val: impl Into) -> Self { + self.x_placement + .get_or_insert_with(Default::default) + .device_or_deltas = val.into(); + self + } + + /// Builder style method to set the device or deltas for y_placement + /// + /// The argument can be a `Device` table or a `Vec<(VariationRegion, i16)>` + pub fn with_y_placement_device(mut self, val: impl Into) -> Self { + self.y_placement + .get_or_insert_with(Default::default) + .device_or_deltas = val.into(); + self + } + + /// Builder style method to set the device or deltas for x_advance + /// + /// The argument can be a `Device` table or a `Vec<(VariationRegion, i16)>` + pub fn with_x_advance_device(mut self, val: impl Into) -> Self { + self.x_advance + .get_or_insert_with(Default::default) + .device_or_deltas = val.into(); + self + } + + /// Builder style method to set the device or deltas for y_advance + /// + /// The argument can be a `Device` table or a `Vec<(VariationRegion, i16)>` + pub fn with_y_advance_device(mut self, val: impl Into) -> Self { + self.y_advance + .get_or_insert_with(Default::default) + .device_or_deltas = val.into(); + self + } + pub(crate) fn clear_zeros(mut self) -> Self { self.x_advance = self.x_advance.filter(|m| !m.is_zero()); self.y_advance = self.y_advance.filter(|m| !m.is_zero()); @@ -154,6 +231,40 @@ impl ValueRecord { } impl Anchor { + /// Create a new anchor. + pub fn new(x: i16, y: i16) -> Self { + Anchor { + x: x.into(), + y: y.into(), + contourpoint: None, + } + } + + /// Builder style method to set the device or deltas for the x value + /// + /// The argument can be a `Device` table or a `Vec<(VariationRegion, i16)>` + pub fn with_x_device(mut self, val: impl Into) -> Self { + self.x.device_or_deltas = val.into(); + self + } + + /// Builder style method to set the device or deltas for the y value + /// + /// The argument can be a `Device` table or a `Vec<(VariationRegion, i16)>` + pub fn with_y_device(mut self, val: impl Into) -> Self { + self.y.device_or_deltas = val.into(); + self + } + + /// Builder-style method to set the contourpoint. + /// + /// This is for the little-used format2 AnchorTable; it will be ignored + /// if any device or deltas have been set. + pub fn with_contourpoint(mut self, idx: u16) -> Self { + self.contourpoint = Some(idx); + self + } + pub(crate) fn build(self, var_store: &mut VariationStoreBuilder) -> AnchorTable { let x = self.x.default; let y = self.y.default; @@ -174,6 +285,7 @@ impl Metric { self.default == 0 && !self.has_device_or_deltas() } + /// `true` if this metric has either a device table or deltas pub fn has_device_or_deltas(&self) -> bool { !self.device_or_deltas.is_none() } @@ -212,3 +324,19 @@ impl From> for DeviceOrDeltas { src.map(DeviceOrDeltas::Device).unwrap_or_default() } } + +impl From for DeviceOrDeltas { + fn from(value: Device) -> Self { + DeviceOrDeltas::Device(value) + } +} + +impl From> for DeviceOrDeltas { + fn from(src: Vec<(VariationRegion, i16)>) -> DeviceOrDeltas { + if src.is_empty() { + DeviceOrDeltas::None + } else { + DeviceOrDeltas::Deltas(src) + } + } +} From 9957f9ad3d9a9f213404e56f26b25b74e6927a39 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Thu, 23 Nov 2023 13:31:07 -0500 Subject: [PATCH 4/5] [fontc] Work with the new fea-rs API This does the minimum to get us compiling again; there's a larger patch to write that skips the intermediate step and just creates these types directly. --- fea-rs/src/compile/feature_writer.rs | 20 +------ fontbe/src/features.rs | 14 ++--- fontbe/src/kern.rs | 32 ++++------- fontbe/src/orchestration.rs | 79 ++++++---------------------- 4 files changed, 32 insertions(+), 113 deletions(-) diff --git a/fea-rs/src/compile/feature_writer.rs b/fea-rs/src/compile/feature_writer.rs index 8a2858e32..65f2288db 100644 --- a/fea-rs/src/compile/feature_writer.rs +++ b/fea-rs/src/compile/feature_writer.rs @@ -2,13 +2,7 @@ use std::collections::{BTreeMap, HashMap}; -use write_fonts::{ - tables::{ - layout::{LookupFlag, PendingVariationIndex}, - variations::VariationRegion, - }, - types::Tag, -}; +use write_fonts::{tables::layout::LookupFlag, types::Tag}; use crate::GlyphSet; @@ -92,18 +86,6 @@ impl<'a> FeatureBuilder<'a> { next_id } - /// Add a set of deltas to the `ItemVariationStore`. - /// - /// Returns a `PendingVariationIndex` which should be stored whereever a - /// `VariationIndex` table would be expected (it will be remapped during - /// compilation). - pub fn add_deltas>( - &mut self, - deltas: Vec<(VariationRegion, T)>, - ) -> PendingVariationIndex { - todo!("remove me!") - } - /// Add lookups to every default language system. /// /// Convenience method for recurring pattern. diff --git a/fontbe/src/features.rs b/fontbe/src/features.rs index 4b00518c9..f73ae3d27 100644 --- a/fontbe/src/features.rs +++ b/fontbe/src/features.rs @@ -199,12 +199,8 @@ impl<'a> FeatureWriter<'a> { } let mut ppos_subtables = PairPosBuilder::default(); - let mut var_indices = HashMap::new(); - for (idx, deltas) in self.kerning.deltas().enumerate() { - var_indices.insert(idx, builder.add_deltas(deltas.clone())); - } for kern in self.kerning.kerns() { - kern.insert_into(&var_indices, &mut ppos_subtables); + kern.insert_into(&mut ppos_subtables); } // now we have a builder for the pairpos subtables, so we can make @@ -252,7 +248,7 @@ impl<'a> FeatureWriter<'a> { .insert_mark( mark.gid, mark_base.class.clone(), - mark.create_anchor_table(builder)?, + mark.create_anchor_table(), ) .map_err(Error::PreviouslyAssignedClass)?; } @@ -261,7 +257,7 @@ impl<'a> FeatureWriter<'a> { mark_base_builder.insert_base( base.gid, &mark_base.class, - base.create_anchor_table(builder)?, + base.create_anchor_table(), ) } @@ -282,7 +278,7 @@ impl<'a> FeatureWriter<'a> { .insert_mark1( mark.gid, mark_mark.class.clone(), - mark.create_anchor_table(builder)?, + mark.create_anchor_table(), ) .map_err(Error::PreviouslyAssignedClass)?; } @@ -291,7 +287,7 @@ impl<'a> FeatureWriter<'a> { mark_mark_builder.insert_mark2( base.gid, &mark_mark.class, - base.create_anchor_table(builder)?, + base.create_anchor_table(), ); } mark_mark_lookups.push(builder.add_lookup( diff --git a/fontbe/src/kern.rs b/fontbe/src/kern.rs index 6b689bc37..c893e84cd 100644 --- a/fontbe/src/kern.rs +++ b/fontbe/src/kern.rs @@ -1,17 +1,17 @@ //! Generates a [Kerning] datastructure to be fed to fea-rs -use std::collections::{BTreeMap, HashMap, HashSet}; +use std::collections::{BTreeMap, HashSet}; -use fea_rs::GlyphSet; +use fea_rs::{compile::ValueRecord as ValueRecordBuilder, GlyphSet}; use fontdrasil::orchestration::{Access, Work}; -use write_fonts::{tables::gpos::ValueRecord, types::GlyphId}; +use fontir::{ir::KernParticipant, orchestration::WorkId as FeWorkId}; +use write_fonts::types::GlyphId; use crate::{ error::Error, features::resolve_variable_metric, orchestration::{AnyWorkId, BeWork, Context, Kerning, WorkId}, }; -use fontir::{ir::KernParticipant, orchestration::WorkId as FeWorkId}; #[derive(Debug)] struct KerningWork {} @@ -62,25 +62,15 @@ impl Work for KerningWork { let mut kerning = Kerning::default(); // now for each kerning entry, directly add a rule to the builder: - let mut delta_indices = HashMap::new(); for ((left, right), values) in &ir_kerning.kerns { let (default_value, deltas) = resolve_variable_metric(&static_metadata, values)?; - let delta_idx = if !deltas.is_empty() { - let mut current = delta_indices.get(&deltas).cloned(); - if current.is_none() { - let idx = kerning.add_deltas(deltas.clone()); - delta_indices.insert(deltas, idx); - current = Some(idx); - } - current - } else { - None - }; - let x_adv_record = ValueRecord::new().with_x_advance(default_value); + let x_adv_record = ValueRecordBuilder::new() + .with_x_advance(default_value) + .with_x_advance_device(deltas); match (left, right) { (KernParticipant::Glyph(left), KernParticipant::Glyph(right)) => { - kerning.add_pair(gid(left)?, x_adv_record.clone(), gid(right)?, delta_idx); + kerning.add_pair(gid(left)?, x_adv_record.clone(), gid(right)?); } (KernParticipant::Group(left), KernParticipant::Group(right)) => { let left = glyph_classes @@ -91,7 +81,7 @@ impl Work for KerningWork { .get(right) .ok_or_else(|| Error::MissingGlyphId(right.clone()))? .clone(); - kerning.add_class(left, x_adv_record.clone(), right, delta_idx); + kerning.add_class(left, x_adv_record.clone(), right); } // if groups are mixed with glyphs then we enumerate the group (KernParticipant::Glyph(left), KernParticipant::Group(right)) => { @@ -105,7 +95,7 @@ impl Work for KerningWork { .get(&right) .ok_or_else(|| Error::MissingGlyphId(right.clone()))?; for gid1 in right.iter() { - kerning.add_pair(gid0, x_adv_record.clone(), gid1, delta_idx); + kerning.add_pair(gid0, x_adv_record.clone(), gid1); } } (KernParticipant::Group(left), KernParticipant::Glyph(right)) => { @@ -114,7 +104,7 @@ impl Work for KerningWork { .ok_or_else(|| Error::MissingGlyphId(left.clone()))?; let gid1 = gid(right)?; for gid0 in left.iter() { - kerning.add_pair(gid0, x_adv_record.clone(), gid1, delta_idx); + kerning.add_pair(gid0, x_adv_record.clone(), gid1); } } } diff --git a/fontbe/src/orchestration.rs b/fontbe/src/orchestration.rs index 58672480a..a27beaab7 100644 --- a/fontbe/src/orchestration.rs +++ b/fontbe/src/orchestration.rs @@ -1,7 +1,6 @@ //! Helps coordinate the graph execution for BE use std::{ - collections::HashMap, fs::File, io::{self, BufReader, BufWriter, Read, Write}, path::{Path, PathBuf}, @@ -9,7 +8,7 @@ use std::{ }; use fea_rs::{ - compile::{FeatureBuilder, PairPosBuilder}, + compile::{PairPosBuilder, ValueRecord as ValueRecordBuilder}, GlyphMap, GlyphSet, }; use fontdrasil::{ @@ -38,13 +37,12 @@ use write_fonts::{ fvar::Fvar, gdef::Gdef, glyf::Glyph as RawGlyph, - gpos::{AnchorTable, Gpos, ValueRecord}, + gpos::Gpos, gsub::Gsub, gvar::{GlyphDelta, GlyphDeltas}, head::Head, hhea::Hhea, hvar::Hvar, - layout::PendingVariationIndex, loca::LocaFormat, maxp::Maxp, name::Name, @@ -275,56 +273,32 @@ pub enum Kern { Pair { glyph0: GlyphId, glyph1: GlyphId, - x_advance: ValueRecord, - delta_idx: Option, + x_advance: ValueRecordBuilder, }, Class { glyphs0: GlyphSet, glyphs1: GlyphSet, - x_advance: ValueRecord, - delta_idx: Option, + x_advance: ValueRecordBuilder, }, } impl Kern { - pub fn insert_into( - &self, - var_indices: &HashMap, - ppos_subtables: &mut PairPosBuilder, - ) { - let with_deltas = |x_advance: &ValueRecord, delta_idx: Option| { - if let Some(delta_idx) = delta_idx { - x_advance.clone().with_x_advance_device( - var_indices - .get(&delta_idx) - .unwrap_or_else(|| panic!("No entry for {delta_idx} in {var_indices:?}")) - .clone(), - ) - } else { - x_advance.clone() - } - }; - + pub fn insert_into(&self, ppos_subtables: &mut PairPosBuilder) { match self { Kern::Pair { glyph0, glyph1, x_advance, - delta_idx, - } => ppos_subtables.insert_pair( - *glyph0, - with_deltas(x_advance, *delta_idx), - *glyph1, - Default::default(), - ), + } => { + ppos_subtables.insert_pair(*glyph0, x_advance.clone(), *glyph1, Default::default()) + } Kern::Class { glyphs0, glyphs1, x_advance, - delta_idx, } => ppos_subtables.insert_classes( glyphs0.clone(), - with_deltas(x_advance, *delta_idx), + x_advance.clone(), glyphs1.clone(), Default::default(), ), @@ -381,24 +355,10 @@ impl MarkEntry { }) } - pub(crate) fn create_anchor_table( - &self, - builder: &mut FeatureBuilder, - ) -> Result { - let x_var_idx = - (!self.x_deltas.is_empty()).then(|| builder.add_deltas(self.x_deltas.clone())); - let y_var_idx = - (!self.y_deltas.is_empty()).then(|| builder.add_deltas(self.y_deltas.clone())); - if x_var_idx.is_some() || y_var_idx.is_some() { - Ok(AnchorTable::format_3( - self.x_default, - self.y_default, - x_var_idx.map(Into::into), - y_var_idx.map(Into::into), - )) - } else { - Ok(AnchorTable::format_1(self.x_default, self.y_default)) - } + pub(crate) fn create_anchor_table(&self) -> fea_rs::compile::Anchor { + fea_rs::compile::Anchor::new(self.x_default, self.y_default) + .with_x_device(self.x_deltas.clone()) + .with_y_device(self.y_deltas.clone()) } } @@ -500,33 +460,24 @@ impl Kerning { self.deltas.len() - 1 } - pub fn add_pair( - &mut self, - glyph0: GlyphId, - x_advance: ValueRecord, - glyph1: GlyphId, - delta_idx: Option, - ) { + pub fn add_pair(&mut self, glyph0: GlyphId, x_advance: ValueRecordBuilder, glyph1: GlyphId) { self.kerns.push(Kern::Pair { glyph0, glyph1, x_advance, - delta_idx, }) } pub fn add_class( &mut self, glyphs0: GlyphSet, - x_advance: ValueRecord, + x_advance: ValueRecordBuilder, glyphs1: GlyphSet, - delta_idx: Option, ) { self.kerns.push(Kern::Class { glyphs0, glyphs1, x_advance, - delta_idx, }) } } From 06638826c22362f12b3764ed11421735151fa43b Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Tue, 28 Nov 2023 12:35:57 -0500 Subject: [PATCH 5/5] [fea-rs] Review feedback --- fea-rs/src/compile/compile_ctx.rs | 15 ++++++--------- fea-rs/src/compile/lookups.rs | 9 ++++++++- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/fea-rs/src/compile/compile_ctx.rs b/fea-rs/src/compile/compile_ctx.rs index 847fbed8f..2701e105d 100644 --- a/fea-rs/src/compile/compile_ctx.rs +++ b/fea-rs/src/compile/compile_ctx.rs @@ -1094,15 +1094,13 @@ impl<'a> CompilationCtx<'a> { if let Some(adv) = record.advance() { let adv = self.resolve_metric(&adv); if self.vertical_feature.in_eligible_vertical_feature() { - return ValueRecord { - y_advance: Some(adv), - ..Default::default() - }; + return ValueRecord::new() + .with_y_advance(adv.default) + .with_y_advance_device(adv.device_or_deltas); } else { - return ValueRecord { - x_advance: Some(adv), - ..Default::default() - }; + return ValueRecord::new() + .with_x_advance(adv.default) + .with_x_advance_device(adv.device_or_deltas); } } @@ -1141,7 +1139,6 @@ impl<'a> CompilationCtx<'a> { result } - //fn resolve_metric(&mut self, metric: &typed::Metric) -> (i16, Option) { fn resolve_metric(&mut self, metric: &typed::Metric) -> Metric { match metric { typed::Metric::Scalar(value) => value.parse_signed().into(), diff --git a/fea-rs/src/compile/lookups.rs b/fea-rs/src/compile/lookups.rs index 4e6c6d356..3556544c2 100644 --- a/fea-rs/src/compile/lookups.rs +++ b/fea-rs/src/compile/lookups.rs @@ -53,7 +53,14 @@ pub(crate) use helpers::ClassDefBuilder2; // This exists because we use it to implement `LookupBuilder` pub(crate) trait Builder { type Output; - // the var_store is only used in GPOS, but we pass it everywhere + // the var_store is only used in GPOS, but we pass it everywhere. + // This is annoying but feels like the lesser of two evils. It's easy to + // ignore this argument where it isn't used, and this makes the logic + // in LookupBuilder simpler, since it is identical for GPOS/GSUB. + // + // It would be nice if this could then be Option<&mut T>, but that type is + // annoying to work with, as Option<&mut _> doesn't impl Copy, so you need + // to do a dance anytime you use it. fn build(self, var_store: &mut VariationStoreBuilder) -> Self::Output; }