Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

anthrotype
Copy link
Member

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.

… variable kern

We already prune class-class kerning pairs when the 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
Copy link
Collaborator

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
Copy link
Member Author

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):

# Ignore zero-valued class kern pairs. They are the most general
# kerns, so they don't override anything else like glyph kerns would
# and zero is the default.
if firstIsClass and secondIsClass and value == 0:
continue

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

yeah, that should not be a problem in this case

@khaledhosny
Copy link
Collaborator

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):

# Ignore zero-valued class kern pairs. They are the most general
# kerns, so they don't override anything else like glyph kerns would
# and zero is the default.
if firstIsClass and secondIsClass and value == 0:
continue

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

@anthrotype
Copy link
Member Author

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

@anthrotype anthrotype merged commit 8ec3ef3 into main Aug 29, 2024
9 checks passed
@anthrotype anthrotype deleted the prune-zero-class-variable-kerns branch August 29, 2024 10:48
madig added a commit that referenced this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants