Merge pull request #1263 from googlefonts/glyphs2-implicit-axes-direc…

…t-hvar

fix Glyphs 2 sources with implicit axes always getting indirect HVAR

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<Axis>,
+    /// Set of axis tags
+    axis_tags: BTreeSet<Tag>,
     /// Sparse variation models, keyed by the set of locations they define
     models: HashMap<BTreeSet<NormalizedLocation>, 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<NormalizedLocation>) -> Self {
+    fn new<'a>(
+        global_model: VariationModel,
+        glyph_locations: impl IntoIterator<Item = &'a NormalizedLocation>,
+    ) -> Self {
         let axes = global_model.axes().cloned().collect::<Vec<_>>();
+        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::<BTreeSet<_>>();
         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<Context, AnyWorkId, Error> 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() {

fontc/src/lib.rs

@@ -1947,6 +1947,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::<Vec<_>>();
+        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

fontdrasil/src/coords.rs

@@ -3,7 +3,7 @@
 //! See <https://github.com/googlefonts/fontmake-rs/blob/main/resources/text/units.md>
 
 use std::{
-    collections::{BTreeMap, HashMap},
+    collections::{BTreeMap, BTreeSet, HashMap},
     fmt::{Debug, Write},
     marker::PhantomData,
     ops::Sub,
@@ -323,6 +323,22 @@ impl<Space> Location<Space> {
         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<Tag>) -> Self {
+        Self(
+            self.0
+                .iter()
+                .filter_map(|(tag, coord)| {
+                    if axis_tags.contains(tag) {
+                        Some((*tag, *coord))
+                    } else {
+                        None
+                    }
+                })
+                .collect(),
+        )
+    }
+
     pub fn convert<ToSpace>(&self, axes: &HashMap<Tag, &Axis>) -> Location<ToSpace>
     where
         Space: ConvertSpace<ToSpace>,

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,77 +48,77 @@ unicode = 0020;
 },
 {
 glyphname = exclam;
-lastChange = "2022-12-01 05:10:49 +0000";
+lastChange = "2025-02-13 12:27:02 +0000";
 layers = (
 {
 layerId = m01;
 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";
 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;
 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";