Merge pull request #3337 from fonttools/instancer-sync-vmetrics

[instancer] Ensure hhea vertical metrics stay in sync with OS/2 after MVAR instancing

Lib/fontTools/varLib/instancer/__init__.py

@@ -105,6 +105,7 @@ from fontTools.misc.cliTools import makeOutputFileName
 from fontTools.varLib.instancer import solver
 import collections
 import dataclasses
+from contextlib import contextmanager
 from copy import deepcopy
 from enum import IntEnum
 import logging
@@ -694,6 +695,43 @@ def setMvarDeltas(varfont, deltas):
             )
 
 
+@contextmanager
+def verticalMetricsKeptInSync(varfont):
+    """Ensure hhea vertical metrics stay in sync with OS/2 ones after instancing.
+
+    When applying MVAR deltas to the OS/2 table, if the ascender, descender and
+    line gap change but they were the same as the respective hhea metrics in the
+    original font, this context manager ensures that hhea metrcs also get updated
+    accordingly.
+    The MVAR spec only has tags for the OS/2 metrics, but it is common in fonts
+    to have the hhea metrics be equal to those for compat reasons.
+
+    https://learn.microsoft.com/en-us/typography/opentype/spec/mvar
+    https://googlefonts.github.io/gf-guide/metrics.html#7-hhea-and-typo-metrics-should-be-equal
+    https://github.com/fonttools/fonttools/issues/3297
+    """
+    current_os2_vmetrics = [
+        getattr(varfont["OS/2"], attr)
+        for attr in ("sTypoAscender", "sTypoDescender", "sTypoLineGap")
+    ]
+    metrics_are_synced = current_os2_vmetrics == [
+        getattr(varfont["hhea"], attr) for attr in ("ascender", "descender", "lineGap")
+    ]
+
+    yield metrics_are_synced
+
+    if metrics_are_synced:
+        new_os2_vmetrics = [
+            getattr(varfont["OS/2"], attr)
+            for attr in ("sTypoAscender", "sTypoDescender", "sTypoLineGap")
+        ]
+        if current_os2_vmetrics != new_os2_vmetrics:
+            for attr, value in zip(
+                ("ascender", "descender", "lineGap"), new_os2_vmetrics
+            ):
+                setattr(varfont["hhea"], attr, value)
+
+
 def instantiateMVAR(varfont, axisLimits):
     log.info("Instantiating MVAR table")
 
@@ -701,6 +739,8 @@ def instantiateMVAR(varfont, axisLimits):
     fvarAxes = varfont["fvar"].axes
     varStore = mvar.VarStore
     defaultDeltas = instantiateItemVariationStore(varStore, fvarAxes, axisLimits)
+
+    with verticalMetricsKeptInSync(varfont):
         setMvarDeltas(varfont, defaultDeltas)
 
     if varStore.VarRegionList.Region:

Tests/varLib/instancer/data/PartialInstancerTest-VF.ttx

@@ -108,9 +108,9 @@
     <fsSelection value="00000000 01000000"/>
     <usFirstCharIndex value="32"/>
     <usLastCharIndex value="8722"/>
-    <sTypoAscender value="800"/>
+    <sTypoAscender value="1000"/>
     <sTypoDescender value="-200"/>
-    <sTypoLineGap value="200"/>
+    <sTypoLineGap value="0"/>
     <usWinAscent value="1000"/>
     <usWinDescent value="200"/>
     <ulCodePageRange1 value="00000000 00000000 00000000 00000001"/>
@@ -687,6 +687,7 @@
         <!-- VarRegionCount=1 -->
         <VarRegionIndex index="0" value="0"/>
         <Item index="0" value="[30]"/>
+        <Item index="1" value="[100]"/>
       </VarData>
     </VarStore>
     <ValueRecord index="0">
@@ -705,6 +706,10 @@
       <ValueTag value="xhgt"/>
       <VarIdx value="65536"/>
     </ValueRecord>
+    <ValueRecord index="3">
+      <ValueTag value="hasc"/>
+      <VarIdx value="65537"/>
+    </ValueRecord>
   </MVAR>
 
   <STAT>

Tests/varLib/instancer/instancer_test.py

@@ -304,39 +304,69 @@ class InstantiateMVARTest(object):
         assert len(mvar.VarStore.VarData) == 1
 
     @pytest.mark.parametrize(
-        "location, expected",
+        "location, expected, sync_vmetrics",
         [
             pytest.param(
                 {"wght": 1.0, "wdth": 0.0},
-                {"strs": 100, "undo": -200, "unds": 150},
+                {"strs": 100, "undo": -200, "unds": 150, "hasc": 1100},
+                True,
                 id="wght=1.0,wdth=0.0",
             ),
             pytest.param(
                 {"wght": 0.0, "wdth": -1.0},
-                {"strs": 20, "undo": -100, "unds": 50},
+                {"strs": 20, "undo": -100, "unds": 50, "hasc": 1000},
+                True,
                 id="wght=0.0,wdth=-1.0",
             ),
             pytest.param(
                 {"wght": 0.5, "wdth": -0.5},
-                {"strs": 55, "undo": -145, "unds": 95},
+                {"strs": 55, "undo": -145, "unds": 95, "hasc": 1050},
+                True,
                 id="wght=0.5,wdth=-0.5",
             ),
             pytest.param(
                 {"wght": 1.0, "wdth": -1.0},
-                {"strs": 50, "undo": -180, "unds": 130},
+                {"strs": 50, "undo": -180, "unds": 130, "hasc": 1100},
+                True,
                 id="wght=0.5,wdth=-0.5",
             ),
+            pytest.param(
+                {"wght": 1.0, "wdth": 0.0},
+                {"strs": 100, "undo": -200, "unds": 150, "hasc": 1100},
+                False,
+                id="wght=1.0,wdth=0.0,no_sync_vmetrics",
+            ),
         ],
     )
-    def test_full_instance(self, varfont, location, expected):
+    def test_full_instance(self, varfont, location, sync_vmetrics, expected):
         location = instancer.NormalizedAxisLimits(location)
 
+        # check vertical metrics are in sync before...
+        if sync_vmetrics:
+            assert varfont["OS/2"].sTypoAscender == varfont["hhea"].ascender
+            assert varfont["OS/2"].sTypoDescender == varfont["hhea"].descender
+            assert varfont["OS/2"].sTypoLineGap == varfont["hhea"].lineGap
+        else:
+            # force them not to be in sync
+            varfont["OS/2"].sTypoDescender -= 100
+            varfont["OS/2"].sTypoLineGap += 200
+
         instancer.instantiateMVAR(varfont, location)
 
         for mvar_tag, expected_value in expected.items():
             table_tag, item_name = MVAR_ENTRIES[mvar_tag]
             assert getattr(varfont[table_tag], item_name) == expected_value
 
+        # ... as well as after instancing, but only if they were already
+        # https://github.com/fonttools/fonttools/issues/3297
+        if sync_vmetrics:
+            assert varfont["OS/2"].sTypoAscender == varfont["hhea"].ascender
+            assert varfont["OS/2"].sTypoDescender == varfont["hhea"].descender
+            assert varfont["OS/2"].sTypoLineGap == varfont["hhea"].lineGap
+        else:
+            assert varfont["OS/2"].sTypoDescender != varfont["hhea"].descender
+            assert varfont["OS/2"].sTypoLineGap != varfont["hhea"].lineGap
+
         assert "MVAR" not in varfont