From ddf16c913002c36009d055160f355b399233468b Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Thu, 1 Jun 2023 11:02:28 -0600 Subject: [PATCH] [designspaceLib/avar2] Apply review comments --- Doc/source/designspaceLib/index.rst | 6 +- Lib/fontTools/designspaceLib/split.py | 10 +- Lib/fontTools/varLib/__init__.py | 18 +- .../data/test_avar2.designspace | 1428 +---------------- Tests/designspaceLib/designspace_test.py | 15 +- Tests/designspaceLib/split_test.py | 13 +- 6 files changed, 69 insertions(+), 1421 deletions(-) diff --git a/Doc/source/designspaceLib/index.rst b/Doc/source/designspaceLib/index.rst index 8b7f931a1..7b8b48787 100644 --- a/Doc/source/designspaceLib/index.rst +++ b/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 ```` elements now can have a ```` element that -specifies such mappings, which when present carries data that is used to -compile to an ``avar`` version 2 table. +designspace locations. The ```` elements now can have a ```` +element that specifies such mappings, which when present carries data that is +used to compile to an ``avar`` version 2 table. Version 5.0 ----------- diff --git a/Lib/fontTools/designspaceLib/split.py b/Lib/fontTools/designspaceLib/split.py index 9742e3298..af370f143 100644 --- a/Lib/fontTools/designspaceLib/split.py +++ b/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, diff --git a/Lib/fontTools/varLib/__init__.py b/Lib/fontTools/varLib/__init__.py index 0211ad3ec..5a2c0d0d2 100644 --- a/Lib/fontTools/varLib/__init__.py +++ b/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", - t, + "No input location specified for non-hidden axis '%s' in axis mapping %s; axis default value assumed.", + 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: diff --git a/Tests/designspaceLib/data/test_avar2.designspace b/Tests/designspaceLib/data/test_avar2.designspace index db99a4697..d54588a66 100644 --- a/Tests/designspaceLib/data/test_avar2.designspace +++ b/Tests/designspaceLib/data/test_avar2.designspace @@ -1,7 +1,7 @@ - + @@ -20,32 +20,46 @@ - - - - - - - - + - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -100,1382 +114,4 @@ - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 2 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic Thin - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 3 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic ExtLt - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 4 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic Light - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 5 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 6 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic Med - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 7 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic SemBd - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 8 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 9 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic ExtBd - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 10 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic Blk - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 2 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic Cond Thin - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 3 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic Cond ExtLt - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 4 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic Cond Light - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 5 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic Cond - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 6 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic Cond Med - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 7 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic Cond SemBd - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 8 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic Cond - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 9 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic Cond ExtBd - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 10 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic Cond Blk - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 2 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic SemCond Thin - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 3 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic SemCond ExtLt - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 4 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic SemCond Light - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 5 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic SemCond - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 6 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic SemCond Med - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 7 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic SemCond SemBd - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 8 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic SemCond - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 9 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic SemCond ExtBd - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 10 - 2 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic SemCond Blk - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 2 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic ExtCond Thin - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 3 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic ExtCond ExtLt - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 4 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic ExtCond Light - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 5 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic ExtCond - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 6 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic ExtCond Med - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 7 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic ExtCond SemBd - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 8 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic ExtCond - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 9 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic ExtCond ExtBd - - - - - - - - - - - - - com.schriftgestaltung.customParameters - - - panose - - 2 - 11 - 10 - 6 - 4 - 5 - 4 - 2 - 2 - 4 - - - - xHeight - 536 - - - styleMapFamilyName - Noto Sans Arabic ExtCond Blk - - - - - - - - - GSDimensionPlugin.Dimensions - - 6D0603BE-7277-4DA4-8C59-7934AF28E45B - - arAlef - 82 - arBar - 82 - - 8594390A-478C-48BC-BAB2-0B8797A53DFB - - arAlef - 178 - arBar - 178 - - - public.skipExportGlyphs - - TahSmallTwoDots - TahSmallTwoDots.restricted - _bar - _barshort - _dots.horz.below - _doublebar - _doublebarshort - _hehgoalcomma - _invertedstroke - _part.Gaf.stroke - _part.beh.fina - _part.instroke - _stroke - _stroke.wide - _vbelow-ar - _vinvertedbelow-ar - _vinvertedbelow-ar.001 - _wavyhamzaabove-ar - alef-ar.fina.short - alef-ar.fina.wide - alef-ar.short - alefMaksura-ar.init.short - alefMaksura-ar.medi.tall - allah-ar.pt1 - allah-ar.pt2 - beeh-ar.init.short - beh-ar.init.short - dotcenter-ar - dotcenter-ar.small - dotlessbeh-ar.init.short - dotlessbeh-ar.init.short.wide - dotlessbeh-ar.init.wide - dotlessbeh-ar.medi.tall - dotlessbeh-ar.medi.tall.wide - dotlessbeh-ar.medi.wide - fdfd.alrahem - fdfd.alrhman - fdfd.bsm - four-persian.small01 - fourdotsabove-ar.small - fourthroot-ar.pt - hah-ar.fina.wide - hah-ar.wide - jal - jallajalalouhou1-ar.001 - jlalh - noon-ar.init.short - ringArabic - tehabove-ar.small - three-persian.small01 - threedotsdownabove-ar.small - threedotsupabove-ar.small - threedotsupabove-ar.v2 - threedotsupbelow-ar.small - two-persian.small01 - twodotshorizontalabove-ar.small - twodotshorizontalabove-ar.v2 - twodotsverticalabove-ar.small - uniFBC0.small - wasla-ar - wawDotcenter-ar.pt - wawDotcenter-ar.pt.fina - yehRohingya-ar.fina - yehRohingya-ar.pt.fina - - - diff --git a/Tests/designspaceLib/designspace_test.py b/Tests/designspaceLib/designspace_test.py index b222c84ab..9f94b1902 100644 --- a/Tests/designspaceLib/designspace_test.py +++ b/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 doc.axisMappings[0].inputLocation == {"Justify": 0.0} + assert len(doc.axisMappings) == 1 + 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 = (pathlib.Path(__file__) / "../data/test_v4_original.designspace").resolve() + source = (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() diff --git a/Tests/designspaceLib/split_test.py b/Tests/designspaceLib/split_test.py index df3819843..3364133f7 100644 --- a/Tests/designspaceLib/split_test.py +++ b/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] - assert len(out.axisMappings) == 3 + _, subDoc = next(splitInterpolable(ds)) + 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