-
Notifications
You must be signed in to change notification settings - Fork 223
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
Fixes issue #340 #341
base: master
Are you sure you want to change the base?
Fixes issue #340 #341
Conversation
Can you provide a test that fails before the PR and passes after? |
The change doesn't even look correct. Version should be two uint16 values and not a single int32. https://learn.microsoft.com/en-us/typography/opentype/spec/vhea |
The other change does seem correct. https://learn.microsoft.com/en-us/typography/opentype/spec/otff |
IMO, the first change should be:
|
Since hhea and vhea have the same structure, I believed it would be clearer to present them consistently in the program, even though they are described differently in the specification. For testing, use test/data/NotoSansCJK/NotoSansCJKkr-Regular.otf. Any font that includes a vhea table will work. Before the PR, vhea.numberOfMetrics is set to 0, which causes the Glyph object's advanceHeight to be 0 for all glyphs. |
I'm pretty sure you will get the wrong version. The version should resolve to a string: major.minor. Defining it as an int32 should cause it to resolve to a non-sensical number. |
That doesn't change the parsing of the rest of the table, but the version should be read correctly. |
Thank you for the feedback. I’ve made the requested changes. |
Cool. To be clear: I'm just some random guy who happens to like this project saying things as I understand them. 😄 I have no control over accepting the PR. If you have a way of adding tests for this fix, it'll probably make it easier for those who can accept it to merge the PR. |
My child has caught a cold, so I am unable to respond immediately. However, I believe I will be able to add the test code to the test folder within a few days. |
LGTM @devongovett can you look at this? |
The vhea table can now be read properly. As a result, the vmtx table information can also be taken correctly and the advanceHeight can be obtained. I have confirmed that there are no problems with Japanese fonts.