[write-fonts] Fix miscalculation of ppf2 subgraph size #610
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rsheeter
reviewed
Sep 13, 2023
rsheeter
reviewed
Sep 13, 2023
rsheeter
reviewed
Sep 13, 2023
write-fonts/src/graph/splitting.rs
Outdated
| // let's have single class1class, and three class2 classes | ||
| // we want a duplicate varidx, a null varidx, and a device table? | ||
|
|
||
| fn make_val_record( |
There was a problem hiding this comment.
Optional, this would be easier to read hoisted out like make_class_def.
It's also hard to tell what the args are in the code. What if it took only the advance so this:
make_val_record(5, None),
make_val_record(6, DeviceOrVariationIndex::variation_index(4, 20)),read more like this:
val_with_xadvance(5),
val_with_xadvance(6).with_x_advance_device(DeviceOrVariationIndex::variation_index(4, 20)),make_class_def is similar, what's a class def 3,3,1?!
There was a problem hiding this comment.
This is defined inline since it's only used in this one test, whereas make_class_def is shared. :)
rsheeter
reviewed
Sep 13, 2023
rsheeter
reviewed
Sep 13, 2023
rsheeter
approved these changes
Sep 13, 2023
I had run into a problem here that I didn't have time to dig into last week. It turns out were incrementing the current index only when the subtable had not been seen before, which meant we would never advance the index and would not visit all subtables.
cmyr
force-pushed
the
fix-ppf2-device-size
branch
from
9dcf366 to
b7a2d28
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I had run into a probley friday afternoon that I didn't have time to dig into, and I didn't want to block on getting fontc working again. This fixes that.
It turns out were incrementing the current index only when the subtable had not been seen before, which meant we would never advance the index and would not visit all subtables.