Merge pull request #2288 from fonttools/remove-overlaps-print-glyph-error

removeOverlaps: work around pathops.simplify error

Lib/fontTools/ttLib/removeOverlaps.py

@@ -5,8 +5,9 @@ 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
 from fontTools.ttLib.tables import _g_l_y_f
 from fontTools.ttLib.tables import _h_m_t_x
@@ -18,6 +19,10 @@ import pathops
 __all__ = ["removeOverlaps"]
 
 
+class RemoveOverlapsError(Exception):
+    pass
+
+
 log = logging.getLogger("fontTools.ttLib.removeOverlaps")
 
 _TTGlyphMapping = Mapping[str, ttFont._TTGlyph]
@@ -76,6 +81,48 @@ 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.
+    # 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
+    # TODO(anthrotype): remove once this Skia bug is fixed
+    try:
+        return pathops.simplify(path, clockwise=path.clockwise)
+    except pathops.PathOpsError:
+        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(
     glyphName: str,
     glyphSet: _TTGlyphMapping,
@@ -93,7 +140,7 @@ def removeTTGlyphOverlaps(
         path = skPathFromGlyph(glyphName, glyphSet)
 
         # remove overlaps
-        path2 = pathops.simplify(path, clockwise=path.clockwise)
+        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}:

Tests/ttLib/removeOverlaps_test.py

@@ -0,0 +1,51 @@
+import logging
+import pytest
+
+pathops = pytest.importorskip("pathops")
+
+from fontTools.ttLib.removeOverlaps import _simplify, _round_path
+
+
+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()
+
+    assert expected == _round_path(result, round=lambda v: round(v, 3))