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

[subset] FeatureVariations subsetting is wrong #2334

Closed
behdad opened this issue Apr 17, 2020 · 7 comments · Fixed by #2352
Closed

[subset] FeatureVariations subsetting is wrong #2334

behdad opened this issue Apr 17, 2020 · 7 comments · Fixed by #2352

Comments

@behdad
Copy link
Member

behdad commented Apr 17, 2020

We cannot drop FeatureVariationsRecords that have empty substitutions. That changes the behavior. A FeatureVariationsRecords stops the search when the conditions match; so even if there's no susbstitutions we cannot drop the record as that will keep the search going.

The only optimizations we can do are 1. dropping from the end of the array any records with no susbtitutions, and 2. dropping records earlier with no susbstitution only after analysing the conditions and proving that the dropping does not change functionality.

Both of those are unnecessary right now. We should fix the functionality.

This seems to be wrong in FontTools as well: https://github.com/fonttools/fonttools/blob/master/Lib/fontTools/subset/__init__.py#L1325

cc @anthrotype @qxliu76

@behdad
Copy link
Member Author

behdad commented Apr 17, 2020

@qxliu76
Copy link
Collaborator

qxliu76 commented Apr 20, 2020

@anthrotype please let me know when this is fixed in fonttools, I'll then fix it in harfbuzz

@ebraminio
Copy link
Collaborator

@qxliu76 and colleagues, hadn't the chance to state this somewhere else, thanks for all the tireless efforts you've put on implementing this and other in progress layout subset works, hopefully we will soon fix all the mentioned issues providing layout subset as a quality works to the users. Thanks again :)

@behdad
Copy link
Member Author

behdad commented Apr 20, 2020

Here's fonttools fix. We don't need to implement the pruning here initially. fonttools/fonttools#1882

behdad added a commit that referenced this issue Apr 20, 2020
Never drop FeatureVariationRecord for now.

Fixes #2334
ebraminio pushed a commit that referenced this issue Apr 20, 2020
Never drop FeatureVariationRecord for now.

Fixes #2334
@qxliu76
Copy link
Collaborator

qxliu76 commented Apr 20, 2020

Now fonttools is pruning empty record at the end, while harfbuzz is not doing the same thing. Is it an issue in testing?

@qxliu76
Copy link
Collaborator

qxliu76 commented Apr 20, 2020

@ebraminio, thank you!

@behdad
Copy link
Member Author

behdad commented Apr 22, 2020

Now fonttools is pruning empty record at the end, while harfbuzz is not doing the same thing. Is it an issue in testing?

We'll deal with that when we get to.

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 a pull request may close this issue.

3 participants