Improve and fix behaviour when collapsing straight paths segments (#146)

* Do not collapse straight path segments in paths that have intermediate markers (see #145). The intermediate nodes might be unnecessary for the shape of the path, but their markers would be lost.
* Collapse subpaths of moveto `m` and lineto `l` commands if they have the same direction (before we only collapsed horizontal/vertical `h`/`v` lineto commands)
* Attempt to collapse lineto `l` commands into a preceding moveto `m` command (these are then called "implicit lineto commands")
* Preserve empty path segments if they have `stroke-linecap` set to `round` or `square`. They render no visible line but a tiny dot or square.

scour/scour.py

@@ -403,7 +403,16 @@ default_properties = {  # excluded all properties with 'auto' as default
 }
 
 
-def isSameSign(a, b): return (a <= 0 and b <= 0) or (a >= 0 and b >= 0)
+def is_same_sign(a, b):
+    return (a <= 0 and b <= 0) or (a >= 0 and b >= 0)
+
+
+def is_same_direction(x1, y1, x2, y2):
+    if is_same_sign(x1, x2) and is_same_sign(y1, y2):
+        diff = y1/x1 - y2/x2
+        return scouringContext.plus(1 + diff) == 1
+    else:
+        return False
 
 
 scinumber = re.compile(r"[-+]?(\d*\.?)?\d+[eE][-+]?\d+")
@@ -2044,10 +2053,23 @@ def cleanPath(element, options):
     # this gets the parser object from svg_regex.py
     oldPathStr = element.getAttribute('d')
     path = svg_parser.parse(oldPathStr)
+    style = _getStyle(element)
 
-    # This determines whether the stroke has round linecaps.  If it does,
+    # This determines whether the stroke has round or square linecaps.  If it does, we do not want to collapse empty
-    # we do not want to collapse empty segments, as they are actually rendered.
+    # segments, as they are actually rendered (as circles or squares with diameter/dimension matching the path-width).
-    withRoundLineCaps = element.getAttribute('stroke-linecap') == 'round'
+    has_round_or_square_linecaps = (
+        element.getAttribute('stroke-linecap') in ['round', 'square']
+        or 'stroke-linecap' in style and style['stroke-linecap'] in ['round', 'square']
+    )
+
+    # This determines whether the stroke has intermediate markers.  If it does, we do not want to collapse
+    # straight segments running in the same direction, as markers are rendered on the intermediate nodes.
+    has_intermediate_markers = (
+        element.hasAttribute('marker')
+        or element.hasAttribute('marker-mid')
+        or 'marker' in style
+        or 'marker-mid' in style
+    )
 
     # The first command must be a moveto, and whether it's relative (m)
     # or absolute (M), the first set of coordinates *is* absolute. So
@@ -2057,7 +2079,7 @@ def cleanPath(element, options):
     # Reuse the data structure 'path', since we're not adding or removing subcommands.
     # Also reuse the coordinate lists since we're not adding or removing any.
     x = y = 0
-    for pathIndex in range(0, len(path)):
+    for pathIndex in range(len(path)):
         cmd, data = path[pathIndex]  # Changes to cmd don't get through to the data structure
         i = 0
         # adjust abs to rel
@@ -2158,8 +2180,8 @@ def cleanPath(element, options):
     # remove empty segments
     # Reuse the data structure 'path' and the coordinate lists, even if we're
     # deleting items, because these deletions are relatively cheap.
-    if not withRoundLineCaps:
+    if not has_round_or_square_linecaps:
-        for pathIndex in range(0, len(path)):
+        for pathIndex in range(len(path)):
             cmd, data = path[pathIndex]
             i = 0
             if cmd in ['m', 'l', 't']:
@@ -2253,26 +2275,25 @@ def cleanPath(element, options):
     prevData = []
     newPath = []
     for (cmd, data) in path:
-        # flush the previous command if it is not the same type as the current command
+        if prevCmd == '':
-        if prevCmd != '':
+            # initialize with current path cmd and data
-            if cmd != prevCmd or cmd == 'm':
-                newPath.append((prevCmd, prevData))
-                prevCmd = ''
-                prevData = []
-
-        # if the previous and current commands are the same type,
-        # or the previous command is moveto and the current is lineto, collapse,
-        # but only if they are not move commands (since move can contain implicit lineto commands)
-        if (cmd == prevCmd or (cmd == 'l' and prevCmd == 'm')) and cmd != 'm':
-            prevData.extend(data)
-
-        # save last command and data
-        else:
             prevCmd = cmd
             prevData = data
+        else:
+            # collapse if
+            # - cmd is not moveto (explicit moveto commands are not drawn)
+            # - the previous and current commands are the same type,
+            # - the previous command is moveto and the current is lineto
+            #   (subsequent moveto pairs are treated as implicit lineto commands)
+            if cmd != 'm' and (cmd == prevCmd or (cmd == 'l' and prevCmd == 'm')):
+                prevData.extend(data)
+            # else flush the previous command if it is not the same type as the current command
+            else:
+                newPath.append((prevCmd, prevData))
+                prevCmd = cmd
+                prevData = data
     # flush last command and data
-    if prevCmd != '':
+    newPath.append((prevCmd, prevData))
-        newPath.append((prevCmd, prevData))
     path = newPath
 
     # convert to shorthand path segments where possible
@@ -2396,22 +2417,52 @@ def cleanPath(element, options):
             newPath.append((cmd, data))
     path = newPath
 
-    # for each h or v, collapse unnecessary coordinates that run in the same direction
+    # For each m, l, h or v, collapse unnecessary coordinates that run in the same direction
-    # i.e. "h-100-100" becomes "h-200" but "h300-100" does not change
+    # i.e. "h-100-100" becomes "h-200" but "h300-100" does not change.
+    # If the path has intermediate markers we have to preserve intermediate nodes, though.
     # Reuse the data structure 'path', since we're not adding or removing subcommands.
     # Also reuse the coordinate lists, even if we're deleting items, because these
     # deletions are relatively cheap.
-    for pathIndex in range(1, len(path)):
+    if not has_intermediate_markers:
-        cmd, data = path[pathIndex]
+        for pathIndex in range(len(path)):
-        if cmd in ['h', 'v'] and len(data) > 1:
+            cmd, data = path[pathIndex]
-            coordIndex = 1
+
-            while coordIndex < len(data):
+            # h / v expects only one parameter and we start drawing with the first (so we need at least 2)
-                if isSameSign(data[coordIndex - 1], data[coordIndex]):
+            if cmd in ['h', 'v'] and len(data) >= 2:
-                    data[coordIndex - 1] += data[coordIndex]
+                coordIndex = 0
-                    del data[coordIndex]
+                while coordIndex+1 < len(data):
-                    _num_path_segments_removed += 1
+                    if is_same_sign(data[coordIndex], data[coordIndex+1]):
-                else:
+                        data[coordIndex] += data[coordIndex+1]
-                    coordIndex += 1
+                        del data[coordIndex+1]
+                        _num_path_segments_removed += 1
+                    else:
+                        coordIndex += 1
+
+            # l expects two parameters and we start drawing with the first (so we need at least 4)
+            elif cmd == 'l' and len(data) >= 4:
+                coordIndex = 0
+                while coordIndex+2 < len(data):
+                    if is_same_direction(*data[coordIndex:coordIndex+4]):
+                        data[coordIndex] += data[coordIndex+2]
+                        data[coordIndex+1] += data[coordIndex+3]
+                        del data[coordIndex+2]  # delete the next two elements
+                        del data[coordIndex+2]
+                        _num_path_segments_removed += 1
+                    else:
+                        coordIndex += 2
+
+            # m expects two parameters but we have to skip the first pair as it's not drawn (so we need at least 6)
+            elif cmd == 'm' and len(data) >= 6:
+                coordIndex = 2
+                while coordIndex+2 < len(data):
+                    if is_same_direction(*data[coordIndex:coordIndex+4]):
+                        data[coordIndex] += data[coordIndex+2]
+                        data[coordIndex+1] += data[coordIndex+3]
+                        del data[coordIndex+2]  # delete the next two elements
+                        del data[coordIndex+2]
+                        _num_path_segments_removed += 1
+                    else:
+                        coordIndex += 2
 
     # it is possible that we have consecutive h, v, c, t commands now
     # so again collapse all consecutive commands of the same type into one command
@@ -2542,7 +2593,7 @@ def controlPoints(cmd, data):
     """
     cmd = cmd.lower()
     if cmd in ['c', 's', 'q']:
-        indices = range(0, len(data))
+        indices = range(len(data))
         if cmd == 'c':  # c: (x1 y1 x2 y2 x y)+
             return [(index % 6) < 4 for index in indices]
         elif cmd in ['s', 'q']:  # s: (x2 y2 x y)+   q: (x1 y1 x y)+

testscour.py

@@ -964,10 +964,10 @@ class KeepPrecisionInPathDataIfSameLength(unittest.TestCase):
         doc = scourXmlFile('unittests/path-precision.svg', parse_args(['--set-precision=1']))
         paths = doc.getElementsByTagNameNS(SVGNS, 'path')
         for path in paths[1:3]:
-            self.assertEqual(path.getAttribute('d'), "m1 12 123 1e3 1e4 1e5",
+            self.assertEqual(path.getAttribute('d'), "m1 21 321 4e3 5e4 7e5",
                              'Precision not correctly reduced with "--set-precision=1" '
                              'for path with ID ' + path.getAttribute('id'))
-        self.assertEqual(paths[4].getAttribute('d'), "m-1-12-123-1e3 -1e4 -1e5",
+        self.assertEqual(paths[4].getAttribute('d'), "m-1-21-321-4e3 -5e4 -7e5",
                          'Precision not correctly reduced with "--set-precision=1" '
                          'for path with ID ' + paths[4].getAttribute('id'))
         self.assertEqual(paths[5].getAttribute('d'), "m123 101-123-101",
@@ -977,10 +977,10 @@ class KeepPrecisionInPathDataIfSameLength(unittest.TestCase):
         doc = scourXmlFile('unittests/path-precision.svg', parse_args(['--set-precision=2']))
         paths = doc.getElementsByTagNameNS(SVGNS, 'path')
         for path in paths[1:3]:
-            self.assertEqual(path.getAttribute('d'), "m1 12 123 1234 12345 1.2e5",
+            self.assertEqual(path.getAttribute('d'), "m1 21 321 4321 54321 6.5e5",
                              'Precision not correctly reduced with "--set-precision=2" '
                              'for path with ID ' + path.getAttribute('id'))
-        self.assertEqual(paths[4].getAttribute('d'), "m-1-12-123-1234-12345-1.2e5",
+        self.assertEqual(paths[4].getAttribute('d'), "m-1-21-321-4321-54321-6.5e5",
                          'Precision not correctly reduced with "--set-precision=2" '
                          'for path with ID ' + paths[4].getAttribute('id'))
         self.assertEqual(paths[5].getAttribute('d'), "m123 101-123-101",
@@ -990,10 +990,10 @@ class KeepPrecisionInPathDataIfSameLength(unittest.TestCase):
         doc = scourXmlFile('unittests/path-precision.svg', parse_args(['--set-precision=3']))
         paths = doc.getElementsByTagNameNS(SVGNS, 'path')
         for path in paths[1:3]:
-            self.assertEqual(path.getAttribute('d'), "m1 12 123 1234 12345 123456",
+            self.assertEqual(path.getAttribute('d'), "m1 21 321 4321 54321 654321",
                              'Precision not correctly reduced with "--set-precision=3" '
                              'for path with ID ' + path.getAttribute('id'))
-        self.assertEqual(paths[4].getAttribute('d'), "m-1-12-123-1234-12345-123456",
+        self.assertEqual(paths[4].getAttribute('d'), "m-1-21-321-4321-54321-654321",
                          'Precision not correctly reduced with "--set-precision=3" '
                          'for path with ID ' + paths[4].getAttribute('id'))
         self.assertEqual(paths[5].getAttribute('d'), "m123 101-123-101",
@@ -1003,10 +1003,10 @@ class KeepPrecisionInPathDataIfSameLength(unittest.TestCase):
         doc = scourXmlFile('unittests/path-precision.svg', parse_args(['--set-precision=4']))
         paths = doc.getElementsByTagNameNS(SVGNS, 'path')
         for path in paths[1:3]:
-            self.assertEqual(path.getAttribute('d'), "m1 12 123 1234 12345 123456",
+            self.assertEqual(path.getAttribute('d'), "m1 21 321 4321 54321 654321",
                              'Precision not correctly reduced with "--set-precision=4" '
                              'for path with ID ' + path.getAttribute('id'))
-        self.assertEqual(paths[4].getAttribute('d'), "m-1-12-123-1234-12345-123456",
+        self.assertEqual(paths[4].getAttribute('d'), "m-1-21-321-4321-54321-654321",
                          'Precision not correctly reduced with "--set-precision=4" '
                          'for path with ID ' + paths[4].getAttribute('id'))
         self.assertEqual(paths[5].getAttribute('d'), "m123.5 101-123.5-101",
@@ -1036,16 +1036,25 @@ class RemoveEmptyLineSegmentsFromPath(unittest.TestCase):
         self.assertEqual(path[4][0], 'z',
                          'Did not remove an empty line segment from path')
 
-# Do not remove empty segments if round linecaps.
 
-
+class RemoveEmptySegmentsFromPathWithButtLineCaps(unittest.TestCase):
-class DoNotRemoveEmptySegmentsFromPathWithRoundLineCaps(unittest.TestCase):
 
     def runTest(self):
-        doc = scourXmlFile('unittests/path-with-caps.svg')
+        doc = scourXmlFile('unittests/path-with-caps.svg', parse_args(['--disable-style-to-xml']))
-        path = svg_parser.parse(doc.getElementsByTagNameNS(SVGNS, 'path')[0].getAttribute('d'))
+        for id in ['none', 'attr_butt', 'style_butt']:
-        self.assertEqual(len(path), 2,
+            path = svg_parser.parse(doc.getElementById(id).getAttribute('d'))
-                         'Did not preserve empty segments when path had round linecaps')
+            self.assertEqual(len(path), 1,
+                             'Did not remove empty segments when path had butt linecaps')
+
+
+class DoNotRemoveEmptySegmentsFromPathWithRoundSquareLineCaps(unittest.TestCase):
+
+    def runTest(self):
+        doc = scourXmlFile('unittests/path-with-caps.svg', parse_args(['--disable-style-to-xml']))
+        for id in ['attr_round', 'attr_square', 'style_round', 'style_square']:
+            path = svg_parser.parse(doc.getElementById(id).getAttribute('d'))
+            self.assertEqual(len(path), 2,
+                             'Did remove empty segments when path had round or square linecaps')
 
 
 class ChangeLineToHorizontalLineSegmentInPath(unittest.TestCase):
@@ -1215,35 +1224,51 @@ class RemoveFontStylesFromNonTextShapes(unittest.TestCase):
                          'font-size not removed from rect')
 
 
-class CollapseConsecutiveHLinesSegments(unittest.TestCase):
+class CollapseStraightPathSegments(unittest.TestCase):
 
     def runTest(self):
-        p = scourXmlFile('unittests/consecutive-hlines.svg').getElementsByTagNameNS(SVGNS, 'path')[0]
+        doc = scourXmlFile('unittests/collapse-straight-path-segments.svg', parse_args(['--disable-style-to-xml']))
-        self.assertEqual(p.getAttribute('d'), 'm100 100h200v100h-200z',
+        paths = doc.getElementsByTagNameNS(SVGNS, 'path')
-                         'Did not collapse consecutive hlines segments')
+        path_data = [path.getAttribute('d') for path in paths]
+        path_data_expected = ['m0 0h30',
+                              'm0 0v30',
+                              'm0 0h10.5v10.5',
+                              'm0 0h10-1v10-1',
+                              'm0 0h30',
+                              'm0 0h30',
+                              'm0 0h10 20',
+                              'm0 0h10 20',
+                              'm0 0h10 20',
+                              'm0 0h10 20',
+                              'm0 0 20 40v1l10 20',
+                              'm0 0 10 10-20-20 10 10-20-20',
+                              'm0 0 1 2m1 2 2 4m1 2 2 4',
+                              'm6.3228 7.1547 81.198 45.258']
 
+        self.assertEqual(path_data[0:3], path_data_expected[0:3],
+                         'Did not collapse h/v commands into a single h/v commands')
+        self.assertEqual(path_data[3], path_data_expected[3],
+                         'Collapsed h/v commands with different direction')
+        self.assertEqual(path_data[4:6], path_data_expected[4:6],
+                         'Did not collapse h/v commands with only start/end markers present')
+        self.assertEqual(path_data[6:10], path_data_expected[6:10],
+                         'Did not preserve h/v commands with intermediate markers present')
 
-class CollapseConsecutiveHLinesCoords(unittest.TestCase):
+        self.assertEqual(path_data[10], path_data_expected[10],
-
+                         'Did not collapse lineto commands into a single (implicit) lineto command')
-    def runTest(self):
+        self.assertEqual(path_data[11], path_data_expected[11],
-        p = scourXmlFile('unittests/consecutive-hlines.svg').getElementsByTagNameNS(SVGNS, 'path')[1]
+                         'Collapsed lineto commands with different direction')
-        self.assertEqual(p.getAttribute('d'), 'm100 300h200v100h-200z',
+        self.assertEqual(path_data[12], path_data_expected[12],
-                         'Did not collapse consecutive hlines coordinates')
+                         'Collapsed first parameter pair of a moveto subpath')
-
+        self.assertEqual(path_data[13], path_data_expected[13],
-
+                         'Did not collapse the nodes of a straight real world path')
-class DoNotCollapseConsecutiveHLinesSegsWithDifferingSigns(unittest.TestCase):
-
-    def runTest(self):
-        p = scourXmlFile('unittests/consecutive-hlines.svg').getElementsByTagNameNS(SVGNS, 'path')[2]
-        self.assertEqual(p.getAttribute('d'), 'm100 500h300-100v100h-200z',
-                         'Collapsed consecutive hlines segments with differing signs')
 
 
 class ConvertStraightCurvesToLines(unittest.TestCase):
 
     def runTest(self):
         p = scourXmlFile('unittests/straight-curve.svg').getElementsByTagNameNS(SVGNS, 'path')[0]
-        self.assertEqual(p.getAttribute('d'), 'm10 10l40 40 40-40z',
+        self.assertEqual(p.getAttribute('d'), 'm10 10 40 40 40-40z',
                          'Did not convert straight curves into lines')
 
 
@@ -1380,7 +1405,7 @@ class CollapseSamePathPoints(unittest.TestCase):
 
     def runTest(self):
         p = scourXmlFile('unittests/collapse-same-path-points.svg').getElementsByTagNameNS(SVGNS, 'path')[0]
-        self.assertEqual(p.getAttribute('d'), "m100 100l100.12 100.12c14.877 4.8766-15.123-5.1234 0 0z",
+        self.assertEqual(p.getAttribute('d'), "m100 100 100.12 100.12c14.877 4.8766-15.123-5.1234 0 0z",
                          'Did not collapse same path points')
 
 
@@ -1986,7 +2011,7 @@ class PathEmptyMove(unittest.TestCase):
 
     def runTest(self):
         doc = scourXmlFile('unittests/path-empty-move.svg')
-        self.assertEqual(doc.getElementsByTagName('path')[0].getAttribute('d'), 'm100 100l200 100z')
+        self.assertEqual(doc.getElementsByTagName('path')[0].getAttribute('d'), 'm100 100 200 100z')
         self.assertEqual(doc.getElementsByTagName('path')[1].getAttribute('d'), 'm100 100v200l100 100z')
 
 

unittests/collapse-straight-path-segments.svg

@@ -0,0 +1,33 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<svg width="100" height="100" xmlns="http://www.w3.org/2000/svg">
+ <defs>
+  <marker id="dot">
+   <circle r="5px"/>
+  </marker>
+ </defs>
+
+ <!-- h/v commands should be collapsed into a single h/v commands -->
+ <path d="m0 0h10 20"/>
+ <path d="m0 0v10 20"/>
+ <path d="m0 0h10 0.5v10 0.5"/>
+ <!-- h/v commands should not be collapsed if they have different direction -->
+ <path d="m0 0h10 -1v10 -1"/>
+ <!-- h/v commands should also be collapsed if only start/end markers are present -->
+ <path d="m0 0h10 20" marker-start="url(#dot)" marker-end="url(#dot)"/>
+ <path d="m0 0h10 20" style="marker-start:url(#dot);marker-end:url(#dot)"/>
+ <!-- h/v commands should be preserved if intermediate markers are present -->
+ <path d="m0 0h10 20" marker="url(#dot)"/>
+ <path d="m0 0h10 20" marker-mid="url(#dot)"/>
+ <path d="m0 0h10 20" style="marker:url(#dot)"/>
+ <path d="m0 0h10 20" style="marker-mid:url(#dot)"/>
+
+ <!-- all consecutive lineto commands pointing into the sam direction
+      should be collapsed into a single (implicit if possible) lineto command -->
+ <path d="m 0 0 l 10 20 0.25 0.5 l 0.75 1.5 l 5 10 0.2 0.4 l 3 6 0.8 1.6 l 0 1 l 1 2 9 18"/>
+ <!-- must not be collapsed (same slope, but different direction) -->
+ <path d="m 0 0 10 10 -20 -20 l 10 10 -20 -20"/>
+ <!-- first parameter pair of a moveto subpath must not be collapsed as it's not drawn on canvas -->
+ <path d="m0 0 1 2 m 1 2 1 2l 1 2 m 1 2 1 2 1 2"/>
+ <!-- real world example of straight path with multiple nodes -->
+ <path d="m 6.3227953,7.1547422 10.6709787,5.9477588 9.20334,5.129731 22.977448,12.807101 30.447251,16.970601 7.898986,4.402712"/>
+</svg>

unittests/consecutive-hlines.svg

@@ -1,6 +0,0 @@
-<?xml version="1.0" encoding="UTF-8" standalone="no"?>
-<svg xmlns="http://www.w3.org/2000/svg">
-<path fill="#F00" stroke="#0F0" d="M100,100h100h100v100h-200z"/>
-<path fill="#F00" stroke="#0F0" d="M100,300h100,100v100h-200z"/>
-<path fill="#F00" stroke="#0F0" d="M100,500h300h-100v100h-200z"/>
-</svg>

unittests/path-precision.svg

@@ -2,10 +2,10 @@
 <svg xmlns="http://www.w3.org/2000/svg">
   <path id="p0" d="M 100.0000001 99.9999999 h100.01 v123456789.123456789 h-100 z" />
   
-  <path id="p1" d="m  1    12    123    1234    12345    123456  " />
+  <path id="p1" d="m  1    21    321    4321    54321    654321  " />
-  <path id="p2" d="m  1.0  12.0  123.0  1234.0  12345.0  123456.0" />
+  <path id="p2" d="m  1.0  21.0  321.0  4321.0  54321.0  654321.0" />
-  <path id="p3" d="m 01   012   0123   01234   012345   0123456  " />
+  <path id="p3" d="m 01   021   0321   04321   054321   0654321  " />
-  <path id="p4" d="m -1   -12   -123   -1234   -12345   -123456  " />
+  <path id="p4" d="m -1   -21   -321   -4321   -54321   -654321  " />
   
   <path id="p6" d="m 123.456 101.001 -123.456 -101.001" />
 </svg>

unittests/path-with-caps.svg

@@ -1,4 +1,10 @@
 <?xml version="1.0" encoding="UTF-8" standalone="no"?>
 <svg xmlns="http://www.w3.org/2000/svg">
-  <path fill="none" stroke="#000" stroke-width="5" stroke-linecap="round" d="m 11,8 0,0" />
+ <path id="none" d="m0 0 0 0"/>
+ <path id="attr_butt" d="m0 0 0 0" stroke-linecap="butt"/>
+ <path id="attr_round" d="m0 0 0 0" stroke-linecap="round"/>
+ <path id="attr_square" d="m0 0 0 0" stroke-linecap="square"/>
+ <path id="style_butt" d="m0 0 0 0" style="stroke-linecap:butt"/>
+ <path id="style_round" d="m0 0 0 0" style="stroke-linecap:round"/>
+ <path id="style_square" d="m0 0 0 0" style="stroke-linecap:square"/>
 </svg>