mathKerning is wrong in many ways

[behdad]

All in _processMathOne(). Two major issues I see:

• If a pair is not available in one of the sets, it assumes 0, whereas it should instead use the effective kerning value for that pair, which might come from a glyph-class, class-glyph, or class-class. This one is not too hard to fix.
• If the two sides have different groups, it arbitrarily merges them. Instead, we should classify glyphs into new groups and translate kerns over. I started https://github.com/behdad/fonttools/blob/master/Lib/fontTools/misc/classifyTools.py for that purpose, though I want to extend it to support named sets.

[behdad]

cc @LettError @anthrotype @typemytype

[typesupply]

If a pair is not available in one of the sets, it assumes 0, whereas it should instead use the effective kerning value for that pair, which might come from a glyph-class, class-glyph, or class-class. This one is not too hard to fix.

Yikes. It shouldn't be that way.

If the two sides have different groups, it arbitrarily merges them. Instead, we should classify glyphs into new groups and translate kerns over.

Hm. I'll look into it when I have time to think through that logic again. Test cases would be really helpful if you can post some here.

[behdad]

If a pair is not available in one of the sets, it assumes 0, whereas it should instead use the effective kerning value for that pair, which might come from a glyph-class, class-glyph, or class-class. This one is not too hard to fix.
Yikes. It shouldn't be that way.

This one is easy to fix. I'll give it a try later this week.

If the two sides have different groups, it arbitrarily merges them. Instead, we should classify glyphs into new groups and translate kerns over.
Hm. I'll look into it when I have time to think through that logic again. Test cases would be really helpful if you can post some here.

I also want to give it a try fixing. Will keep you posted.

[Erik says hi as well!]

[typesupply]

If a pair is not available in one of the sets, it assumes 0, whereas it should instead use the effective kerning value for that pair, which might come from a glyph-class, class-glyph, or class-class. This one is not too hard to fix.

I pulled the value finding code from MetricsMachine and add it as an example algorithm to the UFO 3 spec. Look at the "Kerning Value Lookup Algorithm" here. I've been thinking about making (a slightly more efficient version of) this code available in a central location for years. I think ufoLib is the best candidate. I don't think this would create a dependency loop because ufoLib relies on any modules that would need this. fontMath has an existing ufoLib dependency so that wouldn't be an issue.

If you'd like, I can do this tonight or tomorrow.

[typesupply]

If a pair is not available in one of the sets, it assumes 0, whereas it should instead use the effective kerning value for that pair, which might come from a glyph-class, class-glyph, or class-class. This one is not too hard to fix.

@behdad I'm looking into this issue, but I'm not able to reproduce it. Here is my test code:

from fontMath.mathKerning import MathKerning

groups = {
    "public.kern1.A" : ["A", "A.alt"],
    "public.kern2.O" : ["O", "O.alt"]
}
kerning1 = {
    ("public.kern1.A", "public.kern2.O") : 100
}
kerning2 = {
    ("public.kern1.A", "public.kern2.O") : 200
}

kerning1 = MathKerning(kerning1, groups)
kerning2 = MathKerning(kerning2, groups)

kerning3 = kerning1 + kerning2

print kerning3["A.alt", "O.alt"]

This gives me the expected value 300, not 0.

I see a reference to a predefined value of 0 in __getitem__ but it is a fallback if the various glyph and class combinations aren't defined.

Was the error somewhere else?

[behdad]

Ok, my bad. The first issue does not exist. I didn't note that the code is using "self.get()" which properly does fallback. So, the only issue outstanding is the fact that if the two sides have different groups, then they are merged wrongly as far as I see.

I'll send a PR to add test for the first issue.

[benkiel]

@behdad should this be closed?

[behdad]

@behdad should this be closed?

No, I think the following is still outstanding: if the two sides have different groups, then they are merged wrongly as far as I see.

[LettError]

Fontmath Kerning really needs all participants to have the same groups. Erik

…

On 2 Feb 2018, at 21:38, Behdad Esfahbod ***@***.***> wrote: @behdad should this be closed? No, I think the following is still outstanding: if the two sides have different groups, then they are merged wrongly as far as I see. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[typesupply]

Fontmath Kerning really needs all participants to have the same groups.

Perhaps this should be the fix for this issue. I just reviewed the code and it's much simpler than I remembered. It simply combines them without any checking for conflicts. I vaguely remember trying to solve this about 15 years ago in v0.001a1 but I probably got lost in the edge cases. If someone wants to take a shot at the statistical work on algorithmically resolving group conflicts, go for it. Otherwise, I think we should add something like this to the start of _processMathOne:

g1 = self.groups()
g2 = other.groups()
if g1 != g2:
    raise SomeKindOfError("Kerning groups must be exactly the same.")

[behdad]

Yes, error would work.

[madig]

I just found that in

fontMath/Lib/fontMath/mathKerning.py

Lines 229 to 232 in b79e82b

	def round(self, multiple=1):
	multiple = float(multiple)
	for k, v in self._kerning.items():
	self._kerning[k] = int(round2(int(round2(v / multiple)) * multiple))

, MathKerning.round() changes the instance's data instead of returning a rounded copy like MathGlyph and MathInfo. Is this intentional?