Skip to content
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

Fix "Axis Mappings" interpretation #1040

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 55 additions & 10 deletions Lib/glyphsLib/builder/axes.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
simoncozens marked this conversation as resolved.
Show resolved Hide resolved
# 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
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly.

Copy link
Member

Choose a reason for hiding this comment

The 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:
Expand Down
44 changes: 44 additions & 0 deletions tests/builder/axes_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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:

https://github.com/googlefonts/glyphsLib/blob/1cb4fc5ae2cf385df95d2b7768e7ab4eb60a5ac3/tests/data/gf/Rubik.glyphs#L49-L59

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
Loading