[feaLib] More efficient inline multisub compilation #3770
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anthrotype
reviewed
Feb 14, 2025
Lib/fontTools/otlLib/builder.py
Outdated
| lookup.mapping[glyph] = replacements | ||
| rule.glyphs[0].add(glyph) | ||
|
|
||
| print("reused a mapping") |
There was a problem hiding this comment.
remove stray print - or if you like to keep, turn it into a log.debug perhaps
Specifically we were always creating a new ChainContextualRule for each glyph in an input class, even when their replacements were identical, which means we would later miss the chance to use a format3 encoding, which would have been smaller. The implementation here feels... fine, I guess? In fea-rs we don't decompose the input set, but we also don't support the syntax, `sub a [b c]' d by e f` which is supported by fontmake (but has not yet appeared in any font we've handled)
d141f40 to
eda024c
Compare
|
huh let's hold off on this for now, I'm seeing some regressions on crater that I will need to investigate.. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Specifically we were always creating a new ChainContextualRule for each glyph in an input class, even when their replacements were identical, which means we would later miss the chance to use a format3 encoding, which would have been smaller.
The implementation here feels... fine, I guess? In fea-rs we don't decompose the input set, but we also don't support the syntax,
sub a [b c]' d by e fwhich is supported by fontmake (but has not yet appeared in any font we've handled) which might motivate the decomposition.