From b1ed40485a5a1f71db847324ac7b36467ed00565 Mon Sep 17 00:00:00 2001 From: rsheeter Date: Thu, 3 Oct 2024 15:05:45 -0700 Subject: [PATCH] Expose and fix rounding difference vs python. --- fontbe/src/mvar.rs | 4 +- fontir/src/glyph.rs | 4 +- fontir/src/ir.rs | 140 ++++++++++++++++++++++++++---------- glyphs2fontir/src/source.rs | 3 +- ufo2fontir/src/source.rs | 9 ++- 5 files changed, 115 insertions(+), 45 deletions(-) diff --git a/fontbe/src/mvar.rs b/fontbe/src/mvar.rs index bd5026a9..44d75bb2 100644 --- a/fontbe/src/mvar.rs +++ b/fontbe/src/mvar.rs @@ -60,7 +60,7 @@ impl MvarBuilder { fn add_sources(&mut self, mvar_tag: Tag, sources: &GlobalMetricValues) -> Result<(), Error> { let sources: HashMap<_, _> = sources .iter() - .map(|(loc, src)| (loc.clone(), vec![src.into_inner() as f64])) + .map(|(loc, src)| (loc.clone(), vec![src.into_inner()])) .collect(); if sources.len() == 1 { assert!(sources.keys().next().unwrap().is_default()); @@ -205,7 +205,7 @@ mod tests { ) { let sources = sources .iter() - .map(|(loc, value)| ((*loc).clone(), (*value).into())) + .map(|(loc, value)| ((*loc).clone(), (*value as f64).into())) .collect::>(); builder .add_sources(Tag::from_str(mvar_tag).unwrap(), &sources) diff --git a/fontir/src/glyph.rs b/fontir/src/glyph.rs index eca1cd46..0f2a4033 100644 --- a/fontir/src/glyph.rs +++ b/fontir/src/glyph.rs @@ -356,8 +356,8 @@ fn ensure_notdef_exists_and_is_gid_0( let builder = GlyphBuilder::new_notdef( static_metadata.default_location().clone(), static_metadata.units_per_em, - metrics.ascender.0, - metrics.descender.0, + metrics.ascender.0 as f32, + metrics.descender.0 as f32, ); context.glyphs.set(builder.build()?); } diff --git a/fontir/src/ir.rs b/fontir/src/ir.rs index 90ec49a8..5a75437d 100644 --- a/fontir/src/ir.rs +++ b/fontir/src/ir.rs @@ -422,7 +422,7 @@ impl StaticMetadata { /// At a minimum should be defined at the default location. #[derive(Serialize, Deserialize, Default, Debug, Clone, PartialEq, Eq)] pub struct GlobalMetrics( - pub(crate) HashMap>>, + pub(crate) HashMap>>, ); #[derive(Serialize, Deserialize, Debug, Copy, Clone, PartialEq, Eq, Hash)] @@ -516,7 +516,7 @@ fn adjust_offset(offset: f64, angle: f64) -> f64 { } /// Map of values associated with a given global metric at various locations. -pub type GlobalMetricValues = HashMap>; +pub type GlobalMetricValues = HashMap>; impl GlobalMetrics { /// Creates a new, empty, [GlobalMetrics] @@ -542,8 +542,7 @@ impl GlobalMetrics { ) { let units_per_em = units_per_em as f64; - let mut set_if_absent = - |metric, value| self.set_if_absent(metric, pos.clone(), value as f32); + let mut set_if_absent = |metric, value| self.set_if_absent(metric, pos.clone(), value); // https://github.com/googlefonts/ufo2ft/blob/fca66fe3ea1ea88ffb36f8264b21ce042d3afd05/Lib/ufo2ft/fontInfoData.py#L38-L45 let ascender = ascender.unwrap_or(0.8 * units_per_em); @@ -625,7 +624,7 @@ impl GlobalMetrics { set_if_absent(GlobalMetric::UnderlinePosition, -0.1 * units_per_em); // // https://github.com/googlefonts/ufo2ft/blob/0d2688cd847d003b41104534d16973f72ef26c40/Lib/ufo2ft/fontInfoData.py#L313-L316 - set_if_absent(GlobalMetric::StrikeoutSize, underline_thickness.0 as f64); + set_if_absent(GlobalMetric::StrikeoutSize, underline_thickness.0); set_if_absent( GlobalMetric::StrikeoutPosition, x_height.map(|v| v * 0.6).unwrap_or(units_per_em * 0.22), @@ -641,7 +640,7 @@ impl GlobalMetrics { self.0.entry(metric).or_default() } - pub fn get(&self, metric: GlobalMetric, pos: &NormalizedLocation) -> OrderedFloat { + pub fn get(&self, metric: GlobalMetric, pos: &NormalizedLocation) -> OrderedFloat { let Some(value) = self.values(metric).get(pos) else { panic!("interpolated metric values are not yet supported"); }; @@ -652,7 +651,7 @@ impl GlobalMetrics { &mut self, metric: GlobalMetric, pos: NormalizedLocation, - value: impl Into>, + value: impl Into>, ) { self.values_mut(metric).insert(pos, value.into()); } @@ -661,8 +660,8 @@ impl GlobalMetrics { &mut self, metric: GlobalMetric, pos: NormalizedLocation, - value: impl Into>, - ) -> OrderedFloat { + value: impl Into>, + ) -> OrderedFloat { *self.values_mut(metric).entry(pos).or_insert(value.into()) } @@ -673,7 +672,7 @@ impl GlobalMetrics { maybe_value: Option>>, ) { if let Some(value) = maybe_value { - self.set(metric, pos, value.into().into_inner() as f32); + self.set(metric, pos, value.into().into_inner()); } } @@ -719,33 +718,86 @@ impl GlobalMetrics { #[derive(Default, Debug, Clone, PartialEq, Eq)] pub struct GlobalMetricsInstance { pub pos: NormalizedLocation, - pub ascender: OrderedFloat, - pub descender: OrderedFloat, - pub caret_slope_rise: OrderedFloat, - pub caret_slope_run: OrderedFloat, - pub caret_offset: OrderedFloat, - pub cap_height: OrderedFloat, - pub x_height: OrderedFloat, - pub os2_typo_ascender: OrderedFloat, - pub os2_typo_descender: OrderedFloat, - pub os2_typo_line_gap: OrderedFloat, - pub os2_win_ascent: OrderedFloat, - pub os2_win_descent: OrderedFloat, - pub hhea_ascender: OrderedFloat, - pub hhea_descender: OrderedFloat, - pub hhea_line_gap: OrderedFloat, - pub strikeout_position: OrderedFloat, - pub strikeout_size: OrderedFloat, - pub subscript_x_offset: OrderedFloat, - pub subscript_x_size: OrderedFloat, - pub subscript_y_offset: OrderedFloat, - pub subscript_y_size: OrderedFloat, - pub superscript_x_offset: OrderedFloat, - pub superscript_x_size: OrderedFloat, - pub superscript_y_offset: OrderedFloat, - pub superscript_y_size: OrderedFloat, - pub underline_thickness: OrderedFloat, - pub underline_position: OrderedFloat, + pub ascender: OrderedFloat, + pub descender: OrderedFloat, + pub caret_slope_rise: OrderedFloat, + pub caret_slope_run: OrderedFloat, + pub caret_offset: OrderedFloat, + pub cap_height: OrderedFloat, + pub x_height: OrderedFloat, + pub os2_typo_ascender: OrderedFloat, + pub os2_typo_descender: OrderedFloat, + pub os2_typo_line_gap: OrderedFloat, + pub os2_win_ascent: OrderedFloat, + pub os2_win_descent: OrderedFloat, + pub hhea_ascender: OrderedFloat, + pub hhea_descender: OrderedFloat, + pub hhea_line_gap: OrderedFloat, + pub strikeout_position: OrderedFloat, + pub strikeout_size: OrderedFloat, + pub subscript_x_offset: OrderedFloat, + pub subscript_x_size: OrderedFloat, + pub subscript_y_offset: OrderedFloat, + pub subscript_y_size: OrderedFloat, + pub superscript_x_offset: OrderedFloat, + pub superscript_x_size: OrderedFloat, + pub superscript_y_offset: OrderedFloat, + pub superscript_y_size: OrderedFloat, + pub underline_thickness: OrderedFloat, + pub underline_position: OrderedFloat, +} + +// It's not *that* fun to write long decimals in unit tests +#[doc(hidden)] +pub mod test_helpers { + use ordered_float::OrderedFloat; + + use super::GlobalMetricsInstance; + + pub trait Round2 { + fn round2(self) -> Self; + } + + impl Round2 for OrderedFloat { + fn round2(self) -> Self { + OrderedFloat((self.0 * 100.0).round() / 100.0) + } + } + + impl Round2 for GlobalMetricsInstance { + fn round2(self) -> Self { + GlobalMetricsInstance { + pos: self.pos.clone(), + ascender: self.ascender.round2(), + descender: self.descender.round2(), + caret_slope_rise: self.caret_slope_rise.round2(), + cap_height: self.cap_height.round2(), + x_height: self.x_height.round2(), + subscript_x_size: self.subscript_x_size.round2(), + subscript_y_size: self.subscript_y_size.round2(), + subscript_y_offset: self.subscript_y_offset.round2(), + superscript_x_size: self.superscript_x_size.round2(), + superscript_y_size: self.superscript_y_size.round2(), + superscript_y_offset: self.superscript_y_offset.round2(), + strikeout_position: self.strikeout_position.round2(), + strikeout_size: self.strikeout_size.round2(), + os2_typo_ascender: self.os2_typo_ascender.round2(), + os2_typo_descender: self.os2_typo_descender.round2(), + os2_typo_line_gap: self.os2_typo_line_gap.round2(), + os2_win_ascent: self.os2_win_ascent.round2(), + os2_win_descent: self.os2_win_descent.round2(), + hhea_ascender: self.hhea_ascender.round2(), + hhea_descender: self.hhea_descender.round2(), + hhea_line_gap: self.hhea_line_gap.round2(), + underline_thickness: self.underline_thickness.round2(), + underline_position: self.underline_position.round2(), + caret_slope_run: self.caret_slope_run.round2(), + caret_offset: self.caret_offset.round2(), + subscript_x_offset: self.subscript_x_offset.round2(), + superscript_x_offset: self.superscript_x_offset.round2(), + } + } + } } /// Helps accumulate 'name' values. @@ -2535,4 +2587,18 @@ mod tests { names, ) } + + // We had a bug where ((1290 as f64 * 0.35) as f32).ot_round() was 452. + // Without the as f32 (caused by GlobalMetrics using f32) it's 451. In Python it's 451. + #[test] + fn round_like_py() { + let pos = NormalizedLocation::for_pos(&[("wght", 0.0)]); + let mut metrics = GlobalMetrics::new(); + metrics.populate_defaults(&pos, 1290, None, None, None, None); + let rounded: i16 = metrics + .get(GlobalMetric::SuperscriptYOffset, &pos) + .0 + .ot_round(); + assert_eq!(451, rounded); + } } diff --git a/glyphs2fontir/src/source.rs b/glyphs2fontir/src/source.rs index cb5d35a0..4144e7b9 100644 --- a/glyphs2fontir/src/source.rs +++ b/glyphs2fontir/src/source.rs @@ -1091,6 +1091,7 @@ mod tests { }; use glyphs_reader::{glyphdata::Category, Font}; use indexmap::IndexSet; + use ir::test_helpers::Round2; use write_fonts::types::{NameId, Tag}; use crate::source::names; @@ -1676,7 +1677,7 @@ mod tests { underline_position: (-100.0).into(), ..Default::default() }, - default_metrics + default_metrics.round2() ); } diff --git a/ufo2fontir/src/source.rs b/ufo2fontir/src/source.rs index c417c8dd..62fe3702 100644 --- a/ufo2fontir/src/source.rs +++ b/ufo2fontir/src/source.rs @@ -1006,7 +1006,7 @@ fn set_default_underline_pos( metrics.set_if_absent( GlobalMetric::UnderlinePosition, location.clone(), - -0.075 * units_per_em as f32, + -0.075 * units_per_em as f64, ); } @@ -1576,7 +1576,10 @@ mod tests { types::GlyphName, }; use fontir::{ - ir::{AnchorKind, GlobalMetricsInstance, GlyphOrder, NameKey, PostscriptNames}, + ir::{ + test_helpers::Round2, AnchorKind, GlobalMetricsInstance, GlyphOrder, NameKey, + PostscriptNames, + }, orchestration::{Context, Flags, WorkId}, paths::Paths, source::{Input, Source}, @@ -2014,7 +2017,7 @@ mod tests { underline_position: (-75.0).into(), ..Default::default() }, - default_metrics + default_metrics.round2() ); }