Skip to content

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