From a0d4725969755485ce35dd70300544775fa9b013 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 13 Feb 2025 13:24:55 +0000 Subject: [PATCH 1/2] add test to repro #1256 --- fontc/src/lib.rs | 49 ++++++++++++++++ .../glyphs2/WghtVar_ImplicitAxes.glyphs | 58 +++++++++---------- 2 files changed, 77 insertions(+), 30 deletions(-) diff --git a/fontc/src/lib.rs b/fontc/src/lib.rs index 2b5ead84b..244e71783 100644 --- a/fontc/src/lib.rs +++ b/fontc/src/lib.rs @@ -1960,6 +1960,55 @@ mod tests { ); } + #[test] + fn compile_hvar_single_model_direct_varstore_from_glyphs2_with_implicit_axes() { + // This test font is a Glyphs 2 source without an 'Axes' custom parameter, so + // the axes default to "Weight", "Width", and "Custom". However, the two masters + // only define coordinates along Weight, so the other two are so-called 'point' + // axes which don't vary and are discarded by the VariationModel. The IR Glyph + // instances' locations still contain these static 'point' axes. When building + // HVAR, we must prune these out lest their presence would trigger the creation + // of a multi-model indirect VarStore where a single-model direct one would suffice. + // https://github.com/googlefonts/fontc/issues/1256 + let result = TestCompile::compile_source("glyphs2/WghtVar_ImplicitAxes.glyphs"); + let font = result.font(); + let num_glyphs = font.maxp().unwrap().num_glyphs(); + assert_eq!(num_glyphs, 4); + let hvar = font.hvar().unwrap(); + // we expect a 'direct' VarStore with implicit variation indices and no mapping + assert!(hvar.advance_width_mapping().is_none()); + let varstore = hvar.item_variation_store().unwrap(); + let regions = varstore + .variation_region_list() + .unwrap() + .variation_regions(); + // this simple test font only has two masters (Regular [default] and Bold) + // so we expect a single region with a single wght axis + assert_eq!(regions.len(), 1); + let region_coords = regions + .get(0) + .unwrap() + .region_axes() + .iter() + .map(|coords| { + [ + coords.start_coord().to_f32(), + coords.peak_coord().to_f32(), + coords.end_coord().to_f32(), + ] + }) + .collect::>(); + assert_eq!(region_coords, vec![[0.0, 1.0, 1.0]]); + // we expect one ItemVariationData and 4 delta sets, one per glyph + assert_eq!(varstore.item_variation_data_count(), 1, "{varstore:#?}"); + let vardata = varstore.item_variation_data().get(0).unwrap().unwrap(); + assert_eq!(vardata.region_indexes(), &[0]); + assert_eq!( + vec![vec![0], vec![400], vec![200], vec![200],], + delta_sets(&vardata) + ); + } + #[test] fn compile_hvar_single_model_indirect_varstore() { // This is the same as the previous font but with additional 10 glyphs that diff --git a/resources/testdata/glyphs2/WghtVar_ImplicitAxes.glyphs b/resources/testdata/glyphs2/WghtVar_ImplicitAxes.glyphs index 1154b3d98..30e2c2f08 100644 --- a/resources/testdata/glyphs2/WghtVar_ImplicitAxes.glyphs +++ b/resources/testdata/glyphs2/WghtVar_ImplicitAxes.glyphs @@ -1,9 +1,5 @@ { -.appVersion = "3148"; -DisplayStrings = ( -"-", -"!" -); +.appVersion = "3340"; customParameters = ( ); date = "2022-12-01 04:52:20 +0000"; @@ -13,6 +9,8 @@ fontMaster = ( alignmentZones = ( "{800, 16}", "{0, -16}", +"{0, -16}", +"{0, -16}", "{-200, -16}" ); ascender = 800; @@ -50,7 +48,7 @@ unicode = 0020; }, { glyphname = exclam; -lastChange = "2022-12-01 05:10:49 +0000"; +lastChange = "2025-02-13 12:27:02 +0000"; layers = ( { layerId = m01; @@ -58,23 +56,23 @@ paths = ( { closed = 1; nodes = ( -"354 183 LINE", -"414 585 LINE", -"178 585 LINE", -"238 182 LINE" +"240 183 LINE", +"300 585 LINE", +"64 585 LINE", +"124 182 LINE" ); }, { closed = 1; nodes = ( -"354 0 LINE", -"354 107 LINE", -"238 107 LINE", -"238 0 LINE" +"240 0 LINE", +"240 107 LINE", +"124 107 LINE", +"124 0 LINE" ); } ); -width = 600; +width = 350; }, { layerId = "E09E0C54-128D-4FEA-B209-1B70BEFE300B"; @@ -82,30 +80,30 @@ paths = ( { closed = 1; nodes = ( -"364 176 LINE", -"434 605 LINE", -"159 605 LINE", -"228 174 LINE" +"342 176 LINE", +"412 605 LINE", +"137 605 LINE", +"206 174 LINE" ); }, { closed = 1; nodes = ( -"364 -20 LINE", -"364 94 LINE", -"228 94 LINE", -"228 -20 LINE" +"342 -20 LINE", +"342 94 LINE", +"206 94 LINE", +"206 -20 LINE" ); } ); -width = 600; +width = 550; } ); unicode = 0021; }, { glyphname = hyphen; -lastChange = "2022-12-01 04:57:39 +0000"; +lastChange = "2025-02-13 12:26:44 +0000"; layers = ( { layerId = m01; @@ -113,14 +111,14 @@ paths = ( { closed = 1; nodes = ( -"131 250 LINE", -"470 250 LINE", -"470 330 LINE", -"131 330 LINE" +"33 250 LINE", +"372 250 LINE", +"372 330 LINE", +"33 330 LINE" ); } ); -width = 600; +width = 400; }, { layerId = "E09E0C54-128D-4FEA-B209-1B70BEFE300B"; From 7716fb640a713cf9f6401a44861543880646a72f Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 13 Feb 2025 11:18:30 +0000 Subject: [PATCH 2/2] [hvar] prune point axes from glyph locations lest they trigger sub-model Fixes #1256 --- fontbe/src/hvar.rs | 25 ++++++++++++++++++------- fontdrasil/src/coords.rs | 18 +++++++++++++++++- 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/fontbe/src/hvar.rs b/fontbe/src/hvar.rs index 921bd4b50..82abaec6d 100644 --- a/fontbe/src/hvar.rs +++ b/fontbe/src/hvar.rs @@ -18,6 +18,7 @@ use write_fonts::{ hvar::Hvar, variations::{ivs_builder::VariationStoreBuilder, DeltaSetIndexMap, VariationRegion}, }, + types::Tag, validate::Validate, FontWrite, OtRound, }; @@ -50,6 +51,8 @@ where struct AdvanceWidthDeltas { /// Variation axes axes: Vec, + /// Set of axis tags + axis_tags: BTreeSet, /// Sparse variation models, keyed by the set of locations they define models: HashMap, VariationModel>, /// Glyph's advance width deltas sorted by glyph order @@ -59,13 +62,25 @@ struct AdvanceWidthDeltas { } impl AdvanceWidthDeltas { - fn new(global_model: VariationModel, glyph_locations: HashSet) -> Self { + fn new<'a>( + global_model: VariationModel, + glyph_locations: impl IntoIterator, + ) -> Self { let axes = global_model.axes().cloned().collect::>(); + let axis_tags: BTreeSet<_> = axes.iter().map(|axis| axis.tag).collect(); + // prune axes that are not in the global model (e.g. 'point' axes) which might + // be confused for a distinct sub-model + // https://github.com/googlefonts/fontc/issues/1256 + let glyph_locations = glyph_locations + .into_iter() + .map(|loc| loc.subset_axes(&axis_tags)) + .collect(); let global_locations = global_model.locations().cloned().collect::>(); let mut models = HashMap::new(); models.insert(global_locations, global_model); AdvanceWidthDeltas { axes, + axis_tags, models, deltas: Vec::new(), glyph_locations, @@ -78,7 +93,7 @@ impl AdvanceWidthDeltas { .iter() // widths must be rounded before the computing deltas to match fontmake // https://github.com/googlefonts/fontc/issues/1043 - .map(|(loc, src)| (loc.clone(), vec![src.width.ot_round()])) + .map(|(loc, src)| (loc.subset_axes(&self.axis_tags), vec![src.width.ot_round()])) .collect(); let name = glyph.name.clone(); let i = self.deltas.len(); @@ -169,11 +184,7 @@ impl Work for HvarWork { .names() .map(|name| context.ir.glyphs.get(&FeWorkId::Glyph(name.clone()))) .collect(); - let glyph_locations: HashSet<_> = glyphs - .iter() - .flat_map(|glyph| glyph.sources().keys()) - .cloned() - .collect(); + let glyph_locations = glyphs.iter().flat_map(|glyph| glyph.sources().keys()); let mut glyph_width_deltas = AdvanceWidthDeltas::new(var_model.clone(), glyph_locations); for glyph in glyphs.into_iter() { diff --git a/fontdrasil/src/coords.rs b/fontdrasil/src/coords.rs index b41c2434e..a539ee2a9 100644 --- a/fontdrasil/src/coords.rs +++ b/fontdrasil/src/coords.rs @@ -3,7 +3,7 @@ //! See use std::{ - collections::{BTreeMap, HashMap}, + collections::{BTreeMap, BTreeSet, HashMap}, fmt::{Debug, Write}, marker::PhantomData, ops::Sub, @@ -323,6 +323,22 @@ impl Location { self.0.retain(pred); } + /// Creates a new `Location` containing only the axis tags contained in the given set. + pub fn subset_axes(&self, axis_tags: &BTreeSet) -> Self { + Self( + self.0 + .iter() + .filter_map(|(tag, coord)| { + if axis_tags.contains(tag) { + Some((*tag, *coord)) + } else { + None + } + }) + .collect(), + ) + } + pub fn convert(&self, axes: &HashMap) -> Location where Space: ConvertSpace,