Skip to content

Commit

Permalink
Avoid merging paths beyond string length limits (#112)
Browse files Browse the repository at this point in the history 
* Prevent merging paths beyond Android Lint's vector path limit (800 chars)

* Prevent merging paths beyond the maximum framework string length

* Add a test

* Add a changelog entry

* Update changelog
  • Loading branch information
@jzbrooks
jzbrooks authored Nov 30, 2024
1 parent 5fba321 commit 49eac60
Show file tree
Hide file tree
Showing 6 changed files with 143 additions and 16 deletions.
6 changes: 4 additions & 2 deletions changelog.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@

### Added
- `com.jzbrooks.vgo.core.util.math.Surveyor`, which computes the bounding box of an arbitrary list of commands
- Bezier curve interpolation for all variants and elliptical arc bounding box functions
- Bézier curve interpolation for all variants and elliptical arc bounding box functions

### Changed

- `vgo-plugin` (`com.jzbrooks.vgo.plugin`) no longer requires a particular version of Android Gradle Plugin.
Note: `:vgo` is an abstract implementation of the tool which does not assume either a cli or plugin context. CLI related logic has been relocated into `:vgo-cli`.
- **Breaking:** `CubicCurve<*>.interpolate` has been split into `CubicBezierCurve.interpolate` and `SmoothCubicBezierCurve.interpolate`
- `com.jzbrooks.vgo.core.optimization.MergePaths` constructor accepts constraints. See `com.jzbrooks.vgo.core.optimization.MergePaths.Constraints`.
- Paths with an even odd fill rule can be merged

### Deprecated
Expand All @@ -22,7 +23,8 @@
- Overlapping paths are no longer merged, which avoids some image warping issues (#88, #101)
- Conversions without a specified output file will write a file the file extension corresponding to the format.
- Decimal separators are locale-invariant.
- Crash when using the cli to convert an svg containing a clip path to vector drawable.
- Crash when using the CLI to convert an SVG containing a clip path to vector drawable.
- (Vector Drawable) Path merging avoids merging a single path data string beyond the framework string length limit (#82)

### Security

Expand Down
25 changes: 25 additions & 0 deletions vgo-core/api/vgo-core.api
View file
Original file line number Diff line number Diff line change
Expand Up @@ -545,13 +545,38 @@ public final class com/jzbrooks/vgo/core/optimization/ConvertCurvesToArcs : com/

public final class com/jzbrooks/vgo/core/optimization/MergePaths : com/jzbrooks/vgo/core/optimization/BottomUpOptimization {
public fun <init> ()V
public fun <init> (Lcom/jzbrooks/vgo/core/optimization/MergePaths$Constraints;)V
public synthetic fun <init> (Lcom/jzbrooks/vgo/core/optimization/MergePaths$Constraints;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun visit (Lcom/jzbrooks/vgo/core/graphic/ClipPath;)V
public fun visit (Lcom/jzbrooks/vgo/core/graphic/Extra;)V
public fun visit (Lcom/jzbrooks/vgo/core/graphic/Graphic;)V
public fun visit (Lcom/jzbrooks/vgo/core/graphic/Group;)V
public fun visit (Lcom/jzbrooks/vgo/core/graphic/Path;)V
}

public abstract interface class com/jzbrooks/vgo/core/optimization/MergePaths$Constraints {
}

public final class com/jzbrooks/vgo/core/optimization/MergePaths$Constraints$None : com/jzbrooks/vgo/core/optimization/MergePaths$Constraints {
public static final field INSTANCE Lcom/jzbrooks/vgo/core/optimization/MergePaths$Constraints$None;
public fun equals (Ljava/lang/Object;)Z
public fun hashCode ()I
public fun toString ()Ljava/lang/String;
}

public final class com/jzbrooks/vgo/core/optimization/MergePaths$Constraints$PathLength : com/jzbrooks/vgo/core/optimization/MergePaths$Constraints {
public fun <init> (Lcom/jzbrooks/vgo/core/graphic/command/CommandPrinter;I)V
public final fun component1 ()Lcom/jzbrooks/vgo/core/graphic/command/CommandPrinter;
public final fun component2 ()I
public final fun copy (Lcom/jzbrooks/vgo/core/graphic/command/CommandPrinter;I)Lcom/jzbrooks/vgo/core/optimization/MergePaths$Constraints$PathLength;
public static synthetic fun copy$default (Lcom/jzbrooks/vgo/core/optimization/MergePaths$Constraints$PathLength;Lcom/jzbrooks/vgo/core/graphic/command/CommandPrinter;IILjava/lang/Object;)Lcom/jzbrooks/vgo/core/optimization/MergePaths$Constraints$PathLength;
public fun equals (Ljava/lang/Object;)Z
public final fun getCommandPrinter ()Lcom/jzbrooks/vgo/core/graphic/command/CommandPrinter;
public final fun getMaxLength ()I
public fun hashCode ()I
public fun toString ()Ljava/lang/String;
}

public abstract interface class com/jzbrooks/vgo/core/optimization/Optimization {
public abstract fun optimize (Lcom/jzbrooks/vgo/core/graphic/Element;)V
}
Expand Down
73 changes: 67 additions & 6 deletions vgo-core/src/main/kotlin/com/jzbrooks/vgo/core/optimization/MergePaths.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,16 @@ import com.jzbrooks.vgo.core.graphic.Extra
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.CommandPrinter
import com.jzbrooks.vgo.core.util.math.Surveyor
import com.jzbrooks.vgo.core.util.math.intersects

/**
* Merges multiple paths into a single path where possible
*/
class MergePaths : BottomUpOptimization {
class MergePaths(
private val constraints: Constraints = Constraints.None,
) : BottomUpOptimization {
private val surveyor = Surveyor()

override fun visit(graphic: Graphic) = merge(graphic)
Expand Down Expand Up @@ -52,28 +55,75 @@ class MergePaths : BottomUpOptimization {
element.elements = elements
}

private fun merge(paths: List<Path>): List<Path> {
private fun merge(paths: List<Path>): List<Path> =
when (constraints) {
is Constraints.PathLength -> mergeConstrained(paths, constraints)
Constraints.None -> mergeUnconstrained(paths)
}

private fun mergeUnconstrained(paths: List<Path>): List<Path> {
if (paths.isEmpty()) return emptyList()

val mergedPaths = ArrayList<Path>(paths.size)
mergedPaths.add(paths.first())

for (current in paths.drop(1)) {
val previous = mergedPaths.last()

if (unableToMerge(previous, current)) {
mergedPaths.add(current)
} else {
previous.commands += current.commands
}
}

return mergedPaths
}

private fun mergeConstrained(
paths: List<Path>,
constraints: Constraints.PathLength,
): List<Path> {
if (paths.isEmpty()) return emptyList()

val mergedPaths = ArrayList<Path>(paths.size)
mergedPaths.add(paths.first())

var pathLength =
paths
.first()
.commands
.joinToString("", transform = constraints.commandPrinter::print)
.length

for (current in paths.drop(1)) {
val previous = mergedPaths.last()

// Intersecting paths can cause problems with path fill rules and with transparency.
if (!haveSameAttributes(current, previous) ||
surveyor.findBoundingBox(previous.commands) intersects surveyor.findBoundingBox(current.commands)
) {
val currentLength = current.commands.joinToString("", transform = constraints.commandPrinter::print).length
val accumulatedLength = pathLength + currentLength

if (accumulatedLength > constraints.maxLength || unableToMerge(previous, current)) {
mergedPaths.add(current)
pathLength = currentLength
} else {
previous.commands += current.commands
pathLength = accumulatedLength
}
}

return mergedPaths
}

// Paths must have the same visual parameters to be merged
// Intersecting paths can cause problems with path fill rules and with transparency
// If constraints exist on a path, they must be updated
private fun unableToMerge(
previous: Path,
current: Path,
): Boolean =
!haveSameAttributes(current, previous) ||
surveyor.findBoundingBox(previous.commands) intersects surveyor.findBoundingBox(current.commands)

private fun haveSameAttributes(
first: Path,
second: Path,
Expand All @@ -87,4 +137,15 @@ class MergePaths : BottomUpOptimization {
first.strokeLineCap == second.strokeLineCap &&
first.strokeLineJoin == second.strokeLineJoin &&
first.strokeMiterLimit == second.strokeMiterLimit

sealed interface Constraints {
/** Constraints the optimization by preventing merging paths beyond a given maximum length */
data class PathLength(
val commandPrinter: CommandPrinter,
/** The maximum length of a single path */
val maxLength: Int,
) : Constraints

data object None : Constraints
}
}
50 changes: 44 additions & 6 deletions vgo-core/src/test/kotlin/com/jzbrooks/vgo/core/optimization/MergePathsTests.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import com.jzbrooks.vgo.core.graphic.Group
import com.jzbrooks.vgo.core.graphic.command.Command
import com.jzbrooks.vgo.core.graphic.command.CommandVariant
import com.jzbrooks.vgo.core.graphic.command.EllipticalArcCurve
import com.jzbrooks.vgo.core.graphic.command.FakeCommandPrinter
import com.jzbrooks.vgo.core.graphic.command.LineTo
import com.jzbrooks.vgo.core.graphic.command.MoveTo
import com.jzbrooks.vgo.core.graphic.command.QuadraticBezierCurve
Expand Down Expand Up @@ -40,7 +41,7 @@ class MergePathsTests {
)

val graphic = createGraphic(paths)
val optimization = MergePaths()
val optimization = MergePaths(MergePaths.Constraints.None)

traverseBottomUp(graphic) { it.accept(optimization) }

Expand Down Expand Up @@ -90,7 +91,7 @@ class MergePathsTests {

val group = Group(paths)
val graphic = createGraphic(listOf(group))
val optimization = MergePaths()
val optimization = MergePaths(MergePaths.Constraints.None)

traverseBottomUp(graphic) { it.accept(optimization) }

Expand Down Expand Up @@ -161,7 +162,7 @@ class MergePathsTests {
)

val graphic = createGraphic(paths)
val optimization = MergePaths()
val optimization = MergePaths(MergePaths.Constraints.None)

traverseBottomUp(graphic) { it.accept(optimization) }

Expand Down Expand Up @@ -229,7 +230,7 @@ class MergePathsTests {
)

val graphic = createGraphic(paths)
val optimization = MergePaths()
val optimization = MergePaths(MergePaths.Constraints.None)

traverseBottomUp(graphic) { it.accept(optimization) }

Expand Down Expand Up @@ -291,7 +292,7 @@ class MergePathsTests {
)

val graphic = createGraphic(paths)
val optimization = MergePaths()
val optimization = MergePaths(MergePaths.Constraints.None)

traverseBottomUp(graphic) { it.accept(optimization) }

Expand Down Expand Up @@ -411,10 +412,47 @@ class MergePathsTests {
)

val graphic = createGraphic(listOf(firstHeart, offsetHeart))
val optimization = MergePaths()
val optimization = MergePaths(MergePaths.Constraints.None)

traverseBottomUp(graphic) { it.accept(optimization) }

assertThat(graphic::elements).hasSize(2)
}

@Test
fun pathLengthConstraints() {
val paths =
listOf(
createPath(
listOf(MoveTo(CommandVariant.ABSOLUTE, listOf(Point(0f, 0f)))),
),
createPath(
listOf(MoveTo(CommandVariant.ABSOLUTE, listOf(Point(10f, 10f)))),
),
createPath(
listOf(MoveTo(CommandVariant.ABSOLUTE, listOf(Point(40f, 40f)))),
),
createPath(
listOf(MoveTo(CommandVariant.ABSOLUTE, listOf(Point(50f, 50f), Point(10f, 10f), Point(20f, 30f), Point(40f, 0f)))),
),
)

val graphic = createGraphic(paths)
val optimization = MergePaths(MergePaths.Constraints.PathLength(FakeCommandPrinter(), 16))

traverseBottomUp(graphic) { it.accept(optimization) }

assertThat(graphic::elements).containsExactly(
createPath(
listOf(
MoveTo(CommandVariant.ABSOLUTE, listOf(Point(0f, 0f))),
MoveTo(CommandVariant.ABSOLUTE, listOf(Point(10f, 10f))),
MoveTo(CommandVariant.ABSOLUTE, listOf(Point(40f, 40f))),
),
),
createPath(
listOf(MoveTo(CommandVariant.ABSOLUTE, listOf(Point(50f, 50f), Point(10f, 10f), Point(20f, 30f), Point(40f, 0f)))),
),
)
}
}
2 changes: 1 addition & 1 deletion vgo/src/main/kotlin/com/jzbrooks/vgo/svg/SvgOptimizationRegistry.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class SvgOptimizationRegistry :
BakeTransformations(),
CollapseGroups(),
RemoveEmptyGroups(),
MergePaths(),
MergePaths(MergePaths.Constraints.None),
),
topDownOptimizations =
listOf(
Expand Down
3 changes: 2 additions & 1 deletion vgo/src/main/kotlin/com/jzbrooks/vgo/vd/VectorDrawableOptimizationRegistry.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ class VectorDrawableOptimizationRegistry :
BakeTransformations(),
CollapseGroups(),
RemoveEmptyGroups(),
MergePaths(),
// https://cs.android.com/android/platform/superproject/main/+/2e48e15a8097916063eacc023044bc90bb93c73e:frameworks/base/libs/androidfw/StringPool.cpp;l=328
MergePaths(MergePaths.Constraints.PathLength(VectorDrawableCommandPrinter(3), (1 shl 15) - 1)),
),
topDownOptimizations =
listOf(
Expand Down

0 comments on commit 49eac60

Please sign in to comment.