-
Notifications
You must be signed in to change notification settings - Fork 55
Fix "Axis Mappings" interpretation #1040
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -187,10 +187,22 @@ def to_designspace_axes(self): | |
| ): | ||
| axis_wanted = True | ||
|
|
||
| # Build the mapping from the instances because they have both | ||
| # a user location and a design location; we build this here | ||
| # because we use it to check if a custom mapping needs to be inverted. | ||
| instance_mapping = {} | ||
| update_mapping_from_instances( | ||
| instance_mapping, | ||
| self.font.instances, | ||
| axis_def, | ||
| minimize_glyphs_diffs=self.minimize_glyphs_diffs, | ||
| ) | ||
|
|
||
| # See https://github.com/googlefonts/glyphsLib/issues/568 | ||
| if custom_mapping: | ||
| if axis.tag in custom_mapping: | ||
| mapping = {float(k): v for k, v in custom_mapping[axis.tag].items()} | ||
| mapping = _potentially_invert_mapping(self, mapping, instance_mapping) | ||
| regularDesignLoc = axis_def.get_design_loc(regular_master) | ||
| reverse_mapping = {dl: ul for ul, dl in sorted(mapping.items())} | ||
| regularUserLoc = piecewiseLinearMap(regularDesignLoc, reverse_mapping) | ||
|
|
@@ -230,16 +242,6 @@ def to_designspace_axes(self): | |
| regularDesignLoc = axis_def.get_design_loc(regular_master) | ||
| regularUserLoc = axis_def.get_user_loc(regular_master) | ||
| else: | ||
| # Build the mapping from the instances because they have both | ||
| # a user location and a design location. | ||
| instance_mapping = {} | ||
| update_mapping_from_instances( | ||
| instance_mapping, | ||
| self.font.instances, | ||
| axis_def, | ||
| minimize_glyphs_diffs=self.minimize_glyphs_diffs, | ||
| ) | ||
|
|
||
| master_mapping = {} | ||
| for master in self.font.masters: | ||
| # Glyphs masters don't have a user location | ||
|
|
@@ -322,6 +324,49 @@ def font_uses_axis_locations(font): | |
| ) | ||
|
|
||
|
|
||
| def _potentially_invert_mapping(self, mapping, instance_mapping): | ||
| # Fontmake introduced an Axis Mapping custom parameter before Glyphs did; | ||
| # then Glyphs introduced it, but with an inverted interpretation. Then | ||
| # Glyphs changed the interpretation to match fontmake's. Try and figure out | ||
| # what's going on. | ||
| # See https://github.com/googlefonts/glyphsLib/issues/745 | ||
|
|
||
| # We want to return a userspace->designspace map for our purposes | ||
| inverted = {v: k for k, v in mapping.items()} | ||
| if self.font.format_version != 3: | ||
| # Glyphs wasn't using this custom parameter, only we were | ||
| return mapping | ||
| # Let's get heuristical | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm i'm a bit skeptical we need to go "heuristical".. Can't we simply check the version and do accordingly? If the source file doesn't match the interpretation for the version it was saved with, we can say it's not our problem
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there were some fonts that would set it "wrong" in Glyphs to make it work with fontMake.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about fixing the actual sources instead? |
||
| if min(mapping.keys()) == min(mapping.values()) or max(mapping.keys()) == max( | ||
| mapping.values() | ||
| ): | ||
| # No way of knowing | ||
| logger.warning( | ||
| "Axis %s: The Axis Mappings custom parameter is ambiguous, " | ||
| "assuming it is in userspace->designspace format. Please check the mapping " | ||
| "and save in a recent version of Glyphs to avoid this warning." | ||
| ) | ||
| return mapping | ||
| if instance_mapping: | ||
| if min(mapping.keys()) == min(instance_mapping.keys()) and max( | ||
| mapping.keys() | ||
| ) == max(instance_mapping.keys()): | ||
| # Looks like userspace->designspace | ||
| return mapping | ||
| if min(mapping.keys()) == min(instance_mapping.values()) and max( | ||
| mapping.values() | ||
| ) == max(instance_mapping.keys()): | ||
| # Looks like designspace->userspace | ||
| return inverted | ||
| # No idea | ||
| logger.warning( | ||
| "Axis %s: The Axis Mappings custom parameter is ambiguous, " | ||
| "assuming it is in userspace->designspace format. Please check the mapping " | ||
| "and save in a recent version of Glyphs to avoid this warning." | ||
| ) | ||
| return mapping | ||
|
|
||
|
|
||
| def to_glyphs_axes(self): | ||
| axes_parameter = [] | ||
| for axis in self.designspace.axes: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -740,3 +740,47 @@ def test_virtual_masters_extend_min_max_for_unmapped_axis(ufo_module, datadir): | |
| assert [ | ||
| cp.value for cp in font2.customParameters if cp.name == "Virtual Master" | ||
| ] == virtual_masters | ||
|
|
||
|
|
||
| def test_axis_mappings_different_interpretations(ufo_module, datadir): | ||
| # https://github.com/googlefonts/glyphsLib/issues/859 | ||
| font = GSFont(datadir.join("gf", "Rubik.glyphs")) # Saved under Glyphs 3079 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this Rubik.glyphs seems to already have a correct user->design mapping even if it was saved with an old version:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, because it was a format 2 file. |
||
| expected_map = [ | ||
| (300.0, 60), | ||
| (400.0, 90), | ||
| (500.0, 125), | ||
| (600.0, 142), | ||
| (700.0, 160), | ||
| (800.0, 190), | ||
| (900.0, 220), | ||
| ] | ||
| assert "Axis Mappings" in font.customParameters | ||
| ds = to_designspace(font, ufo_module=ufo_module) | ||
| assert ds.axes[0].minimum == 300 | ||
| assert ds.axes[0].maximum == 900 | ||
| assert ds.axes[0].map == expected_map | ||
|
|
||
| font.format_version = 3 # Ambiguous, work out interpretation hueristically | ||
| ds = to_designspace(font, ufo_module=ufo_module) | ||
| assert ds.axes[0].minimum == 300 | ||
| assert ds.axes[0].maximum == 900 | ||
| assert ds.axes[0].map == expected_map | ||
|
|
||
| # Pretend we saved it under a new version | ||
| font.appVersion = 3217 | ||
| font.customParameters["Axis Mappings"] = { | ||
| "wght": { | ||
| 60: 300, | ||
| 90: 400, | ||
| 125: 500, | ||
| 142: 600, | ||
| 160: 700, | ||
| 190: 800, | ||
| 220: 900, | ||
| } | ||
| } | ||
|
|
||
| ds = to_designspace(font, ufo_module=ufo_module) | ||
| assert ds.axes[0].minimum == 300 | ||
| assert ds.axes[0].maximum == 900 | ||
| assert ds.axes[0].map == expected_map | ||
Uh oh!
There was an error while loading. Please reload this page.