search

thednp / svg-path-commander

Typescript tools for advanced processing of SVG path data.
https://thednp.github.io/svg-path-commander/
MIT License
222 stars 19 forks source link

Unsupported implicit `lineTo` when using additional `moveTo` parameters #15

Closed usr-ein closed 2 years ago

usr-ein commented 2 years ago

Here is the doc for the relative moveTo:

Move the current point by shifting the last known position of the path by dx along the x-axis and by dy along the y-axis. Any subsequent coordinate pair(s) are interpreted as parameter(s) for implicit relative LineTo (l) command(s) (see below).

Replication

<!DOCTYPE html>
<html>
<head>
    <title>SVG recaling</title>
    <script src="https://cdn.jsdelivr.net/npm/svg-path-commander@1.0.3/dist/svg-path-commander.min.js"></script>
</head>
<body>
<p id="myPathString"></p>
</body>
<script type="text/javascript">
    const path = "m133.67 263.88-28.317-13.209-26.934 15.839 3.8116-31.013-23.387-20.722 30.673-5.9585 12.48-28.646 15.145 27.33 31.1 3.0176-21.313 22.85z";
    const transform = {
      origin: [0,0],
      scale: 1 // uniform scale on X, Y, Z axis
    };
    // This will output:
    // M133.67 263.88M105.353 250.671M78.419 266.51M82.2306 235.497M58.8436 214.775M89.5166 208.8165M101.9966 180.1705M117.1416 207.5005M148.2416 210.5181M126.9286 233.3681Z
    const transformed2DPathString = new SVGPathCommander(path).transform(transform).toString();
    document.getElementById('myPathString').innerText = transformed2DPathString;
</script>
</html>

The following path describes a star shape (one of Inkscape's primitives):

m133.67 263.88
  -28.317-13.209
  -26.934 15.839 
  3.8116-31.013
  -23.387-20.722 
  30.673-5.9585 
  12.48-28.646 
  15.145 27.33 
  31.1 3.0176
  -21.313 22.85
  z

However, it uses the implicit lineTo mentionned in the doc. Thus, it is equivalent to that:

m133.67 263.88
  l -28.317-13.209
  l -26.934 15.839 
  l 3.8116-31.013
  l -23.387-20.722 
  l 30.673-5.9585 
  l 12.48-28.646 
  l 15.145 27.33 
  l 31.1 3.0176
  l -21.313 22.85
  z

However, SVG Path Commander, when asked to rescale it, will convert every subsequent pair of moveTo parameter into new moveTo commands, which is not the behaviour described in the doc. Hence, the above, when scaled by scale: 1 will become:

M133.67 263.88
M105.353 250.671
M78.419 266.51
M82.2306 235.497
M58.8436 214.775
M89.5166 208.8165
M101.9966 180.1705
M117.1416 207.5005
M148.2416 210.5181
M126.9286 233.3681Z

Which doesn't draw anything (it just moves the starting point around). Instead, it should look like this:

M133.67 263.88
105.353 250.671
78.419 266.51
82.2306 235.497
58.8436 214.775
89.5166 208.8165
101.9966 180.1705
117.1416 207.5005
148.2416 210.5181
126.9286 233.3681Z

which is the same as 

M133.67 263.88
L 105.353 250.671
L 78.419 266.51
L 82.2306 235.497
L 58.8436 214.775
L 89.5166 208.8165
L 101.9966 180.1705
L 117.1416 207.5005
L 148.2416 210.5181
L 126.9286 233.3681Z
thednp commented 2 years ago

This might be related to the recent changes resulted in the implementation of tests, basically I found many lines never executed with "valid" path strings. In my books this string

m133.67 263.88
  -28.317-13.209
  -26.934 15.839 
  3.8116-31.013
  -23.387-20.722 
  30.673-5.9585 
  12.48-28.646 
  15.145 27.33 
  31.1 3.0176
  -21.313 22.85
  z

is an invalid string, since M/m is configured to always parse 2 numerical parameters. The reason I say this is because the original source of the parser (SVGPath) at its time, the SVG working draft was pretty messed up, it had to be decided to simplify the logic somehow to have a working script to get the job done for them (Fontello).

However if reverting some of the recent changes fixes that, I'm happy to do it. Perhaps you would care to test some of the previous versions to confirm? Would save us some much needed time.

Thanks for the report.

thednp commented 2 years ago

Update, I did a quick test with v0.1.25 and it has the same behavior. I have no solution right now, I can only suggest to configure Inkskape to export 100% valid path strings if possible.

We are also open to any code change suggestion.

thednp commented 2 years ago

Update: you ready to test a new build?

thednp commented 2 years ago

@sam1902 please disregard my previous posts and confirm the latest build is valid for you, we're ready to publish as soon as you confirm.

thednp commented 2 years ago

@sam1902 ?

usr-ein commented 2 years ago

Yep, fixed for me! I re-ran the above test HTML/JS code and now the output is:

M133.67 263.88
L105.353 250.671
L78.419 266.51
L82.2306 235.497
L58.8436 214.775
L89.5166 208.8165
L101.9966 180.1705
L117.1416 207.5005
L148.2416 210.5181
L126.9286 233.3681Z

As you can see, lineTos are now correctly used instead of moveTos.

Thank you for quickly fixing this 💯