From e13b781526029fb9288e308fb463457773cc755a Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Tue, 4 May 2021 17:08:59 +0100 Subject: [PATCH 1/6] removeOverlaps: print glyph name when pathops.simplify fails Sometimes skia-pathops simplify may fail (for unknown reasons which I'm still trying to debug). It's a good idea to know the name of the offending glyph https://github.com/google/fonts/issues/3365 --- Lib/fontTools/ttLib/removeOverlaps.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/Lib/fontTools/ttLib/removeOverlaps.py b/Lib/fontTools/ttLib/removeOverlaps.py index fb5c77ab6..7a5567233 100644 --- a/Lib/fontTools/ttLib/removeOverlaps.py +++ b/Lib/fontTools/ttLib/removeOverlaps.py @@ -18,6 +18,10 @@ import pathops __all__ = ["removeOverlaps"] +class RemoveOverlapsError(Exception): + pass + + log = logging.getLogger("fontTools.ttLib.removeOverlaps") _TTGlyphMapping = Mapping[str, ttFont._TTGlyph] @@ -93,7 +97,12 @@ def removeTTGlyphOverlaps( path = skPathFromGlyph(glyphName, glyphSet) # remove overlaps - path2 = pathops.simplify(path, clockwise=path.clockwise) + try: + path2 = pathops.simplify(path, clockwise=path.clockwise) + except pathops.PathOpsError as e: + raise RemoveOverlapsError( + f"Failed to remove overlaps from glyph {glyphName!r}" + ) from e # replace TTGlyph if simplified path is different (ignoring contour order) if {tuple(c) for c in path.contours} != {tuple(c) for c in path2.contours}: From 84b851398b9ff594866f7194892d839b3c8fc014 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 6 May 2021 13:02:00 +0100 Subject: [PATCH 2/6] removeOverlaps: try rounding to ints before simplify to workaround skia bug Fixes https://github.com/google/fonts/issues/3365 See https://bugs.chromium.org/p/skia/issues/detail?id=11958 for details --- Lib/fontTools/ttLib/removeOverlaps.py | 30 +++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/Lib/fontTools/ttLib/removeOverlaps.py b/Lib/fontTools/ttLib/removeOverlaps.py index 7a5567233..7069cf707 100644 --- a/Lib/fontTools/ttLib/removeOverlaps.py +++ b/Lib/fontTools/ttLib/removeOverlaps.py @@ -7,6 +7,7 @@ import itertools import logging from typing import Iterable, Optional, Mapping +from fontTools.misc.roundTools import otRound from fontTools.ttLib import ttFont from fontTools.ttLib.tables import _g_l_y_f from fontTools.ttLib.tables import _h_m_t_x @@ -99,10 +100,31 @@ def removeTTGlyphOverlaps( # remove overlaps try: path2 = pathops.simplify(path, clockwise=path.clockwise) - except pathops.PathOpsError as e: - raise RemoveOverlapsError( - f"Failed to remove overlaps from glyph {glyphName!r}" - ) from e + except pathops.PathOpsError: + # skia-pathops has a bug where it sometimes fails to simplify paths when + # there are float coordinates and control points are very close to one + # another. Rounding coordinates to integers works around the bug. + # Since we are going to round glyf coordinates later on anyway, here + # it is ok(-ish) to also round before simplify. Better than failing the + # whole process for the entire font. + # https://bugs.chromium.org/p/skia/issues/detail?id=11958 + # https://github.com/google/fonts/issues/3365 + path2 = pathops.Path() + for verb, points in path: + path2.add(verb, *((otRound(p[0]), otRound(p[1])) for p in points)) + try: + path2.simplify(clockwise=path.clockwise) + except pathops.PathOpsError as e: + path.dump() + raise RemoveOverlapsError( + f"Failed to remove overlaps from glyph {glyphName!r}" + ) from e + else: + log.debug( + "skia-pathops failed to simplify '%s' with float coordinates, " + "but succeded using rounded integer coordinates", + glyphName, + ) # replace TTGlyph if simplified path is different (ignoring contour order) if {tuple(c) for c in path.contours} != {tuple(c) for c in path2.contours}: From d4d3d954140707d6e45df2c3939e2f0025cbe366 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 6 May 2021 20:02:00 +0100 Subject: [PATCH 3/6] split simplify logic to separate func for easier unit-testing --- Lib/fontTools/ttLib/removeOverlaps.py | 59 +++++++++++++++------------ 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/Lib/fontTools/ttLib/removeOverlaps.py b/Lib/fontTools/ttLib/removeOverlaps.py index 7069cf707..188edb1e8 100644 --- a/Lib/fontTools/ttLib/removeOverlaps.py +++ b/Lib/fontTools/ttLib/removeOverlaps.py @@ -81,6 +81,37 @@ def ttfGlyphFromSkPath(path: pathops.Path) -> _g_l_y_f.Glyph: return glyph +def _simplify(path: pathops.Path, debugGlyphName: str) -> pathops.Path: + # skia-pathops has a bug where it sometimes fails to simplify paths when there + # are float coordinates and control points are very close to one another. + # Rounding coordinates to integers works around the bug. + # Since we are going to round glyf coordinates later on anyway, here it is + # ok(-ish) to also round before simplify. Better than failing the whole process + # for the entire font. + # https://bugs.chromium.org/p/skia/issues/detail?id=11958 + # https://github.com/google/fonts/issues/3365 + try: + path2 = pathops.simplify(path, clockwise=path.clockwise) + except pathops.PathOpsError: + path2 = pathops.Path() + for verb, points in path: + path2.add(verb, *((otRound(p[0]), otRound(p[1])) for p in points)) + try: + path2.simplify(clockwise=path.clockwise) + except pathops.PathOpsError as e: + path.dump() + raise RemoveOverlapsError( + f"Failed to remove overlaps from glyph {debugGlyphName!r}" + ) from e + else: + log.debug( + "skia-pathops failed to simplify '%s' with float coordinates, " + "but succeded using rounded integer coordinates", + debugGlyphName, + ) + return path2 + + def removeTTGlyphOverlaps( glyphName: str, glyphSet: _TTGlyphMapping, @@ -98,33 +129,7 @@ def removeTTGlyphOverlaps( path = skPathFromGlyph(glyphName, glyphSet) # remove overlaps - try: - path2 = pathops.simplify(path, clockwise=path.clockwise) - except pathops.PathOpsError: - # skia-pathops has a bug where it sometimes fails to simplify paths when - # there are float coordinates and control points are very close to one - # another. Rounding coordinates to integers works around the bug. - # Since we are going to round glyf coordinates later on anyway, here - # it is ok(-ish) to also round before simplify. Better than failing the - # whole process for the entire font. - # https://bugs.chromium.org/p/skia/issues/detail?id=11958 - # https://github.com/google/fonts/issues/3365 - path2 = pathops.Path() - for verb, points in path: - path2.add(verb, *((otRound(p[0]), otRound(p[1])) for p in points)) - try: - path2.simplify(clockwise=path.clockwise) - except pathops.PathOpsError as e: - path.dump() - raise RemoveOverlapsError( - f"Failed to remove overlaps from glyph {glyphName!r}" - ) from e - else: - log.debug( - "skia-pathops failed to simplify '%s' with float coordinates, " - "but succeded using rounded integer coordinates", - glyphName, - ) + path2 = _simplify(path, glyphName) # replace TTGlyph if simplified path is different (ignoring contour order) if {tuple(c) for c in path.contours} != {tuple(c) for c in path2.contours}: From 27e8943d3c5ed616ea9c579d4ad994ca256d884a Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 6 May 2021 20:07:44 +0100 Subject: [PATCH 4/6] add test for pathops simplify bug workaround --- Tests/ttLib/removeOverlaps_test.py | 53 ++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 Tests/ttLib/removeOverlaps_test.py diff --git a/Tests/ttLib/removeOverlaps_test.py b/Tests/ttLib/removeOverlaps_test.py new file mode 100644 index 000000000..d4cb8558f --- /dev/null +++ b/Tests/ttLib/removeOverlaps_test.py @@ -0,0 +1,53 @@ +import logging +from fontTools.ttLib.removeOverlaps import _simplify +import pathops +import pytest + + +def test_pathops_simplify_bug_workaround(caplog): + # Paths extracted from Noto Sans Ethiopic instance that fails skia-pathops + # https://github.com/google/fonts/issues/3365 + # https://bugs.chromium.org/p/skia/issues/detail?id=11958 + path = pathops.Path() + path.moveTo(550.461, 0) + path.lineTo(550.461, 366.308) + path.lineTo(713.229, 366.308) + path.lineTo(713.229, 0) + path.close() + path.moveTo(574.46, 0) + path.lineTo(574.46, 276.231) + path.lineTo(737.768, 276.231) + path.quadTo(820.075, 276.231, 859.806, 242.654) + path.quadTo(899.537, 209.077, 899.537, 144.154) + path.quadTo(899.537, 79, 853.46, 39.5) + path.quadTo(807.383, 0, 712.383, 0) + path.close() + + # check that it fails without workaround + with pytest.raises(pathops.PathOpsError): + pathops.simplify(path) + + # check our workaround works (but with a warning) + with caplog.at_level(logging.DEBUG, logger="fontTools.ttLib.removeOverlaps"): + result = _simplify(path, debugGlyphName="a") + + assert "skia-pathops failed to simplify 'a' with float coordinates" in caplog.text + + expected = pathops.Path() + expected.moveTo(550, 0) + expected.lineTo(550, 366) + expected.lineTo(713, 366) + expected.lineTo(713, 276) + expected.lineTo(738, 276) + expected.quadTo(820, 276, 860, 243) + expected.quadTo(900, 209, 900, 144) + expected.quadTo(900, 79, 853, 40) + expected.quadTo(807.242, 0.211, 713, 0.001) + expected.lineTo(713, 0) + expected.close() + + rounded_result = pathops.Path() + for verb, pts in result: + rounded_result.add(verb, *((round(p[0], 3), round(p[1], 3)) for p in pts)) + + assert expected == rounded_result From 80598d3c2c03ab7f2de7ee80a771ce2c28b46afc Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 6 May 2021 20:12:14 +0100 Subject: [PATCH 5/6] skip removeOveraps_test if pathops can't be imported --- Tests/ttLib/removeOverlaps_test.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Tests/ttLib/removeOverlaps_test.py b/Tests/ttLib/removeOverlaps_test.py index d4cb8558f..383ec7b35 100644 --- a/Tests/ttLib/removeOverlaps_test.py +++ b/Tests/ttLib/removeOverlaps_test.py @@ -1,8 +1,10 @@ import logging -from fontTools.ttLib.removeOverlaps import _simplify -import pathops import pytest +pathops = pytest.importorskip("pathops") + +from fontTools.ttLib.removeOverlaps import _simplify + def test_pathops_simplify_bug_workaround(caplog): # Paths extracted from Noto Sans Ethiopic instance that fails skia-pathops From b0e24384c282e07c5a84fb183e912aab3ab219ac Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Fri, 7 May 2021 09:49:33 +0100 Subject: [PATCH 6/6] minor refactorings following review comment https://github.com/fonttools/fonttools/pull/2288#discussion_r627907922 --- Lib/fontTools/ttLib/removeOverlaps.py | 49 ++++++++++++++++----------- Tests/ttLib/removeOverlaps_test.py | 8 ++--- 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/Lib/fontTools/ttLib/removeOverlaps.py b/Lib/fontTools/ttLib/removeOverlaps.py index 188edb1e8..bb6674673 100644 --- a/Lib/fontTools/ttLib/removeOverlaps.py +++ b/Lib/fontTools/ttLib/removeOverlaps.py @@ -5,7 +5,7 @@ Requires https://github.com/fonttools/skia-pathops import itertools import logging -from typing import Iterable, Optional, Mapping +from typing import Callable, Iterable, Optional, Mapping from fontTools.misc.roundTools import otRound from fontTools.ttLib import ttFont @@ -81,6 +81,15 @@ def ttfGlyphFromSkPath(path: pathops.Path) -> _g_l_y_f.Glyph: return glyph +def _round_path( + path: pathops.Path, round: Callable[[float], float] = otRound +) -> pathops.Path: + rounded_path = pathops.Path() + for verb, points in path: + rounded_path.add(verb, *((round(p[0]), round(p[1])) for p in points)) + return rounded_path + + def _simplify(path: pathops.Path, debugGlyphName: str) -> pathops.Path: # skia-pathops has a bug where it sometimes fails to simplify paths when there # are float coordinates and control points are very close to one another. @@ -90,26 +99,28 @@ def _simplify(path: pathops.Path, debugGlyphName: str) -> pathops.Path: # for the entire font. # https://bugs.chromium.org/p/skia/issues/detail?id=11958 # https://github.com/google/fonts/issues/3365 + # TODO(anthrotype): remove once this Skia bug is fixed try: - path2 = pathops.simplify(path, clockwise=path.clockwise) + return pathops.simplify(path, clockwise=path.clockwise) except pathops.PathOpsError: - path2 = pathops.Path() - for verb, points in path: - path2.add(verb, *((otRound(p[0]), otRound(p[1])) for p in points)) - try: - path2.simplify(clockwise=path.clockwise) - except pathops.PathOpsError as e: - path.dump() - raise RemoveOverlapsError( - f"Failed to remove overlaps from glyph {debugGlyphName!r}" - ) from e - else: - log.debug( - "skia-pathops failed to simplify '%s' with float coordinates, " - "but succeded using rounded integer coordinates", - debugGlyphName, - ) - return path2 + pass + + path = _round_path(path) + try: + path = pathops.simplify(path, clockwise=path.clockwise) + log.debug( + "skia-pathops failed to simplify '%s' with float coordinates, " + "but succeded using rounded integer coordinates", + debugGlyphName, + ) + return path + except pathops.PathOpsError as e: + path.dump() + raise RemoveOverlapsError( + f"Failed to remove overlaps from glyph {debugGlyphName!r}" + ) from e + + raise AssertionError("Unreachable") def removeTTGlyphOverlaps( diff --git a/Tests/ttLib/removeOverlaps_test.py b/Tests/ttLib/removeOverlaps_test.py index 383ec7b35..1320c9be5 100644 --- a/Tests/ttLib/removeOverlaps_test.py +++ b/Tests/ttLib/removeOverlaps_test.py @@ -3,7 +3,7 @@ import pytest pathops = pytest.importorskip("pathops") -from fontTools.ttLib.removeOverlaps import _simplify +from fontTools.ttLib.removeOverlaps import _simplify, _round_path def test_pathops_simplify_bug_workaround(caplog): @@ -48,8 +48,4 @@ def test_pathops_simplify_bug_workaround(caplog): expected.lineTo(713, 0) expected.close() - rounded_result = pathops.Path() - for verb, pts in result: - rounded_result.add(verb, *((round(p[0], 3), round(p[1], 3)) for p in pts)) - - assert expected == rounded_result + assert expected == _round_path(result, round=lambda v: round(v, 3))