Repository navigation
SVG: draw a path up to an error in its data, after a correct start - #2177
Conversation
Numbers after a closepath failed with ValueError: Unallowed implicit command, and a command without its numbers with IndexError, so the whole document failed. As the SVG implementation notes say, and as _complete_points() already does for points, the path is now drawn up to the last correct command; a path that does not start with a moveto still fails.
…r-start # Conflicts: # CHANGELOG.md
andersonhc
left a comment
andersonhc
left a comment
There was a problem hiding this comment.
I noticed one related case that still fails: M0 0L10 0 A or M0 0L10 0 A5 5 0 0 raises ValueError: Invalid arc command instead of drawing the valid prefix. FontTools tokenizes the whole path before making any pen calls, so pen.first_is_move is still unset when these errors reach the handler.
Could you extend the recovery to cover incomplete arc commands and add a regression test? That would also make the behavior consistent with the changelog’s description.
# Conflicts: # CHANGELOG.md
andersonhc
left a comment
andersonhc
left a comment
There was a problem hiding this comment.
I found one remaining edge case: recovery at command-letter boundaries can discard valid segments within an implicit command.
For example, M0 0 L10 0 A5 5 0 0 0 20 0 5 loses the completed arc before the trailing incomplete arguments. Similarly, M0 0 L10 0 20 A loses the valid line to (10, 0).
Could we preserve complete argument groups before the error? That would make recovery consistent with the “last correct command” behavior described in the changelog.
|
Thanks, fixed. When the error comes before any pen call, as with an arc, the converter now finds the longest run of whole commands at the start of the data that fontTools can read (by bisection, so a broken 20,000-command path takes 0.4 s) and draws that. A path that does not start with a moveto still raises. Added "arc without numbers" (M 0 1 L 2 3 A) and "arc without all its numbers" (M 0 1 L 2 3 A 5 5 0 0) to svg_path_directives; both fail without the change, and the changelog now mentions incomplete arcs. |
# Conflicts: # CHANGELOG.md
|
Thanks, fixed. After finding the last whole command that can be read, recovery now goes into the failing command and keeps its complete sets of numbers: it tries the prefixes ending after each number of that command, longest first. Your two cases give M0 0 L10 0 A5 5 0 0 0 20 0 (the completed arc is kept) and M0 0 L10 0 (the line to (10, 0) is kept); both are in svg_path_directives as "repeated arc without all its numbers" and "repeated line without all its numbers" and fail without the change. A broken 20,000-command path still takes 0.4 s, and the changelog now says "up to the last complete set of numbers". |
andersonhc
left a comment
andersonhc
left a comment
There was a problem hiding this comment.
I noticed one performance edge case in the new loop. With an invalid arc flag followed by many numbers, such as "M0 0 L10 0 A5 5 0 2 " + "1 " * n, every candidate prefix fails, so we repeatedly parse progressively shorter strings. Locally, 1,000 / 2,000 / 4,000 trailing numbers took about 0.08 / 0.28 / 1.07 seconds.
Could we find the first invalid argument while scanning forward, keeping track of the last complete argument group? That would let recovery stop at the error without reparsing every prefix.
# Conflicts: # CHANGELOG.md
Trying shorter and shorter prefixes parsed the path again for every number, so an invalid arc flag followed by many numbers took quadratic time. Read each command's numbers forward once, in complete sets, and parse only the prefix before the first incomplete one.
# Conflicts: # CHANGELOG.md
|
Thanks. It now reads the path forward once: |
|
Thanks. It now reads the path forward once: |
A
<path>whose data has an error after a correct start makes the whole document fail:M0 0L10 0Z 20 20:ValueError: Unallowed implicit commandM0 0L10 0 L:IndexError: pop from empty listThe SVG implementation notes say a path is rendered up to and including the last correct command.
_complete_points()already does that for<polyline>and<polygon>, and its docstring says "like an incorrect<path>".svg_path_converter()now keeps whatparse_path()drew before such an error. A path that does not start with a moveto still raises, sotest_bad_path_startis unchanged.Two new
svg_path_edge_casescover it; both fail without the change.