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/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/Lib/ufo2ft/util.py b/Lib/ufo2ft/util.py index 0a1511b39..87a2b7f25 100644 --- a/Lib/ufo2ft/util.py +++ b/Lib/ufo2ft/util.py @@ -546,3 +546,32 @@ 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: + try: + baseGlyph = glyphSet[component.baseGlyph] + except KeyError: + continue + componentDepth = getMaxComponentDepth( + baseGlyph, glyphSet, initialMaxComponentDepth + ) + maxComponentDepth = max(maxComponentDepth, componentDepth) + + return maxComponentDepth diff --git a/tests/filters/decomposeTransformedComponents_test.py b/tests/filters/decomposeTransformedComponents_test.py index 7c528016b..54a79c57b 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() diff --git a/tests/filters/propagateAnchors_test.py b/tests/filters/propagateAnchors_test.py index 757e88c6d..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,72 +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}) - 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_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"