From 85dfb9545c6266b37691d579c133ac9d368d7780 Mon Sep 17 00:00:00 2001 From: Rod S Date: Mon, 26 Jun 2023 14:19:00 -0700 Subject: [PATCH 1/5] Generate GPOS by producing variable value records for fea-rs --- Cargo.toml | 2 +- fontbe/src/features.rs | 463 +++++++++++++++++- fontc/src/args.rs | 2 +- fontir/src/coords.rs | 20 + fontir/src/ir.rs | 19 +- fontir/src/variations.rs | 39 +- glyphs2fontir/src/source.rs | 54 +- glyphs2fontir/src/toir.rs | 5 +- .../testdata/glyphs2/KernImplicitAxes.glyphs | 79 +++ 9 files changed, 630 insertions(+), 53 deletions(-) create mode 100644 resources/testdata/glyphs2/KernImplicitAxes.glyphs diff --git a/Cargo.toml b/Cargo.toml index bbefec6f..1638b7f0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,7 +11,7 @@ env_logger = "0.10.0" parking_lot = "0.12.1" -fea-rs = "0.13.0" +fea-rs = "0.13.1" font-types = { version = "0.3.3", features = ["serde"] } read-fonts = "0.10.0" write-fonts = "0.14.1" diff --git a/fontbe/src/features.rs b/fontbe/src/features.rs index f94b576c..168e4117 100644 --- a/fontbe/src/features.rs +++ b/fontbe/src/features.rs @@ -1,7 +1,8 @@ //! Feature binary compilation. use std::{ - collections::HashSet, + collections::{BTreeMap, HashMap, HashSet}, + error::Error as StdError, ffi::{OsStr, OsString}, fmt::Display, fs, @@ -9,17 +10,20 @@ use std::{ }; use fea_rs::{ - compile::Compilation, + compile::{Compilation, VariationInfo}, parse::{SourceLoadError, SourceResolver}, Compiler, GlyphMap, GlyphName as FeaRsGlyphName, }; +use font_types::{F2Dot14, Fixed, Tag}; use fontir::{ - ir::Features, + coords::{CoordConverter, UserCoord, UserLocation}, + ir::{Axis, Features, GlyphOrder, KernParticipant, Kerning, StaticMetadata}, orchestration::{Flags, WorkId as FeWorkId}, }; -use log::{debug, error, warn}; +use log::{debug, error, trace, warn}; use fontdrasil::orchestration::{Access, Work}; +use write_fonts::{tables::variations::RegionAxisCoordinates, OtRound}; use crate::{ error::Error, @@ -72,12 +76,153 @@ impl Display for NotSupportedError { } } +// TODO: ask Colin if we need to be threadsafe +struct FeaVariationInfo<'a> { + fea_rs_axes: HashMap, + axes: HashMap, + static_metadata: &'a StaticMetadata, +} + +impl<'a> FeaVariationInfo<'a> { + fn new(static_metadata: &'a StaticMetadata) -> FeaVariationInfo<'a> { + FeaVariationInfo { + fea_rs_axes: static_metadata + .variable_axes + .iter() + .enumerate() + .map(|(i, a)| { + ( + a.tag, + ( + &a.converter, + fea_rs::compile::AxisInfo { + index: i as u16, + default_value: a.default.into(), + min_value: a.min.into(), + max_value: a.max.into(), + }, + ), + ) + }) + .collect(), + axes: static_metadata + .variable_axes + .iter() + .map(|a| (a.tag, a)) + .collect(), + static_metadata, + } + } +} + +impl<'a> VariationInfo for FeaVariationInfo<'a> { + fn axis_info(&self, axis_tag: font_types::Tag) -> Option { + self.fea_rs_axes.get(&axis_tag).map(|a| a.1) + } + + fn normalize_coordinate( + &self, + axis_tag: font_types::Tag, + value: font_types::Fixed, + ) -> font_types::F2Dot14 { + let converter = &self + .axes + .get(&axis_tag) + .expect("Unsupported axis") + .converter; + let user_coord = UserCoord::new(value.to_f64() as f32); + F2Dot14::from_f32(user_coord.to_normalized(converter).to_f32()) + } + + fn resolve_variable_metric( + &self, + values: &HashMap, i16>, + ) -> Result< + ( + i16, + Vec<(write_fonts::tables::variations::VariationRegion, i16)>, + ), + Box<(dyn StdError + 'static)>, + > { + // WARNING: this will fail if the fea location isn't also a glyph location. In time we may wish to fix that. + let var_model = &self.static_metadata.variation_model; + + // Compute deltas using f64 as 1d point and delta, then ship them home as i16 + let point_seqs: HashMap<_, _> = values + .iter() + .map(|(pos, value)| { + let user = UserLocation::from_iter( + pos.iter() + .map(|(tag, value)| (*tag, UserCoord::new(value.to_f64() as f32))), + ); + (user.to_normalized(&self.axes), vec![*value as f64]) + }) + .collect(); + + // Only 1 value per region for our input + // TODO is that actually guaranteed? + let deltas: Vec<_> = var_model + .deltas(&point_seqs)? + .into_iter() + .map(|(region, values)| { + assert!(values.len() == 1, "{} values?!", values.len()); + (region, values[0]) + }) + .collect(); + + // Compute the default on the unrounded deltas + let default_value = deltas + .iter() + .filter_map(|(region, value)| { + let scaler = region.scalar_at(&var_model.default).into_inner(); + match scaler { + scaler if scaler == 0.0 => None, + scaler => Some(scaler * *value as f32), + } + }) + .sum::() + .ot_round(); + + // Produce the desired delta type + let deltas = deltas + .into_iter() + .map(|(region, value)| { + ( + write_fonts::tables::variations::VariationRegion { + region_axes: region + .iter() + .zip(self.static_metadata.axes.iter()) + .map(|((tag, tent), expected_axis)| { + assert_eq!(*tag, expected_axis.tag); + RegionAxisCoordinates { + start_coord: F2Dot14::from_f32(tent.min.to_f32()), + peak_coord: F2Dot14::from_f32(tent.peak.to_f32()), + end_coord: F2Dot14::from_f32(tent.max.to_f32()), + } + }) + .collect(), + }, + value.ot_round(), + ) + }) + .collect(); + + Ok((default_value, deltas)) + } +} + impl FeatureWork { pub fn create() -> Box { Box::new(FeatureWork {}) } - fn compile(&self, features: &Features, glyph_order: GlyphMap) -> Result { + fn compile( + &self, + static_metadata: &StaticMetadata, + features: &Features, + glyph_order: GlyphMap, + ) -> Result { + let var_info = FeaVariationInfo::new(static_metadata); let compiler = match features { Features::File { fea_file, @@ -89,21 +234,36 @@ impl FeatureWork { } compiler } - Features::Memory(fea_content) => { + Features::Memory { + fea_content, + include_dir, + } => { let root = OsString::new(); - Compiler::new(root.clone(), &glyph_order).with_resolver(InMemoryResolver { - content_path: root, - content: Arc::from(fea_content.as_str()), - }) + let mut compiler = + Compiler::new(root.clone(), &glyph_order).with_resolver(InMemoryResolver { + content_path: root, + content: Arc::from(fea_content.as_str()), + }); + if let Some(include_dir) = include_dir { + compiler = compiler.with_project_root(include_dir) + } + compiler } Features::Empty => panic!("compile isn't supposed to be called for Empty"), - }; + } + .with_variable_info(&var_info); compiler.compile().map_err(Error::FeaCompileError) } } fn write_debug_fea(context: &Context, is_error: bool, why: &str, fea_content: &str) { - let debug_file = context.debug_dir().join("glyphs.fea"); + if !context.flags.contains(Flags::EMIT_DEBUG) { + if is_error { + warn!("Debug fea not written for '{why}' because --emit_debug is off"); + } + return; + } + let debug_file = context.debug_dir().join("features.fea"); match fs::write(&debug_file, fea_content) { Ok(..) => { if is_error { @@ -116,6 +276,155 @@ fn write_debug_fea(context: &Context, is_error: bool, why: &str, fea_content: &s }; } +fn create_glyphmap(glyph_order: &GlyphOrder) -> GlyphMap { + if glyph_order.is_empty() { + warn!("Glyph order is empty; feature compile improbable"); + } + glyph_order + .iter() + .map(|n| Into::::into(n.as_str())) + .collect() +} + +fn push_identifier(fea: &mut String, identifier: &KernParticipant) { + match identifier { + KernParticipant::Glyph(name) => fea.push_str(name.as_str()), + KernParticipant::Group(name) => { + fea.push('@'); + fea.push_str(name.as_str()); + } + } +} + +/// Create a single variable fea describing the kerning for the entire variation space. +/// +/// No merge baby! - [context](https://github.com/fonttools/fonttools/issues/3168#issuecomment-1608787520) +/// +/// To match existing behavior, all kerns must have values for all locations for which any kerning is specified. +/// See for more. +/// Missing values are populated using the . +/// In future it is likely sparse kerning - blanks filled by interpolation - will be permitted. +/// +/// * See wrt sparse kerning. +/// * See wrt variable fea. +fn create_kerning_fea(axes: &HashMap, kerning: &Kerning) -> Result { + // Every kern must be defined at these locations. For human readability lets order things consistently. + let kerned_locations: HashSet<_> = kerning.kerns.values().flat_map(|v| v.keys()).collect(); + let mut kerned_locations: Vec<_> = kerned_locations.into_iter().collect(); + kerned_locations.sort(); + + if log::log_enabled!(log::Level::Trace) { + trace!( + "The following {} locations have kerning:", + kerned_locations.len() + ); + for pos in kerned_locations.iter() { + trace!(" {pos:?}"); + } + } + + // For any kern that is incompletely specified fill in the missing values using UFO kerning lookup + // Not 100% sure if this is correct for .glyphs but lets start there + // Generate variable format kerning per https://github.com/adobe-type-tools/afdko/pull/1350 + // Use design values per discussion on https://github.com/harfbuzz/boring-expansion-spec/issues/94 + let mut fea = String::new(); + fea.reserve(8192); // TODO is this a good value? + fea.push_str("\n\n# fontc generated kerning\n\n"); + + if kerning.is_empty() { + return Ok(fea); + } + + // TODO eliminate singleton groups, e.g. @public.kern1.Je-cy = [Je-cy]; + + // 1) Generate classes (http://adobe-type-tools.github.io/afdko/OpenTypeFeatureFileSpecification.html#2.g.ii) + // @classname = [glyph1 glyph2 glyph3]; + for (name, members) in kerning.groups.iter() { + fea.push('@'); + fea.push_str(name.as_str()); + fea.push_str(" = ["); + for member in members { + fea.push_str(member.as_str()); + fea.push(' '); + } + fea.remove(fea.len() - 1); + fea.push_str("];\n"); + } + fea.push_str("\n\n"); + + // 2) Generate pairpos (http://adobe-type-tools.github.io/afdko/OpenTypeFeatureFileSpecification.html#6.b) + // it's likely that many kerns use the same location string, might as well remember the string edition + + let mut pos_strings = HashMap::new(); + fea.push_str("feature kern {\n"); + for ((participant1, participant2), values) in kerning.kerns.iter() { + fea.push_str(" pos "); + push_identifier(&mut fea, participant1); + fea.push(' '); + push_identifier(&mut fea, participant2); + + // See https://github.com/adobe-type-tools/afdko/pull/1350#issuecomment-845219109 for syntax + // n for normalized, per https://github.com/harfbuzz/boring-expansion-spec/issues/94#issuecomment-1608007111 + fea.push_str(" ("); + for location in kerned_locations.iter() { + // TODO can we skip some values by dropping where value == interpolated value? + let advance_adjustment = values + .get(location) + .map(|f| f.into_inner()) + // TODO: kerning lookup + .unwrap_or_else(|| 0.0); + + // TODO: use the n suffix as soon as fea-rs supports it + let location = location.to_user(axes); + + let pos_str = pos_strings.entry(location.clone()).or_insert_with(|| { + location + .iter() + // TODO normalized: .map(|(tag, value)| format!("{tag}={}n", value.into_inner())) + .map(|(tag, value)| format!("{tag}={}", value.into_inner())) + .collect::>() + .join(",") + }); + + fea.push_str(pos_str); + fea.push(':'); + fea.push_str(&format!("{} ", advance_adjustment)); + } + fea.remove(fea.len() - 1); + fea.push_str(");\n"); + } + fea.push_str("} kern;\n"); + + Ok(fea) +} + +fn integrate_kerning(features: &Features, kern_fea: String) -> Result { + // TODO: insert at proper spot, there's a magic marker that might be present + match features { + Features::Empty => Ok(Features::Memory { + fea_content: kern_fea, + include_dir: None, + }), + Features::Memory { + fea_content, + include_dir, + } => Ok(Features::Memory { + fea_content: format!("{fea_content}{kern_fea}"), + include_dir: include_dir.clone(), + }), + Features::File { + fea_file, + include_dir, + } => { + let fea_content = fs::read_to_string(fea_file).map_err(Error::IoError)?; + Ok(Features::Memory { + fea_content: format!("{fea_content}{kern_fea}"), + include_dir: include_dir.clone(), + }) + } + } +} + impl Work for FeatureWork { fn id(&self) -> AnyWorkId { WorkId::Features.into() @@ -123,30 +432,41 @@ impl Work for FeatureWork { fn read_access(&self) -> Access { Access::Set(HashSet::from([ - FeWorkId::Features.into(), - FeWorkId::GlyphOrder.into(), + AnyWorkId::Fe(FeWorkId::GlyphOrder), + AnyWorkId::Fe(FeWorkId::StaticMetadata), + AnyWorkId::Fe(FeWorkId::Kerning), + AnyWorkId::Fe(FeWorkId::Features), ])) } fn also_completes(&self) -> Vec { - vec![WorkId::Gpos.into(), WorkId::Gsub.into()] + vec![WorkId::Gsub.into(), WorkId::Gpos.into()] } fn exec(&self, context: &Context) -> Result<(), Error> { - let features = context.ir.features.get(); - if !matches!(*features, Features::Empty) { - let glyph_order = &context.ir.glyph_order.get(); - if glyph_order.is_empty() { - warn!("Glyph order is empty; feature compile improbable"); + let static_metadata = context.ir.static_metadata.get(); + let glyph_order = context.ir.glyph_order.get(); + let kerning = context.ir.kerning.get(); + + let features = if !kerning.is_empty() { + let axes = static_metadata.axes.iter().map(|a| (a.tag, a)).collect(); + let kern_fea = create_kerning_fea(&axes, &kerning)?; + integrate_kerning(&context.ir.features.get(), kern_fea)? + } else { + (*context.ir.features.get()).clone() + }; + + if !matches!(features, Features::Empty) { + if log::log_enabled!(log::Level::Trace) { + if let Features::Memory { fea_content, .. } = &features { + trace!("in-memory fea content:\n{fea_content}"); + } } - let glyph_map = glyph_order - .iter() - .map(|n| Into::::into(n.as_str())) - .collect(); - let result = self.compile(&features, glyph_map); + let glyph_map = create_glyphmap(glyph_order.as_ref()); + let result = self.compile(&static_metadata, &features, glyph_map); if result.is_err() || context.flags.contains(Flags::EMIT_DEBUG) { - if let Features::Memory(fea_content) = &*features { + if let Features::Memory { fea_content, .. } = &features { write_debug_fea(context, result.is_err(), "compile failed", fea_content); } } @@ -166,6 +486,7 @@ impl Work for FeatureWork { } else { debug!("No fea file, dull compile"); } + // Enables the assumption that if the file exists features were compiled if context.flags.contains(Flags::EMIT_IR) { fs::write( @@ -180,3 +501,93 @@ impl Work for FeatureWork { Ok(()) } } + +#[cfg(test)] +mod tests { + use std::collections::{BTreeMap, HashMap, HashSet}; + + use fea_rs::compile::VariationInfo; + use font_types::{Fixed, Tag}; + use fontir::{ + coords::{CoordConverter, DesignCoord, NormalizedCoord, NormalizedLocation, UserCoord}, + ir::{Axis, StaticMetadata}, + }; + use write_fonts::tables::variations::RegionAxisCoordinates; + + use super::FeaVariationInfo; + + fn single_axis_norm_loc(tag: Tag, value: f32) -> NormalizedLocation { + let mut loc = NormalizedLocation::new(); + loc.insert(tag, NormalizedCoord::new(value)); + loc + } + + fn weight_variable_static_metadata(min: f32, def: f32, max: f32) -> StaticMetadata { + let min_wght_user = UserCoord::new(min); + let def_wght_user = UserCoord::new(def); + let max_wght_user = UserCoord::new(max); + let wght = Tag::new(b"wght"); + let min_wght = single_axis_norm_loc(wght, -1.0); + let def_wght = single_axis_norm_loc(wght, 0.0); + let max_wght = single_axis_norm_loc(wght, 1.0); + StaticMetadata::new( + 1024, + Default::default(), + vec![Axis { + name: "Weight".to_string(), + tag: Tag::new(b"wght"), + min: min_wght_user, + default: def_wght_user, + max: max_wght_user, + hidden: false, + converter: CoordConverter::new( + vec![ + // the design values don't really matter + (min_wght_user, DesignCoord::new(0.0)), + (def_wght_user, DesignCoord::new(1.0)), + (max_wght_user, DesignCoord::new(2.0)), + ], + 1, + ), + }], + Default::default(), + HashSet::from([min_wght, def_wght, max_wght]), + ) + .unwrap() + } + + fn is_default(region_coords: &RegionAxisCoordinates) -> bool { + region_coords.start_coord.to_f32() == 0.0 + && region_coords.peak_coord.to_f32() == 0.0 + && region_coords.end_coord.to_f32() == 0.0 + } + + #[test] + fn resolve_kern() { + let _ = env_logger::builder().is_test(true).try_init(); + + let wght = Tag::new(b"wght"); + let static_metadata = weight_variable_static_metadata(300.0, 400.0, 700.0); + let var_info = FeaVariationInfo::new(&static_metadata); + + let (default, regions) = var_info + .resolve_variable_metric(&HashMap::from([ + (BTreeMap::from([(wght, Fixed::from_f64(300.0))]), 10), + (BTreeMap::from([(wght, Fixed::from_f64(400.0))]), 15), + (BTreeMap::from([(wght, Fixed::from_f64(700.0))]), 20), + ])) + .unwrap(); + let mut region_values: Vec<_> = regions + .into_iter() + .map(|(r, v)| { + if r.region_axes.iter().all(is_default) { + v + } else { + v + default + } + }) + .collect(); + region_values.sort(); + assert_eq!((15, vec![10, 15, 20]), (default, region_values)); + } +} diff --git a/fontc/src/args.rs b/fontc/src/args.rs index ce4e36d7..706f9047 100644 --- a/fontc/src/args.rs +++ b/fontc/src/args.rs @@ -64,7 +64,7 @@ impl Args { glyph_name_filter: None, source, emit_ir: true, - emit_debug: false, + emit_debug: false, // they get destroyed by test cleanup build_dir: build_dir.to_path_buf(), prefer_simple_glyphs: Flags::default().contains(Flags::PREFER_SIMPLE_GLYPHS), flatten_components: Flags::default().contains(Flags::FLATTEN_COMPONENTS), diff --git a/fontir/src/coords.rs b/fontir/src/coords.rs index 77314337..dd6fb016 100644 --- a/fontir/src/coords.rs +++ b/fontir/src/coords.rs @@ -277,6 +277,26 @@ impl Location { } } +impl UserLocation { + pub fn to_normalized(&self, axes: &HashMap) -> NormalizedLocation { + Location::( + self.0 + .iter() + .map(|(tag, dc)| (*tag, dc.to_normalized(&axes.get(tag).unwrap().converter))) + .collect(), + ) + } + + pub fn to_design(&self, axes: &HashMap) -> DesignLocation { + Location::( + self.0 + .iter() + .map(|(tag, coord)| (*tag, coord.to_design(&axes.get(tag).unwrap().converter))) + .collect(), + ) + } +} + impl DesignLocation { pub fn to_normalized(&self, axes: &HashMap) -> NormalizedLocation { Location::( diff --git a/fontir/src/ir.rs b/fontir/src/ir.rs index c741b96a..5d0a8dc0 100644 --- a/fontir/src/ir.rs +++ b/fontir/src/ir.rs @@ -55,7 +55,8 @@ pub struct StaticMetadata { /// /// This copy includes all locations used in the entire font. That is, every /// location any glyph has an instance. Use of a location not in the global model - /// is an error. + /// is an error. This model enforces the no delta at the default location constraint + /// used in things like gvar. pub variation_model: VariationModel, axes_default: NormalizedLocation, @@ -185,6 +186,12 @@ pub struct Kerning { pub kerns: HashMap, } +impl Kerning { + pub fn is_empty(&self) -> bool { + self.kerns.is_empty() + } +} + /// A participant in kerning, one of the entries in a kerning pair. /// /// Concretely, a glyph or a group of glyphs. @@ -765,7 +772,10 @@ pub enum Features { fea_file: PathBuf, include_dir: Option, }, - Memory(String), + Memory { + fea_content: String, + include_dir: Option, + }, } impl Features { @@ -779,7 +789,10 @@ impl Features { } } pub fn from_string(fea_content: String) -> Features { - Features::Memory(fea_content) + Features::Memory { + fea_content, + include_dir: None, + } } } diff --git a/fontir/src/variations.rs b/fontir/src/variations.rs index 24d56a1e..52e06ee7 100644 --- a/fontir/src/variations.rs +++ b/fontir/src/variations.rs @@ -29,7 +29,7 @@ const ONE: OrderedFloat = OrderedFloat(1.0); /// See `class VariationModel` in #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] pub struct VariationModel { - default: NormalizedLocation, + pub default: NormalizedLocation, /// Non-point axes axes: Vec, @@ -155,6 +155,10 @@ impl VariationModel { P: Copy + Default + Sub, V: Copy + Mul + Sub, { + if point_seqs.is_empty() { + return Ok(Vec::new()); + } + let point_seqs: HashMap<_, _> = point_seqs .iter() .map(|(loc, seq)| { @@ -164,15 +168,15 @@ impl VariationModel { }) .collect(); - let Some(defaults) = point_seqs.get(&self.default) else { - return Err(DeltaError::DefaultUndefined); - }; for loc in point_seqs.keys() { if !self.locations.contains(loc) { return Err(DeltaError::UnknownLocation(loc.clone())); } } - if point_seqs.values().any(|pts| pts.len() != defaults.len()) { + + // we know point_seqs is non-empty + let point_seq_len = point_seqs.values().next().unwrap().len(); + if point_seqs.values().any(|pts| pts.len() != point_seq_len) { return Err(DeltaError::InconsistentNumbersOfPoints); } @@ -403,11 +407,9 @@ impl VariationRegion { /// The scalar multiplier for the provided location for this region /// - /// Returns None for no influence, Some(non-zero value) for influence. - /// /// In Python, supportScalar. We only implement the ot=True, extrapolate=False paths. /// . - fn scalar_at(&self, location: &NormalizedLocation) -> OrderedFloat { + pub fn scalar_at(&self, location: &NormalizedLocation) -> OrderedFloat { let scalar = self.axis_tents.iter().filter(|(_, ar)| ar.validate()).fold( ONE, |scalar, (tag, tent)| { @@ -475,7 +477,7 @@ impl VariationRegion { /// /// Visualize as a tent of influence, starting at min, peaking at peak, /// and dropping off to zero at max. -#[derive(Serialize, Deserialize, Default, Clone, PartialEq, Eq)] +#[derive(Serialize, Deserialize, Clone, PartialEq, Eq)] pub struct Tent { pub min: NormalizedCoord, pub peak: NormalizedCoord, @@ -493,6 +495,11 @@ impl Tent { Tent { min, peak, max } } + pub fn zeroes() -> Tent { + let zero = NormalizedCoord::new(0.0); + Tent::new(zero, zero, zero) + } + /// OT-specific validation of whether we could have any influence /// /// (0,0,0) IS valid, meaning apply my deltas at full scale always @@ -506,7 +513,6 @@ impl Tent { if min > peak || peak > max { return false; } - // In fonts the influence at zero must be zero so we cannot span zero if min < ZERO && max > ZERO { return false; } @@ -647,7 +653,7 @@ fn master_influence(axis_order: &[Tag], regions: &[VariationRegion]) -> Vec Axis { let (name, tag, min, default, max) = match tag { @@ -1164,14 +1170,7 @@ mod tests { fn region(spec: &[(&str, f32, f32, f32)]) -> VariationRegion { let mut region = VariationRegion::new(); for (tag, min, peak, max) in spec { - region.insert( - Tag::from_str(tag).unwrap(), - Tent::new( - NormalizedCoord::new(*min), - NormalizedCoord::new(*peak), - NormalizedCoord::new(*max), - ), - ); + region.insert(Tag::from_str(tag).unwrap(), (*min, *peak, *max).into()); } region } diff --git a/glyphs2fontir/src/source.rs b/glyphs2fontir/src/source.rs index aa96069c..8951544c 100644 --- a/glyphs2fontir/src/source.rs +++ b/glyphs2fontir/src/source.rs @@ -568,20 +568,31 @@ impl Work for KerningWork { } fn read_access(&self) -> Access { - Access::One(WorkId::GlyphOrder) + Access::Set(HashSet::from([WorkId::GlyphOrder, WorkId::StaticMetadata])) } fn exec(&self, context: &Context) -> Result<(), WorkError> { trace!("Generate IR for kerning"); + let static_metadata = context.static_metadata.get(); let arc_glyph_order = context.glyph_order.get(); let glyph_order = arc_glyph_order.as_ref(); let font_info = self.font_info.as_ref(); let font = &font_info.font; + let variable_axes: HashSet<_> = static_metadata + .variable_axes + .iter() + .map(|a| a.tag) + .collect(); let master_positions: HashMap<_, _> = font .masters .iter() .map(|m| (&m.id, font_info.locations.get(&m.axes_values).unwrap())) + .map(|(id, pos)| { + let mut pos = pos.clone(); + pos.retain(|tag, _| variable_axes.contains(tag)); + (id, pos) + }) .collect(); let mut kerning = Kerning::default(); @@ -1014,6 +1025,30 @@ mod tests { (source, context) } + fn build_kerning(glyphs_file: PathBuf) -> (impl Source, Context) { + let (source, context) = build_static_metadata(glyphs_file); + + // static metadata includes preliminary glyph order; just copy it to be the final one + context + .copy_for_work( + Access::One(WorkId::PreliminaryGlyphOrder), + Access::One(WorkId::GlyphOrder), + ) + .glyph_order + .set((*context.preliminary_glyph_order.get()).clone()); + + let task_context = context.copy_for_work( + Access::Set(HashSet::from([WorkId::StaticMetadata, WorkId::GlyphOrder])), + Access::one(WorkId::Kerning), + ); + source + .create_kerning_ir_work(&context.input) + .unwrap() + .exec(&task_context) + .unwrap(); + (source, context) + } + fn build_glyphs( source: &impl Source, context: &Context, @@ -1365,4 +1400,21 @@ mod tests { assert_eq!(first_contour.to_svg(), expected); } } + + // .glyphs v2 defaults to Weight, Width, Custom if no axes are specified + // Avoid ending up with kerning for locations like {XXXX: 0.00, wdth: 0.00, wght: 1.00} + // when XXXX and wdth are point axes that won't be in fvar. Oswald was hitting this. + #[test] + fn kern_positions_on_live_axes() { + let (_, context) = build_kerning(glyphs2_dir().join("KernImplicitAxes.glyphs")); + let kerning = context.kerning.get(); + assert!(!kerning.is_empty(), "{kerning:#?}"); + let bad_kerns: Vec<_> = kerning + .kerns + .values() + .flat_map(|v| v.keys()) + .filter(|pos| !pos.axis_tags().all(|tag| *tag == Tag::new(b"wght"))) + .collect(); + assert!(bad_kerns.is_empty(), "{bad_kerns:#?}"); + } } diff --git a/glyphs2fontir/src/toir.rs b/glyphs2fontir/src/toir.rs index 6d6c3fe6..1f00ac7a 100644 --- a/glyphs2fontir/src/toir.rs +++ b/glyphs2fontir/src/toir.rs @@ -118,7 +118,10 @@ pub(crate) fn to_ir_features(features: &[FeatureSnippet]) -> Result = features.iter().map(|f| f.as_str().to_string()).collect(); - Ok(ir::Features::Memory(fea_snippets.join("\n\n"))) + Ok(ir::Features::Memory { + fea_content: fea_snippets.join("\n\n"), + include_dir: None, + }) } fn design_location(axes: &[ir::Axis], axes_values: &[OrderedFloat]) -> DesignLocation { diff --git a/resources/testdata/glyphs2/KernImplicitAxes.glyphs b/resources/testdata/glyphs2/KernImplicitAxes.glyphs new file mode 100644 index 00000000..8dc46a29 --- /dev/null +++ b/resources/testdata/glyphs2/KernImplicitAxes.glyphs @@ -0,0 +1,79 @@ +{ +familyName = WghtVar; +fontMaster = ( +{ +alignmentZones = ( +"{737, 16}", +"{0, -16}", +"{-42, -16}" +); +ascender = 737; +capHeight = 702; +descender = -42; +id = m01; +weightValue = 400; +xHeight = 501; +}, +{ +ascender = 800; +capHeight = 700; +descender = -200; +id = "E09E0C54-128D-4FEA-B209-1B70BEFE300B"; +weight = Bold; +weightValue = 700; +xHeight = 500; +} +); +glyphs = ( +{ +glyphname = hyphen; +lastChange = "2023-06-05 23:23:03 +0000"; +layers = ( +{ +layerId = m01; +paths = ( +{ +closed = 1; +nodes = ( +"131 250 LINE", +"470 250 LINE", +"470 330 LINE", +"131 330 LINE" +); +} +); +width = 600; +}, +{ +layerId = "E09E0C54-128D-4FEA-B209-1B70BEFE300B"; +paths = ( +{ +closed = 1; +nodes = ( +"92 224 LINE", +"508 224 LINE", +"508 356 LINE", +"92 356 LINE" +); +} +); +width = 600; +} +); +unicode = 002D; +} +); +kerning = { +m01 = { +hyphen = { +hyphen = -150; +}; +}; +"E09E0C54-128D-4FEA-B209-1B70BEFE300B" = { +hyphen = { +hyphen = -50; +}; +}; +}; +unitsPerEm = 1000; +} From c4b4f3fdf88bc7ae1f3e0a1f811e1fd27df838fc Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Sun, 20 Aug 2023 14:06:08 +0100 Subject: [PATCH 2/5] bump fea-rs to 0.13.2 --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 1638b7f0..8ed59da4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,7 +11,7 @@ env_logger = "0.10.0" parking_lot = "0.12.1" -fea-rs = "0.13.1" +fea-rs = "0.13.2" font-types = { version = "0.3.3", features = ["serde"] } read-fonts = "0.10.0" write-fonts = "0.14.1" From 632db7a0a289330b426a18c3373c80c2f0610f11 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Sun, 20 Aug 2023 15:26:06 +0100 Subject: [PATCH 3/5] must prune default (0,0,0) region in resolve_variable_metric otherwise we end with with default kerning values being applied twice, see https://github.com/googlefonts/fontc/issues/402#issuecomment-1685286074 When we build glyph variations we also filter the default region deltas that VariationModel::deltas returns, we must do the same for variable FEA --- fontbe/src/features.rs | 66 ++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 35 deletions(-) diff --git a/fontbe/src/features.rs b/fontbe/src/features.rs index 35e3fc39..ce903008 100644 --- a/fontbe/src/features.rs +++ b/fontbe/src/features.rs @@ -186,24 +186,28 @@ impl<'a> VariationInfo for FeaVariationInfo<'a> { // Produce the desired delta type let deltas = deltas .into_iter() - .map(|(region, value)| { - ( - write_fonts::tables::variations::VariationRegion { - region_axes: region - .iter() - .zip(self.static_metadata.axes.iter()) - .map(|((tag, tent), expected_axis)| { - assert_eq!(*tag, expected_axis.tag); - RegionAxisCoordinates { - start_coord: F2Dot14::from_f32(tent.min.to_f32()), - peak_coord: F2Dot14::from_f32(tent.peak.to_f32()), - end_coord: F2Dot14::from_f32(tent.max.to_f32()), - } - }) - .collect(), - }, - value.ot_round(), - ) + .filter_map(|(region, value)| { + if region.is_default() { + None + } else { + Some(( + write_fonts::tables::variations::VariationRegion { + region_axes: region + .iter() + .zip(self.static_metadata.axes.iter()) + .map(|((tag, tent), expected_axis)| { + assert_eq!(*tag, expected_axis.tag); + RegionAxisCoordinates { + start_coord: F2Dot14::from_f32(tent.min.to_f32()), + peak_coord: F2Dot14::from_f32(tent.peak.to_f32()), + end_coord: F2Dot14::from_f32(tent.max.to_f32()), + } + }) + .collect(), + }, + value.ot_round(), + )) + } }) .collect(); @@ -520,7 +524,6 @@ mod tests { coords::{CoordConverter, DesignCoord, NormalizedCoord, NormalizedLocation, UserCoord}, ir::{Axis, StaticMetadata}, }; - use write_fonts::tables::variations::RegionAxisCoordinates; use super::FeaVariationInfo; @@ -564,10 +567,12 @@ mod tests { .unwrap() } - fn is_default(region_coords: &RegionAxisCoordinates) -> bool { - region_coords.start_coord.to_f32() == 0.0 - && region_coords.peak_coord.to_f32() == 0.0 - && region_coords.end_coord.to_f32() == 0.0 + fn is_default(region: &write_fonts::tables::variations::VariationRegion) -> bool { + region.region_axes.iter().all(|axis_coords| { + axis_coords.start_coord.to_f32() == 0.0 + && axis_coords.peak_coord.to_f32() == 0.0 + && axis_coords.end_coord.to_f32() == 0.0 + }) } #[test] @@ -585,17 +590,8 @@ mod tests { (BTreeMap::from([(wght, Fixed::from_f64(700.0))]), 20), ])) .unwrap(); - let mut region_values: Vec<_> = regions - .into_iter() - .map(|(r, v)| { - if r.region_axes.iter().all(is_default) { - v - } else { - v + default - } - }) - .collect(); - region_values.sort(); - assert_eq!((15, vec![10, 15, 20]), (default, region_values)); + assert!(!regions.iter().any(|(r, _)| is_default(r))); + let region_values: Vec<_> = regions.into_iter().map(|(_, v)| v + default).collect(); + assert_eq!((15, vec![10, 20]), (default, region_values)); } } From 9661684f112dfdd9c4554a2cdf8dd0e5f98ecbde Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Sun, 20 Aug 2023 17:26:42 +0100 Subject: [PATCH 4/5] add 'emum' prefix to class/glyph and glyph/class exceptions that's what ufo2ft kernFeatureWriter does -- fixes the aV kerning in Oswald and similar --- fontbe/src/features.rs | 15 ++++++++++++++- fontir/src/ir.rs | 12 ++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/fontbe/src/features.rs b/fontbe/src/features.rs index ce903008..54566b36 100644 --- a/fontbe/src/features.rs +++ b/fontbe/src/features.rs @@ -300,6 +300,15 @@ fn push_identifier(fea: &mut String, identifier: &KernParticipant) { } } +#[inline] +fn enumerated(kp1: &KernParticipant, kp2: &KernParticipant) -> bool { + // Glyph to class or class to glyph pairs are interpreted as 'class exceptions' to class pairs + // and are thus prefixed with 'enum' keyword so they will be enumerated as specific glyph-glyph pairs. + // http://adobe-type-tools.github.io/afdko/OpenTypeFeatureFileSpecification.html#6bii-enumerating-pairs + // https://github.com/googlefonts/ufo2ft/blob/b3895a9/Lib/ufo2ft/featureWriters/kernFeatureWriter.py#L360 + kp1.is_group() ^ kp2.is_group() +} + /// Create a single variable fea describing the kerning for the entire variation space. /// /// No merge baby! - [context](https://github.com/fonttools/fonttools/issues/3168#issuecomment-1608787520) @@ -362,7 +371,11 @@ fn create_kerning_fea(axes: &HashMap, kerning: &Kerning) -> Result bool { + matches!(self, KernParticipant::Glyph(_)) + } + + #[inline] + pub fn is_group(&self) -> bool { + matches!(self, KernParticipant::Group(_)) + } +} + impl StaticMetadata { pub fn new( units_per_em: u16, From c30c4b302d153bbb08b139ec26a8bff3121655c9 Mon Sep 17 00:00:00 2001 From: Rod S Date: Mon, 21 Aug 2023 12:10:24 -0700 Subject: [PATCH 5/5] Code review adjustments --- fontbe/src/features.rs | 10 ++-------- fontir/src/variations.rs | 12 +++++++++++- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/fontbe/src/features.rs b/fontbe/src/features.rs index 54566b36..cba7ee7f 100644 --- a/fontbe/src/features.rs +++ b/fontbe/src/features.rs @@ -23,7 +23,7 @@ use fontir::{ use log::{debug, error, trace, warn}; use fontdrasil::orchestration::{Access, Work}; -use write_fonts::{tables::variations::RegionAxisCoordinates, OtRound}; +use write_fonts::OtRound; use crate::{ error::Error, @@ -76,7 +76,6 @@ impl Display for NotSupportedError { } } -// TODO: ask Colin if we need to be threadsafe struct FeaVariationInfo<'a> { fea_rs_axes: HashMap, axes: HashMap, @@ -160,7 +159,6 @@ impl<'a> VariationInfo for FeaVariationInfo<'a> { .collect(); // Only 1 value per region for our input - // TODO is that actually guaranteed? let deltas: Vec<_> = var_model .deltas(&point_seqs)? .into_iter() @@ -197,11 +195,7 @@ impl<'a> VariationInfo for FeaVariationInfo<'a> { .zip(self.static_metadata.axes.iter()) .map(|((tag, tent), expected_axis)| { assert_eq!(*tag, expected_axis.tag); - RegionAxisCoordinates { - start_coord: F2Dot14::from_f32(tent.min.to_f32()), - peak_coord: F2Dot14::from_f32(tent.peak.to_f32()), - end_coord: F2Dot14::from_f32(tent.max.to_f32()), - } + tent.to_region_axis_coords() }) .collect(), }, diff --git a/fontir/src/variations.rs b/fontir/src/variations.rs index 52e06ee7..b5ddddfe 100644 --- a/fontir/src/variations.rs +++ b/fontir/src/variations.rs @@ -6,11 +6,12 @@ use std::{ ops::{Mul, Sub}, }; -use font_types::Tag; +use font_types::{F2Dot14, Tag}; use log::{log_enabled, trace}; use ordered_float::OrderedFloat; use serde::{Deserialize, Serialize}; use thiserror::Error; +use write_fonts::tables::variations::RegionAxisCoordinates; use crate::{ coords::{NormalizedCoord, NormalizedLocation}, @@ -523,6 +524,15 @@ impl Tent { let zero = NormalizedCoord::new(0.0); (zero, zero, zero) != (self.min, self.peak, self.max) } + + /// Create an equivalent [RegionAxisCoordinates] instance. + pub fn to_region_axis_coords(&self) -> RegionAxisCoordinates { + RegionAxisCoordinates { + start_coord: F2Dot14::from_f32(self.min.to_f32()), + peak_coord: F2Dot14::from_f32(self.peak.to_f32()), + end_coord: F2Dot14::from_f32(self.max.to_f32()), + } + } } impl Debug for Tent {