[designspaceLib/avar2] Apply review comments

Doc/source/designspaceLib/index.rst

@@ -178,9 +178,9 @@ Version 5.1
 -----------
 
 The format was extended to support arbitrary mapping between input and output
-axes. The ``<axes>`` elements now can have a ``<mappings>`` element that
+designspace locations. The ``<axes>`` elements now can have a ``<mappings>``
-specifies such mappings, which when present carries data that is used to
+element that specifies such mappings, which when present carries data that is
-compile to an ``avar`` version 2 table.
+used to compile to an ``avar`` version 2 table.
 
 Version 5.0
 -----------

Lib/fontTools/designspaceLib/split.py

@@ -228,11 +228,17 @@ def _extractSubSpace(
 
     # TODO Trim out-of-range values? :-(
     subDoc.axisMappings = mappings = []
-    subDocAxes = [axis.name for axis in subDoc.axes]
+    subDocAxes = {axis.name for axis in subDoc.axes}
     for mapping in doc.axisMappings:
         if not all(axis in subDocAxes for axis in mapping.inputLocation.keys()):
             continue
-        assert all(axis in subDocAxes for axis in mapping.outputLocation.keys())
+        if not all(axis in subDocAxes for axis in mapping.outputLocation.keys()):
+            LOGGER.error(
+                "In axis mapping from input %s, some output axes are not in the variable-font: %s",
+                mapping.inputLocation,
+                mapping.outputLocation,
+            )
+            continue
         mappings.append(
             AxisMappingDescriptor(
                 inputLocation=mapping.inputLocation,

Lib/fontTools/varLib/__init__.py

@@ -231,6 +231,7 @@ def _add_avar(font, axes, mappings, axisTags):
             }
             for mapping in mappings
         ]
+        assert len(inputLocations) == len(outputLocations)
 
         # If base-master is missing, insert it at zero location.
         if not any(all(v == 0 for k, v in loc.items()) for loc in inputLocations):
@@ -241,21 +242,21 @@ def _add_avar(font, axes, mappings, axisTags):
         storeBuilder = varStore.OnlineVarStoreBuilder(axisTags)
         storeBuilder.setModel(model)
         varIdxes = {}
-        for t in axisTags:
+        for tag in axisTags:
             masterValues = []
             for vo, vi in zip(outputLocations, inputLocations):
-                if t not in vo:
+                if tag not in vo:
                     masterValues.append(0)
                     continue
-                if t not in vi and t not in hiddenAxes:
+                if tag not in vi and tag not in hiddenAxes:
                     log.warning(
-                        "No input location specified for non-hidden axis '%s' in axis mapping %s",
+                        "No input location specified for non-hidden axis '%s' in axis mapping %s; axis default value assumed.",
-                        t,
+                        tag,
                         vi,
                     )
-                v = vo[t] - vi.get(t, 0)
+                v = vo[tag] - vi.get(tag, 0)
                 masterValues.append(fl2fi(v, 14))
-            varIdxes[t] = storeBuilder.storeMasters(masterValues)[1]
+            varIdxes[tag] = storeBuilder.storeMasters(masterValues)[1]
 
         store = storeBuilder.finish()
         optimized = store.optimize()
@@ -908,7 +909,8 @@ def load_designspace(designspace):
     log.info("Axes:\n%s", pformat([axis.asdict() for axis in axes.values()]))
 
     axisMappings = ds.axisMappings
-    log.info("Mapping:\n%s", pformat(axisMappings))
+    if axisMappings:
+        log.info("Mappings:\n%s", pformat(axisMappings))
 
     # Check all master and instance locations are valid and fill in defaults
     for obj in masters + instances:

Tests/designspaceLib/designspace_test.py

@@ -691,16 +691,15 @@ def test_axisMapping():
 
 def test_axisMappingsRoundtrip(tmpdir):
     # tests of axisMappings in a document, roundtripping.
-    import pathlib
 
     tmpdir = str(tmpdir)
-    testDocPath = (pathlib.Path(__file__) / "../data/test_avar2.designspace").resolve()
+    testDocPath = (Path(__file__) / "../data/test_avar2.designspace").resolve()
     testDocPath2 = os.path.join(tmpdir, "test_avar2_roundtrip.designspace")
     doc = DesignSpaceDocument()
     doc.read(testDocPath)
     assert doc.axisMappings
-    assert len(doc.axisMappings) == 3
+    assert len(doc.axisMappings) == 1
-    assert doc.axisMappings[0].inputLocation == {"Justify": 0.0}
+    assert doc.axisMappings[0].inputLocation == {"Justify": -100.0, "Width": 100.0}
 
     # This is a bit of a hack, but it's the only way to make sure
     # that the save works on Windows if the tempdir and the data
@@ -955,19 +954,15 @@ def test_updatePaths(tmpdir):
 
 
 def test_read_with_path_object():
-    import pathlib
+    source = (Path(__file__) / "../data/test_v4_original.designspace").resolve()
-
-    source = (pathlib.Path(__file__) / "../data/test_v4_original.designspace").resolve()
     assert source.exists()
     doc = DesignSpaceDocument()
     doc.read(source)
 
 
 def test_with_with_path_object(tmpdir):
-    import pathlib
-
     tmpdir = str(tmpdir)
-    dest = pathlib.Path(tmpdir) / "test_v4_original.designspace"
+    dest = Path(tmpdir) / "test_v4_original.designspace"
     doc = DesignSpaceDocument()
     doc.write(dest)
     assert dest.exists()

Tests/designspaceLib/split_test.py

@@ -216,5 +216,14 @@ def test_optional_min_max_internal(condition, expected_set: ConditionSet):
 def test_avar2(datadir):
     ds = DesignSpaceDocument()
     ds.read(datadir / "test_avar2.designspace")
-    out = list(splitInterpolable(ds))[0][1]
+    _, subDoc = next(splitInterpolable(ds))
-    assert len(out.axisMappings) == 3
+    assert len(subDoc.axisMappings) == 1
+
+    subDocs = list(splitVariableFonts(ds))
+    assert len(subDocs) == 5
+    for i, (_, subDoc) in enumerate(subDocs):
+        # Only the first one should have a mapping, according to the document
+        if i == 0:
+            assert len(subDoc.axisMappings) == 1
+        else:
+            assert len(subDoc.axisMappings) == 0