Merge pull request #779 from googlefonts/fix-incorrect-feature-deltas…

…-when-sparse

[fontbe] Fix incorrect feature deltas w/ sparse glyph sources

fontbe/src/features.rs

@@ -1,8 +1,9 @@
 //! Feature binary compilation.
 
 use std::{
+    borrow::Cow,
     cell::RefCell,
-    collections::HashMap,
+    collections::{HashMap, HashSet},
     ffi::{OsStr, OsString},
     fmt::Display,
     fs,
@@ -26,7 +27,7 @@ use fea_rs::{
 use fontir::{
     ir::{FeaturesSource, GlyphOrder, StaticMetadata},
     orchestration::{Flags, WorkId as FeWorkId},
-    variations::DeltaError,
+    variations::{DeltaError, VariationModel},
 };
 
 use fontdrasil::{
@@ -151,12 +152,27 @@ pub(crate) fn resolve_variable_metric<'a>(
     static_metadata: &StaticMetadata,
     values: impl Iterator<Item = (&'a NormalizedLocation, &'a OrderedFloat<f32>)>,
 ) -> Result<(i16, Vec<(VariationRegion, i16)>), DeltaError> {
-    let var_model = &static_metadata.variation_model;
-
-    let point_seqs = values
+    let point_seqs: HashMap<_, _> = values
         .into_iter()
         .map(|(pos, value)| (pos.to_owned(), vec![value.0 as f64]))
         .collect();
+    let locations: HashSet<_> = point_seqs.keys().collect();
+    let global_locations: HashSet<_> = static_metadata.variation_model.locations().collect();
+
+    // Try to reuse the global model, or make a new sub-model only with the locations we
+    // are asked for so we can support sparseness
+    let var_model: Cow<'_, VariationModel> = if locations == global_locations {
+        Cow::Borrowed(&static_metadata.variation_model)
+    } else {
+        Cow::Owned(
+            VariationModel::new(
+                locations.into_iter().cloned().collect(),
+                static_metadata.axes.clone(),
+            )
+            .unwrap(),
+        )
+    };
+
     let raw_deltas: Vec<_> = var_model
         .deltas(&point_seqs)?
         .into_iter()
@@ -268,21 +284,29 @@ impl<'a> VariationInfo for FeaVariationInfo<'a> {
         &self,
         values: &HashMap<NormalizedLocation, i16>,
     ) -> Result<(i16, Vec<(VariationRegion, i16)>), Error> {
-        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)| (pos.clone(), vec![*value as f64]))
             .collect();
 
-        // We only support use when the point seq is at a location our variation model supports
-        // TODO: get a model for the location we are asked for so we can support sparseness
-        for loc in point_seqs.keys() {
-            if !var_model.supports(loc) {
-                return Err(Error::NoVariationModel(loc.clone()));
-            }
-        }
+        let locations: HashSet<_> = point_seqs.keys().collect();
+        let global_locations: HashSet<_> =
+            self.static_metadata.variation_model.locations().collect();
+
+        // Try to reuse the global model, or make a new sub-model only with the locations we
+        // are asked for so we can support sparseness
+        let var_model: Cow<'_, VariationModel> = if locations == global_locations {
+            Cow::Borrowed(&self.static_metadata.variation_model)
+        } else {
+            Cow::Owned(
+                VariationModel::new(
+                    locations.into_iter().cloned().collect(),
+                    self.static_metadata.axes.clone(),
+                )
+                .unwrap(),
+            )
+        };
 
         // Only 1 value per region for our input
         let deltas: Vec<_> = var_model

fontbe/src/hvar.rs

@@ -1,10 +1,9 @@
 //! Generates an [HVAR](https://learn.microsoft.com/en-us/typography/opentype/spec/HVAR) table.
 
 use std::any::type_name;
-use std::collections::{BTreeSet, HashMap};
+use std::collections::{BTreeSet, HashMap, HashSet};
 
 use fontdrasil::orchestration::AccessBuilder;
-use indexmap::IndexMap;
 
 use fontdrasil::{
     coords::NormalizedLocation,
@@ -52,30 +51,27 @@ struct AdvanceWidthDeltas {
     /// Variation axes
     axes: Vec<Axis>,
     /// Sparse variation models, keyed by the set of locations they define
-    models: IndexMap<BTreeSet<NormalizedLocation>, VariationModel>,
+    models: HashMap<BTreeSet<NormalizedLocation>, VariationModel>,
     /// Glyph's advance width deltas sorted by glyph order
     deltas: Vec<Vec<(VariationRegion, i16)>>,
+    /// All the glyph locations that are defined in the font
+    glyph_locations: HashSet<NormalizedLocation>,
 }
 
 impl AdvanceWidthDeltas {
-    fn new(global_model: VariationModel) -> Self {
+    fn new(global_model: VariationModel, glyph_locations: HashSet<NormalizedLocation>) -> Self {
         let axes = global_model.axes().cloned().collect::<Vec<_>>();
         let global_locations = global_model.locations().cloned().collect::<BTreeSet<_>>();
-        // using IndexMap to quickly get a ref to the first entry (in insertion order)
-        // in the global_locations() method below
-        let mut models = IndexMap::new();
+        let mut models = HashMap::new();
         models.insert(global_locations, global_model);
         AdvanceWidthDeltas {
             axes,
             models,
             deltas: Vec::new(),
+            glyph_locations,
         }
     }
 
-    fn global_locations(&self) -> &BTreeSet<NormalizedLocation> {
-        self.models.first().unwrap().0
-    }
-
     fn add(&mut self, glyph: &Glyph) -> Result<(), Error> {
         let mut advance_widths: HashMap<_, _> = glyph
             .sources()
@@ -101,7 +97,7 @@ impl AdvanceWidthDeltas {
             // all other glyph locations...
             if i == 0 && name == GlyphName::NOTDEF {
                 let notdef_width = advance_widths.values().next().unwrap()[0];
-                for loc in self.global_locations().iter() {
+                for loc in self.glyph_locations.iter() {
                     advance_widths
                         .entry(loc.clone())
                         .or_insert_with(|| vec![notdef_width]);
@@ -167,10 +163,18 @@ impl Work<Context, AnyWorkId, Error> for HvarWork {
         let var_model = &static_metadata.variation_model;
         let glyph_order = context.ir.glyph_order.get();
         let axis_count = var_model.axes().count().try_into().unwrap();
+        let glyphs: Vec<_> = glyph_order
+            .iter()
+            .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 mut glyph_width_deltas = AdvanceWidthDeltas::new(var_model.clone());
-        for name in glyph_order.iter() {
-            let glyph = context.ir.glyphs.get(&FeWorkId::Glyph(name.clone()));
+        let mut glyph_width_deltas = AdvanceWidthDeltas::new(var_model.clone(), glyph_locations);
+        for glyph in glyphs.into_iter() {
             glyph_width_deltas.add(glyph.as_ref())?;
         }
 

fontir/src/ir.rs

@@ -328,7 +328,7 @@ impl StaticMetadata {
         names: HashMap<NameKey, String>,
         axes: Vec<Axis>,
         named_instances: Vec<NamedInstance>,
-        glyph_locations: HashSet<NormalizedLocation>,
+        global_locations: HashSet<NormalizedLocation>,
         postscript_names: PostscriptNames,
         italic_angle: f64,
         gdef_categories: GdefCategories,
@@ -352,7 +352,7 @@ impl StaticMetadata {
                 key_to_name.insert(NameKey::new(name_id_gen.into(), name), name.clone());
             });
 
-        let variation_model = VariationModel::new(glyph_locations, variable_axes.clone())?;
+        let variation_model = VariationModel::new(global_locations, variable_axes.clone())?;
 
         let default_location = axes
             .iter()

glyphs2fontir/src/source.rs

@@ -351,7 +351,7 @@ impl Work<Context, WorkId, WorkError> for StaticMetadataWork {
                 })
             })
             .collect();
-        let glyph_locations = font
+        let global_locations = font
             .masters
             .iter()
             .map(|m| font_info.locations.get(&m.axes_values).cloned().unwrap())
@@ -387,7 +387,7 @@ impl Work<Context, WorkId, WorkError> for StaticMetadataWork {
             names(font),
             axes,
             named_instances,
-            glyph_locations,
+            global_locations,
             Default::default(), // TODO: impl reading PS names from Glyphs
             italic_angle,
             categories,

ufo2fontir/src/source.rs

@@ -865,8 +865,15 @@ impl Work<Context, WorkId, WorkError> for StaticMetadataWork {
             })
             .collect();
 
-        let master_locations = master_locations(&axes, &self.designspace.sources);
-        let glyph_locations = master_locations.values().cloned().collect();
+        let global_locations = master_locations(
+            &axes,
+            self.designspace
+                .sources
+                .iter()
+                .filter(|s| !is_glyph_only(s)),
+        )
+        .into_values()
+        .collect();
 
         let lib_plist =
             match load_plist(&designspace_dir.join(&default_master.filename), "lib.plist") {
@@ -917,7 +924,7 @@ impl Work<Context, WorkId, WorkError> for StaticMetadataWork {
             names,
             axes,
             named_instances,
-            glyph_locations,
+            global_locations,
             postscript_names,
             italic_angle,
             glyph_categories,
@@ -1997,15 +2004,17 @@ mod tests {
 
     // Was tripping up on wght_var having two <source> with the same filename, different name and xvalue
     #[test]
-    fn glyph_locations() {
+    fn global_locations() {
         let (_, context) = build_static_metadata("wght_var.designspace", default_test_flags());
         let static_metadata = &context.static_metadata.get();
         let wght = static_metadata.axes.first().unwrap();
 
         assert_eq!(
             vec![
                 (UserCoord::new(400.0), NormalizedCoord::new(0.0)),
-                (UserCoord::new(600.0), NormalizedCoord::new(0.6666667,)),
+                // this intermediate/sparse layer location is omitted from the global model
+                // because it only contributes glyphs, not metrics or kerning
+                // (UserCoord::new(600.0), NormalizedCoord::new(0.6666667,)),
                 (UserCoord::new(700.0), NormalizedCoord::new(1.0)),
             ],
             static_metadata

ufo2fontir/src/toir.rs

@@ -89,14 +89,14 @@ fn to_ir_glyph_instance(glyph: &norad::Glyph) -> Result<ir::GlyphInstance, WorkE
 }
 
 /// Create a map from source filename (e.g. x.ufo) => normalized location
-pub fn master_locations(
+pub fn master_locations<'a>(
     axes: &[fontdrasil::types::Axis],
-    sources: &[designspace::Source],
+    sources: impl IntoIterator<Item = &'a designspace::Source>,
 ) -> HashMap<String, NormalizedLocation> {
     let tags_by_name: HashMap<_, _> = axes.iter().map(|a| (a.name.as_str(), a.tag)).collect();
     let axes = axes.iter().map(|a| (a.tag, a)).collect();
     sources
-        .iter()
+        .into_iter()
         .map(|s| {
             (
                 s.name.clone().unwrap(),