From a99597e7875032b352b84255812b053bfbb49fb6 Mon Sep 17 00:00:00 2001
From: Colin Rofls <colin@cmyr.net>
Date: Tue, 26 Nov 2024 14:18:18 -0500
Subject: [PATCH] [glyphs] Parse kerning as floats

Glyphs kern values can be floats, but we were parsing them as integers.

This also uncovered something else fishy: our integer parsing logic in
glyps-reader/ascii-plist-derive will happy parse something like "1.321"
as an integer, by parsing to a float and casting.

I think this is incorrect? I think it is reasonable to go the other way
(to accept an integer value as a float) and it _might_ be reasonable to
accept an integer value with no fractional component as a float, but I
definitely don't think we should parse a float with a fractional value
as an integer; it hides bugs like this.
---
 glyphs-reader/src/font.rs                    | 38 ++++++++++++--------
 glyphs2fontir/src/source.rs                  |  2 +-
 resources/testdata/glyphs3/KernFloats.glyphs | 28 +++++++++++++++
 3 files changed, 53 insertions(+), 15 deletions(-)
 create mode 100644 resources/testdata/glyphs3/KernFloats.glyphs

diff --git a/glyphs-reader/src/font.rs b/glyphs-reader/src/font.rs
index 255fb652..d9811b43 100644
--- a/glyphs-reader/src/font.rs
+++ b/glyphs-reader/src/font.rs
@@ -98,10 +98,10 @@ pub struct FontCustomParameters {
 
 /// master id => { (name or class, name or class) => adjustment }
 #[derive(Clone, Debug, Default, PartialEq, Hash)]
-pub struct Kerning(BTreeMap<String, BTreeMap<(String, String), i32>>);
+pub struct Kerning(BTreeMap<String, BTreeMap<(String, String), OrderedFloat<f64>>>);
 
 impl Kerning {
-    pub fn get(&self, master_id: &str) -> Option<&BTreeMap<(String, String), i32>> {
+    pub fn get(&self, master_id: &str) -> Option<&BTreeMap<(String, String), OrderedFloat<f64>>> {
         self.0.get(master_id)
     }
 
@@ -109,7 +109,9 @@ impl Kerning {
         self.0.keys()
     }
 
-    pub fn iter(&self) -> impl Iterator<Item = (&String, &BTreeMap<(String, String), i32>)> {
+    pub fn iter(
+        &self,
+    ) -> impl Iterator<Item = (&String, &BTreeMap<(String, String), OrderedFloat<f64>>)> {
         self.0.iter()
     }
 
@@ -118,14 +120,14 @@ impl Kerning {
         master_id: String,
         lhs_class_or_group: String,
         rhs_class_or_group: String,
-        kern: i64,
+        kern: f64,
     ) {
         *self
             .0
             .entry(master_id)
             .or_default()
             .entry((lhs_class_or_group, rhs_class_or_group))
-            .or_default() = kern as i32;
+            .or_default() = kern.into();
     }
 }
 
@@ -163,7 +165,7 @@ impl FromPlist for Kerning {
 
                     let rhs_name_or_class: String = tokenizer.parse()?;
                     tokenizer.eat(b'=')?;
-                    let value: i64 = tokenizer.parse()?;
+                    let value: f64 = tokenizer.parse()?;
                     tokenizer.eat(b';')?;
 
                     kerning.insert(
@@ -3384,7 +3386,7 @@ mod tests {
             .get("m01")
             .unwrap()
             .iter()
-            .map(|((n1, n2), value)| (n1.as_str(), n2.as_str(), *value))
+            .map(|((n1, n2), value)| (n1.as_str(), n2.as_str(), value.0))
             .collect::<Vec<_>>();
 
         assert_eq!(
@@ -3398,12 +3400,12 @@ mod tests {
                     ),
                 ],
                 vec![
-                    ("@MMK_L_bracketleft_R", "exclam", -165),
-                    ("bracketleft", "bracketright", -300),
-                    ("exclam", "@MMK_R_bracketright_L", -160),
-                    ("exclam", "exclam", -360),
-                    ("exclam", "hyphen", 20),
-                    ("hyphen", "hyphen", -150),
+                    ("@MMK_L_bracketleft_R", "exclam", -165.),
+                    ("bracketleft", "bracketright", -300.),
+                    ("exclam", "@MMK_R_bracketright_L", -160.),
+                    ("exclam", "exclam", -360.),
+                    ("exclam", "hyphen", 20.),
+                    ("hyphen", "hyphen", -150.),
                 ],
             ),
             (actual_groups, actual_kerning),
@@ -3412,6 +3414,15 @@ mod tests {
         );
     }
 
+    #[test]
+    fn kern_floats() {
+        let font = Font::load(&glyphs3_dir().join("KernFloats.glyphs")).unwrap();
+
+        let kerns = font.kerning_ltr.get("m01").unwrap();
+        let key = ("space".to_string(), "space".to_string());
+        assert_eq!(kerns.get(&key), Some(&OrderedFloat(4.2001)));
+    }
+
     #[test]
     fn read_simple_anchor() {
         let font = Font::load(&glyphs3_dir().join("WghtVar_Anchors.glyphs")).unwrap();
@@ -3569,7 +3580,6 @@ mod tests {
             Some(&OrderedFloat(0f64))
         );
     }
-
     #[test]
     fn read_font_metrics() {
         let font =
diff --git a/glyphs2fontir/src/source.rs b/glyphs2fontir/src/source.rs
index d3bfce9f..55ba4cd7 100644
--- a/glyphs2fontir/src/source.rs
+++ b/glyphs2fontir/src/source.rs
@@ -804,7 +804,7 @@ impl Work<Context, WorkId, Error> for KerningInstanceWork {
                 Some(((side1, side2), pos_adjust))
             })
             .for_each(|(participants, (_, value))| {
-                *kerning.kerns.entry(participants).or_default() = (value as f32).into();
+                *kerning.kerns.entry(participants).or_default() = (value.0 as f32).into();
             });
 
         context.kerning_at.set(kerning);
diff --git a/resources/testdata/glyphs3/KernFloats.glyphs b/resources/testdata/glyphs3/KernFloats.glyphs
new file mode 100644
index 00000000..aa5e05e2
--- /dev/null
+++ b/resources/testdata/glyphs3/KernFloats.glyphs
@@ -0,0 +1,28 @@
+{
+.formatVersion = 3;
+familyName = KernFloats;
+fontMaster = (
+{
+id = m01;
+}
+);
+glyphs = (
+{
+glyphname = space;
+layers = (
+{
+layerId = m01;
+width = 200;
+}
+);
+}
+);
+unitsPerEm = 1000;
+kerning = {
+    "m01" = {
+        space = {
+        space = 4.2001;
+        };
+    };
+};
+}