Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[features] Share more code, do less work #1289

Merged
merged 2 commits into from
Feb 21, 2025
Merged

[features] Share more code, do less work #1289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
191e276
[features] Share more code, do less work
cmyr Feb 20, 2025
df05d78
[marks] Convert to GIDs earlier
cmyr Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 49 additions & 10 deletions fontbe/src/features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ use ordered_float::OrderedFloat;

use fea_rs::{
compile::{
error::CompilerError, Compilation, FeatureBuilder, FeatureProvider, PendingLookup,
VariationInfo,
error::CompilerError, Compilation, FeatureBuilder, FeatureProvider, NopFeatureProvider,
PendingLookup, VariationInfo,
},
parse::{FileSystemResolver, SourceLoadError, SourceResolver},
DiagnosticSet, GlyphMap, Opts, ParseTree,
Expand All @@ -35,15 +35,16 @@ use fontdrasil::{
types::Axis,
};
use write_fonts::{
tables::{layout::ClassDef, variations::VariationRegion},
types::{NameId, Tag},
tables::{gdef::GlyphClassDef, layout::ClassDef, variations::VariationRegion},
types::{GlyphId16, NameId, Tag},
OtRound,
};

use crate::{
error::Error,
orchestration::{
AnyWorkId, BeWork, Context, ExtraFeaTables, FeaAst, FeaRsKerns, FeaRsMarks, WorkId,
AnyWorkId, BeWork, Context, ExtraFeaTables, FeaFirstPassOutput, FeaRsKerns, FeaRsMarks,
WorkId,
},
};

Expand All @@ -59,7 +60,7 @@ const DFLT_SCRIPT: Tag = Tag::new(b"DFLT");
const DFLT_LANG: Tag = Tag::new(b"dflt");

#[derive(Debug)]
pub struct FeatureParsingWork {}
pub struct FeatureFirstPassWork {}

#[derive(Debug)]
pub struct FeatureCompilationWork {}
Expand Down Expand Up @@ -151,6 +152,31 @@ impl<'a> FeaVariationInfo<'a> {
}
}

/// Return GDEF classes.
///
/// If the source is one where we prefer classes declared explicitly in FEA,
/// and those exist, return those; otherwise return computed classes (from
/// public.openTypeCatgories or from Glyphs.xml, depending on the source type)
pub(crate) fn get_gdef_classes(
meta: &StaticMetadata,
ast: &FeaFirstPassOutput,
glyph_order: &GlyphOrder,
) -> HashMap<GlyphId16, GlyphClassDef> {
ast.gdef_classes
.as_ref()
.filter(|_| meta.gdef_categories.prefer_gdef_categories_in_fea)
.cloned()
.unwrap_or_else(|| {
meta.gdef_categories
.categories
.iter()
.filter_map(|(name, category)| {
glyph_order.glyph_id(name).map(|gid| (gid, *category))
})
.collect()
})
}

//NOTE: this is basically identical to the same method on FeaVariationInfo,
//except they have slightly different inputs?
pub(crate) fn resolve_variable_metric<'a>(
Expand Down Expand Up @@ -376,7 +402,7 @@ impl FeatureCompilationWork {
fn compile(
&self,
static_metadata: &StaticMetadata,
ast: &FeaAst,
ast: &FeaFirstPassOutput,
kerns: &FeaRsKerns,
marks: &FeaRsMarks,
) -> Result<Compilation, Error> {
Expand Down Expand Up @@ -426,7 +452,7 @@ fn write_debug_fea(context: &Context, is_error: bool, why: &str, fea_content: &s
};
}

impl Work<Context, AnyWorkId, Error> for FeatureParsingWork {
impl Work<Context, AnyWorkId, Error> for FeatureFirstPassWork {
fn id(&self) -> AnyWorkId {
WorkId::FeaturesAst.into()
}
Expand Down Expand Up @@ -458,13 +484,26 @@ impl Work<Context, AnyWorkId, Error> for FeatureParsingWork {
// after parsing we validate; we only need to do this once, and future
// work can trust the AST.
self.validate(&ast, &glyph_map, &static_metadata)?;
let var_info = FeaVariationInfo::new(&static_metadata);

context.fea_ast.set(ast.into());
let (compilation, _) = fea_rs::compile::compile::<_, NopFeatureProvider>(
&ast,
&glyph_map,
Some(&var_info),
None,
Opts::new().compile_gpos(false),
)
.map_err(|err| {
Error::FeaCompileError(fea_rs::compile::error::CompilerError::CompilationFail(err))
})?;
context
.fea_ast
.set(FeaFirstPassOutput::new(ast, compilation)?);
Ok(())
}
}

impl FeatureParsingWork {
impl FeatureFirstPassWork {
pub fn create() -> Box<BeWork> {
Box::new(Self {})
}
Expand Down
71 changes: 17 additions & 54 deletions fontbe/src/features/kern.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,9 @@ use std::{
};

use fea_rs::{
compile::{
FeatureKey, FeatureProvider, NopFeatureProvider, NopVariationInfo, PairPosBuilder,
ValueRecord as ValueRecordBuilder,
},
compile::{FeatureKey, FeatureProvider, PairPosBuilder, ValueRecord as ValueRecordBuilder},
typed::{AstNode, LanguageSystem},
GlyphSet, Opts, ParseTree,
GlyphSet, ParseTree,
};
use fontdrasil::{
coords::NormalizedLocation,
Expand All @@ -26,7 +23,7 @@ use icu_properties::props::BidiClass;
use log::debug;
use ordered_float::OrderedFloat;
use write_fonts::{
read::{tables::gsub::Gsub, FontRead, ReadError},
read::{tables::gsub::Gsub, ReadError},
tables::{gdef::GlyphClassDef, layout::LookupFlag},
types::{GlyphId16, Tag},
};
Expand All @@ -38,8 +35,8 @@ use crate::{
resolve_variable_metric,
},
orchestration::{
AllKerningPairs, AnyWorkId, BeWork, Context, FeaRsKerns, KernAdjustments, KernFragment,
KernPair, KernSide, WorkId,
AllKerningPairs, AnyWorkId, BeWork, Context, FeaFirstPassOutput, FeaRsKerns,
KernAdjustments, KernFragment, KernPair, KernSide, WorkId,
},
};

Expand Down Expand Up @@ -410,7 +407,7 @@ impl Work<Context, AnyWorkId, Error> for KerningGatherWork {

let lookups = finalize_kerning(
&pairs,
&ast.ast,
&ast,
&meta,
&glyph_order,
char_map,
Expand All @@ -426,51 +423,14 @@ impl Work<Context, AnyWorkId, Error> for KerningGatherWork {
// This includes much of the logic from the ufo2ft KernFeatureWriter
fn finalize_kerning(
pairs: &[&KernPair],
ast: &ParseTree,
ast: &FeaFirstPassOutput,
meta: &StaticMetadata,
glyph_order: &GlyphOrder,
char_map: HashMap<u32, GlyphId16>,
non_spacing_glyphs: HashSet<GlyphId16>,
) -> Result<FeaRsKerns, Error> {
let glyph_map = glyph_order.names().cloned().collect();
// ignore diagnostics, they'll get logged during actual GSUB compilation
let (compilation, _) = fea_rs::compile::compile::<NopVariationInfo, NopFeatureProvider>(
ast,
&glyph_map,
None,
None,
Opts::new().compile_gpos(false),
)
.map_err(|err| {
Error::FeaCompileError(fea_rs::compile::error::CompilerError::CompilationFail(err))
})?;

let gsub = compilation
.gsub
.as_ref()
.map(write_fonts::dump_table)
.transpose()
.expect(
"if this doesn't compile we will already panic when we try to add it to the context",
);
let gsub = gsub
.as_ref()
.map(|data| write_fonts::read::tables::gsub::Gsub::read(data.as_slice().into()))
.transpose()?;

let glyph_classes = compilation
.gdef_classes
.filter(|_| meta.gdef_categories.prefer_gdef_categories_in_fea)
.unwrap_or_else(|| {
meta.gdef_categories
.categories
.iter()
.filter_map(|(name, category)| {
glyph_order.glyph_id(name).map(|gid| (gid, *category))
})
.collect()
});
let known_scripts = guess_font_scripts(ast, &char_map);
let known_scripts = guess_font_scripts(&ast.ast, &char_map);
let glyph_classes = super::get_gdef_classes(meta, ast, glyph_order);

let mark_glyphs = glyph_order
.iter()
Expand All @@ -487,13 +447,13 @@ fn finalize_kerning(
})
.collect();

let split_ctx = KernSplitContext::new(&char_map, &known_scripts, gsub, mark_glyphs)?;
let split_ctx = KernSplitContext::new(&char_map, &known_scripts, ast.gsub(), mark_glyphs)?;

let lookups = split_ctx.make_lookups(pairs);
let (lookups_by_script, lookups) = split_lookups_by_script(lookups);

let kern_features = assign_lookups_to_scripts(lookups_by_script.clone(), ast, KERN);
let dist_features = assign_lookups_to_scripts(lookups_by_script, ast, DIST);
let kern_features = assign_lookups_to_scripts(lookups_by_script.clone(), &ast.ast, KERN);
let dist_features = assign_lookups_to_scripts(lookups_by_script, &ast.ast, DIST);
let features = kern_features.into_iter().chain(dist_features).collect();
debug_ordered_lookups(&features, &lookups);
Ok(FeaRsKerns { lookups, features })
Expand Down Expand Up @@ -1144,7 +1104,9 @@ fn merge_scripts(
mod tests {
use std::{path::Path, sync::Arc};

use fea_rs::compile::NopVariationInfo;
use fontir::ir::GdefCategories;
use write_fonts::read::FontRead;

use super::*;

Expand Down Expand Up @@ -1313,6 +1275,7 @@ mod tests {
}),
)
.unwrap();
let fea_first_pass = FeaFirstPassOutput::for_test(ast, &glyph_map).unwrap();
let fake_meta = StaticMetadata::new(
1000,
Default::default(),
Expand All @@ -1330,7 +1293,7 @@ mod tests {
.unwrap();
let kerns = finalize_kerning(
&pairs,
&ast,
&fea_first_pass,
&fake_meta,
&self.glyph_order,
self.charmap,
Expand All @@ -1339,7 +1302,7 @@ mod tests {
.unwrap();

let (comp, _) = fea_rs::compile::compile::<NopVariationInfo, _>(
&ast,
&fea_first_pass.ast,
&glyph_map,
None,
Some(&kerns),
Expand Down
Loading