SVG import does not handle relative "moveto" commands correctly

[iconexperience]

If the path data of an SVG path contains relative moveto (m) commands, the coordinates are always calculated relative to [0, 0] instead of the current point calculated using wrong reference coordinates.

Relative moveto commands are not used often, but I found that they are used by some of Google's Material Icons. Here is the source of ic_all_out_black_24px.svg:

<svg fill="#000000" height="24" viewBox="0 0 24 24" width="24" xmlns="http://www.w3.org/2000/svg">
    <path d="M16.21 4.16l4 4v-4zm4 12l-4 4h4zm-12 4l-4-4v4zm-4-12l4-4h-4zm12.95-.95c-2.73-2.73-7.17-2.73-9.9 0s-2.73 7.17 0 9.9 7.17 2.73 9.9 0 2.73-7.16 0-9.9zm-1.1 8.8c-2.13 2.13-5.57 2.13-7.7 0s-2.13-5.57 0-7.7 5.57-2.13 7.7 0 2.13 5.57 0 7.7z" fill="#010101"/>
    <path d="M.21.16h24v24h-24z" fill="none"/>
</svg>

And here is the sketch

In PathItem.setPathData() when closing the path on the z command, the current coordinates are not updated. Inserting the following code at PathItem.js line 173 updates the current coordinates to the start coordinates and fixes the issue:

current = start;

[lehni]

Well spotted! I'll fix it right away. Thanks!

[iconexperience]

@lehni Thanks. There is also something wrong with the absolute vertical and horizontal lineto commands. I will write another bug report, but those are a bit trickier.

[iconexperience]

@lehni Fixing this issue has made another problem come to light. Now the following example does not work anymore (again from the Google Material Icons):

var item = PathItem.create("M11 18V6l-8.5 6 8.5 6zm.5-6l8.5 6V6l-8.5 6z");
item.fillColor = "green"
item.fitBounds(new paper.Rectangle(0, 0, 512, 512));

This works in the sketch but not with https://github.com/paperjs/paper.js/commit/73fc111b implemented.

Here is what is going wrong:

• In PathItem.js line 115 the current point is assigned to the start point. After that, current and start are one and the same object.
• On a vertical or horizontal lineto command (V, v, H or h), the coordinates of the current point are set to new values instead of creating a new Point object. If this command follows the moveto command, it means that the coordinates of the start point are changed, because current and start are one and the same. Uh-oh, the start point has moved!

IMHO the best solution would be to keep the Point object that is created at the beginning for the start point and only get and set the corrdinates. This would affect to following lines:

PathItem.js line 115 becomes:

start.set(current.x, current.y);

PathItem.js line 176 becomes:

current.set(start.x, start.y);

The latter could stay current = start but I think with the change the code becomes cleaner and start will always be the same object.

While we are at this, do you know if the + in PathItem.js line 78 serves any purpose? It is either unnecessary or some Javascript magic that I do not understand (I did not know that a plus is a unary operator for conversion to a number).

[lehni]

Wells potted! I'm wondering if we should optionally support point.set(point) for shorter syntax...

[iconexperience]

I did not dare to ask :)

[lehni]

We should also have unit tests in place for these edge cases...

[iconexperience]

This issue is not really an SVG issue, but more a problem with PathItem. Therefore the appropriate test file would be tests/CompoundPath.js? Anyway, for the first test case you can simply compare these two path items, they must be the same for the test to pass.

var p1 = PathItem.create("M20 20l20 20v-20zm20 20l-20 20h20z");
var p2 = PathItem.create("M20 20l20 20v-20zM40 40l-20 20h20z");

[iconexperience]

And the second test can look like this:

var p1 = PathItem.create("M11 18V6l-8.5 6 8.5 6zm.5-6l8.5 6V6l-8.5 6z");
var p2 = PathItem.create("M11,18l0,-12l-8.5,6z M11.5,12l8.5,6l0,-12z");
equals(function() {
        return p1.equals(p2);
    }, true);

[iconexperience]

And while we are at this, here is a simple test to cover issue #705

var p = PathItem.create("M10,10 L20,20 L10,30 M30,10 L30,30");
equals(function() {
    return p.children[0].isClosed();
  }, false);

[iconexperience]

I googled a bit for path data test cases and found this page:

http://www.princexml.com/forum/topic/1437/svg-path-data-parser-lacks-correct-exponent-parsing

To my surprise Paper.js handles the path data correctly! But I suggest that we add this as a test case.

var p1 = PathItem.create("M 0 1.5 l 1e1 0 m -10 2 l 1e+1 0  m -10 2  l 100e-1 0");
var p2 = PathItem.create("M0,1.5l10,0 M0,3.5l10,0 M0,5.5l10,0");
equals(function() {
        return p1.equals(p2);
    }, true);

[lehni]

Great, thanks @iconexperience! Very useful. I agree that although it's SVG data, it does not directly belong to the SVG import / export tests.

[iconexperience]

@lehni There is a comprehensive test suite at https://www.w3.org/Graphics/SVG/WG/wiki/Test_Suite_Overview

I am not sure if they can be used for testing with Paper.js, but I decided that you should know about it.

[iconexperience]

@lehni My earlier comment about something being wrong with horizontal and vertical lineto commands is wrong. This was actually caused by the moving start point.

The simple fix described in this comment should work for all cases that I have found.

[iconexperience]

I manually tested the examples at

https://www.w3.org/Graphics/SVG/Test/20110816/svg/paths-data-01-t.svg and https://www.w3.org/Graphics/SVG/Test/20110816/svg/paths-data-02-t.svg

and they all work with the new code! This one fails with the old code:

var p = paper.PathItem.create('M372 130Q272 50 422 10zm70 0q50-150-80-90z');
p.strokeWidth = 3;
p.strokeColor = "green";

Do you have any idea how to convert the examples to proper test for our test cases? If I have a template I can convert the others.

[lehni]

@iconexperience the mentioned change to make set() accept the same arguments as the constructors became quite the undertaking: 32d8c969fbe349808ad602fc294989bc45d8be54

But now it's time to fix this issue here :)

[iconexperience]

Oh, that looks like quite a lot of work. So set() will now accept the same arguments as the constructor? Excellent!

[lehni]

Yes it does, and that works on Point, Size, Rectangle, Matrix, Color. But now that I've done that, I realized that I don't need it to fix #1101 : )

The thing is this: We already keep pointing current to new values in each command. So what we should be doing instead is simply making a clone of it at the beginning of the 'h' & 'v' commands, and it'll solve it all.

[iconexperience]

Yes, of course, also the old set() implementation would have done the job. But I assume you did the work to make the API better, right?

Anyway, should I create those test cases from the W3C tests? Here is an example how one could look like:

// source data from W3C test
var p1 = PathItem.create("m 360  325  c -40  -60     95 -100     80    0      z  ");
// internal Paper.js representation
var p2 = new paper.Path({segments:[[360, 325, 0, 0, -40, -60], [440, 325, 15, -100, 0, 0]], closed:true});
equals(function() {
      return p1.equals(p2);
  }, true)

[lehni]

Yes, exactly : )

Regarding the tests, I started working on it, but got too tired. I'd like to avoid comparing the results of two path-data strings, so what you suggest is better, but then it gets complicated with compound paths... Give me some time to wrap this up. Once it's in place, you can then add more paths. Sounds good?

Meanwhile, you probably saw it, but I'd really like to wrap this thing up here https://github.com/paperjs/paper.js/issues/1075#issuecomment-232584185 Not sure what can be done? The overlap check could be a way to achieve it, but is that making it too complicated for nothing?

[lehni]

Regarding the unit tests, I had two thoughts: I'd be nice if this notation was possible:

var compoundPath = new CompoundPath([
     [<path1 segments>],
     [<path2 segments>],
     [<path3 segments>],
     ...
]);

But for this to work, we'd need a way to also include the closed information in the passed segments array themselves. So what if this was also possible:

var path = new Path([
     [<point 1 coordinates>],
     [<point 2 coordinates>],
     [<point 3 coordinates>], 
     true // closed
]);

We could then use this notation for more compact exportJSON() exports of compound-paths.

[iconexperience]

Why don't we use for now this simple notation:

var path = new CompoundPath({children: [
  new Path({segments:[[...], [...], [...]], closed:true});
  new Path({segments:[[...], [...], [...]], closed:true});
  new Path({segments:[[...], [...], [...]], closed:true});
]});

Converting this to the simplified notation, once it is available, will be very simple.

Oh, and here is another difference in my codebase: I threw out the whole reorientation when adding children to a CompoundPath. The old behavior got more and more in the way, especially when reconstruction CompoundPath objects during debugging. The only reason that I see why you should keep the reorientation is to stay backwards compatible.

[lehni]

@iconexperience Yeah of course we can also do it this way, but I've already implemented the new notation (only a few lines of code), as it's much easier to output programmatically from existing content, which is why I started thinking about it in the first place. I was wondering what you think about the suggestion itself, the syntax... The boolean at the end of the segments list is a bit strange, perhaps?

The reorientation feature will also get into the way of things with this test, and I really want to throw it out too. So who knows, maybe we do it now, and the next release is v1.0.0? I could then use that to break with some other odd backward compatibility. and the planned change of Style handling could then land in v2.0.0... I think that's a good idea, actually.

[lehni]

@iconexperience I've implemented this locally now right with the newly implemented tests. There's also a helper function called describe() that you can use to output further SVG paths: https://github.com/paperjs/paper.js/blob/288c3d4012f1de9617db18eec94b2c638665dd27/test/tests/PathItem.js#L84

Feel free to add more SVG data.

[lehni]

And here it is now, in the core: e53963385291755370399eb4bf4e2d6c4ba8777d

For now, import only.

[iconexperience]

@lehni I just tried adding the tests from the W3C Test Suite and two of the test failed, because of a different length. Looking at you code I noticed that you are comparing a path built from the raw path data to a path built from rounded data (the data build by describe()). Maybe rounding the coordinates is not the best idea here?

[iconexperience]

Here is one of the path data that currently fail:

'M240 296q25-100 47 0t47 0t47 0t47 0t47 0z'

And here is what describe() produces:

[[240,296,0,0,16.66667,-66.66667],
    [287,296,-14.66667,-66.66667,14.66667,66.66667],
    [334,296,-16.66667,66.66667,16.66667,-66.66667],
    [381,296,-14.66667,-66.66667,14.66667,66.66667],
    [428,296,-16.66667,66.66667,16.66667,-66.66667],
    [475,296,-14.66667,-66.66667,0,0],true]

[lehni]

Ah maybe! Lemme check.

[lehni]

It just needs one digit more in the rounding. Can I commit this one?

[lehni]

Actually, maybe I should just relax the length comparison a bit more instead.

[lehni]

After comparing segments, and closed, why do we even compare length there? Testing of length is in another part of the test suite already. I'll remove that check.

[lehni]

@iconexperience I've just done that in c338e9a6ecd48700f696c7ea61524273788aabde. Forgot to mention this issue.

[iconexperience]

@lehni Thanks, I just created a PR. Is there anything left to do for this issue?

[lehni]

Perfect, thanks! No, that's it, we can close this now.