Merge pull request #627 from googlefonts/underline-to-global-metrics

move underline_thickness/position to GlobalMetrics as they are MVAR-iable
googlefonts · Dec 5, 2023 · 626c77f · 626c77f
2 parents b217f65 + ac76139
commit 626c77f
Show file tree

Hide file tree

Showing 5 changed files with 68 additions and 53 deletions.
diff --git a/fontbe/src/post.rs b/fontbe/src/post.rs
@@ -7,6 +7,7 @@ use fontir::orchestration::WorkId as FeWorkId;
 use write_fonts::{
     tables::post::Post,
     types::{FWord, Fixed},
+    OtRound,
 };
 
 use crate::{
@@ -29,6 +30,7 @@ impl Work<Context, AnyWorkId, Error> for PostWork {
     fn read_access(&self) -> Access<AnyWorkId> {
         Access::Set(HashSet::from([
             FeWorkId::StaticMetadata.into(),
+            FeWorkId::GlobalMetrics.into(),
             FeWorkId::GlyphOrder.into(),
         ]))
     }
@@ -39,6 +41,11 @@ impl Work<Context, AnyWorkId, Error> for PostWork {
         // TODO optionally drop glyph names with format 3.0.
         // TODO a more serious post
         let static_metadata = context.ir.static_metadata.get();
+        let metrics = context
+            .ir
+            .global_metrics
+            .get()
+            .at(static_metadata.default_location());
         let postscript_names = &static_metadata.postscript_names;
         let glyph_order = context.ir.glyph_order.get();
         let mut post = Post::new_v2(
@@ -47,8 +54,8 @@ impl Work<Context, AnyWorkId, Error> for PostWork {
                 .map(|g| postscript_names.get(g).unwrap_or(g).as_str()),
         );
         post.italic_angle = Fixed::from_f64(static_metadata.italic_angle.into_inner());
-        post.underline_position = FWord::new(static_metadata.misc.underline_position.0 as i16);
-        post.underline_thickness = FWord::new(static_metadata.misc.underline_thickness.0 as i16);
+        post.underline_position = FWord::new(metrics.underline_position.ot_round());
+        post.underline_thickness = FWord::new(metrics.underline_thickness.ot_round());
         context.post.set_unconditionally(post.into());
         Ok(())
     }

diff --git a/fontir/src/ir.rs b/fontir/src/ir.rs
@@ -94,9 +94,6 @@ pub struct MiscMetadata {
     /// See <https://learn.microsoft.com/en-us/typography/opentype/spec/os2#achvendid>
     pub vendor_id: Tag,
 
-    pub underline_thickness: OrderedFloat<f32>,
-    pub underline_position: OrderedFloat<f32>,
-
     /// UFO appears to allow negative major versions.
     ///
     /// See <https://unifiedfontobject.org/versions/ufo3/fontinfo.plist/#generic-identification-information>
@@ -294,8 +291,6 @@ impl StaticMetadata {
                 fs_type: None, // default is, sigh, inconsistent across source formats
                 selection_flags: Default::default(),
                 vendor_id: DEFAULT_VENDOR_ID_TAG,
-                underline_thickness: 0.0.into(),
-                underline_position: 0.0.into(),
                 // https://github.com/googlefonts/ufo2ft/blob/0d2688cd847d003b41104534d16973f72ef26c40/Lib/ufo2ft/fontInfoData.py#L353-L354
                 version_major: 0,
                 version_minor: 0,
@@ -339,6 +334,8 @@ pub enum GlobalMetric {
     CaretSlopeRise,
     CaretSlopeRun,
     CaretOffset,
+    UnderlineThickness,
+    UnderlinePosition,
     XHeight,
     YSubscriptXSize,
     YSubscriptYSize,
@@ -450,6 +447,14 @@ impl GlobalMetrics {
         set(GlobalMetric::YStrikeoutSize, 0.05 * units_per_em as f32);
         set(GlobalMetric::YStrikeoutPosition, x_height * 0.6);
 
+        // ufo2ft and Glyphs.app have different defaults for the post.underlinePosition:
+        // the former uses 0.075*UPEM whereas the latter 0.1*UPEM (both use the same
+        // underlineThickness 0.05*UPEM). We prefer to match Glyphs.app as more widely used.
+        // https://github.com/googlefonts/ufo2ft/blob/main/Lib/ufo2ft/fontInfoData.py#L313-L322
+        // https://github.com/googlefonts/glyphsLib/blob/main/Lib/glyphsLib/builder/custom_params.py#L1116-L1125
+        set(GlobalMetric::UnderlineThickness, 0.05 * units_per_em as f32);
+        set(GlobalMetric::UnderlinePosition, -0.1 * units_per_em as f32);
+
         metrics
     }
 
@@ -520,6 +525,8 @@ impl GlobalMetrics {
             hhea_ascender: self.get(GlobalMetric::HheaAscender, pos),
             hhea_descender: self.get(GlobalMetric::HheaDescender, pos),
             hhea_line_gap: self.get(GlobalMetric::HheaLineGap, pos),
+            underline_thickness: self.get(GlobalMetric::UnderlineThickness, pos),
+            underline_position: self.get(GlobalMetric::UnderlinePosition, pos),
         }
     }
 
@@ -559,6 +566,8 @@ pub struct GlobalMetricsInstance {
     pub hhea_ascender: OrderedFloat<f32>,
     pub hhea_descender: OrderedFloat<f32>,
     pub hhea_line_gap: OrderedFloat<f32>,
+    pub underline_thickness: OrderedFloat<f32>,
+    pub underline_position: OrderedFloat<f32>,
 }
 
 /// Helps accumulate 'name' values.
@@ -1580,8 +1589,6 @@ mod tests {
                 fs_type: None,
                 selection_flags: SelectionFlags::default(),
                 vendor_id: Tag::from_be_bytes(*b"DUCK"),
-                underline_thickness: 0.15.into(),
-                underline_position: 16.0.into(),
                 version_major: 42,
                 version_minor: 24,
                 head_flags: 42,

diff --git a/glyphs-reader/src/font.rs b/glyphs-reader/src/font.rs
@@ -293,6 +293,15 @@ impl CustomParameters {
         Some(*i)
     }
 
+    fn float(&self, name: &str) -> Option<OrderedFloat<f64>> {
+        let value = self.get(name)?;
+        match value {
+            CustomParameterValue::Int(i) => Some((*i as f64).into()),
+            CustomParameterValue::Float(f) => Some(*f),
+            _ => None,
+        }
+    }
+
     fn bool(&self, name: &str) -> Option<bool> {
         self.int(name).map(|v| v == 1)
     }
@@ -734,6 +743,8 @@ pub struct FontMaster {
     pub hhea_ascender: Option<i64>,
     pub hhea_descender: Option<i64>,
     pub hhea_line_gap: Option<i64>,
+    pub underline_thickness: Option<OrderedFloat<f64>>,
+    pub underline_position: Option<OrderedFloat<f64>>,
 }
 
 impl FontMaster {
@@ -1907,6 +1918,8 @@ impl TryFrom<RawFont> for Font {
                 hhea_ascender: m.custom_parameters.int("hheaAscender"),
                 hhea_descender: m.custom_parameters.int("hheaDescender"),
                 hhea_line_gap: m.custom_parameters.int("hheaLineGap"),
+                underline_thickness: m.custom_parameters.float("underlineThickness"),
+                underline_position: m.custom_parameters.float("underlinePosition"),
             })
             .collect();
 

diff --git a/glyphs2fontir/src/source.rs b/glyphs2fontir/src/source.rs
@@ -385,10 +385,6 @@ impl Work<Context, WorkId, WorkError> for StaticMetadataWork {
         // Default per <https://github.com/googlefonts/glyphsLib/blob/cb8a4a914b0a33431f0a77f474bf57eec2f19bcc/Lib/glyphsLib/builder/custom_params.py#L1117-L1119>
         static_metadata.misc.fs_type = font.fs_type.or(Some(1 << 3));
 
-        // <https://github.com/googlefonts/glyphsLib/blob/main/Lib/glyphsLib/builder/custom_params.py#L1116-L1125>
-        static_metadata.misc.underline_thickness = 50.0.into();
-        static_metadata.misc.underline_position = (-100.0).into();
-
         static_metadata.misc.version_major = font.version_major;
         static_metadata.misc.version_minor = font.version_minor;
 
@@ -497,6 +493,16 @@ impl Work<Context, WorkId, WorkError> for GlobalMetricWork {
                 pos.clone(),
                 master.hhea_line_gap.map(|v| v as f64),
             );
+            metrics.set_if_some(
+                GlobalMetric::UnderlineThickness,
+                pos.clone(),
+                master.underline_thickness,
+            );
+            metrics.set_if_some(
+                GlobalMetric::UnderlinePosition,
+                pos.clone(),
+                master.underline_position,
+            )
         }
 
         context.global_metrics.set(metrics);
@@ -1427,6 +1433,8 @@ mod tests {
                 hhea_ascender: 1000.0.into(),
                 hhea_descender: (-200.0).into(),
                 hhea_line_gap: 0.0.into(),
+                underline_thickness: 50.0.into(),
+                underline_position: (-100.0).into(),
                 ..Default::default()
             },
             default_metrics
@@ -1442,20 +1450,6 @@ mod tests {
         );
     }
 
-    #[test]
-    fn default_underline_settings() {
-        let (_, context) = build_static_metadata(glyphs3_dir().join("Oswald-O.glyphs"));
-        let static_metadata = context.static_metadata.get();
-        assert_eq!(
-            (1000, 50.0, -100.0),
-            (
-                static_metadata.units_per_em,
-                static_metadata.misc.underline_thickness.0,
-                static_metadata.misc.underline_position.0
-            )
-        );
-    }
-
     #[test]
     fn build_glyph_contour_ir_containing_qcurves() {
         let glyph_name = "i";

diff --git a/ufo2fontir/src/source.rs b/ufo2fontir/src/source.rs
@@ -847,18 +847,6 @@ impl Work<Context, WorkId, WorkError> for StaticMetadataWork {
         // <https://github.com/googlefonts/glyphsLib/blob/cb8a4a914b0a33431f0a77f474bf57eec2f19bcc/Lib/glyphsLib/builder/custom_params.py#L1117-L1119>
         static_metadata.misc.fs_type = Some(1 << 2);
 
-        // <https://github.com/googlefonts/ufo2ft/blob/main/Lib/ufo2ft/fontInfoData.py#L313-L322>
-        static_metadata.misc.underline_thickness = font_info_at_default
-            .postscript_underline_thickness
-            .map(|v| v as f32)
-            .unwrap_or(0.05 * units_per_em as f32)
-            .into();
-        static_metadata.misc.underline_position = font_info_at_default
-            .postscript_underline_position
-            .map(|v| v as f32)
-            .unwrap_or(-0.075 * units_per_em as f32)
-            .into();
-
         static_metadata.misc.version_major = font_info_at_default
             .version_major
             .unwrap_or(static_metadata.misc.version_major);
@@ -923,6 +911,14 @@ impl Work<Context, WorkId, WorkError> for GlobalMetricsWork {
                 .map(|v| v as f32),
             static_metadata.italic_angle.into_inner(),
         );
+        // ufo2ft default underline position differs from fontir/Glyphs.app's so
+        // we override it here.
+        // https://github.com/googlefonts/ufo2ft/blob/a421acc/Lib/ufo2ft/fontInfoData.py#L319-L322
+        metrics.set(
+            GlobalMetric::UnderlinePosition,
+            static_metadata.default_location().clone(),
+            -0.075 * static_metadata.units_per_em as f32,
+        );
         for source in self
             .designspace
             .sources
@@ -993,6 +989,16 @@ impl Work<Context, WorkId, WorkError> for GlobalMetricsWork {
                 pos.clone(),
                 font_info.open_type_hhea_caret_offset.map(|v| v as f64),
             );
+            metrics.set_if_some(
+                GlobalMetric::UnderlineThickness,
+                pos.clone(),
+                font_info.postscript_underline_thickness,
+            );
+            metrics.set_if_some(
+                GlobalMetric::UnderlinePosition,
+                pos.clone(),
+                font_info.postscript_underline_position,
+            );
         }
 
         trace!("{:#?}", metrics);
@@ -1726,6 +1732,8 @@ mod tests {
                 hhea_ascender: 1194.0.into(),
                 hhea_descender: (-290.0).into(),
                 hhea_line_gap: 43.0.into(),
+                underline_thickness: 50.0.into(),
+                underline_position: (-75.0).into(),
                 ..Default::default()
             },
             default_metrics
@@ -1784,20 +1792,6 @@ mod tests {
         );
     }
 
-    #[test]
-    fn default_underline_settings() {
-        let (_, context) = build_static_metadata("wght_var.designspace", default_test_flags());
-        let static_metadata = &context.static_metadata.get();
-        assert_eq!(
-            (1000, 50.0, -75.0),
-            (
-                static_metadata.units_per_em,
-                static_metadata.misc.underline_thickness.0,
-                static_metadata.misc.underline_position.0
-            )
-        );
-    }
-
     #[test]
     fn groups_renamed_to_match_master() {
         let (_, context) = build_kerning("wght_var.designspace");