[fontbe/avar] don't skip no-op segment maps #586
Merged
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
GS contains a GRAD axis whose avar segment maps does not have any interesting mappings. It happens to be the last one in the axis order. We were filtering out axis segment maps that contain only identity maps, leading to an avar table with fewer segment maps than there are fvar axes, which is invalid as OTS correctly points out. So we now still emit these when bulding avar and there's at least one segment map that is not an identity map.
Note that, despite the OT spec allows for empty segment maps for axis that don't distort the default normalization, we want to match fontmake/fonttools and emit the required {-1:1, 0:0, 1:1} mappings even when no other interesting mappings are present for an axis.
Fixes #582 avar axis mismatch OTS error
after this GS avar table is the same when built with fontc vs fontmake.
cmyr
approved these changes
Nov 20, 2023
fontbe/src/avar.rs
Outdated
Comment on lines
25
to
31
| /// Return true if the SegmentMaps is not a boring identity map | ||
| fn is_interesting(segment_map: &SegmentMaps) -> bool { | ||
| segment_map | ||
| .axis_value_maps | ||
| .iter() | ||
| .any(|av| av.from_coordinate != av.to_coordinate) | ||
| } |
There was a problem hiding this comment.
I think this makes sense as a method on the SegmentMaps type in write-fonts? I'd probably flip the logic and call it is_identity.
One option would be to add the method there, merge this as-is, and open an issue to fix this when we next publish write-fonts.
There was a problem hiding this comment.
+1 making the method is_identity
| } | ||
|
|
||
| /// Return a default avar SegmentMaps containing the required {-1:-1, 0:0, 1:1} maps | ||
| fn default_segment_map() -> SegmentMaps { |
There was a problem hiding this comment.
and this feels like it should probably be the impl of Default for SegmentMaps; however we currently autogenerate Default impls for all write types, and there's no mechanism to override this, so this is probably fine.
Vigorous +1, it's helpful to us now and incredibly helpful to future editors |
we shall use the SegmentMaps instance method once that is added to write-fonts also, use then() instead of then_some() so it's lazily eval
anthrotype
added a commit
to googlefonts/fontations
that referenced
this pull request
Nov 21, 2023
as proposed in googlefonts/fontc#586 to be used when building avar table in fontc
This was referenced Nov 21, 2023
dfrg
pushed a commit
to googlefonts/fontations
that referenced
this pull request
Nov 21, 2023
as proposed in googlefonts/fontc#586 to be used when building avar table in fontc
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.
GS contains a
GRADaxis whose avar segment maps does not have any interesting mappings.It happens to be the last one in the axis order. We were filtering out axis segment maps that contain only identity maps, leading to an avar table with fewer segment maps than there are fvar axes, which is invalid as OTS correctly points out.
So we now still emit these non-interesting segment maps when bulding avar and there's at least one of them that is not an identity map.
Also note that, despite the OT spec allows for empty segment maps for axis that don't distort the default normalization, we want to match fontmake/fonttools and emit the required {-1:1, 0:0, 1:1} mappings even when no other interesting mappings are present for an axis, because apparently some old implementations need that (see fonttools/fonttools#1026).
Fixes #582 avar axis mismatch OTS error
I can confirm that after this, the GS avar table is the same when built with fontc vs fontmake.