From 1c25210360b752f2598f24ff2e5250f318d3bea6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D8=AE=D8=A7=D9=84=D8=AF=20=D8=AD=D8=B3=D9=86=D9=8A=20=28K?= =?UTF-8?q?haled=20Hosny=29?= Date: Wed, 10 Jan 2024 19:06:26 +0200 Subject: [PATCH] [featureVars] Re-use FeatureVariationRecord's when possible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a FeatureVariationRecord with the same ConditionTable exists re-use it and append FeatureTableSubstitutionRecord’s. Without this, in the following feature code only the first lookup will be applied since there will be two FeatureVariationRecord with the same ConditionTable, so the first will be matched and the other will be skipped: conditionset test { wght 600 1000; wdth 150 200; } test; variation ccmp test { sub e by a; } ccmp; variation rlig test { sub b by c; } rlig; With this change only one FeatureVariationRecord will be created with two FeatureTableSubstitutionRecord’s. --- Lib/fontTools/varLib/featureVars.py | 35 +++++++++++++++++++++--- Tests/feaLib/builder_test.py | 41 +++++++++++++++++++++++++++++ Tests/varLib/featureVars_test.py | 32 ++++++++++++++++++++++ 3 files changed, 104 insertions(+), 4 deletions(-) diff --git a/Lib/fontTools/varLib/featureVars.py b/Lib/fontTools/varLib/featureVars.py index a6beb5c7d..828b84359 100644 --- a/Lib/fontTools/varLib/featureVars.py +++ b/Lib/fontTools/varLib/featureVars.py @@ -414,6 +414,10 @@ def addFeatureVariationsRaw(font, table, conditionalSubstitutions, featureTag="r axis.axisTag: axisIndex for axisIndex, axis in enumerate(font["fvar"].axes) } + hasFeatureVariations = ( + hasattr(table, "FeatureVariations") and table.FeatureVariations is not None + ) + featureVariationRecords = [] for conditionSet, lookupIndices in conditionalSubstitutions: conditionTable = [] @@ -440,11 +444,19 @@ def addFeatureVariationsRaw(font, table, conditionalSubstitutions, featureTag="r varFeatureIndex, combinedLookupIndices ) ) - featureVariationRecords.append( - buildFeatureVariationRecord(conditionTable, records) - ) + if hasFeatureVariations and ( + fvr := findFeatureVariationRecord(table.FeatureVariations, conditionTable) + ): + fvr.FeatureTableSubstitution.SubstitutionRecord.extend(records) + fvr.FeatureTableSubstitution.SubstitutionCount = len( + fvr.FeatureTableSubstitution.SubstitutionRecord + ) + else: + featureVariationRecords.append( + buildFeatureVariationRecord(conditionTable, records) + ) - if hasattr(table, "FeatureVariations") and table.FeatureVariations is not None: + if hasFeatureVariations: if table.FeatureVariations.Version != 0x00010000: raise VarLibError( "Unsupported FeatureVariations table version: " @@ -614,6 +626,21 @@ def buildConditionTable(axisIndex, filterRangeMinValue, filterRangeMaxValue): return ct +def findFeatureVariationRecord(featureVariations, conditionTable): + """Find a FeatureVariationRecord that has the same conditionTable.""" + if featureVariations.Version != 0x00010000: + raise VarLibError( + "Unsupported FeatureVariations table version: " + f"0x{featureVariations.Version:08x} (expected 0x00010000)." + ) + + for fvr in featureVariations.FeatureVariationRecord: + if conditionTable == fvr.ConditionSet.ConditionTable: + return fvr + + return None + + def sortFeatureList(table): """Sort the feature list by feature tag, and remap the feature indices elsewhere. This is needed after the feature list has been modified. diff --git a/Tests/feaLib/builder_test.py b/Tests/feaLib/builder_test.py index 6855d5410..875223af7 100644 --- a/Tests/feaLib/builder_test.py +++ b/Tests/feaLib/builder_test.py @@ -990,6 +990,47 @@ class BuilderTest(unittest.TestCase): f'{name}.fea:{line}:12: Ambiguous "ignore {sub}", there should be least one marked glyph' ) + def test_conditionset_multiple_features(self): + """Test that using the same `conditionset` for multiple features reuses the + `FeatureVariationRecord`.""" + + features = """ + languagesystem DFLT dflt; + + conditionset test { + wght 600 1000; + wdth 150 200; + } test; + + variation ccmp test { + sub e by a; + } ccmp; + + variation rlig test { + sub b by c; + } rlig; + """ + + def make_mock_vf(): + font = makeTTFont() + font["name"] = newTable("name") + addFvar( + font, + [("wght", 0, 0, 1000, "Weight"), ("wdth", 100, 100, 200, "Width")], + [], + ) + del font["name"] + return font + + font = make_mock_vf() + addOpenTypeFeaturesFromString(font, features) + + table = font["GSUB"].table + assert table.FeatureVariations.FeatureVariationCount == 1 + + fvr = table.FeatureVariations.FeatureVariationRecord[0] + assert fvr.FeatureTableSubstitution.SubstitutionCount == 2 + def test_condition_set_avar(self): """Test that the `avar` table is consulted when normalizing user-space values.""" diff --git a/Tests/varLib/featureVars_test.py b/Tests/varLib/featureVars_test.py index 99f41e776..ef32ee031 100644 --- a/Tests/varLib/featureVars_test.py +++ b/Tests/varLib/featureVars_test.py @@ -133,6 +133,38 @@ def test_addFeatureVariations_new_feature(varfont): assert _substitution_features(gsub, rec_index=1) == [(0, "rclt")] +def test_addFeatureVariations_existing_condition(varfont): + assert "GSUB" not in varfont + + # Add a feature variation for 'ccmp' feature tag with a condition + addFeatureVariations( + varfont, [([{"wght": (0.5, 1.0)}], {"A": "A.alt"})], featureTag="ccmp" + ) + + gsub = varfont["GSUB"].table + + # Should now have one feature record, one lookup, and one feature variation record + assert len(gsub.FeatureList.FeatureRecord) == 1 + assert gsub.FeatureList.FeatureRecord[0].FeatureTag == "ccmp" + assert len(gsub.LookupList.Lookup) == 1 + assert len(gsub.FeatureVariations.FeatureVariationRecord) == 1 + assert _substitution_features(gsub, rec_index=0) == [(0, "ccmp")] + + # Add a feature variation for 'rlig' feature tag with the same condition + addFeatureVariations( + varfont, [([{"wght": (0.5, 1.0)}], {"B": "B.alt"})], featureTag="rlig" + ) + + # Should now have two feature records, two lookups, and one feature variation + # record, since the condition is the same for both feature variations + assert len(gsub.FeatureList.FeatureRecord) == 2 + assert gsub.FeatureList.FeatureRecord[0].FeatureTag == "ccmp" + assert gsub.FeatureList.FeatureRecord[1].FeatureTag == "rlig" + assert len(gsub.LookupList.Lookup) == 2 + assert len(gsub.FeatureVariations.FeatureVariationRecord) == 1 + assert _substitution_features(gsub, rec_index=0) == [(0, "ccmp"), (1, "rlig")] + + def _test_linear(n): conds = [] for i in range(n):