Paper.js: Bug when exporting CompoundPath to JSON

[lehni]

As reported by Daniel Doinov on the mailing list:

I stumbled upon an interesting bug that accures when exporting complex paths to JSON.

Here are the examples:

Before export

After export

The shape you are looking at is a CompoundPath and no manipulation i do on it fixes the problem.

Exporting it as .svg produses the correct shape but i cant use that feature beacause i need the JSON for the whole project to work.

SVG:

pc034 Stay tough_mountains.svg.zip

[danieldoinov]

Hello,
Here are more shapes that produce the error if you need them for testing.

problematicshapes.zip

[lehni]

Here the sketch to reproduce the issue.

[lehni]

This is caused by the same issue as #590.

@iconexperience, would you have time to discuss the best approach of handling CompoundPath sub-path orientation when they are added? The current approach is really dumb and causes more harm than good. I wonder if we should never reorient them? See here:

        // All children except for the bottom one (first one in list) are set
        // to anti-clockwise orientation, so that they appear as holes, but
        // only if their orientation was not already specified before
        // (= _clockwise is defined).
        for (var i = 0, l = !_preserve && items && items.length; i < l; i++) {
            var item = items[i];
            if (item._clockwise === undefined)
                item.setClockwise(item._index === 0);
        }

[iconexperience]

So let's first explain what the current algorithm in CompoundPath.insertChildren() does and what the ideas behind it (probably) was.

CompoundPath.insertChildren() automatically set the orientation of the child paths if it has not been explicitly been set before. The orientation of the first path is be set to clockwise, the orienation of all other paths is set to counter-clockwise.

The idea behind this behaviour was to allow creating paths with holes easily. All that needs to be done is to create a path as the container and the paths for the holes and add them to a CompoundPath. This works well if first path is the largest and the following paths define the holes in the first path.

Here is an example that shows how it works:

As you can see the orientation of the paths will always be the same after the children have been added. This works very will for cases with a large first path and subsequent smaller paths that define the holes in the large path.

But problem occur if the largest path is not the first path (or if there are several large paths next to each other, or if there are crossing paths, or if there are more than two paths on to of each other, or...)

Here is an example where the large path (the head of the emoji) is not the first child of the CompoundPath, but the left eye is. As you can see the orientation of the left eye is clockwise, while all other paths are oriented counter clockwise. Surprisingly the result with evenodd fill rule looks correct, but the result with nonzerofill rule is broken. Here is the sketch

BTW, in the original example with the mountain, the first part is one of the tiny holes at the right of the mountain:

[lehni]

@iconexperience I think what we should ask ourselves also is if we should do any automatic sorting at all when creating CompoundPath items / adding children to them...

[iconexperience]

@lehni I agree, but I am first trying to understand in which cases the reorientation works where ist causes problems.

Here are three more examples that show that the problem occur mainly with nonzero fill rule.

But an interesting point is, that for evenoddfill rule the reorientation is not required at all in the examples above. For evenoddfill rule all examples above are displayed correctly independently from the orientation of the child paths. Here is the first example again, but this time it randomly hard wires the orientation of the child paths, so they will not get reoriented by insertChildren(). If you run the script several times you will see that for evenodd fill rule the result will always look correctly.

So the result of these simple test is

1. The current automatic reorientation of the child paths causes massive problems if nonzero fill rule is used.
2. The reorientation is not necessary if evenodd fill rule is used.

As a conclusion I would say that the automatic reorientation of the paths should not happen at all when inserting paths into a CompoundPath. I currently cannot think of a situation where this would be helpful.

[lehni]

Yes that was my conclusion as well. We could then offer the smart reorient() for nonzero as a method that you call when you're done adding children. And what if we made evenodd the default fill-rule? It would help people who expect compound paths to magically do the right thing...

[iconexperience]

If you set the default fill rule to evenodd, things may become easier because creating compound paths will be more intuitive. On the other hand, if you display the image somewhere else where nonzero is the default fill rule, everything will look messed up and you do not understand why.

I have been through this (before working with Paper.JS). All graphics looked great during development, but when we displayed them in a certain browser (was it Firefox?), all looked totally messed up, because we were using evenodd, but the browser only supported nonzero.

This is not an easy decision. I personally would stick with nonzero.

Regarding the reorient() I think this function is awesome and must be separated from resolveCrossings(). I am not fully sure if it will work on all cases. We are using it after all crossings have been resolved and there are no crossing paths, but what will happen if there are crossing paths? This should be examined before making it part of the public API.

[lehni]

Yes, reorient() only works correctly if there are no crossings. It can still be exposed even with that limitation though, as long as it is documented. If there are crossings, the solution is not really defined I would say, as a path with crossings will be both clockwise and anti-clockwise, in parts. reorient() will then just take into account which one of these it is _more_, if I understand it correctly.

[lehni]

I am also weary of switching the fill rule. Many older browsers don't do evenodd in Canvas... Maybe it's one for v2.0.0 : )

[iconexperience]

reorient() is useful in any case, and maybe we can later improve it to handle crossing paths better or even correctly (if that is possible at all). For for now simply using the current code should be sufficient.

Maybe there could be an option to automatically call reorient() after inserting children into a CompoundPath, but that option should be off by default.

[lehni]

I'm against performing such magic behind the scenes... What we currently do is already confusing, e.g. what if you manually created the path, and expect the sequence to be the same after adding?

[iconexperience]

Agreed, a change in the sequence would be a real problem.

While we are at this we should think about the behaviour of isClockwise() and setClockwise() in CompoundPath(). They don't make much sense, either, because the only look at the first child (which can be totally insignificant, as in the mountain svg). Maybe isClockwise() should calculate the orientation just like it is done is Path, but for all child paths? Or should they even be removed from CompoundPath? Are they required?

[lehni]

@iconexperience at the moment we use them only in the boolean operations, where we determine how to deal with the operands in relation to the performed type of operation: https://github.com/paperjs/paper.js/blob/develop/src/path/PathItem.Boolean.js#L93

// Give both paths the same orientation except for subtraction
// and exclusion, where we need them at opposite orientation.
if (_path2 && (operator.subtract || operator.exclude)
        ^ (_path2.isClockwise() ^ _path1.isClockwise()))
    _path2.reverse();

I agree that it doesn't make that much sense to have CompundPath#isClockwise()...

If we keep the sort parameter that I just added to reorient(), then we could just peek at the first child here, since at this point it is safe to assume that it's the defining path. Is this the way to go, then?

See fc72c05e69c3e6a159c7226a3e9483c15f58667d for the change to reorient()

[lehni]

On further thought... We already have CompoundPath#getArea() which caches its results, and Path#isClockwise() basically returns this.getArea() >= 0 already. All we need to do is move it to CompoundPath! I think that's the solution, and then we don't need the sorting at all, I believe. Let me investigate.

[lehni]

Closed in a0417040f8d2b8518a3c44915ca1aa1c99c8d784