-
Notifications
You must be signed in to change notification settings - Fork 43
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
Split kerning by script, not by direction #636
Conversation
…re than one category
Given a pair and mapping between glyph names and a set of scripts, split the pair into (potentially) multiple pairs each with a dominant script.
allSecondScripts = {} | ||
for g in self.firstGlyphs: | ||
if g not in glyphScripts: | ||
glyphScripts[g] = set(["Zyyy"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a named constant for "Zyyy".
1bd006e
to
1e5a043
Compare
They are added to all other lookups as well, right? The main deficiency I see in this PR is lack of support for Script_Extensions data-file. That's not a huge deal though. If glyphs with script A and B are kerned and you insert that kern in both lookups for A and for B, that handles most of the cases already. |
No, but the common lookup is added to all script/language combinations
I think it does support this. <key>comma-ar</key>
<dict>
<key>gba-nko</key>
<integer>-120</integer>
<key>lam-ar</key>
<integer>-30</integer>
</dict>
<key>gba-nko</key>
<dict>
<key>gba-nko</key>
<integer>-20</integer>
</dict>
<key>lam-ar</key>
<dict>
<key>lam-ar</key>
<integer>50</integer>
</dict>
<key>three</key>
<dict>
<key>three</key>
<integer>-50</integer>
</dict> becomes lookup kern_Arab {
lookupflag IgnoreMarks;
pos comma-ar lam-ar <-30 0 -30 0>;
pos lam-ar lam-ar <50 0 50 0>;
} kern_Arab;
lookup kern_Nkoo {
lookupflag IgnoreMarks;
pos comma-ar gba-nko <-120 0 -120 0>;
pos gba-nko gba-nko <-20 0 -20 0>;
} kern_Nkoo;
lookup kern_Common {
lookupflag IgnoreMarks;
pos three three -50;
} kern_Common; (check out test_split_pair in the tests.) |
Thanks for the explanation. This is neat. LGTM! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with comments
thanks Simon for tacking this 👍
This reverts commit bdac61e. # Conflicts: # Lib/ufo2ft/featureWriters/kernFeatureWriter.py # Lib/ufo2ft/util.py
Revert "Split kerning by script, not by direction (#636)"
Currently we split kerning into lookups based on a single factor, horizontal direction. However, shaping engines will perform script segmentation and so there will never be any cross-script kerning. By splitting the kerning into lookups based on the script of the glyphs involved, we can produce smaller lookups for large multi-script fonts, hopefully causing less overflows (faster compilation) and reducing file space by giving the binary compiler a better starting point for splitting lookups into subtables.
There are a few failsafes, such as glyphs without identifiable scripts (as well as purely common-script glyphs) go into a "Common" pot which is added to DFLT/dflt.
This may be easiest to review commit by commit; the changes are fairly small and self-contained apart from f56eaf6 which is the big rewrite.