googlefonts / glyphsLib

A bridge from Glyphs source files (.glyphs) to UFOs
Apache License 2.0
178 stars 51 forks source link

Must strictly separate RTL and LTR group kerning for glyphs: intended? #1004

Open madig opened 1 month ago

madig commented 1 month ago

Alternative title: possible kerning data loss when glyph is group-kerned in LTR and RTL mode

As far as I understand, the way glyphsLib folds RTL into LTR kerning is like this:

  1. Gather all glyphs touched by RTL kerning in the Glyphs.app file.
  2. When generating UFO kerning groups, for each glyph check if it is in the set generated in step 1. a. If not, put the glyph's (leftKerningGroup, rightKerningGroup) into the corresponding (public.kern1, public.kern2) group b. If so, put the glyph's (leftKerningGroup, rightKerningGroup) into the corresponding (public.kern2, public.kern1) group
  3. For each kerning pair in kerningRTL, flip the kerning group name (kern1 -> kern2 and vice versa) and then merge them into the regular UFO kerning dictionary

The either-or logic in step two implies that a glyph must be used either in LTR or RTL group kerning, but never in both. Glyph-to-glyph kerning has no such limitation because it obviously works without groups.

This means that neutral glyphs like period should, if they are put in a group, kerned only in LTR or in RTL mode, but not both, because currently the LTR kerning will point at the wrong kerning name.

Some test cases:

Note how the presence of RTL kerning flips the side of the period_right and period_left group, as well as the alef group. Also note how the first kerning between a and period_left gets voided because there is no public.kern2.period_left anymore. glyphs2ufo indeed logs a warning saying "Non-existent glyph class public.kern2.period_left found in kerning rules".

Do I understand correctly that this is a known limitation of folding RTL into LTR kerning and that group kerning must be strictly separated into LTR and RTL? I.e. if you want to group-kern period, you should make a period-ar for RTL and group-kern that instead?

madig commented 1 month ago

FWIW, the mekkablue conversion script is behaving in the same way with the same possible loss of kerning.

madig commented 1 month ago

I found an interdasting detail where I'm not sure how intentional it was. In https://github.com/googlefonts/glyphsLib/blob/cedaacdc2bd17e879c3332b30e567ce5bafa0f37/Lib/glyphsLib/builder/kerning.py#L46, a flipped class will overwrite any unflipped class that was there before from LTR kerning. I.e. if you have a pair like (public.kern1.one, public.kern2.b), and you also group-kerned one against some RTL glyph, you'd get an RTL pair like (public.kern1.one, public.kern2.hah), the highlighted code would drop all LTR kerning pairs starting with public.kern1.one in favor of RTL pairs starting with the same group name. You can see this in Kerning.zip where the LTR pair (public.kern1.one, public.kern2.b): -121 is dropped from the resulting UFO.