From 9331de16a016e57cf7d29b9c371ea9dc9c5c468c Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Mon, 20 May 2024 14:33:31 +0100 Subject: [PATCH 1/5] Add variable kerning to old kern writer --- .../featureWriters/kernFeatureWriter2.py | 171 +++++++++++++++++- 1 file changed, 166 insertions(+), 5 deletions(-) diff --git a/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py b/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py index 82fab4db..3455280c 100644 --- a/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py +++ b/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py @@ -1,12 +1,25 @@ """Old implementation of KernFeatureWriter as of ufo2ft v2.30.0 for backward compat.""" +from __future__ import annotations + from types import SimpleNamespace +from typing import Any, Mapping, Set from fontTools import unicodedata +from fontTools.designspaceLib import DesignSpaceDocument +from fontTools.feaLib.variableScalar import Location as VariableScalarLocation +from fontTools.feaLib.variableScalar import VariableScalar +from fontTools.ufoLib.kerning import lookupKerningValue from ufo2ft.constants import INDIC_SCRIPTS, USE_SCRIPTS from ufo2ft.featureWriters import BaseFeatureWriter, ast -from ufo2ft.util import classifyGlyphs, quantize, unicodeScriptDirection +from ufo2ft.util import ( + classifyGlyphs, + quantize, + unicodeScriptDirection, + get_userspace_location, + collapse_varscalar, +) SIDE1_PREFIX = "public.kern1." SIDE2_PREFIX = "public.kern2." @@ -113,7 +126,9 @@ class KernFeatureWriter(BaseFeatureWriter): def setContext(self, font, feaFile, compiler=None): ctx = super().setContext(font, feaFile, compiler=compiler) ctx.gdefClasses = self.getGDEFGlyphClasses() - ctx.kerning = self.getKerningData(font, feaFile, self.getOrderedGlyphSet()) + ctx.kerning = self.getKerningData( + font, self.options, feaFile, self.getOrderedGlyphSet() + ) feaScripts = ast.getScriptLanguageSystems(feaFile) ctx.scriptGroups = self._groupScriptsByTagAndDirection(feaScripts) @@ -168,9 +183,9 @@ def _write(self): return True @classmethod - def getKerningData(cls, font, feaFile=None, glyphSet=None): + def getKerningData(cls, font, options, feaFile=None, glyphSet=None): side1Classes, side2Classes = cls.getKerningClasses(font, feaFile, glyphSet) - pairs = cls.getKerningPairs(font, side1Classes, side2Classes, glyphSet) + pairs = cls.getKerningPairs(font, side1Classes, side2Classes, glyphSet, options) return SimpleNamespace( side1Classes=side1Classes, side2Classes=side2Classes, pairs=pairs ) @@ -183,6 +198,13 @@ def getKerningGroups(font, glyphSet=None): allGlyphs = set(font.keys()) side1Groups = {} side2Groups = {} + + if isinstance(font, DesignSpaceDocument): + default_font = font.findDefault() + assert default_font is not None + font = default_font.font + assert font is not None + for name, members in font.groups.items(): # prune non-existent or skipped glyphs members = [g for g in members if g in allGlyphs] @@ -208,7 +230,16 @@ def getKerningClasses(cls, font, feaFile=None, glyphSet=None): return side1Classes, side2Classes @staticmethod - def getKerningPairs(font, side1Classes, side2Classes, glyphSet=None): + def getKerningPairs(font, side1Classes, side2Classes, glyphSet=None, options=None): + if isinstance(font, DesignSpaceDocument): + return KernFeatureWriter.getVariableKerningPairs( + font, + side1Classes, + side2Classes, + glyphSet or set(), + options or SimpleNamespace(), + ) + if glyphSet: allGlyphs = set(glyphSet.keys()) else: @@ -240,6 +271,136 @@ def getKerningPairs(font, side1Classes, side2Classes, glyphSet=None): result.append(KerningPair(side1, side2, value)) return result + @staticmethod + def getVariableKerningPairs( + designspace: DesignSpaceDocument, + side1Classes: Any, + side2Classes: Any, + glyphSet: Set[str], + options: SimpleNamespace, + ) -> list[KerningPair]: + quantization = options.quantization + + # Gather utility variables for faster kerning lookups. + # TODO: Do we construct these in code elsewhere? + assert not (set(side1Classes) & set(side2Classes)) + unified_groups = {**side1Classes, **side2Classes} + + side1ClassesRaw: Mapping[str, tuple[str, ...]] = { + group_name: tuple( + glyph for glyphs in glyph_defs.glyphSet() for glyph in glyphs.glyphSet() + ) + for group_name, glyph_defs in side1Classes.items() + } + side2ClassesRaw: Mapping[str, tuple[str, ...]] = { + group_name: tuple( + glyph for glyphs in glyph_defs.glyphSet() for glyph in glyphs.glyphSet() + ) + for group_name, glyph_defs in side2Classes.items() + } + + glyphToFirstGroup = { + glyph_name: group_name # TODO: Is this overwrite safe? User input is adversarial + for group_name, glyphs in side1ClassesRaw.items() + for glyph_name in glyphs + } + glyphToSecondGroup = { + glyph_name: group_name + for group_name, glyphs in side2ClassesRaw.items() + for glyph_name in glyphs + } + + # Collate every kerning pair in the designspace, as even UFOs that + # provide no entry for the pair must contribute a value at their + # source's location in the VariableScalar. + # NOTE: This is required as the DS+UFO kerning model and the OpenType + # variation model handle the absence of a kerning value at a + # given location differently: + # - DS+UFO: + # If the missing pair excepts another pair, take its value; + # Otherwise, take a value of 0. + # - OpenType: + # Always interpolate from other locations, ignoring more + # general pairs that this one excepts. + # See discussion: https://github.com/googlefonts/ufo2ft/pull/635 + all_pairs: set[tuple[str, str]] = set() + for source in designspace.sources: + if source.layerName is not None: + continue + assert source.font is not None + all_pairs |= set(source.font.kerning) + + kerning_pairs_in_progress: dict[ + tuple[str | tuple[str], str | tuple[str]], VariableScalar + ] = {} + for source in designspace.sources: + # Skip sparse sources, because they can have no kerning. + if source.layerName is not None: + continue + assert source.font is not None + + location = VariableScalarLocation( + get_userspace_location(designspace, source.location) + ) + + kerning: Mapping[tuple[str, str], float] = source.font.kerning + for pair in all_pairs: + side1, side2 = pair + firstIsClass = side1 in side1Classes + secondIsClass = side2 in side2Classes + + # Filter out pairs that reference missing groups or glyphs. + # TODO: Can we do this outside of the loop? We know the pairs already. + if not firstIsClass and side1 not in glyphSet: + continue + if not secondIsClass and side2 not in glyphSet: + continue + + # Get the kerning value for this source and quantize, following + # the DS+UFO semantics described above. + value = quantize( + lookupKerningValue( + pair, + kerning, + unified_groups, + glyphToFirstGroup=glyphToFirstGroup, + glyphToSecondGroup=glyphToSecondGroup, + ), + quantization, + ) + + if firstIsClass: + side1 = side1Classes[side1] + if secondIsClass: + side2 = side2Classes[side2] + + # TODO: Can we instantiate these outside of the loop? We know the pairs already. + var_scalar = kerning_pairs_in_progress.setdefault( + (side1, side2), VariableScalar() + ) + # NOTE: Avoid using .add_value because it instantiates a new + # VariableScalarLocation on each call. + var_scalar.values[location] = value + + # We may need to provide a default location value to the variation + # model, find out where that is. + default_source = designspace.findDefault() + assert default_source is not None + default_location = VariableScalarLocation( + get_userspace_location(designspace, default_source.location) + ) + + result = [] + for (side1, side2), value in kerning_pairs_in_progress.items(): + # TODO: Should we interpolate a default value if it's not in the + # sources, rather than inserting a zero? What would varLib do? + if default_location not in value.values: + value.values[default_location] = 0 + value = collapse_varscalar(value) + result.append(KerningPair(side1, side2, value)) + + return result + def _intersectPairs(self, attribute, glyphSets): allKeys = set() for pair in self.context.kerning.pairs: From 59f7dc1c5c15d095a8f1520a08e8ae7f25e0c98c Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Mon, 20 May 2024 14:40:52 +0100 Subject: [PATCH 2/5] Fix import sorting --- Lib/ufo2ft/featureWriters/kernFeatureWriter2.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py b/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py index 3455280c..7b1d6ba6 100644 --- a/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py +++ b/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py @@ -15,10 +15,10 @@ from ufo2ft.featureWriters import BaseFeatureWriter, ast from ufo2ft.util import ( classifyGlyphs, + collapse_varscalar, + get_userspace_location, quantize, unicodeScriptDirection, - get_userspace_location, - collapse_varscalar, ) SIDE1_PREFIX = "public.kern1." From 0db65953930a6cf03ca8a453b7d6e3ca544c70c0 Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Mon, 20 May 2024 15:17:34 +0100 Subject: [PATCH 3/5] Test variable kerning for old kern writer --- .../variableFeatureWriter_test.py | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/tests/featureWriters/variableFeatureWriter_test.py b/tests/featureWriters/variableFeatureWriter_test.py index 5ce55779..077fd4f3 100644 --- a/tests/featureWriters/variableFeatureWriter_test.py +++ b/tests/featureWriters/variableFeatureWriter_test.py @@ -59,3 +59,76 @@ def test_variable_features(FontClass): } curs; """ # noqa: B950 ) + + +def test_variable_features_old_kern_writer(FontClass): + tmp = io.StringIO() + designspace = designspaceLib.DesignSpaceDocument.fromfile( + "tests/data/TestVarfea.designspace" + ) + designspace.loadSourceFonts(FontClass) + + default_source = designspace.findDefault() + assert default_source is not None + default_ufo = default_source.font + assert default_ufo is not None + default_ufo.lib["com.github.googlei18n.ufo2ft.featureWriters"] = [ + { + "module": "ufo2ft.featureWriters.kernFeatureWriter2", + "class": "KernFeatureWriter", + }, + { + "module": "ufo2ft.featureWriters.markFeatureWriter", + "class": "MarkFeatureWriter", + }, + { + "module": "ufo2ft.featureWriters.gdefFeatureWriter", + "class": "GdefFeatureWriter", + }, + { + "module": "ufo2ft.featureWriters.cursFeatureWriter", + "class": "CursFeatureWriter", + }, + ] + + _ = compileVariableTTF(designspace, debugFeatureFile=tmp) + + assert dedent("\n" + tmp.getvalue()) == dedent( + """ + markClass dotabove-ar @MC_top; + markClass gravecmb @MC_top; + + lookup kern_rtl { + lookupflag IgnoreMarks; + pos alef-ar.fina alef-ar.fina <(wght=100:15 wght=1000:35) 0 (wght=100:15 wght=1000:35) 0>; + } kern_rtl; + + feature kern { + lookup kern_rtl; + } kern; + + feature mark { + lookup mark2base { + pos base alef-ar.fina + mark @MC_top; + pos base a + mark @MC_top; + } mark2base; + + } mark; + + table GDEF { + LigatureCaretByPos peh-ar.init 100; + } GDEF; + + feature curs { + lookup curs_rtl { + lookupflag RightToLeft IgnoreMarks; + pos cursive alef-ar.fina ; + pos cursive peh-ar.init ; + pos cursive peh-ar.init.BRACKET.varAlt01 ; + } curs_rtl; + + } curs; +""" # noqa: B950 + ) From ce869619bb17af24aaf1cdf0dce4695b5c8e7f96 Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Mon, 20 May 2024 15:48:24 +0100 Subject: [PATCH 4/5] Reuse the newer kern writer's var kern extractor --- .../featureWriters/kernFeatureWriter.py | 17 +- .../featureWriters/kernFeatureWriter2.py | 173 +++--------------- 2 files changed, 40 insertions(+), 150 deletions(-) diff --git a/Lib/ufo2ft/featureWriters/kernFeatureWriter.py b/Lib/ufo2ft/featureWriters/kernFeatureWriter.py index 524c4ddd..aa4695fd 100644 --- a/Lib/ufo2ft/featureWriters/kernFeatureWriter.py +++ b/Lib/ufo2ft/featureWriters/kernFeatureWriter.py @@ -402,7 +402,13 @@ def getKerningPairs( side2Classes: Mapping[str, tuple[str, ...]], ) -> list[KerningPair]: if self.context.isVariable: - return self.getVariableKerningPairs(side1Classes, side2Classes) + return self.getVariableKerningPairs( + self.context.font, + side1Classes, + side2Classes, + self.context.glyphSet, + self.options, + ) glyphSet = self.context.glyphSet font = self.context.font @@ -432,14 +438,15 @@ def getKerningPairs( return result + @staticmethod def getVariableKerningPairs( - self, + designspace: DesignSpaceDocument, side1Classes: Mapping[str, tuple[str, ...]], side2Classes: Mapping[str, tuple[str, ...]], + glyphSet: Mapping[str, str], + options: SimpleNamespace, ) -> list[KerningPair]: - designspace: DesignSpaceDocument = self.context.font - glyphSet = self.context.glyphSet - quantization = self.options.quantization + quantization = options.quantization # Gather utility variables for faster kerning lookups. # TODO: Do we construct these in code elsewhere? diff --git a/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py b/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py index 7b1d6ba6..a59cddc6 100644 --- a/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py +++ b/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py @@ -3,23 +3,17 @@ from __future__ import annotations from types import SimpleNamespace -from typing import Any, Mapping, Set +from typing import Mapping from fontTools import unicodedata from fontTools.designspaceLib import DesignSpaceDocument -from fontTools.feaLib.variableScalar import Location as VariableScalarLocation -from fontTools.feaLib.variableScalar import VariableScalar -from fontTools.ufoLib.kerning import lookupKerningValue from ufo2ft.constants import INDIC_SCRIPTS, USE_SCRIPTS from ufo2ft.featureWriters import BaseFeatureWriter, ast -from ufo2ft.util import ( - classifyGlyphs, - collapse_varscalar, - get_userspace_location, - quantize, - unicodeScriptDirection, +from ufo2ft.featureWriters.kernFeatureWriter import ( + KernFeatureWriter as NewKernFeatureWriter, ) +from ufo2ft.util import classifyGlyphs, quantize, unicodeScriptDirection SIDE1_PREFIX = "public.kern1." SIDE2_PREFIX = "public.kern2." @@ -232,13 +226,32 @@ def getKerningClasses(cls, font, feaFile=None, glyphSet=None): @staticmethod def getKerningPairs(font, side1Classes, side2Classes, glyphSet=None, options=None): if isinstance(font, DesignSpaceDocument): - return KernFeatureWriter.getVariableKerningPairs( + # Reuse the newer kern writers variable kerning extractor. Repack + # some arguments and the return type for this. + side1ClassesRaw: Mapping[str, tuple[str, ...]] = { + group_name: tuple( + glyph + for glyphs in glyph_defs.glyphSet() + for glyph in glyphs.glyphSet() + ) + for group_name, glyph_defs in side1Classes.items() + } + side2ClassesRaw: Mapping[str, tuple[str, ...]] = { + group_name: tuple( + glyph + for glyphs in glyph_defs.glyphSet() + for glyph in glyphs.glyphSet() + ) + for group_name, glyph_defs in side2Classes.items() + } + pairs = NewKernFeatureWriter.getVariableKerningPairs( font, - side1Classes, - side2Classes, - glyphSet or set(), - options or SimpleNamespace(), + side1ClassesRaw, + side2ClassesRaw, + glyphSet or {}, + options or KernFeatureWriter.options, ) + return [KerningPair(pair.side1, pair.side2, pair.value) for pair in pairs] if glyphSet: allGlyphs = set(glyphSet.keys()) @@ -271,136 +284,6 @@ def getKerningPairs(font, side1Classes, side2Classes, glyphSet=None, options=Non result.append(KerningPair(side1, side2, value)) return result - @staticmethod - def getVariableKerningPairs( - designspace: DesignSpaceDocument, - side1Classes: Any, - side2Classes: Any, - glyphSet: Set[str], - options: SimpleNamespace, - ) -> list[KerningPair]: - quantization = options.quantization - - # Gather utility variables for faster kerning lookups. - # TODO: Do we construct these in code elsewhere? - assert not (set(side1Classes) & set(side2Classes)) - unified_groups = {**side1Classes, **side2Classes} - - side1ClassesRaw: Mapping[str, tuple[str, ...]] = { - group_name: tuple( - glyph for glyphs in glyph_defs.glyphSet() for glyph in glyphs.glyphSet() - ) - for group_name, glyph_defs in side1Classes.items() - } - side2ClassesRaw: Mapping[str, tuple[str, ...]] = { - group_name: tuple( - glyph for glyphs in glyph_defs.glyphSet() for glyph in glyphs.glyphSet() - ) - for group_name, glyph_defs in side2Classes.items() - } - - glyphToFirstGroup = { - glyph_name: group_name # TODO: Is this overwrite safe? User input is adversarial - for group_name, glyphs in side1ClassesRaw.items() - for glyph_name in glyphs - } - glyphToSecondGroup = { - glyph_name: group_name - for group_name, glyphs in side2ClassesRaw.items() - for glyph_name in glyphs - } - - # Collate every kerning pair in the designspace, as even UFOs that - # provide no entry for the pair must contribute a value at their - # source's location in the VariableScalar. - # NOTE: This is required as the DS+UFO kerning model and the OpenType - # variation model handle the absence of a kerning value at a - # given location differently: - # - DS+UFO: - # If the missing pair excepts another pair, take its value; - # Otherwise, take a value of 0. - # - OpenType: - # Always interpolate from other locations, ignoring more - # general pairs that this one excepts. - # See discussion: https://github.com/googlefonts/ufo2ft/pull/635 - all_pairs: set[tuple[str, str]] = set() - for source in designspace.sources: - if source.layerName is not None: - continue - assert source.font is not None - all_pairs |= set(source.font.kerning) - - kerning_pairs_in_progress: dict[ - tuple[str | tuple[str], str | tuple[str]], VariableScalar - ] = {} - for source in designspace.sources: - # Skip sparse sources, because they can have no kerning. - if source.layerName is not None: - continue - assert source.font is not None - - location = VariableScalarLocation( - get_userspace_location(designspace, source.location) - ) - - kerning: Mapping[tuple[str, str], float] = source.font.kerning - for pair in all_pairs: - side1, side2 = pair - firstIsClass = side1 in side1Classes - secondIsClass = side2 in side2Classes - - # Filter out pairs that reference missing groups or glyphs. - # TODO: Can we do this outside of the loop? We know the pairs already. - if not firstIsClass and side1 not in glyphSet: - continue - if not secondIsClass and side2 not in glyphSet: - continue - - # Get the kerning value for this source and quantize, following - # the DS+UFO semantics described above. - value = quantize( - lookupKerningValue( - pair, - kerning, - unified_groups, - glyphToFirstGroup=glyphToFirstGroup, - glyphToSecondGroup=glyphToSecondGroup, - ), - quantization, - ) - - if firstIsClass: - side1 = side1Classes[side1] - if secondIsClass: - side2 = side2Classes[side2] - - # TODO: Can we instantiate these outside of the loop? We know the pairs already. - var_scalar = kerning_pairs_in_progress.setdefault( - (side1, side2), VariableScalar() - ) - # NOTE: Avoid using .add_value because it instantiates a new - # VariableScalarLocation on each call. - var_scalar.values[location] = value - - # We may need to provide a default location value to the variation - # model, find out where that is. - default_source = designspace.findDefault() - assert default_source is not None - default_location = VariableScalarLocation( - get_userspace_location(designspace, default_source.location) - ) - - result = [] - for (side1, side2), value in kerning_pairs_in_progress.items(): - # TODO: Should we interpolate a default value if it's not in the - # sources, rather than inserting a zero? What would varLib do? - if default_location not in value.values: - value.values[default_location] = 0 - value = collapse_varscalar(value) - result.append(KerningPair(side1, side2, value)) - - return result - def _intersectPairs(self, attribute, glyphSets): allKeys = set() for pair in self.context.kerning.pairs: From 0d15b13651bf308f32d87dc8aef2374b4beb6fa3 Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Tue, 21 May 2024 13:43:31 +0100 Subject: [PATCH 5/5] Convert options to simple namespace --- Lib/ufo2ft/featureWriters/kernFeatureWriter2.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py b/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py index a59cddc6..13c8c959 100644 --- a/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py +++ b/Lib/ufo2ft/featureWriters/kernFeatureWriter2.py @@ -249,7 +249,7 @@ def getKerningPairs(font, side1Classes, side2Classes, glyphSet=None, options=Non side1ClassesRaw, side2ClassesRaw, glyphSet or {}, - options or KernFeatureWriter.options, + options or SimpleNamespace(**KernFeatureWriter.options), ) return [KerningPair(pair.side1, pair.side2, pair.value) for pair in pairs]