[subset] Add more variable font subsetting tests #2912
Conversation
garretrieger
changed the title
That's definitely not what your code does. "conditions" vs "substitutions". Which can be dropped is non-trivial. See: #2334 (comment) Since we are not instancing, the Conditions don't really change... |
|
Good catch, I'll rework this. |
garretrieger
force-pushed
the
vf_tests
branch
2 times, most recently
from
37f5f55
to
eaa3240
Compare
| @@ -1117,8 +1117,9 @@ struct Feature | |||
| ; | |||
|
|
|||
| out->lookupIndex.serialize (c->serializer, l, it); | |||
| return_trace (bool (it) || subset_featureParams | |||
| || (tag && *tag == HB_TAG ('p', 'r', 'e', 'f'))); | |||
There was a problem hiding this comment.
What happens to this pref case? There was a reason we added that. Breaks shaping with certain scripts (Khmer?) I think. Probably fonttools doesn't have it?
There was a problem hiding this comment.
I'll look into it. If we want this then we'll need to check for it during feature collection since if we drop a feature here that the feature collection was planning to keep the remapped feature indices get out of sync with the actual list of subsetted features.
There was a problem hiding this comment.
If we want this then we'll need to check for it during feature collection
Right.
There was a problem hiding this comment.
Oh misread that, this always keeps "pref" which fontTools does too: https://github.com/fonttools/fonttools/blob/main/Lib/fontTools/subset/__init__.py#L1280. I'll check the feature collection code to make sure it's doing the same.
There was a problem hiding this comment.
I'll fix this in a separate PR, this change won't make the current behaviour any worse since this method now always returns true.
Only drop records with no matching features that are at the end of the list. See: fonttools/fonttools@cab7d13
|
Sure. I trust that you would. :) LGTM. |
Fix a couple issues that popped up: