From a92b2b0725300b13d457cffd9d8a3b1678a34073 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Wed, 15 Nov 2023 13:55:06 +0000 Subject: [PATCH 1/2] [instancer_test] check that vertical metrics stay in sync after MVAR instancing --- .../data/PartialInstancerTest-VF.ttx | 9 +++- Tests/varLib/instancer/instancer_test.py | 42 ++++++++++++++++--- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/Tests/varLib/instancer/data/PartialInstancerTest-VF.ttx b/Tests/varLib/instancer/data/PartialInstancerTest-VF.ttx index 2f1754b05..cee188460 100644 --- a/Tests/varLib/instancer/data/PartialInstancerTest-VF.ttx +++ b/Tests/varLib/instancer/data/PartialInstancerTest-VF.ttx @@ -108,9 +108,9 @@ - + - + @@ -687,6 +687,7 @@ + @@ -705,6 +706,10 @@ + + + + diff --git a/Tests/varLib/instancer/instancer_test.py b/Tests/varLib/instancer/instancer_test.py index 20d9194f6..da6dd9ee0 100644 --- a/Tests/varLib/instancer/instancer_test.py +++ b/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 From 69636098bdfc990805975a35ed55b6d143535323 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Wed, 15 Nov 2023 13:56:22 +0000 Subject: [PATCH 2/2] [instancer] Ensure hhea vertical metrics stay in sync with OS/2 ones after instancing Fixes https://github.com/fonttools/fonttools/issues/3297 --- Lib/fontTools/varLib/instancer/__init__.py | 42 +++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/Lib/fontTools/varLib/instancer/__init__.py b/Lib/fontTools/varLib/instancer/__init__.py index cde1d39f6..a887e5d38 100644 --- a/Lib/fontTools/varLib/instancer/__init__.py +++ b/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,7 +739,9 @@ def instantiateMVAR(varfont, axisLimits): fvarAxes = varfont["fvar"].axes varStore = mvar.VarStore defaultDeltas = instantiateItemVariationStore(varStore, fvarAxes, axisLimits) - setMvarDeltas(varfont, defaultDeltas) + + with verticalMetricsKeptInSync(varfont): + setMvarDeltas(varfont, defaultDeltas) if varStore.VarRegionList.Region: varIndexMapping = varStore.optimize()