fonttools / skia-pathops

Python bindings for the Skia library's Path Ops
https://skia.org/docs/dev/present/pathops/
BSD 3-Clause "New" or "Revised" License
47 stars 14 forks source link

Regression from 0.7 to 0.8 with Path.segments converting to qCurveTo #68

Closed cas-- closed 1 year ago

cas-- commented 1 year ago

Testing Picosvg with a flag for Mexico and encountered the error below. It was resolved by downgrading skia-pathops to version 0.7.4.

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "./picosvg/svg.py", line 1337, in topicosvg
    svg.topicosvg(
  File "./picosvg/svg.py", line 1361, in topicosvg
    self.simplify(inplace=True)
  File "./picosvg/svg.py", line 813, in simplify
    self._simplify()
  File "./picosvg/svg.py", line 745, in _simplify
    paths = list(self._stroke(paths[0]))
  File "./picosvg/svg.py", line 825, in _stroke
    stroke = shape.as_path().update_path(shape.stroke_commands(self.tolerance))
  File "./picosvg/svg_types.py", line 696, in update_path
    for cmd, args in svg_cmds:
  File "./picosvg/svg_pathops.py", line 102, in svg_commands
    for svg_cmd, svg_args in _SKIA_CMD_TO_SVG_CMD[cmd](points):
  File "./picosvg/svg_pathops.py", line 56, in _qcurveto_to_svg
    yield ("Q", control_pt + end_pt)
TypeError: can only concatenate tuple (not "NoneType") to tuple

I managed to isolate the path with the issue and it seems that the path.segments is returning None as one of the points and missing the initial moveTo.

>>> list(path.segments)
[('qCurveTo', ((493.5769958496094, 279.9679870605469), (492.3710021972656, 281.17401123046875), (490.6650085449219, 281.17401123046875), (489.4590148925781, 279.9679870605469), (489.4590148925781, 278.2619934082031), (490.6650085449219, 277.0559997558594), (492.3710021972656, 277.0559997558594), (493.5769958496094, 278.2619934082031), None)), ('closePath', ())]

I have created a quick test however I am unable to speculate on the actual issue

    def test_path_qcurve(self):
        path = Path()
        path.moveTo(493.577, 279.115)
        path.quadTo(493.577, 279.968, 492.974, 280.571)
        path.quadTo(492.371, 281.174, 491.518, 281.174)
        path.quadTo(490.665, 281.174, 490.062, 280.571)
        path.quadTo(489.459, 279.968, 489.459, 279.115)
        path.quadTo(489.459, 278.262, 490.062, 277.659)
        path.quadTo(490.665, 277.056, 491.518, 277.056)
        path.quadTo(492.371, 277.056, 492.974, 277.659)
        path.quadTo(493.577, 278.262, 493.577, 279.115)
        path.close()

        items = list(path)
        assert len(items) == 10
        assert list(path.segments) == [
            ("moveTo", ((493.5769958496094, 279.114990234375),)),
            (
                "qCurveTo",
                (
                    (493.5769958496094, 279.9679870605469),
                    (492.3710021972656, 281.17401123046875),
                    (490.6650085449219, 281.17401123046875),
                    (489.4590148925781, 279.9679870605469),
                    (489.4590148925781, 278.2619934082031),
                    (490.6650085449219, 277.0559997558594),
                    (492.3710021972656, 277.0559997558594),
                    (493.5769958496094, 278.2619934082031),
                    (493.5769958496094, 279.114990234375),
                ),
            ),
            ("closePath", ()),
        ]
anthrotype commented 1 year ago

thanks, I believe this is the same as https://github.com/googlefonts/nanoemoji/issues/455, and it's picosvg not knowing about a new feature that had been added in https://github.com/fonttools/skia-pathops/pull/66

I will fix it soon, please keep it pinned for now, apologies for any unexpected breakages

anthrotype commented 1 year ago

fixed in picosvg https://github.com/googlefonts/picosvg/pull/305