Fix a rare crash involving subpath relativity (#57)

* Handle current coordinate position according to spec

* Avoid resetting the subpath stack if m doesn't follow z

* Format code

* Add a changelog entry

changelog.md

@@ -4,7 +4,11 @@
 
 ### Added
 
-- More robust SVG → vector conversions by Android Studio tools
+- More robust SVG → vector conversions by Android Studio tools (#47)
+
+### Fixed
+
+- In rare cases, subpath start points were tracked incorrectly which resulted in a crash (#57)
 
 ## 2.1.0 - 09-14-2021 
 

vgo-core/src/main/kotlin/com/jzbrooks/vgo/core/optimization/BreakoutImplicitCommands.kt

@@ -19,7 +19,7 @@ import com.jzbrooks.vgo.core.graphic.command.SmoothQuadraticBezierCurve
 import com.jzbrooks.vgo.core.graphic.command.VerticalLineTo
 
 /**
- * Enables more resolution in the the other command
+ * Enables more resolution in the other command
  * related optimizations like [CommandVariant] and [RemoveRedundantCommands]
  */
 class BreakoutImplicitCommands : TopDownOptimization {

vgo-core/src/main/kotlin/com/jzbrooks/vgo/core/optimization/CommandVariant.kt

@@ -6,6 +6,7 @@ import com.jzbrooks.vgo.core.graphic.Graphic
 import com.jzbrooks.vgo.core.graphic.Group
 import com.jzbrooks.vgo.core.graphic.Path
 import com.jzbrooks.vgo.core.graphic.command.ClosePath
+import com.jzbrooks.vgo.core.graphic.command.Command
 import com.jzbrooks.vgo.core.graphic.command.CommandPrinter
 import com.jzbrooks.vgo.core.graphic.command.CommandVariant
 import com.jzbrooks.vgo.core.graphic.command.CubicBezierCurve
@@ -19,15 +20,14 @@ import com.jzbrooks.vgo.core.graphic.command.SmoothCubicBezierCurve
 import com.jzbrooks.vgo.core.graphic.command.SmoothQuadraticBezierCurve
 import com.jzbrooks.vgo.core.graphic.command.VerticalLineTo
 import com.jzbrooks.vgo.core.util.math.Point
-import java.util.Stack
 
 /**
  * Converts commands to use relative, absolute,
  * or the shortest representation of coordinates
  * @param mode determines the operating mode of the command
  */
 class CommandVariant(private val mode: Mode) : TopDownOptimization {
-    private val pathStart = Stack<Point>()
+    private val pathStart = ArrayDeque<Point>()
 
     // Updated once per process call when computing
     // the other variant of the command. This works
@@ -56,12 +56,20 @@ class CommandVariant(private val mode: Mode) : TopDownOptimization {
                 // Absolute values are relative to the origin, so += means
                 // the same thing here.
                 currentPoint += moveTo.parameters.last()
-                pathStart.push(currentPoint.copy())
+                pathStart.addFirst(currentPoint.copy())
                 moveTo
             }
 
-        val commands =
-            path.commands.drop(1).map { command ->
+        val modifiedCommands = mutableListOf<Command>()
+        for (i in path.commands.indices.drop(1)) {
+            val command = path.commands[i]
+            val previousCommand = path.commands.getOrNull(i - 1)
+
+            if (previousCommand is ClosePath && command !is MoveTo) {
+                pathStart.addFirst(currentPoint.copy())
+            }
+
+            val modifiedCommand =
                 when (command) {
                     is MoveTo -> process(command)
                     is LineTo -> process(command)
@@ -74,9 +82,11 @@ class CommandVariant(private val mode: Mode) : TopDownOptimization {
                     is EllipticalArcCurve -> process(command)
                     is ClosePath -> process(command)
                 }
-            }
 
-        path.commands = firstMoveTo + commands
+            modifiedCommands.add(modifiedCommand)
+        }
+
+        path.commands = firstMoveTo + modifiedCommands
     }
 
     private fun process(command: MoveTo): MoveTo {
@@ -99,7 +109,7 @@ class CommandVariant(private val mode: Mode) : TopDownOptimization {
                 )
             }
 
-        pathStart.push(currentPoint.copy())
+        pathStart.addFirst(currentPoint.copy())
 
         return choose(convertedCommand, command)
     }
@@ -281,15 +291,15 @@ class CommandVariant(private val mode: Mode) : TopDownOptimization {
                     variant = CommandVariant.ABSOLUTE,
                     parameters =
                         command.parameters.map { commandPoint ->
-                            (commandPoint + currentPoint)
+                            commandPoint + currentPoint
                         }.also { currentPoint = it.last().copy() },
                 )
             } else {
                 command.copy(
                     variant = CommandVariant.RELATIVE,
                     parameters =
                         command.parameters.map { commandPoint ->
-                            (commandPoint - currentPoint)
+                            commandPoint - currentPoint
                         }.also {
                             currentPoint += it.last()
                         },
@@ -328,7 +338,7 @@ class CommandVariant(private val mode: Mode) : TopDownOptimization {
 
     private fun process(command: ClosePath): ClosePath {
         // If there is a close path, there should be a corresponding path start entry on the stack
-        currentPoint = pathStart.pop()
+        currentPoint = pathStart.removeFirst()
         return command
     }
 
@@ -362,9 +372,9 @@ class CommandVariant(private val mode: Mode) : TopDownOptimization {
     }
 
     sealed class Mode {
-        object Absolute : Mode()
+        data object Absolute : Mode()
 
-        object Relative : Mode()
+        data object Relative : Mode()
 
         data class Compact(val printer: CommandPrinter) : Mode()
     }

vgo-core/src/main/kotlin/com/jzbrooks/vgo/core/util/math/Commands.kt

@@ -13,21 +13,26 @@ import com.jzbrooks.vgo.core.graphic.command.QuadraticBezierCurve
 import com.jzbrooks.vgo.core.graphic.command.SmoothCubicBezierCurve
 import com.jzbrooks.vgo.core.graphic.command.SmoothQuadraticBezierCurve
 import com.jzbrooks.vgo.core.graphic.command.VerticalLineTo
-import java.util.Stack
 
 /**
  * Computes absolute coordinates for a given command in the sequence.
  * By default, the absolute coordinate of the last command is returned.
- * @param commands: The complete list of **relative** commands for a given path. The initial moveto can be absolute.
+ * @param commands The complete list of **relative** commands for a given path. The initial moveto can be absolute.
  */
 fun computeAbsoluteCoordinates(commands: List<Command>): Point {
     assert(commands.drop(1).filterIsInstance<ParameterizedCommand<*>>().all { it.variant == CommandVariant.RELATIVE })
 
-    val pathStart = Stack<Point>()
     var currentPoint = Point(0f, 0f)
+    val pathStart = ArrayDeque<Point>()
 
     for (i in commands.indices) {
         val command = commands[i]
+        val previousCommand = commands.getOrNull(i - 1)
+
+        if (previousCommand is ClosePath && command !is MoveTo) {
+            pathStart.addFirst(currentPoint.copy())
+        }
+
         val newCurrentPoint =
             when (command) {
                 is MoveTo -> command.parameters[0]
@@ -39,7 +44,7 @@ fun computeAbsoluteCoordinates(commands: List<Command>): Point {
                 is QuadraticBezierCurve -> command.parameters[0].end
                 is SmoothQuadraticBezierCurve -> command.parameters[0]
                 is EllipticalArcCurve -> command.parameters[0].end
-                is ClosePath -> pathStart.pop()
+                is ClosePath -> pathStart.removeFirst()
             }
 
         if (command !is ClosePath) {
@@ -48,8 +53,8 @@ fun computeAbsoluteCoordinates(commands: List<Command>): Point {
             currentPoint = newCurrentPoint
         }
 
-        if (command is MoveTo) {
-            pathStart.push(currentPoint.copy())
+        if (i == 0 || (previousCommand is ClosePath && command is MoveTo)) {
+            pathStart.addFirst(currentPoint.copy())
         }
     }
 

vgo-core/src/test/kotlin/com/jzbrooks/vgo/core/util/math/CommandsTest.kt

@@ -61,6 +61,25 @@ class CommandsTest {
 
         val absoluteCoordinates = computeAbsoluteCoordinates(commands.take(6))
 
-        assertThat(absoluteCoordinates).isEqualTo(Point(12f, 8f))
+        assertThat(absoluteCoordinates).isEqualTo(Point(20f, 1f))
+    }
+
+    @Test
+    fun `Closing a subpath after a non-moveto command returns to previous coordinate`() {
+        val commands =
+            listOf(
+                MoveTo(CommandVariant.ABSOLUTE, listOf(Point(10f, 1f))),
+                LineTo(CommandVariant.RELATIVE, listOf(Point(-9f, 6f))),
+                MoveTo(CommandVariant.RELATIVE, listOf(Point(1f, 1f))),
+                LineTo(CommandVariant.RELATIVE, listOf(Point(3f, 7f))),
+                ClosePath,
+                HorizontalLineTo(CommandVariant.RELATIVE, listOf(10f)),
+                VerticalLineTo(CommandVariant.RELATIVE, listOf(-4f)),
+                ClosePath,
+            )
+
+        val absoluteCoordinates = computeAbsoluteCoordinates(commands)
+
+        assertThat(absoluteCoordinates).isEqualTo(Point(10f, 1f))
     }
 }