googlefonts / ufo2ft

A bridge from UFOs to FontTools objects (and therefore, OTFs and TTFs).
MIT License
152 stars 43 forks source link

[kernFeatureWriter] ignore zero-valued class kern pairs when building variable kern #866

Closed anthrotype closed 3 months ago

anthrotype commented 3 months ago

We already prune class-class kerning pairs whose value is 0 when building individual per-master GPOS tables (to be merged with varLib). It makes sense to also do that when we build a single variable GPOS table. This is safe to do because we always place the class-class kerning as the last (after glyph-glyph kerns), so these don't override anything else that may follow and 0 is the default anyway, so they are essentially no-op.

khaledhosny commented 3 months ago

I used to insert these to ensure I always get the same number of lookups across masters (since the kern feature writer can produce a different number of lookups when multiple scripts or directions are involved) to keep varLib.merger from failing. This wouldn’t matter when we are directly building variable feature, right?

anthrotype commented 3 months ago

I used to insert these to ensure I always get the same number of lookups across masters

are you sure that works for class-class 0-valued kern pairs? they are currently dropped, see getKerningPairs method below (from main):

https://github.com/googlefonts/ufo2ft/blob/27f7e9b29fc38e05a4b7ce21bff5afa8fd6c2fba/Lib/ufo2ft/featureWriters/kernFeatureWriter.py#L427-L431

This wouldn’t matter when we are directly building variable feature, right?

yeah, that should not be a problem in this case

khaledhosny commented 3 months ago

I used to insert these to ensure I always get the same number of lookups across masters

are you sure that works for class-class 0-valued kern pairs? they are currently dropped, see getKerningPairs method below (from main):

https://github.com/googlefonts/ufo2ft/blob/27f7e9b29fc38e05a4b7ce21bff5afa8fd6c2fba/Lib/ufo2ft/featureWriters/kernFeatureWriter.py#L427-L431

That as a few years ago, so my memory is fuzzy.

anthrotype commented 3 months ago

this PR is basically doing the same thing that getKerningPairs does, but for getVariableKerningPairs as well