svgdotjs / svg.js

The lightweight library for manipulating and animating SVG
https://svgjs.dev
Other
11.04k stars 1.08k forks source link

path string parsing fails for "super"-optimized strings #1145

Closed stranskyjan closed 4 years ago

stranskyjan commented 4 years ago

Consider a path string M2,0a2 2 0 00-2 2a2 2 0 002 2a.5.5 0 011 0z, which seems to be valid (browsers natively renders it correctly), but is "super"-optimized:

Using this in svg.js renders correctly, but path.array() method returns

[
   ["M", 2, 0],
   ["A", 2, 2, 0, 0, -2, 4, NaN],
   ["A", 2, 2, 0, 2, 2, NaN, NaN],
   ["A", 0.5, 0, 11, 0, NaN, NaN, NaN],
]

but should return

[
   ["M", 2, 0],
   ["A", 2, 2, 0, 0, 0, -2, 2],
   ["A", 2, 2, 0, 0, 0, 2, 2],
   ["A", 0.5, 0.5, 0, 0, 1, 1, 0],
   ["Z"],
]

Bug report based on this stackoverflow discussion

jsfiddle

Fuzzyma commented 4 years ago

Wow I didn't know that 00 is allowed. All other cases are covered. Good catch!

stranskyjan commented 4 years ago

@Fuzzyma a.5.5 also seems to be a problem, resulting in ["A", 0.5, 0, ... but should be ["A", 0.5, 0.5, ...

Fuzzyma commented 4 years ago

strange, thought I caught that. But it was 5 years ago - so maybe my memory is just bad :D. I guess at that point I just copy the solution from stackoverflow -.-

Fuzzyma commented 4 years ago

hm ok, because of the 011 part in the flags, we cannot use regex here anymore because no regex knows if 111 is a valid number or 3 flags glued together. Unfortunate... time to write a parser

Fuzzyma commented 4 years ago

@stranskyjan can 00 also occure in other path commands (for coodinates for example (e.g. M00)) or only for the flags in arc? Is 11 also allowed as arc flags?

thednp commented 4 years ago

@stranskyjan try this

stranskyjan commented 4 years ago

@Fuzzyma according to browser test and the grammar for path data, M00 and similar is not allowed, as 00 is treated as a single number ([0-9])+, not a pair of coordinates. The flags are required to be either 0 or 1, so they can be "shrinked"

Fuzzyma commented 4 years ago

Cool, in that case I have a working parser now. However, I want to combine it with changing the coordinates to absolute so I don't have 2 loops hanging around :)

Fuzzyma commented 4 years ago

@stranskyjan just realized that your "correct" example is wrong. You test against absolute coordinates but use the relative coordinates in your example

stranskyjan commented 4 years ago

@Fuzzyma true, sorry, I put wrong numbers. But the main idea that there are numbers instead of NaNs remains :-)

Fuzzyma commented 4 years ago

Yes, ofc! I just wondered why my parser put out the wrong values :D