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

Fixes issue #340 #341

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fixes issue #340 #341

wants to merge 3 commits into from

Conversation

msueyoshi-m
Copy link

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.

@blikblum
Copy link
Member

Can you provide a test that fails before the PR and passes after?

@Harbs
Copy link

Harbs commented Jul 30, 2024

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

@Harbs
Copy link

Harbs commented Jul 30, 2024

The other change does seem correct. advanceHeightMax is a UFWORD which is a uint16.

https://learn.microsoft.com/en-us/typography/opentype/spec/otff

@Harbs
Copy link

Harbs commented Jul 30, 2024

IMO, the first change should be:

  version:                r.fixed32,  // Version number of the Vertical Header Table

@msueyoshi-m
Copy link
Author

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.
The maxp table version is specified as Version16Dot16 in the Document, but the structure is defined as int32 in the program. Therefore, I determined that defining Version16Dot16 as int32 would be better.

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.

@Harbs
Copy link

Harbs commented Jul 30, 2024

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.

@Harbs
Copy link

Harbs commented Jul 30, 2024

That doesn't change the parsing of the rest of the table, but the version should be read correctly.

@msueyoshi-m
Copy link
Author

Thank you for the feedback. I’ve made the requested changes.

@Harbs
Copy link

Harbs commented Jul 30, 2024

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.

@msueyoshi-m
Copy link
Author

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.

@msueyoshi-m
Copy link
Author

@blikblum
Added a test in vhea.js to verify the bug fix for issue #340. The test fails before the PR and passes after the PR.

@blikblum
Copy link
Member

LGTM

@devongovett can you look at this?

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.

3 participants