Repository: googlefonts/glyphsLib

Issue:
Axis mappings are missing in designspace
It seems that glyphsLib only adds axis mappings from the "Axis Location" custom parameter of masters, but not those of exports. Thus the avar table when building with fontmake/glyphsLib is different than when exporting the VF directly from Glyphs.

Discussion:

Patch:
diff --git a/Lib/glyphsLib/builder/axes.py b/Lib/glyphsLib/builder/axes.py
index cb724df9..cb3b7f2d 100644
--- a/Lib/glyphsLib/builder/axes.py
+++ b/Lib/glyphsLib/builder/axes.py
@@ -18,7 +18,7 @@ import logging
 from fontTools.varLib.models import piecewiseLinearMap
 
 from glyphsLib import classes
-from glyphsLib.classes import InstanceType, WEIGHT_CODES, WIDTH_CODES
+from glyphsLib.classes import WEIGHT_CODES, WIDTH_CODES, InstanceType
 from glyphsLib.builder.constants import WIDTH_CLASS_TO_VALUE
 
 logger = logging.getLogger(__name__)
@@ -125,6 +125,33 @@ def user_loc_value_to_instance_string(axis_tag, user_loc):
     )[0]
 
 
+def update_mapping_from_instances(
+    mapping, instances, axis_def, minimize_glyphs_diffs, cp_only=False
+):
+    # Collect the axis mappings from instances and update the mapping dict.
+    for instance in instances:
+        if instance.type == InstanceType.VARIABLE:
+            continue
+        if is_instance_active(instance) or minimize_glyphs_diffs:
+            designLoc = axis_def.get_design_loc(instance)
+            if cp_only:
+                # Only use the Axis Location custom parameter for the user location
+                userLoc = axis_def.get_user_loc_from_axis_location_cp(instance)
+            else:
+                # Use all heuristics to derive a user location
+                userLoc = axis_def.get_user_loc(instance)
+            if userLoc is None:
+                # May happen if the custom parameter is disabled
+                continue
+            if userLoc in mapping and mapping[userLoc] != designLoc:
+                logger.warning(
+                    f"Axis {axis_def.tag}: Instance '{instance.name}' redefines "
+                    f"the mapping for user location {userLoc} "
+                    f"from {mapping[userLoc]} to {designLoc}"
+                )
+            mapping[userLoc] = designLoc
+
+
 def to_designspace_axes(self):
     if not self.font.masters:
         return
@@ -165,40 +192,45 @@ def to_designspace_axes(self):
                 continue
         # See https://github.com/googlefonts/glyphsLib/issues/280
         elif font_uses_axis_locations(self.font):
-            # Build the mapping from the "Axis Location" of the masters
+            # If all masters have an "Axis Location" custom parameter, only the values
+            # from this parameter will be used to build the mapping of the masters and
+            # instances
             # TODO: (jany) use Virtual Masters as well?
+            #       (jenskutilek) virtual masters can't have an Axis Location parameter.
             mapping = {}
             for master in self.font.masters:
                 designLoc = axis_def.get_design_loc(master)
                 userLoc = axis_def.get_user_loc(master)
                 if userLoc in mapping and mapping[userLoc] != designLoc:
                     logger.warning(
-                        "Axis location (%s) was redefined by '%s'", userLoc, master.name
+                        f"Axis {axis_def.tag}: Master '{master.name}' redefines "
+                        f"the mapping for user location {userLoc} "
+                        f"from {mapping[userLoc]} to {designLoc}"
                     )
                 mapping[userLoc] = designLoc
 
+            update_mapping_from_instances(
+                mapping,
+                self.font.instances,
+                axis_def,
+                minimize_glyphs_diffs=self.minimize_glyphs_diffs,
+                # Glyphs doesn't deduce instance mappings if font uses axis locations.
+                # Use only the custom parameter if present.
+                cp_only=True,
+            )
+
             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 = {}
-            for instance in self.font.instances:
-                if (
-                    is_instance_active(instance) or self.minimize_glyphs_diffs
-                ) and instance.type != InstanceType.VARIABLE:
-                    designLoc = axis_def.get_design_loc(instance)
-                    userLoc = axis_def.get_user_loc(instance)
-                    if (
-                        userLoc in instance_mapping
-                        and instance_mapping[userLoc] != designLoc
-                    ):
-                        logger.warning(
-                            "Instance user-space location (%s) redefined by " "'%s'",
-                            userLoc,
-                            instance.name,
-                        )
-                    instance_mapping[userLoc] = designLoc
+            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:
@@ -320,8 +352,8 @@ class AxisDefinition:
 
     def get_user_loc(self, master_or_instance):
         """Get the user location of a Glyphs master or instance.
-        Masters in Glyphs can have a user location in the "Axis Location"
-        custom parameter.
+        Masters and instances in Glyphs can have a user location in the
+        "Axis Location" custom parameter.
 
         The user location is what the user sees on the slider in his
         variable-font-enabled UI. For weight it is a value between 0 and 1000,
@@ -362,22 +394,27 @@ class AxisDefinition:
             if class_ is not None:
                 user_loc = class_to_value(self.tag, class_)
 
-        # Masters have a customParameter that specifies a user location
-        # along custom axes. If this is present it takes precedence over
-        # everything else.
+        axis_location = self.get_user_loc_from_axis_location_cp(master_or_instance)
+        if axis_location is not None:
+            user_loc = axis_location
+
+        return user_loc
+
+    def get_user_loc_from_axis_location_cp(self, master_or_instance):
+        # Masters and instances have a customParameter that specifies a user location
+        # along custom axes. If this is present it takes precedence over everything
+        # else.
         loc_param = master_or_instance.customParameters["Axis Location"]
         try:
             for location in loc_param:
                 if location.get("Axis") == self.name:
-                    user_loc = int(location["Location"])
+                    return int(location["Location"])
         except (TypeError, KeyError):
             pass
 
-        return user_loc
-
     def set_user_loc(self, master_or_instance, value):
         """Set the user location of a Glyphs master or instance."""
-        if hasattr(master_or_instance, "instanceInterpolations"):
+        if isinstance(master_or_instance, classes.GSInstance):
             # The following code is only valid for instances.
             # Masters also the keys `weight` and `width` but they should not be
             # used, they are deprecated and should only be used to store
@@ -405,9 +442,8 @@ class AxisDefinition:
                         pass
             return
 
-        # For masters, set directly the custom parameter (old way)
-        # and also the Axis Location (new way).
-        # Only masters can have an 'Axis Location' parameter.
+        # Directly set the custom parameter (old way) and also the Axis Location
+        # (new way).
         if self.user_loc_param is not None:
             try:
                 class_ = user_loc_value_to_class(self.tag, value)


Test patch:
diff --git a/tests/builder/axes_test.py b/tests/builder/axes_test.py
index 99849293..4fd48915 100644
--- a/tests/builder/axes_test.py
+++ b/tests/builder/axes_test.py
@@ -20,7 +20,7 @@ import pytest
 
 from fontTools import designspaceLib
 from glyphsLib import to_glyphs, to_designspace, to_ufos
-from glyphsLib.classes import GSFont, GSFontMaster, GSAxis
+from glyphsLib.classes import GSFont, GSFontMaster, GSAxis, GSInstance
 from glyphsLib.builder.axes import _is_subset_of_default_axes, get_regular_master
 
 """
@@ -228,6 +228,255 @@ def test_mapping_is_same_regardless_of_axes_custom_parameter(ufo_module):
     assert doc.axes[0].map == []
 
 
+def test_mapping_using_axis_location_custom_parameter_on_instances(ufo_module):
+    # https://github.com/googlefonts/glyphsLib/issues/714
+    # https://github.com/googlefonts/glyphsLib/pull/810
+
+    font = to_glyphs(
+        [ufo_module.Font(), ufo_module.Font(), ufo_module.Font(), ufo_module.Font()]
+    )
+
+    origin_id = "95FB0C11-C828-4064-8966-34220AA4D426"
+    font.customParameters["Axes"] = [
+        {"Name": "Weight", "Tag": "wght"},
+        {"Name": "Width", "Tag": "wdth"},
+    ]
+
+    # Add masters
+
+    font.masters[0].name = "Regular"
+    font.masters[0].id = origin_id
+    font.masters[0].weightValue = 72
+    font.masters[0].widthValue = 448
+    font.masters[0].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 400},
+        {"Axis": "Width", "Location": 100},
+    ]
+    font.masters[1].name = "Bold"
+    font.masters[1].weightValue = 112
+    font.masters[1].widthValue = 448
+    font.masters[1].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 700},
+        {"Axis": "Width", "Location": 100},
+    ]
+    font.masters[2].name = "Thin"
+    font.masters[2].weightValue = 48
+    font.masters[2].widthValue = 448
+    font.masters[2].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 200},
+        {"Axis": "Width", "Location": 100},
+    ]
+    font.masters[3].name = "Cd Regular"
+    font.masters[3].weightValue = 72
+    font.masters[3].widthValue = 224
+    font.masters[3].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 400},
+        {"Axis": "Width", "Location": 50},
+    ]
+
+    font.customParameters["Variable Font Origin"] = origin_id
+
+    # Add some instances with mappings
+
+    font.instances = [
+        GSInstance(),
+        GSInstance(),
+        GSInstance(),
+        GSInstance(),
+        GSInstance(),
+        GSInstance(),
+    ]
+
+    font.instances[0].name = "Thin"
+    font.instances[0].weightValue = 48
+    font.instances[0].widthValue = 448
+    font.instances[0].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 200},
+        {"Axis": "Width", "Location": 100},
+    ]
+    font.instances[1].name = "Light"
+    font.instances[1].weightValue = 62
+    font.instances[1].widthValue = 448
+    font.instances[1].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 300},
+        {"Axis": "Width", "Location": 100},
+    ]
+    font.instances[2].name = "Regular"
+    font.instances[2].weightValue = 72
+    font.instances[2].widthValue = 448
+    font.instances[2].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 400},
+        {"Axis": "Width", "Location": 100},
+    ]
+    font.instances[3].name = "Medium"
+    font.instances[3].weightValue = 92
+    font.instances[3].widthValue = 448
+    font.instances[3].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 600},
+        {"Axis": "Width", "Location": 100},
+    ]
+    font.instances[4].name = "Cd Regular"
+    font.instances[4].weightValue = 72
+    font.instances[4].widthValue = 224
+    font.instances[4].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 400},
+        {"Axis": "Width", "Location": 50},
+    ]
+    font.instances[5].name = "SCd Regular"
+    font.instances[5].weightValue = 72
+    font.instances[5].widthValue = 384
+    font.instances[5].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 400},
+        {"Axis": "Width", "Location": 60},
+    ]
+
+    doc = to_designspace(font, ufo_module=ufo_module)
+    assert doc.axes[0].minimum == 200
+    assert doc.axes[0].default == 400
+    assert doc.axes[0].maximum == 700
+    assert doc.axes[0].map == [(200, 48), (300, 62), (400, 72), (600, 92), (700, 112)]
+
+    assert doc.axes[1].minimum == 50
+    assert doc.axes[1].default == 100
+    assert doc.axes[1].maximum == 100
+    assert doc.axes[1].map == [(50, 224), (60, 384), (100, 448)]
+
+
+def test_mapping_using_axis_location_cp_on_masters_none(ufo_module):
+    # https://github.com/googlefonts/glyphsLib/issues/714
+    # https://github.com/googlefonts/glyphsLib/pull/810
+
+    # When masters have no or disabled Axis Location CP, the ones on the
+    # instances should still be evaluated.
+
+    font = to_glyphs(
+        [ufo_module.Font(), ufo_module.Font(), ufo_module.Font(), ufo_module.Font()]
+    )
+
+    font.customParameters["Axes"] = [
+        {"Name": "Weight", "Tag": "wght"},
+        {"Name": "Width", "Tag": "wdth"},
+    ]
+
+    # Add masters
+
+    font.masters[0].name = "Regular"
+    font.masters[0].weightValue = 72
+    font.masters[0].widthValue = 448
+
+    font.masters[1].name = "Bold"
+    font.masters[1].weightValue = 112
+    font.masters[1].widthValue = 448
+
+    font.masters[2].name = "Thin"
+    font.masters[2].weightValue = 48
+    font.masters[2].widthValue = 448
+
+    font.masters[3].name = "Cd Regular"
+    font.masters[3].weightValue = 72
+    font.masters[3].widthValue = 224
+
+    font.instances = [GSInstance(), GSInstance(), GSInstance()]
+
+    font.instances[0].name = "Regular"
+    font.instances[0].weightValue = 72
+    font.instances[0].widthValue = 448
+    font.instances[0].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 400},
+        {"Axis": "Width", "Location": 100},
+    ]
+    font.instances[1].name = "SCd Regular"
+    font.instances[1].weightValue = 72
+    font.instances[1].widthValue = 384
+    font.instances[1].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 400},
+        {"Axis": "Width", "Location": 60},
+    ]
+    font.instances[2].name = "Cd Regular"
+    font.instances[2].weightValue = 72
+    font.instances[2].widthValue = 224
+    font.instances[2].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 400},
+        {"Axis": "Width", "Location": 50},
+    ]
+
+    doc = to_designspace(font, ufo_module=ufo_module)
+    assert doc.axes[0].minimum == 400
+    assert doc.axes[0].default == 400
+    assert doc.axes[0].maximum == 400
+    assert doc.axes[0].map == [(400, 72)]
+
+    assert doc.axes[1].minimum == 50
+    assert doc.axes[1].default == 100
+    assert doc.axes[1].maximum == 100
+    assert doc.axes[1].map == [(50, 224), (60, 384), (100, 448)]
+
+
+def test_mapping_using_axis_location_cp_on_instances_none(ufo_module):
+    # https://github.com/googlefonts/glyphsLib/issues/714
+    # https://github.com/googlefonts/glyphsLib/pull/810
+
+    # When all masters have Axis Location CP, non-"Axis Location" instance
+    # mappings should be ignored.
+
+    font = to_glyphs(
+        [ufo_module.Font(), ufo_module.Font(), ufo_module.Font(), ufo_module.Font()]
+    )
+
+    font.customParameters["Axes"] = [
+        {"Name": "Weight", "Tag": "wght"},
+        {"Name": "Width", "Tag": "wdth"},
+    ]
+
+    # Add masters
+
+    font.masters[0].name = "Regular"
+    font.masters[0].weightValue = 72
+    font.masters[0].widthValue = 448
+    font.masters[0].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 400},
+        {"Axis": "Width", "Location": 100},
+    ]
+    font.masters[1].name = "Bold"
+    font.masters[1].weightValue = 112
+    font.masters[1].widthValue = 448
+    font.masters[1].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 700},
+        {"Axis": "Width", "Location": 100},
+    ]
+    font.masters[2].name = "Thin"
+    font.masters[2].weightValue = 48
+    font.masters[2].widthValue = 448
+    font.masters[2].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 200},
+        {"Axis": "Width", "Location": 100},
+    ]
+    font.masters[3].name = "Cd Regular"
+    font.masters[3].weightValue = 72
+    font.masters[3].widthValue = 224
+    font.masters[3].customParameters["Axis Location"] = [
+        {"Axis": "Weight", "Location": 400},
+        {"Axis": "Width", "Location": 50},
+    ]
+
+    font.instances = [GSInstance()]
+
+    font.instances[0].name = "SCd Regular"
+    font.instances[0].weightValue = 72
+    font.instances[0].widthValue = 384
+
+    doc = to_designspace(font, ufo_module=ufo_module)
+    assert doc.axes[0].minimum == 200
+    assert doc.axes[0].default == 400
+    assert doc.axes[0].maximum == 700
+    assert doc.axes[0].map == [(200, 48), (400, 72), (700, 112)]
+
+    assert doc.axes[1].minimum == 50
+    assert doc.axes[1].default == 100
+    assert doc.axes[1].maximum == 100
+    assert doc.axes[1].map == [(50, 224), (100, 448)]
+
+
 def test_custom_parameter_vfo_current():
     """Tests get_regular_master when 'Variable Font Origin' custom parameter name
     is used with master set to 'Regular Text'.  This is the current default