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

[makeotfexe] Bad font built when feature file has feature parameters, issue 1080 #1081

Merged
merged 1 commit into from
Jan 17, 2020

Conversation

readroberts
Copy link
Contributor

Fixed bug where a bad font built when feature file has multiple languagesystems and feature parameters #1080

Updated test file to fail the build tests with in versions of makeotf with this bug, and to pass in versions without this bug, by changing from one to three language system statements, and updating the expected ttx file.

…tiple languagesystems and feature parameters #1080

Updated test file to fail the build tests with in versions of makeotf with this bug, and to pass in versions without this bug, by changing from one to three language system statements, and updating the expected ttx file.

This bug  was introduced in commit 5624e02, when fixing how feature parameter tables were ordered in the GSUB/GPOS tables. The bug in in otl.c after line 2035, where bug was in incrementing nFeatureParams for every feature param subtable, instead of skipping the ones that were just references. Fixed by reverting to the original code block for this. The wrong code was left over from my first pass at changing with the subtable organization, and I forgot to revert this code when I pulled that out and used a different approach.
@readroberts readroberts requested review from cjchapman and josh-hadley and removed request for cjchapman and josh-hadley January 17, 2020 19:47
Copy link
Collaborator

@josh-hadley josh-hadley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good; confirmed this works with additional tests, too. Thanks, Read!

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