Merge pull request #178 from nthykier/gh-163-path-rewrite

Correct handling of "m0 0" vs. "z" commands

scour/scour.py

@@ -2188,22 +2188,24 @@ def cleanPath(element, options):
             x, y = startx, starty
             path[pathIndex] = ('z', data)
 
-    # remove empty segments
+    # remove empty segments and redundant commands
     # Reuse the data structure 'path' and the coordinate lists, even if we're
     # deleting items, because these deletions are relatively cheap.
     if not has_round_or_square_linecaps:
+        # remove empty path segments
         for pathIndex in range(len(path)):
             cmd, data = path[pathIndex]
             i = 0
             if cmd in ['m', 'l', 't']:
                 if cmd == 'm':
-                    # remove m0,0 segments
-                    if pathIndex > 0 and data[0] == data[i + 1] == 0:
-                        # 'm0,0 x,y' can be replaces with 'lx,y',
-                        # except the first m which is a required absolute moveto
-                        path[pathIndex] = ('l', data[2:])
-                        _num_path_segments_removed += 1
-                    else:  # else skip move coordinate
+                    # It might be tempting to rewrite "m0 0 ..." into
+                    # "l..." here.  However, this is an unsound
+                    # optimization in general as "m0 0 ... z" is
+                    # different from "l...z".
+                    #
+                    # To do such a rewrite, we need to understand the
+                    # full subpath.  This logic happens after this
+                    # loop.
                     i = 2
                 while i < len(data):
                     if data[i] == data[i + 1] == 0:
@@ -2237,6 +2239,45 @@ def cleanPath(element, options):
                 path[pathIndex] = (cmd, [coord for coord in data if coord != 0])
                 _num_path_segments_removed += len(path[pathIndex][1]) - oldLen
 
+        # remove no-op commands
+        pathIndex = len(path)
+        subpath_needs_anchor = False
+        # NB: We can never rewrite the first m/M command (expect if it
+        # is the only command)
+        while pathIndex > 1:
+            pathIndex -= 1
+            cmd, data = path[pathIndex]
+            if cmd == 'z':
+                next_cmd, next_data = path[pathIndex - 1]
+                if next_cmd == 'm' and len(next_data) == 2:
+                    # mX Yz -> mX Y
+
+                    # note the len check on next_data as it is not
+                    # safe to rewrite "m0 0 1 1z" in general (it is a
+                    # question of where the "pen" ends - you can
+                    # continue a draw on the same subpath after a
+                    # "z").
+                    del path[pathIndex]
+                    _num_path_segments_removed += 1
+                else:
+                    # it is not safe to rewrite "m0 0 ..." to "l..."
+                    # because of this "z" command.
+                    subpath_needs_anchor = True
+            elif cmd == 'm':
+                if len(path) - 1 == pathIndex and len(data) == 2:
+                    # Ends with an empty move (but no line/draw
+                    # following it)
+                    del path[pathIndex]
+                    _num_path_segments_removed += 1
+                    continue
+                if subpath_needs_anchor:
+                    subpath_needs_anchor = False
+                elif data[0] == data[1] == 0:
+                    # unanchored, i.e. we can replace "m0 0 ..." with
+                    # "l..." as there is no "z" after it.
+                    path[pathIndex] = ('l', data[2:])
+                    _num_path_segments_removed += 1
+
     # fixup: Delete subcommands having no coordinates.
     path = [elem for elem in path if len(elem[1]) > 0 or elem[0] == 'z']
 

testscour.py

@@ -2072,12 +2072,25 @@ class StyleToAttr(unittest.TestCase):
         self.assertEqual(line.getAttribute('marker-end'), 'url(#m)')
 
 
-class PathEmptyMove(unittest.TestCase):
+class PathCommandRewrites(unittest.TestCase):
 
     def runTest(self):
-        doc = scourXmlFile('unittests/path-empty-move.svg')
-        self.assertEqual(doc.getElementsByTagName('path')[0].getAttribute('d'), 'm100 100 200 100z')
-        self.assertEqual(doc.getElementsByTagName('path')[1].getAttribute('d'), 'm100 100v200l100 100z')
+        doc = scourXmlFile('unittests/path-command-rewrites.svg')
+        paths = doc.getElementsByTagName('path')
+        expected_paths = [
+            ('m100 100 200 100', "Trailing m0 0z not removed"),
+            ('m100 100v200m0 0 100 100z', "Mangled m0 0 100 100"),
+            ("m100 100v200m0 0 2-1-2 1z", "Should have removed empty m0 0"),
+            ("m100 100v200l3-5-5 3m0 0 2-1-2 1z", "Rewrite m0 0 3-5-5 3 ... -> l3-5-5 3 ..."),
+            ("m100 100v200m0 0 3-5-5 3zm0 0 2-1-2 1z", "No rewrite of m0 0 3-5-5 3z"),
+        ]
+        self.assertEqual(len(paths), len(expected_paths), "len(actual_paths) != len(expected_paths)")
+        for i in range(len(paths)):
+            actual_path = paths[i].getAttribute('d')
+            expected_path, message = expected_paths[i]
+            self.assertEqual(actual_path,
+                             expected_path,
+                             '%s: "%s" != "%s"' % (message, actual_path, expected_path))
 
 
 class DefaultsRemovalToplevel(unittest.TestCase):

unittests/path-command-rewrites.svg

@@ -0,0 +1,8 @@
+<?xml version="1.0"?>
+<svg xmlns="http://www.w3.org/2000/svg">
+  <path d="m100 100 l200 100 m0 0z" />
+  <path d="m100 100 v200 m0 0 100 100z" />
+  <path d="m100 100 v200 m0 0m0 0 2-1-2 1z" />
+  <path d="m100 100 v200 m0 0 3-5-5 3m0 0 2-1-2 1z" />
+  <path d="m100 100 v200 m0 0 3-5-5 3zm0 0 2-1-2 1z" />
+</svg>

unittests/path-empty-move.svg

@@ -1,5 +0,0 @@
-<?xml version="1.0"?>
-<svg xmlns="http://www.w3.org/2000/svg">
-  <path d="m100 100 l200 100 m0 0z" />
-  <path d="m100 100 v200 m0 0 100 100z" />
-</svg>