From 5be6398dfe851c4e09b8033fd2ff3b5bbf08b446 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Thu, 1 Feb 2024 12:47:35 -0500 Subject: [PATCH] [fontc] Split feature parsing from compilation This is the first step towards doing fancier feature generation. By splitting out parsing, we can now access the AST from the code that handes generating marks/kerning, which is required for us to match the behaviour of the fontmake feature writers. --- fontbe/src/features.rs | 195 ++++++++++++++++++++++++------------ fontbe/src/orchestration.rs | 35 ++++++- fontbe/src/paths.rs | 1 + fontc/src/lib.rs | 36 ++++++- fontc/src/timing.rs | 2 + 5 files changed, 201 insertions(+), 68 deletions(-) diff --git a/fontbe/src/features.rs b/fontbe/src/features.rs index 877065c67..8094cd4eb 100644 --- a/fontbe/src/features.rs +++ b/fontbe/src/features.rs @@ -15,13 +15,13 @@ use log::{debug, error, trace, warn}; use ordered_float::OrderedFloat; use fea_rs::{ - compile::{Compilation, FeatureBuilder, FeatureProvider, VariationInfo}, - parse::{SourceLoadError, SourceResolver}, - Compiler, + compile::{error::CompilerError, Compilation, FeatureBuilder, FeatureProvider, VariationInfo}, + parse::{FileSystemResolver, SourceLoadError, SourceResolver}, + DiagnosticSet, ParseTree, }; use fontir::{ - ir::{FeaturesSource, StaticMetadata}, + ir::{FeaturesSource, GlyphOrder, StaticMetadata}, orchestration::{Flags, WorkId as FeWorkId}, }; @@ -36,11 +36,14 @@ use write_fonts::{ use crate::{ error::Error, - orchestration::{AnyWorkId, BeWork, Context, FeaRsKerns, FeaRsMarks, WorkId}, + orchestration::{AnyWorkId, BeWork, Context, FeaAst, FeaRsKerns, FeaRsMarks, WorkId}, }; #[derive(Debug)] -pub struct FeatureWork {} +pub struct FeatureParsingWork {} + +#[derive(Debug)] +pub struct FeatureCompilationWork {} // I did not want to make a struct // I did not want to clone the content @@ -54,6 +57,16 @@ struct InMemoryResolver { include_dir: Option, } +impl InMemoryResolver { + fn empty() -> Self { + InMemoryResolver { + content_path: Default::default(), + content: "".into(), + include_dir: None, + } + } +} + impl SourceResolver for InMemoryResolver { fn get_contents(&self, rel_path: &OsStr) -> Result, SourceLoadError> { if rel_path == &*self.content_path { @@ -342,61 +355,41 @@ impl<'a> VariationInfo for FeaVariationInfo<'a> { } } -impl FeatureWork { +impl FeatureCompilationWork { pub fn create() -> Box { - Box::new(FeatureWork {}) + Box::new(FeatureCompilationWork {}) } fn compile( &self, static_metadata: &StaticMetadata, - features: &FeaturesSource, + ast: &FeaAst, kerns: &FeaRsKerns, marks: &FeaRsMarks, ) -> Result { let var_info = FeaVariationInfo::new(static_metadata); let feature_writer = FeatureWriter::new(kerns, marks); - match features { - FeaturesSource::File { - fea_file, - include_dir, - } => { - let mut compiler = Compiler::new(OsString::from(fea_file), &marks.glyphmap); - if let Some(include_dir) = include_dir { - compiler = compiler.with_project_root(include_dir) - } - compiler - } - FeaturesSource::Memory { - fea_content, - include_dir, - } => { - let root = OsString::new(); - let mut compiler = - Compiler::new(root.clone(), &marks.glyphmap).with_resolver(InMemoryResolver { - content_path: root, - content: Arc::from(fea_content.as_str()), - include_dir: include_dir.clone(), - }); - if let Some(include_dir) = include_dir { - compiler = compiler.with_project_root(include_dir) - } - compiler - } - FeaturesSource::Empty => { - // There is no user feature file but we could still generate kerning, marks, etc - let root = OsString::new(); - Compiler::new(root.clone(), &marks.glyphmap).with_resolver(InMemoryResolver { - content_path: root, - content: Arc::from(""), - include_dir: None, - }) + // first do the validation pass + let diagnostics = fea_rs::compile::validate(&ast.ast, &marks.glyphmap, Some(&var_info)); + if diagnostics.has_errors() { + return Err(CompilerError::ValidationFail(diagnostics).into()); + } + log_fea_warnings("validation", &diagnostics); + // then we do the actual compilation pass: + match fea_rs::compile::compile( + &ast.ast, + &marks.glyphmap, + Some(&var_info), + Some(&feature_writer), + ) { + Ok((result, warnings)) => { + log_fea_warnings("compilation", &warnings); + Ok(result) } + Err(errors) => Err(Error::FeaCompileError(CompilerError::CompilationFail( + errors, + ))), } - .with_variable_info(&var_info) - .with_feature_writer(&feature_writer) - .compile() - .map_err(Error::FeaCompileError) } } @@ -420,9 +413,9 @@ fn write_debug_fea(context: &Context, is_error: bool, why: &str, fea_content: &s }; } -impl Work for FeatureWork { +impl Work for FeatureParsingWork { fn id(&self) -> AnyWorkId { - WorkId::Features.into() + WorkId::FeaturesAst.into() } fn read_access(&self) -> Access { @@ -430,6 +423,82 @@ impl Work for FeatureWork { .variant(FeWorkId::GlyphOrder) .variant(FeWorkId::StaticMetadata) .variant(FeWorkId::Features) + .build() + } + + fn exec(&self, context: &Context) -> Result<(), Error> { + let features = context.ir.features.get(); + let glyph_order = context.ir.glyph_order.get(); + let result = self.parse(&features, &glyph_order); + + if let FeaturesSource::Memory { fea_content, .. } = features.as_ref() { + write_debug_fea(context, result.is_err(), "compile failed", fea_content); + } + context.fea_ast.set(result?.into()); + Ok(()) + } +} + +impl FeatureParsingWork { + pub fn create() -> Box { + Box::new(Self {}) + } + + fn parse( + &self, + features: &FeaturesSource, + glyph_order: &GlyphOrder, + ) -> Result { + let (resolver, root_path) = get_resolver_and_root_path(features); + let glyph_map = glyph_order.iter().map(|g| g.clone().into_inner()).collect(); + let (tree, diagnostics) = fea_rs::parse::parse_root(root_path, Some(&glyph_map), resolver) + .map_err(CompilerError::SourceLoad)?; + if diagnostics.has_errors() { + return Err(CompilerError::ParseFail(diagnostics).into()); + } + log_fea_warnings("parsing", &diagnostics); + Ok(tree) + } +} + +fn get_resolver_and_root_path(features: &FeaturesSource) -> (Box, OsString) { + match features { + FeaturesSource::File { + fea_file, + include_dir, + } => { + let project_root = include_dir + .clone() + .or_else(|| fea_file.parent().map(PathBuf::from)) + .unwrap_or_default(); + ( + Box::new(FileSystemResolver::new(project_root)), + fea_file.to_owned().into_os_string(), + ) + } + FeaturesSource::Memory { + fea_content, + include_dir, + } => ( + Box::new(InMemoryResolver { + include_dir: include_dir.to_owned(), + content_path: OsString::new(), + content: fea_content.as_str().into(), + }), + OsString::new(), + ), + FeaturesSource::Empty => (Box::new(InMemoryResolver::empty()), Default::default()), + } +} + +impl Work for FeatureCompilationWork { + fn id(&self) -> AnyWorkId { + WorkId::Features.into() + } + + fn read_access(&self) -> Access { + AccessBuilder::new() + .variant(WorkId::FeaturesAst) .variant(WorkId::GatherBeKerning) .variant(WorkId::Marks) .build() @@ -445,22 +514,11 @@ impl Work for FeatureWork { fn exec(&self, context: &Context) -> Result<(), Error> { let static_metadata = context.ir.static_metadata.get(); - let features = context.ir.features.get(); + let ast = context.fea_ast.get(); let kerns = context.fea_rs_kerns.get(); let marks = context.fea_rs_marks.get(); - let result = self.compile( - &static_metadata, - features.as_ref(), - kerns.as_ref(), - marks.as_ref(), - ); - if result.is_err() || context.flags.contains(Flags::EMIT_DEBUG) { - if let FeaturesSource::Memory { fea_content, .. } = features.as_ref() { - write_debug_fea(context, result.is_err(), "compile failed", fea_content); - } - } - let result = result?; + let result = self.compile(&static_metadata, &ast, kerns.as_ref(), marks.as_ref())?; debug!( "Built features, gpos? {} gsub? {} gdef? {}", @@ -493,6 +551,17 @@ impl Work for FeatureWork { } } +fn log_fea_warnings(stage: &str, warnings: &DiagnosticSet) { + assert!(!warnings.has_errors(), "of course we checked this already"); + if !warnings.is_empty() { + log::debug!( + "FEA {stage} produced {} warnings:\n{}", + warnings.len(), + warnings.display() + ); + } +} + #[cfg(test)] mod tests { use std::collections::{HashMap, HashSet}; diff --git a/fontbe/src/orchestration.rs b/fontbe/src/orchestration.rs index 20c313a2d..bdd3e128f 100644 --- a/fontbe/src/orchestration.rs +++ b/fontbe/src/orchestration.rs @@ -12,7 +12,7 @@ use fea_rs::{ compile::{ MarkToBaseBuilder, MarkToMarkBuilder, PairPosBuilder, ValueRecord as ValueRecordBuilder, }, - GlyphMap, GlyphSet, + GlyphMap, GlyphSet, ParseTree, }; use fontdrasil::{ coords::NormalizedLocation, @@ -81,6 +81,7 @@ type KernBlock = usize; #[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] pub enum WorkId { Features, + FeaturesAst, Avar, Cmap, Font, @@ -114,6 +115,7 @@ impl Identifier for WorkId { fn discriminant(&self) -> IdentifierDiscriminant { match self { WorkId::Features => "BeFeatures", + WorkId::FeaturesAst => "BeFeaturesAst", WorkId::Avar => "BeAvar", WorkId::Cmap => "BeCmap", WorkId::Font => "BeFont", @@ -373,6 +375,20 @@ pub struct FeaRsKerns { pub lookups: Vec, } +/// The Ast for any user features. +/// +/// This is a newtype so that we can implement `Persistable` +#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)] +pub struct FeaAst { + pub ast: ParseTree, +} + +impl From for FeaAst { + fn from(ast: ParseTree) -> FeaAst { + FeaAst { ast } + } +} + impl FeaRsKerns { pub fn is_empty(&self) -> bool { self.lookups.is_empty() @@ -389,6 +405,16 @@ impl Persistable for FeaRsKerns { } } +impl Persistable for FeaAst { + fn read(from: &mut dyn Read) -> Self { + bincode::deserialize_from(from).unwrap() + } + + fn write(&self, to: &mut dyn Write) { + bincode::serialize_into(to, self).unwrap() + } +} + /// Kerning adjustments at various locations pub type KernAdjustments = BTreeMap>; @@ -608,6 +634,7 @@ pub struct Context { pub mvar: BeContextItem>, pub all_kerning_pairs: BeContextItem, pub kern_fragments: BeContextMap, + pub fea_ast: BeContextItem, pub fea_rs_kerns: BeContextItem, pub fea_rs_marks: BeContextItem, pub stat: BeContextItem>, @@ -647,6 +674,7 @@ impl Context { fea_rs_kerns: self.fea_rs_kerns.clone_with_acl(acl.clone()), fea_rs_marks: self.fea_rs_marks.clone_with_acl(acl.clone()), stat: self.stat.clone_with_acl(acl.clone()), + fea_ast: self.fea_ast.clone_with_acl(acl.clone()), font: self.font.clone_with_acl(acl), } } @@ -702,6 +730,11 @@ impl Context { acl.clone(), persistent_storage.clone(), ), + fea_ast: ContextItem::new( + WorkId::FeaturesAst.into(), + acl.clone(), + persistent_storage.clone(), + ), stat: ContextItem::new(WorkId::Stat.into(), acl.clone(), persistent_storage.clone()), font: ContextItem::new(WorkId::Font.into(), acl, persistent_storage), } diff --git a/fontbe/src/paths.rs b/fontbe/src/paths.rs index debea9744..9ea6d0730 100644 --- a/fontbe/src/paths.rs +++ b/fontbe/src/paths.rs @@ -65,6 +65,7 @@ impl Paths { pub fn target_file(&self, id: &WorkId) -> PathBuf { match id { WorkId::Features => self.build_dir.join("features.marker"), + WorkId::FeaturesAst => self.build_dir.join("features_ast.marker"), WorkId::GlyfFragment(name) => self.glyph_glyf_file(name.as_str()), WorkId::GvarFragment(name) => self.glyph_gvar_file(name.as_str()), WorkId::Avar => self.build_dir.join("avar.table"), diff --git a/fontc/src/lib.rs b/fontc/src/lib.rs index f183e0b6c..f7f7ebfc4 100644 --- a/fontc/src/lib.rs +++ b/fontc/src/lib.rs @@ -24,7 +24,7 @@ use std::{ use fontbe::{ avar::create_avar_work, cmap::create_cmap_work, - features::FeatureWork, + features::{FeatureCompilationWork, FeatureParsingWork}, font::create_font_work, fvar::create_fvar_work, glyphs::{create_glyf_loca_work, create_glyf_work}, @@ -128,8 +128,30 @@ fn add_feature_ir_job(workload: &mut Workload) -> Result<(), Error> { Ok(()) } -fn add_feature_be_job(workload: &mut Workload) -> Result<(), Error> { - let work = FeatureWork::create(); +fn add_feature_parse_be_job(workload: &mut Workload) -> Result<(), Error> { + let work = FeatureParsingWork::create(); + + // copied from below + if workload.change_detector.feature_be_change() { + if workload.change_detector.glyph_name_filter().is_some() { + warn!("Not processing BE Features because a glyph name filter is active"); + } + if workload.change_detector.should_skip_features() { + debug!("Not processing BE Features because FEA compilation is disabled"); + } + } + + workload.add( + work.into(), + workload.change_detector.feature_be_change() + && workload.change_detector.glyph_name_filter().is_none() + && !workload.change_detector.should_skip_features(), + ); + Ok(()) +} + +fn add_feature_comp_be_job(workload: &mut Workload) -> Result<(), Error> { + let work = FeatureCompilationWork::create(); // Features are extremely prone to not making sense when glyphs are filtered if workload.change_detector.feature_be_change() { if workload.change_detector.glyph_name_filter().is_some() { @@ -410,7 +432,9 @@ pub fn create_workload( add_glyph_order_ir_job(&mut workload)?; // BE: f(IR, maybe other BE work) => binary - add_feature_be_job(&mut workload)?; + + add_feature_parse_be_job(&mut workload)?; + add_feature_comp_be_job(&mut workload)?; add_glyph_be_jobs(&mut workload)?; add_glyf_loca_be_job(&mut workload)?; add_avar_be_job(&mut workload)?; @@ -698,6 +722,7 @@ mod tests { FeWorkIdentifier::KernInstance(NormalizedLocation::for_pos(&[("wght", 0.0)])).into(), FeWorkIdentifier::KernInstance(NormalizedLocation::for_pos(&[("wght", 1.0)])).into(), BeWorkIdentifier::Features.into(), + BeWorkIdentifier::FeaturesAst.into(), BeWorkIdentifier::Avar.into(), BeWorkIdentifier::Cmap.into(), BeWorkIdentifier::Font.into(), @@ -809,6 +834,7 @@ mod tests { AnyWorkId::Fe(FeWorkIdentifier::Glyph("bar".into())), FeWorkIdentifier::Anchor("bar".into()).into(), BeWorkIdentifier::Features.into(), + BeWorkIdentifier::FeaturesAst.into(), BeWorkIdentifier::Cmap.into(), BeWorkIdentifier::Font.into(), BeWorkIdentifier::Glyf.into(), @@ -850,6 +876,7 @@ mod tests { FeWorkIdentifier::KernInstance(NormalizedLocation::for_pos(&[("wght", 1.0)])) .into(), BeWorkIdentifier::Features.into(), + BeWorkIdentifier::FeaturesAst.into(), BeWorkIdentifier::Font.into(), BeWorkIdentifier::Gpos.into(), BeWorkIdentifier::Gsub.into(), @@ -963,6 +990,7 @@ mod tests { &[("wght", 1.0)] ))), BeWorkIdentifier::Features.into(), + BeWorkIdentifier::FeaturesAst.into(), BeWorkIdentifier::Font.into(), BeWorkIdentifier::Gpos.into(), BeWorkIdentifier::Gsub.into(), diff --git a/fontc/src/timing.rs b/fontc/src/timing.rs index 79f1e2f88..eb6d00c91 100644 --- a/fontc/src/timing.rs +++ b/fontc/src/timing.rs @@ -135,6 +135,7 @@ fn short_name(id: &AnyWorkId) -> &'static str { AnyWorkId::Be(BeWorkIdentifier::Avar) => "avar", AnyWorkId::Be(BeWorkIdentifier::Cmap) => "cmap", AnyWorkId::Be(BeWorkIdentifier::Features) => "fea", + AnyWorkId::Be(BeWorkIdentifier::FeaturesAst) => "fea.ast", AnyWorkId::Be(BeWorkIdentifier::Font) => "font", AnyWorkId::Be(BeWorkIdentifier::Fvar) => "fvar", AnyWorkId::Be(BeWorkIdentifier::Gdef) => "GDEF", @@ -179,6 +180,7 @@ fn color(id: &AnyWorkId) -> &'static str { AnyWorkId::Be(BeWorkIdentifier::Avar) => "gray", AnyWorkId::Be(BeWorkIdentifier::Cmap) => "gray", AnyWorkId::Be(BeWorkIdentifier::Features) => "gray", + AnyWorkId::Be(BeWorkIdentifier::FeaturesAst) => "gray", AnyWorkId::Be(BeWorkIdentifier::Font) => "gray", AnyWorkId::Be(BeWorkIdentifier::Fvar) => "gray", AnyWorkId::Be(BeWorkIdentifier::Gdef) => "gray",