Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add read support for fvar table #183

Merged
merged 5 commits into from
Dec 16, 2022
Merged

Add read support for fvar table #183

merged 5 commits into from
Dec 16, 2022

Conversation

dfrg
Copy link
Member

@dfrg dfrg commented Dec 16, 2022

This adds the fvar table. Support for the variable size InstanceRecord is handled similarly to ValueRecord using ComputedArray and a custom implementation FontReadWithArgs along with traversal stuff.

Also adds otexplorer support for fvar and avar.

Contains tests for reading axes and instances as well as computing normalized coordinates.

dfrg added 4 commits December 16, 2022 15:00
This adds the `fvar` table. Support for the variable size `InstanceRecord` is handled similarly to `ValueRecord` using `ComputedArray` and a custom implementation `FontReadWithArgs` along with custom traversal stuff.

Contains tests for reading axes and instances as well as computing normalized coordinates.
Copy link
Member

@cmyr cmyr 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, two little notes inline.

@@ -0,0 +1,70 @@
//! An fvar InstanceRecord

use super::InstanceRecord;
Copy link
Member

Choose a reason for hiding this comment

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

any reason not to define the type in this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I think I had done everything in fvar.rs before I noticed the pattern you used with ValueRecord and just didn't copy the base type over. I'll update it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out the fields on that type weren't pub either, so I'm glad you called this out :)

axis_count: u16,
/// The size in bytes of each VariationAxisRecord — set to 20 (0x0014) for this version.
#[compile(20)]
axis_size: u16,
Copy link
Member

Choose a reason for hiding this comment

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

in theory, I imagine we want to be using this value to determine the stride length during parsing, or else we risk incorrect results with some future spec bump. There's a similar issue in STAT, where it is also not resolved; I'm okay with leaving it as-is for now, but we should definitely open an issue.

I have a half-written patch for STAT somewhere, so hopefully the pattern I use there will also be appropriate here.

aside: the idea of a record that may change size in the future is nasty, I feel like this should really just be a table with a version number. :/

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, and I think the spec is completely broken here. I honestly don't see how they can change either of the sizes in this table without breaking just about every implementation in the wild.

I'll keep this as-is and let you carry over whatever pattern you're considering for STAT when compilation comes up for this table.

Also make the fields public
@dfrg dfrg merged commit 3f461a5 into googlefonts:main Dec 16, 2022
@dfrg dfrg deleted the fvar branch December 16, 2022 22:24
@cmyr cmyr mentioned this pull request Mar 15, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants