[glyphs-reader] instances Axis Locations override masters' (#1866)#1964
Merged
anthrotype merged 3 commits intomainfrom Apr 16, 2026
Merged
[glyphs-reader] instances Axis Locations override masters' (#1866)#1964anthrotype merged 3 commits intomainfrom
anthrotype merged 3 commits intomainfrom
Conversation
When a font uses Axis Location custom params on both masters and instances, an instance can legitimately redefine a master's user→design mapping. Previously `add_if_new()` silently dropped the instance entry if the user value was already present (from the master), producing wrong output for fonts like SUSE-Italic (ExtraBold user=800 should map to design=780, not 800). Switch to last-wins overwrite semantics everywhere, matching glyphsLib's `mapping[userLoc] = designLoc`. Both the master Axis Location path and the instance mapping path now use `add_or_replace` (with a log::warn when overwriting a different value). Removed `add_any_new`, no longer needed. Rename `add_instance_mappings_if_new` => `add_instance_mappings` since it now always overwrites. Add an integration test with a minimal .glyphs fixture modeled on SUSE-Italic that compiles end-to-end and checks the avar output.
Member
Author
|
The SUSE-Italic.glyphs |
Member
Author
|
oh looks like I had missed a few other broken test fixtures that don't declare their weightClass #1963 i'm gonna commit them here if you don't mind |
cmyr
approved these changes
Apr 16, 2026
Member
cmyr
left a comment
cmyr
left a comment
There was a problem hiding this comment.
looks good, a couple of things inline :)
add_or_replace now returns the previous design value when overwriting, instead of taking axis_name/source_label strings for a log::warn. Callers format the warning with their own context. as per Colin's review #1964 (comment)
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
When a font uses Axis Location custom params on both masters and instances, an instance can legitimately redefine a master's user→design mapping. Previously
add_if_new()silently dropped the instance entry if the user value was already present (from the master), producing wrong output for fonts like SUSE-Italic (ExtraBold user=800 should map to design=780, not 800).Switch to last-wins overwrite semantics everywhere, matching glyphsLib's
mapping[userLoc] = designLoc.https://github.com/googlefonts/glyphsLib/blob/71f487f9d24e/Lib/glyphsLib/builder/axes.py#L221
https://github.com/googlefonts/glyphsLib/blob/71f487f9d24e/Lib/glyphsLib/builder/axes.py#L153
Both the master Axis Location path and the instance mapping path now use
add_or_replace(with a log::warn when overwriting a different value). Removedadd_any_new, no longer needed.Rename
add_instance_mappings_if_new=>add_instance_mappingssince it now always overwrites.Add an integration test with a minimal .glyphs fixture modeled on SUSE-Italic that compiles end-to-end and checks the avar output.
Fixes #1866