Merge pull request #272 from googlefonts/arcful_dodger

Upgrade arc handling to support circles and ellipses formed of two arcs

src/picosvg/svg_reuse.py

@@ -17,7 +17,7 @@
 import copy
 import dataclasses
 from itertools import islice
-from math import atan2
+from math import atan2, sqrt
 from picosvg.geometric_types import Vector, almost_equal
 from picosvg.svg_types import SVGShape, SVGPath
 from typing import Callable, Generator, Iterable, Optional, Tuple
@@ -36,13 +36,102 @@ def _first_move(path: SVGPath) -> Tuple[float, float]:
     return args
 
 
-def _vectors(path: SVGPath) -> Generator[Vector, None, None]:
+def _farthest(rx, ry, large_arc, line_length) -> float:
+    """
+    We have an ellipse centered on 0,0 with specified rx/ry.
+    There is a horizontal line that intersects it twice and the
+    line segment between intersections has a known length.
+
+    Consider the ellipse as two arcs, one above and one below our
+    line segment. Return y-offset of the farthest point on one of these
+    arcs. If large_arc is 0, choose the smaller, otherwise the larger.
+
+    If the ellipse is too small to accomodate a line segment of the
+    specified length then scale it until it can.
+
+    Note: we skip ellipse rotation for now and solve by placing
+    the ellipse on the origin and solving the ellipse equation for y when
+    x is at +/- 0.5 * the length of our line segment.
+    """
+    assert line_length > 0, f"line_length {line_length} must be > 0"
+    x = line_length / 2
+
+    y = 0
+
+    if almost_equal(2 * rx, line_length):
+        # simple case common in real input: the ellipse is exactly wide enough
+        # no scaling, farthest point for both arcs is simply ry
+        pass
+
+    elif 2 * rx >= line_length:
+        # The ellipse is big enough that a line through could have
+        # the desired length
+        y = pow(ry, 2) - pow(ry, 2) * pow(x, 2) / pow(rx, 2)
+        y = sqrt(y)
+
+    else:
+        # The ellipse is too small and will be scaled
+        # scale: big enough when 2rx == line length at y=0
+        # max distance from line is at scaled ry
+        # same for large vs small arc
+        scale = line_length / (2 * rx)
+        rx *= scale
+        ry *= scale
+
+    large_arc_farthest = y + ry
+    small_arc_farthest = ry - y
+
+    if large_arc == 0:
+        return small_arc_farthest
+    else:
+        return large_arc_farthest
+
+
+def _vectors(path: SVGPath) -> Iterable[Vector]:
     for cmd, args in path:
         x_coord_idxs, y_coord_idxs = svg_meta.cmd_coords(cmd)
-        if cmd.lower() == "z":
+
+        assert cmd == "M" or cmd == cmd.lower(), "path should be relative"
+        if cmd == "M":
+            x, y = args[x_coord_idxs[-1]], args[y_coord_idxs[-1]]
+
+        if cmd == "z":
             yield Vector(0.0, 0.0)
-        else:
-            yield Vector(args[x_coord_idxs[-1]], args[y_coord_idxs[-1]])
+            continue
+
+        if cmd == "a":
+            # arcs can confuse search for "significant" x/y movement
+            # because, if x or y is unchanged it won't trigger
+            # the significant movement test for that axis even if it
+            # curves way off it. When svg circles and ellipses convert
+            # we typically get two arcs with no y movement.
+            #
+            # Address this by treating the arc as two vectors:
+            #   1) to the endpoint
+            #   2) to the farthest point on the arc - to the end point
+            # Sum of these takes you to the endpoint, but you keep interesting
+            # movement along the way in play.
+
+            rx, ry, x_rotation, large_arc, sweep, end_x, end_y = args
+
+            # Dumbed down implementation aimed at what we see in real inputs:
+            # handle only non-rotated ellipses where endpoint didn't move on one axis.
+            # TODO: convert *all* arcs to two-vector form
+            if x_rotation == 0 and 0 in (end_x, end_y):
+                if almost_equal(end_y, 0):
+                    y_max = _farthest(rx, ry, large_arc, abs(end_x))
+                    yield Vector(end_x, 0.0)
+                    yield Vector(0.0, y_max)
+                    continue
+                elif almost_equal(end_x, 0):
+                    # since we have no rotation we can do farthest with coords flipped
+                    x_max = _farthest(ry, rx, large_arc, abs(end_y))
+                    yield Vector(x_max, 0.0)
+                    yield Vector(0.0, end_y)
+                    continue
+
+        # default: vector to endpoint
+        yield Vector(args[x_coord_idxs[-1]], args[y_coord_idxs[-1]])
 
 
 def _nth_vector(path: SVGPath, n: int) -> Vector:
@@ -102,9 +191,9 @@ def _affine_friendly(shape: SVGShape) -> SVGPath:
     if shape is path:
         path = copy.deepcopy(path)
     return (
-        path.relative(inplace=True)
-        .explicit_lines(inplace=True)
+        path.explicit_lines(inplace=True)
         .expand_shorthand(inplace=True)
+        .relative(inplace=True)
     )
 
 
@@ -119,6 +208,7 @@ def _affine_callback(affine, subpath_start, curr_pos, cmd, args, *_unused):
     assert len(x_coord_idxs) == len(y_coord_idxs), f"{cmd}, {args}"
 
     args = list(args)  # we'd like to mutate 'em
+
     for x_coord_idx, y_coord_idx in zip(x_coord_idxs, y_coord_idxs):
         if cmd == cmd.upper():
             # for an absolute cmd allow translation: map_point
@@ -152,14 +242,13 @@ def normalize(shape: SVGShape, tolerance: float) -> SVGPath:
     scaled, rotated, etc.
 
     Intended use is to normalize multiple shapes to identify opportunity for reuse."""
-
     path = _affine_friendly(dataclasses.replace(shape, id=""))
 
     # Make path relative, with first coord at 0,0
     x, y = _first_move(path)
     path.move(-x, -y, inplace=True)
 
-    # Normlize first activity to [1 0]; eliminates rotation and uniform scaling
+    # Normalize first activity to [1 0]; eliminates rotation and uniform scaling
     _, vec_first = _first_significant(_vectors(path), lambda v: v.norm(), tolerance)
     if vec_first and not vec_first.almost_equals(Vector(1, 0)):
         assert (
@@ -168,7 +257,7 @@ def normalize(shape: SVGShape, tolerance: float) -> SVGPath:
         affinex = _affine_vec2vec(vec_first, Vector(1, 0))
         path.walk(lambda *args: _affine_callback(affinex, *args))
 
-    # Normlize first y activity to 1.0; eliminates mirroring and non-uniform scaling
+    # Normalize first y activity to 1.0; eliminates mirroring and non-uniform scaling
     _, vecy = _first_significant(_vectors(path), lambda v: v.y, tolerance)
     if vecy and not almost_equal(vecy.y, 1.0):
         assert vecy.norm() > tolerance, f"vecy too close to 0-magnitude: {vecy}"
@@ -188,7 +277,9 @@ def _apply_affine(affine: Affine2D, s: SVGPath) -> SVGPath:
     return s_prime
 
 
-def _try_affine(affine: Affine2D, s1: SVGPath, s2: SVGPath, tolerance: float):
+def _try_affine(
+    affine: Affine2D, s1: SVGPath, s2: SVGPath, tolerance: float, comment: str
+):
     s1_prime = _apply_affine(affine, s1)
     return s1_prime.almost_equals(s2, tolerance)
 
@@ -197,7 +288,7 @@ def _round(affine, s1, s2, tolerance):
     # TODO bsearch?
     for i in _ROUND_RANGE:
         rounded = affine.round(i)
-        if _try_affine(rounded, s1, s2, tolerance):
+        if _try_affine(rounded, s1, s2, tolerance, f"round {i}"):
             return rounded
     return affine  # give up
 
@@ -225,7 +316,7 @@ def affine_between(s1: SVGShape, s2: SVGShape, tolerance: float) -> Optional[Aff
     s2x, s2y = _first_move(s2)
 
     affine = Affine2D.identity().translate(s2x - s1x, s2y - s1y)
-    if _try_affine(affine, s1, s2, tolerance):
+    if _try_affine(affine, s1, s2, tolerance, "same start point"):
         return _round(affine, s1, s2, tolerance)
 
     # Align the first edge with a significant x part.
@@ -246,7 +337,7 @@ def affine_between(s1: SVGShape, s2: SVGShape, tolerance: float) -> Optional[Aff
     origin_to_s2 = Affine2D.identity().translate(s2x, s2y)
 
     affine = Affine2D.compose_ltr((s1_to_origin, s1_vec1_to_s2_vec1x, origin_to_s2))
-    if _try_affine(affine, s1, s2, tolerance):
+    if _try_affine(affine, s1, s2, tolerance, "align vec1x"):
         return _round(affine, s1, s2, tolerance)
 
     # Could be non-uniform scaling and/or mirroring
@@ -285,7 +376,7 @@ def affine_between(s1: SVGShape, s2: SVGShape, tolerance: float) -> Optional[Aff
                 origin_to_s2,
             )
         )
-        if _try_affine(affine, s1, s2, tolerance):
+        if _try_affine(affine, s1, s2, tolerance, "align vecy"):
             return _round(affine, s1, s2, tolerance)
 
     # If we still aren't the same give up

src/picosvg/svg_types.py

@@ -592,7 +592,11 @@ def absolute_moveto(self, inplace=False) -> "SVGPath":
 
     def relative(self, inplace=False) -> "SVGPath":
         """Returns equivalent path with only relative commands."""
-        return self._rewrite_path(_absolute_to_relative, inplace)
+        result = self._rewrite_path(_absolute_to_relative, inplace)
+        # First move is always absolute
+        if result.d[0] == "m":
+            result.d = "M" + result.d[1:]
+        return result
 
     def explicit_lines(self, inplace=False):
         """Replace all vertical/horizontal lines with line to (x,y)."""

tests/svg_reuse_test.py

@@ -12,9 +12,10 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+from picosvg.geometric_types import Vector
 from picosvg.svg_types import SVGCircle, SVGPath, SVGRect
 from picosvg.svg_transform import Affine2D
-from picosvg.svg_reuse import normalize, affine_between
+from picosvg.svg_reuse import _affine_friendly, _vectors, normalize, affine_between
 import pytest
 
 
@@ -39,6 +40,70 @@ def test_svg_normalization(shape, tolerance, expected_normalization):
     assert normalized.round_floats(4).d == expected_normalization
 
 
+@pytest.mark.parametrize(
+    "path, expected_vectors",
+    [
+        # vectors for a box
+        (
+            "M10,10 h10 v10 h-10 z",
+            (
+                Vector(10.0, 10.0),
+                Vector(10.0, 0.0),
+                Vector(0.0, 10.0),
+                Vector(-10.0, 0.0),
+                Vector(0.0, 0.0),
+            ),
+        ),
+        # observed problem: an arc whose start and end share a dimension is
+        # taken to have 0 magnitude in that direction even if it bulges out
+        # e.g. an arc from (1, 0) to (2, 0) is taken to have 0 magnitude on y
+        # this may result in affine_between failures.
+        # This is particularly problemmatic due to circles and ellipses in svg
+        # converting to two arcs, one for the top and one for the bottom.
+        # https://github.com/googlefonts/picosvg/issues/271
+        (
+            # arc from 0,0 to 2,0. Apex and farthest point at 1,0.5
+            # vectors formed by start => farthest, farthest => end
+            "M0,0 a 1 0.5 0 1 1 2,0",
+            (
+                Vector(0.0, 0.0),
+                Vector(2.0, 0.0),
+                Vector(0.0, 0.5),
+            ),
+        ),
+        # https://github.com/googlefonts/picosvg/issues/271
+        # As above, but on move on y, none on x
+        (
+            # arc from 0,0 to 0,2. Apex and farthest point at 0,0.5
+            # vectors formed by start => farthest, farthest => end
+            "M0,0 a 0.5 1 0 1 1 0,2",
+            (
+                Vector(0.0, 0.0),
+                Vector(0.5, 0.0),
+                Vector(0.0, 2.0),
+            ),
+        ),
+        # https://github.com/googlefonts/picosvg/issues/271
+        # Arc from Noto Emoji that was resulting in sqrt of a very small negative
+        (
+            "M0,0 a1.75 1.73 0 1 1 -3.5,0 a1.75 1.73 0 1 1 3.5,0 z",
+            (
+                Vector(0.0, 0.0),
+                Vector(-3.5, 0.0),
+                Vector(0.0, 1.73),
+                Vector(3.5, 0.0),
+                Vector(0.0, 1.73),
+                Vector(0.0, 0.0),
+            ),
+        ),
+    ],
+)
+def test_vectors_for_path(path, expected_vectors):
+    assert (
+        tuple(_vectors(_affine_friendly(SVGPath(d=path)))) == expected_vectors
+    ), f"Wrong vectors for {path}"
+
+
 @pytest.mark.parametrize(
     "s1, s2, expected_affine, tolerance",
     [
@@ -157,14 +222,50 @@ def test_svg_normalization(shape, tolerance, expected_normalization):
             Affine2D(1.5, 0, 0, 0.5, -4, 0),
             0.01,
         ),
+        # https://github.com/googlefonts/picosvg/issues/271 arcs whose start/end match in a dimension fail
+        # my arc is marginally taller than your arc
+        (
+            SVGPath(d="M0,0 a 1 0.5 0 1 1 2 0"),
+            SVGPath(d="M0,0 a 1 1 0 1 1 2 0"),
+            Affine2D.identity().scale(1, 2),
+            0.01,
+        ),
+        # https://github.com/googlefonts/picosvg/issues/271 arcs whose start/end match in a dimension fail
+        # Example that previously normalized the same but didn't find an affine between.
+        (
+            SVGPath(
+                d="M104.64,10.08 A40.64 6.08 0 1 1 23.36,10.08 A40.64 6.08 0 1 1 104.64,10.08 Z"
+            ),
+            SVGPath(
+                d="M99.63,23.34 A36.19 4.81 0 1 1 27.25,23.34 A36.19 4.81 0 1 1 99.63,23.34 Z"
+            ),
+            Affine2D(0.8905, 0.0, 0.0, 0.7911, 6.4479, 15.3655),
+            0.01,
+        ),
+        # https://github.com/googlefonts/picosvg/issues/271 arcs whose start/end match in a dimension fail, ex2
+        # Example that didn't even normalize the same previously.
+        (
+            SVGPath(
+                d="M119.47,90.07 A55.47 10.49 0 1 1 8.53,90.07 A55.47 10.49 0 1 1 119.47,90.07 Z"
+            ),
+            SVGPath(
+                d="M94.09,71.71 A12.2 3.92 0 1 1 69.69,71.71 A12.2 3.92 0 1 1 94.09,71.71 Z"
+            ),
+            Affine2D(0.2199, 0.0, 0.0, 0.3737, 67.8139, 38.0518),
+            0.01,
+        ),
     ],
 )
 def test_svg_reuse(s1, s2, expected_affine, tolerance):
     # if we can get an affine we should normalize to same shape
     if expected_affine:
-        assert normalize(s1, tolerance) == normalize(s2, tolerance)
+        assert (
+            normalize(s1, tolerance).d == normalize(s2, tolerance).d
+        ), "should have normalized the same"
     else:
-        assert normalize(s1, tolerance) != normalize(s2, tolerance)
+        assert (
+            normalize(s1, tolerance).d != normalize(s2, tolerance).d
+        ), "should NOT have normalized the same"
 
     affine = affine_between(s1, s2, tolerance)
     if expected_affine: