diff --git a/fontir/src/variations.rs b/fontir/src/variations.rs index ff43b3a5..d4538f8a 100644 --- a/fontir/src/variations.rs +++ b/fontir/src/variations.rs @@ -3,7 +3,7 @@ use std::{ cmp::Ordering, collections::{BTreeMap, HashMap, HashSet}, fmt::{Debug, Display}, - ops::{Mul, Sub}, + ops::{Add, Mul, Sub}, }; use fontdrasil::{ @@ -21,6 +21,49 @@ use write_fonts::{ use crate::error::VariationModelError; +/// Trait for rounding half-way values to the nearest even number. +/// +/// For example, 2.5 rounds to 2.0, 3.5 rounds to 4.0, and -2.5 rounds to -2.0. +/// This is the same rounding mode as [`f64::round_ties_even`], which was +/// stabilized in Rust 1.77.0. The trait provides a common way to apply this +/// to different types besides `f64` e.g. `kurbo::Vec2`. +/// We use it below in [`VariationModel`] for rounding variation deltas. +/// +/// It also matches the Python's buit-in `round` function which is used by FontTools' +/// VariationModel for rounding deltas. +/// +/// In general, this is the preferred approach when doing a lot of additions or +/// subtractions of rounded numbers, for it avoids bias both toward positive/negative +/// values and toward/away from zero. +/// +/// For more info, see: +/// - discussion in a related FontTools PR +/// - 'Rounding half to even' section in . +pub trait RoundTiesEven { + fn round_ties_even(self) -> Self; +} + +impl RoundTiesEven for f64 { + #[inline] + fn round_ties_even(self) -> f64 { + self.round_ties_even() + } +} + +impl RoundTiesEven for f32 { + #[inline] + fn round_ties_even(self) -> f32 { + self.round_ties_even() + } +} + +impl RoundTiesEven for kurbo::Vec2 { + #[inline] + fn round_ties_even(self) -> kurbo::Vec2 { + kurbo::Vec2::new(self.x.round_ties_even(), self.y.round_ties_even()) + } +} + const ZERO: OrderedFloat = OrderedFloat(0.0); const ONE: OrderedFloat = OrderedFloat(1.0); @@ -171,7 +214,7 @@ impl VariationModel { ) -> Result)>, DeltaError> where P: Copy + Default + Sub, - V: Copy + Mul + Sub, + V: Copy + Mul + Sub + RoundTiesEven, { if point_seqs.is_empty() { return Ok(Vec::new()); @@ -234,7 +277,14 @@ impl VariationModel { }) .fold(initial_vector, |acc, (other, other_weight)| { acc - *other * other_weight.into() - }), + }) + // deltas will be stored as integers in the VarStore hence must be rounded at + // some point; this is the correct place to round them, instead of at the end, + // otherwise rounding errors can compound especially where master influences + // overlap. This also matches FontTools behavior, see: + // https://github.com/fonttools/fonttools/issues/2213 + // https://github.com/fonttools/fonttools/pull/2214 + .round_ties_even(), ); } model_idx_to_result_idx.insert(model_idx, result.len()); @@ -243,6 +293,43 @@ impl VariationModel { Ok(result) } + + /// Convert relative deltas to absolute values at the given location. + /// + /// Rust version of + /// + /// Not `pub` yet as it's currently only used for self-tests. + /// + /// TODO: document invariants and what we are returning. Perhaps allow a different + /// type parameter for the return value so that e.g. absolute Points are returned + /// when the deltas are Vec2? + #[allow(dead_code)] + fn interpolate_from_deltas( + location: &NormalizedLocation, + deltasets: &[(VariationRegion, Vec)], + ) -> Vec + where + V: Copy + Mul + Add, + { + deltasets + .iter() + .map(|(region, deltas)| (region.scalar_at(location).0 as f64, deltas)) + .filter(|(scalar, _deltas)| *scalar != 0.0) + .fold(None, |result, (scalar, deltas)| { + let contribution = deltas.iter().map(|d| *d * scalar); + match result { + None => Some(contribution.collect::>()), + Some(mut existing) => { + existing + .iter_mut() + .zip(contribution) + .for_each(|(acc, val)| *acc = *acc + val); + Some(existing) + } + } + }) + .unwrap_or_default() + } } #[derive(Error, Debug)] @@ -778,6 +865,7 @@ mod tests { "ital" => ("Italic", "ital", 0, 0, 1), "foo" => ("Foo", "foo ", -1, 0, 1), "bar" => ("Bar", "bar ", -1, 0, 1), + "axis" => ("Axis", "axis", 0, 0, 1), _ => panic!("No definition for {tag}, add it?"), }; let min = UserCoord::new(min as f32); @@ -1314,13 +1402,177 @@ mod tests { (min_wght, vec![5.0]), ]); + let deltas = model.deltas(&point_seqs).unwrap(); + assert_eq!( vec![ (region(&[("wght", 0.0, 0.0, 0.0)]), vec![10.0]), (region(&[("wght", -1.0, -1.0, 0.0)]), vec![-5.0]), (region(&[("wght", 0.0, 1.0, 1.0)]), vec![2.0]), ], - model.deltas(&point_seqs).unwrap() + deltas + ); + + let loc = NormalizedLocation::for_pos(&[("wght", -0.5)]); + let expected = vec![7.5]; + assert_eq!( + expected, + VariationModel::interpolate_from_deltas(&loc, &deltas) + ); + } + + #[derive(Debug, Default, Copy, Clone, PartialEq)] + struct NoRoundF64(f64); + + impl RoundTiesEven for NoRoundF64 { + #[inline] + fn round_ties_even(self) -> NoRoundF64 { + self + } + } + + impl std::ops::Sub for NoRoundF64 { + type Output = Self; + + fn sub(self, rhs: Self) -> Self::Output { + NoRoundF64(self.0 - rhs.0) + } + } + + impl std::ops::Mul for NoRoundF64 { + type Output = Self; + + fn mul(self, rhs: f64) -> Self::Output { + NoRoundF64(self.0 * rhs) + } + } + + #[test] + fn modeling_error_within_tolerance() { + // Compare interpolating from un-rounded float deltas vs rounded deltas, + // and check that rounding errors don't accummulate but stay within <= 0.5. + // This test was ported from: + // https://github.com/fonttools/fonttools/blob/3b9a73ff/Tests/varLib/models_test.py#L167 + let num_locations = 31; + let num_samples = 251; + let locations: Vec<_> = (0..num_locations + 1) + .map(|i| NormalizedLocation::for_pos(&[("axis", i as f32 / num_locations as f32)])) + .collect(); + let master_values: HashMap<_, _> = locations + .iter() + .zip((0..num_locations + 1).map(|i| if i == 0 { 0.0 } else { 100.0 })) + .map(|(loc, value)| (loc.clone(), vec![value])) + .collect(); + // Same as master_values, but using special f64s that won't get rounded. + let master_values_noround: HashMap<_, _> = master_values + .iter() + .map(|(loc, values)| (loc.clone(), values.iter().map(|v| NoRoundF64(*v)).collect())) + .collect(); + + let model = + VariationModel::new(locations.into_iter().collect(), vec![axis("axis")]).unwrap(); + + let mut num_bad_errors = 0; + for i in 0..num_samples { + let loc = NormalizedLocation::for_pos(&[("axis", i as f32 / num_samples as f32)]); + + // unrounded float deltas + let deltas_float: Vec<_> = model + .deltas(&master_values_noround) + .unwrap() + .into_iter() + .map(|(region, deltas)| (region, deltas.iter().map(|d| d.0).collect::>())) + .collect(); + // deltas rounded within the delta computation loop + let deltas_round = model.deltas(&master_values).unwrap(); + // float deltas only rounded at the very end. This is how NOT to round deltas. + let deltas_late_round: Vec<_> = deltas_float + .iter() + .map(|(region, deltas)| { + ( + region.clone(), + deltas + .iter() + .map(|d| d.round_ties_even()) + .collect::>(), + ) + }) + .collect(); + // Sanity checks + assert_ne!(deltas_float, deltas_round); + assert_ne!(deltas_float, deltas_late_round); + assert_ne!(deltas_round, deltas_late_round); + assert!(deltas_round + .iter() + .all(|(_, deltas)| { deltas.iter().all(|d| d.fract() == 0.0) })); + + let expected: Vec = VariationModel::interpolate_from_deltas(&loc, &deltas_float); + let actual: Vec = VariationModel::interpolate_from_deltas(&loc, &deltas_round); + + let err = (actual[0] - expected[0]).abs(); + assert!(err <= 0.5, "i: {}, err: {}", i, err); + + // when the influence of many masters overlap a particular location + // interpolating from late-rounded deltas may lead to an accummulation + // of rounding errors that exceed the max tolerance + let bad = VariationModel::interpolate_from_deltas(&loc, &deltas_late_round); + let err_bad = (bad[0] - expected[0]).abs(); + if err_bad > 0.5 { + num_bad_errors += 1; + } + } + assert!(num_bad_errors > 0); + } + + #[test] + fn rounding_of_deltas_matches_fonttools() { + // This reproduces an off-by-one diff in MVAR table's deltas for Gelasio.glyphspackage + // between fontc and fontmake (rather fonttools) which was caused by the former's incorrect + // (late) rounding of deltas: + // https://github.com/googlefonts/fontc/issues/1043 + // https://github.com/googlefonts/fontc/issues/235 + let min = UserCoord::new(400.0); + let default = UserCoord::new(400.0); + let max = UserCoord::new(700.0); + let model = VariationModel::new( + HashSet::from([ + NormalizedLocation::for_pos(&[("wght", 0.0)]), + NormalizedLocation::for_pos(&[("wght", 1.0)]), + ]), + vec![Axis { + name: "Weight".to_string(), + tag: Tag::new(b"wght"), + min, + default, + max, + hidden: false, + converter: CoordConverter::new( + vec![ + (default, DesignCoord::new(default.into_inner())), + (max, DesignCoord::new(max.into_inner())), + ], + 0, + ), + }], + ) + .unwrap(); + + let master_values = HashMap::from([ + (NormalizedLocation::for_pos(&[("wght", 0.0)]), vec![591.6]), + (NormalizedLocation::for_pos(&[("wght", 1.0)]), vec![596.4]), + ]); + + // when working with unrounded values, the delta at wght=1.0 would be 4.8. By the + // time this gets ot-rounded to i16, it becomes 5, i.e. an extra +1 off the + // expected correct value (4). + assert_eq!( + vec![vec![592.0], vec![4.0]], // not vec![5.0] + model + .deltas(&master_values.clone()) + .unwrap() + .into_iter() + .map(|(_, ds)| ds) + .collect::>() ); } }