From 5f4d5918b923d6113b04334ba62d9d6821c748ae Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Wed, 22 Jun 2022 16:29:30 +0100 Subject: [PATCH 1/5] filters: sort glyphs by decreasing component depth to avoid order-dependent issues Fixes https://github.com/googlefonts/ufo2ft/issues/621 --- Lib/ufo2ft/filters/base.py | 13 ++++++++++--- Lib/ufo2ft/util.py | 25 +++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/Lib/ufo2ft/filters/base.py b/Lib/ufo2ft/filters/base.py index 0286414c7..60c76901d 100644 --- a/Lib/ufo2ft/filters/base.py +++ b/Lib/ufo2ft/filters/base.py @@ -3,7 +3,7 @@ from fontTools.misc.loggingTools import Timer -from ufo2ft.util import _GlyphSet, _LazyFontName +from ufo2ft.util import _GlyphSet, _LazyFontName, getMaxComponentDepth logger = logging.getLogger(__name__) @@ -186,9 +186,16 @@ def __call__(self, font, glyphSet=None): include = self.include modified = context.modified + # process composite glyphs in decreasing component depth order (i.e. composites + # with more deeply nested components before shallower ones) to avoid + # order-dependent interferences while filtering glyphs with nested components + # https://github.com/googlefonts/ufo2ft/issues/621 + orderedGlyphs = sorted( + glyphSet.keys(), key=lambda g: -getMaxComponentDepth(glyphSet[g], glyphSet) + ) + with Timer() as t: - # we sort the glyph names to make loop deterministic - for glyphName in sorted(glyphSet.keys()): + for glyphName in orderedGlyphs: if glyphName in modified: continue glyph = glyphSet[glyphName] diff --git a/Lib/ufo2ft/util.py b/Lib/ufo2ft/util.py index 0a1511b39..4d377cb37 100644 --- a/Lib/ufo2ft/util.py +++ b/Lib/ufo2ft/util.py @@ -546,3 +546,28 @@ def ensure_all_sources_have_names(doc: DesignSpaceDocument) -> None: source.name = f"temp_master.{counter}" counter += 1 used_names.add(source.name) + + +def getMaxComponentDepth(glyph, glyphSet, maxComponentDepth=0): + """Return the height of a composite glyph's tree of components. + + This is equal to the depth of its deepest node, where the depth + means the number of edges (component references) from the node + to the tree's root. + + For glyphs that contain no components, only contours, this is 0. + Composite glyphs have max component depth of 1 or greater. + """ + if not glyph.components: + return maxComponentDepth + + maxComponentDepth += 1 + + initialMaxComponentDepth = maxComponentDepth + for component in glyph.components: + componentDepth = getMaxComponentDepth( + glyphSet[component.baseGlyph], glyphSet, initialMaxComponentDepth + ) + maxComponentDepth = max(maxComponentDepth, componentDepth) + + return maxComponentDepth From a4a3e0479454624afe44ff452bdef946876a671a Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Wed, 22 Jun 2022 16:33:30 +0100 Subject: [PATCH 2/5] ignore missing component base glyphs when computing depth --- Lib/ufo2ft/util.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Lib/ufo2ft/util.py b/Lib/ufo2ft/util.py index 4d377cb37..87a2b7f25 100644 --- a/Lib/ufo2ft/util.py +++ b/Lib/ufo2ft/util.py @@ -565,8 +565,12 @@ def getMaxComponentDepth(glyph, glyphSet, maxComponentDepth=0): initialMaxComponentDepth = maxComponentDepth for component in glyph.components: + try: + baseGlyph = glyphSet[component.baseGlyph] + except KeyError: + continue componentDepth = getMaxComponentDepth( - glyphSet[component.baseGlyph], glyphSet, initialMaxComponentDepth + baseGlyph, glyphSet, initialMaxComponentDepth ) maxComponentDepth = max(maxComponentDepth, componentDepth) From d074c9e70cba63a15b55fb4a3828d9de12a2582c Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Wed, 22 Jun 2022 17:08:42 +0100 Subject: [PATCH 3/5] propagateAnchors: fix reporting of modified glyph _propagate_glyph_anchors modifies not only the current composite glyph but all the nested composite glyphs used in between, however it was only reporting to be modifying the root glyph --- Lib/ufo2ft/filters/propagateAnchors.py | 14 +++++++++++--- tests/filters/propagateAnchors_test.py | 4 +++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/Lib/ufo2ft/filters/propagateAnchors.py b/Lib/ufo2ft/filters/propagateAnchors.py index d81c394e4..a346ed1f2 100644 --- a/Lib/ufo2ft/filters/propagateAnchors.py +++ b/Lib/ufo2ft/filters/propagateAnchors.py @@ -39,11 +39,16 @@ def filter(self, glyph): if not glyph.components: return False before = len(glyph.anchors) - _propagate_glyph_anchors(self.context.glyphSet, glyph, self.context.processed) + _propagate_glyph_anchors( + self.context.glyphSet, + glyph, + self.context.processed, + self.context.modified, + ) return len(glyph.anchors) > before -def _propagate_glyph_anchors(glyphSet, composite, processed): +def _propagate_glyph_anchors(glyphSet, composite, processed, modified): """ Propagate anchors from base glyphs to a given composite glyph, and to all composite glyphs used in between. @@ -69,7 +74,7 @@ def _propagate_glyph_anchors(glyphSet, composite, processed): "in glyph {}".format(component.baseGlyph, composite.name) ) else: - _propagate_glyph_anchors(glyphSet, glyph, processed) + _propagate_glyph_anchors(glyphSet, glyph, processed, modified) if any(a.name.startswith("_") for a in glyph.anchors): mark_components.append(component) else: @@ -110,6 +115,9 @@ def _propagate_glyph_anchors(glyphSet, composite, processed): # fontParts API composite.appendAnchor(name, (x, y)) + if to_add: + modified.add(composite.name) + def _get_anchor_data(anchor_data, glyphSet, components, anchor_name): """Get data for an anchor from a list of components.""" diff --git a/tests/filters/propagateAnchors_test.py b/tests/filters/propagateAnchors_test.py index 757e88c6d..e9c3fb31d 100644 --- a/tests/filters/propagateAnchors_test.py +++ b/tests/filters/propagateAnchors_test.py @@ -170,7 +170,9 @@ def test_three_component_glyph(self, font): def test_nested_component_glyph(self, font): name = "amacrondieresis" philter = PropagateAnchorsFilter(include={name}) - assert philter(font) == {name} + # 'amacron' is used as component by 'amacrondieresis' hence it is modified + # as well... + assert philter(font) == {name, "amacron"} assert [(a.name, a.x, a.y) for a in font[name].anchors] == [ ("bottom", 175, 0), ("top", 175, 660), From 44e50462bd0b3e731042788cfe89a5901401b10b Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Wed, 22 Jun 2022 17:26:39 +0100 Subject: [PATCH 4/5] propagateAnchors_test: parametrize and verify processing whole is same as processing parts --- tests/filters/propagateAnchors_test.py | 102 ++++++++++--------------- 1 file changed, 39 insertions(+), 63 deletions(-) diff --git a/tests/filters/propagateAnchors_test.py b/tests/filters/propagateAnchors_test.py index e9c3fb31d..695f3ba08 100644 --- a/tests/filters/propagateAnchors_test.py +++ b/tests/filters/propagateAnchors_test.py @@ -123,6 +123,35 @@ def font(request, FontClass): return font +EXPECTED = { + # single component glyph + "a-cyr": ([("bottom", 175, 0), ("top", 175, 300)], {"a-cyr"}), + # two component glyph + "adieresis": ([("bottom", 175, 0), ("top", 175, 480)], {"adieresis"}), + # one anchor, two component glyph + "amacron": ([("top", 176, 481), ("bottom", 175, 0)], {"amacron"}), + # three component glyph + "adieresismacron": ([("bottom", 175, 0), ("top", 175, 660)], {"adieresismacron"}), + # nested component glyph + "amacrondieresis": ( + [("bottom", 175, 0), ("top", 175, 660)], + # 'amacron' is used as component by 'amacrondieresis' hence it is modified + # as well... + {"amacrondieresis", "amacron"}, + ), + # ligature glyph + "a_a": ( + [ + ("bottom_1", 175, 0), + ("bottom_2", 525, 0), + ("top_1", 175, 300), + ("top_2", 525, 300), + ], + {"a_a"}, + ), +} + + class PropagateAnchorsFilterTest: def test_empty_glyph(self, font): philter = PropagateAnchorsFilter(include={"space"}) @@ -132,74 +161,21 @@ def test_contour_glyph(self, font): philter = PropagateAnchorsFilter(include={"a"}) assert not philter(font) - def test_single_component_glyph(self, font): - philter = PropagateAnchorsFilter(include={"a-cyr"}) - assert philter(font) == {"a-cyr"} - assert [(a.name, a.x, a.y) for a in font["a-cyr"].anchors] == [ - ("bottom", 175, 0), - ("top", 175, 300), - ] - - def test_two_component_glyph(self, font): - name = "adieresis" - philter = PropagateAnchorsFilter(include={name}) - assert philter(font) == {name} - assert [(a.name, a.x, a.y) for a in font[name].anchors] == [ - ("bottom", 175, 0), - ("top", 175, 480), - ] - - def test_one_anchor_two_component_glyph(self, font): - name = "amacron" - philter = PropagateAnchorsFilter(include={name}) - assert philter(font) == {name} - assert [(a.name, a.x, a.y) for a in font[name].anchors] == [ - ("top", 176, 481), - ("bottom", 175, 0), - ] - - def test_three_component_glyph(self, font): - name = "adieresismacron" + @pytest.mark.parametrize("name", list(EXPECTED)) + def test_include_one_glyph_at_a_time(self, font, name): philter = PropagateAnchorsFilter(include={name}) - assert philter(font) == {name} - assert [(a.name, a.x, a.y) for a in font[name].anchors] == [ - ("bottom", 175, 0), - ("top", 175, 660), - ] - - def test_nested_component_glyph(self, font): - name = "amacrondieresis" - philter = PropagateAnchorsFilter(include={name}) - # 'amacron' is used as component by 'amacrondieresis' hence it is modified - # as well... - assert philter(font) == {name, "amacron"} - assert [(a.name, a.x, a.y) for a in font[name].anchors] == [ - ("bottom", 175, 0), - ("top", 175, 660), - ] - - def test_ligature_glyph(self, font): - name = "a_a" - philter = PropagateAnchorsFilter(include={name}) - assert philter(font) == {name} - assert [(a.name, a.x, a.y) for a in font[name].anchors] == [ - ("bottom_1", 175, 0), - ("bottom_2", 525, 0), - ("top_1", 175, 300), - ("top_2", 525, 300), - ] + modified = philter(font) + + expected_anchors, expected_modified = EXPECTED[name] + assert modified == expected_modified + assert [(a.name, a.x, a.y) for a in font[name].anchors] == expected_anchors def test_whole_font(self, font): philter = PropagateAnchorsFilter() modified = philter(font) - assert modified == { - "a-cyr", - "amacron", - "adieresis", - "adieresismacron", - "amacrondieresis", - "a_a", - } + assert modified == set(EXPECTED) + for name, (expected_anchors, _) in EXPECTED.items(): + assert [(a.name, a.x, a.y) for a in font[name].anchors] == expected_anchors def test_fail_during_anchor_propagation(self, font): name = "emacron" From 4af581079d0b2dd710ecba9f21a5155151fadf1b Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Wed, 22 Jun 2022 18:05:24 +0100 Subject: [PATCH 5/5] typo --- tests/filters/decomposeTransformedComponents_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/filters/decomposeTransformedComponents_test.py b/tests/filters/decomposeTransformedComponents_test.py index 0cbf2604c..d66a42d14 100644 --- a/tests/filters/decomposeTransformedComponents_test.py +++ b/tests/filters/decomposeTransformedComponents_test.py @@ -21,7 +21,7 @@ def test_transformed_components(self, FontClass): pen = c.getPen() pen.addComponent("six.lf", (1, 0, 0, 1, 0, 0)) - # nine.lf has one transformed component of a componenent + # nine.lf has one transformed component of a component b = ufo.newGlyph("nine.lf") b.width = 300 pen = b.getPen()