From 972270d284871d79af5225c53c49da99a3cb4871 Mon Sep 17 00:00:00 2001 From: Rod S Date: Sat, 4 Jun 2022 11:48:07 -0700 Subject: [PATCH] Upgrade arc handling to support circles and ellipses formed of two arcs as we commonly encounter this and fail to reuse in real inputs --- src/picosvg/svg_reuse.py | 121 ++++++++++++++++++++++++++++++++++----- src/picosvg/svg_types.py | 6 +- tests/svg_reuse_test.py | 107 +++++++++++++++++++++++++++++++++- 3 files changed, 215 insertions(+), 19 deletions(-) diff --git a/src/picosvg/svg_reuse.py b/src/picosvg/svg_reuse.py index 2b00973..745baf7 100644 --- a/src/picosvg/svg_reuse.py +++ b/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 diff --git a/src/picosvg/svg_types.py b/src/picosvg/svg_types.py index ebd3656..d0b0ad4 100644 --- a/src/picosvg/svg_types.py +++ b/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).""" diff --git a/tests/svg_reuse_test.py b/tests/svg_reuse_test.py index 2ac5d5c..62ab684 100644 --- a/tests/svg_reuse_test.py +++ b/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: