Skip to content

Commit

Permalink
[glyphs] Parse kerning as floats
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
cmyr committed Nov 28, 2024
1 parent 80cc208 commit a99597e
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 15 deletions.
38 changes: 24 additions & 14 deletions glyphs-reader/src/font.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,18 +98,20 @@ 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)
}

pub fn keys(&self) -> impl Iterator<Item = &String> {
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()
}

Expand All @@ -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();
}
}

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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!(
Expand All @@ -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),
Expand All @@ -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();
Expand Down Expand Up @@ -3569,7 +3580,6 @@ mod tests {
Some(&OrderedFloat(0f64))
);
}

#[test]
fn read_font_metrics() {
let font =
Expand Down
2 changes: 1 addition & 1 deletion glyphs2fontir/src/source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
28 changes: 28 additions & 0 deletions resources/testdata/glyphs3/KernFloats.glyphs
Original file line number Diff line number Diff line change
@@ -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;
};
};
};
}

0 comments on commit a99597e

Please sign in to comment.