Optimize boolean operations when there are no crossings.

[lehni]

When there are no crossings, the result can already be known ahead of tracePaths(), probably leading to a massive speed-up:

• intersect: return null
• unite: return a compound path with both operands
• subtract: return the first operand
• exclude: same as unite.
• divide: no change needed, since it redirects to the other methods.

[lehni]

@iconexperience so far, an intersection of two non-overlapping paths returns an empty path, not null. But I think null would be better, no?

[lehni]

Examples of current behavior:

• intersect
• unite
• subtract
• exclude
• divide

[lehni]

Regarding divide(): This is what it currently returns: new Group(path1.subtract(path2), path1.intersect(path2)). Since intersect() will return null here, this means we're returning a Group with only one child...

Now the question: Should we treat divide() the same as subtract() and return a Path instead, or should we be consistent and always return a Group from divide()? I think the latter...

[iconexperience]

@lehni Regarding divide(), I think we reduce() is called on the result before returning it, so this would take the child out of the group and return it.

[lehni]

@iconexperience If you look at the link above to the current behavior, you'll see that for divide() that's not the case. And I think it's important in this case that it always returns the same type of item (all the others always return a PathItem, also). It's hard to deal with the results otherwise. Agreed?

[lehni]

Ok, it's a bit more complicated than I thought... We can only do this, when the two shapes are also not contained in each other. Otherwise, subtracting a small concentric circle from a larger one stops working, as both will have clockwise orientation and the compound path will not end up with a hole in it. I think when there are no crossings, a simple bounding box check should be enough to determine if one path is fully contained within another or not. Or am I thinking about this wrongly now?

[lehni]

@iconexperience one more thing regarding reduce(): It's true that it's currently called, but since subtract() returns an empty path, that doesn't get reduced away and the result remains a Group. Do you agree with me that it's good to always return a group though?

[lehni]

Unfortunately the situation with the bounding box check is not as simple:

If one of the operands is a compound path with two non-overlapping paths, and the other is a simple path that doesn't intersect with either sub-paths of the compound-path, the check does not suffice.

Here an example sketch. The two small red circles are part of the same compound path, the blue circle is a separate path, and I subtract the red circles from the blue one.

We could check against the bounding boxes of all children in that case?

[iconexperience]

@lehni I do not have a very strong opinion here, because I would just take whatever the function returns as long as it is a paper.Item or null.

But of course making sure that the same object type will be returned can make life easier. So let's do it this way.

Regarding nullor empty path: An empty path allows chaining, plus it keeps the style of the path. Let's say you have three paths p1, p2, and p3. You want to do r = p1.subtract(p2) and then r2 = r.unite(p3). Now if the subtraction is null, r2 must be calculated by p3.unite(r), so it will have the style of p3. If the subtraction returns an empty path, r2 will have the style of p1. Does this make sense?

[lehni]

Yeah I see your point regarding chaining. Better to do it this way, then!

[lehni]

So that was easy, after all. I think this works well. Please let me know if you see / find issues with it.

[iconexperience]

@lehni I just took a look at your implementation. This is quite nice, but in future we could also include paths that contain each other but have no crossings (or maybe no intersections). This operation would only have to do something very similar to what the current reorient() does.

[lehni]

@iconexperience I was thinking about that, and was hoping you'd chip in : ) Could we just call reorient() on the resulting PathItem? I also still need to implement unit tests for this, so will reopen for now...

[iconexperience]

@lehni Calling reorient() would not work, only for unite() maybe. We need a different function, that takes the type of boolean operation as an argument and keeps/removes the sub paths depending on that type. This function will probably rather simple once it is finished, but it will be quite a puzzle to get the correct behavior for each boolean type.

[lehni]

@iconexperience would you mind taking this on? There's no rush for it, but I think your expertise in that realm is much better than mine.

[iconexperience]

@lehni Yep, I can try. This is also very useful for my use case, because I would expect quite a performance improvement.

[iconexperience]

I have started working on this, but there will not be much progress today.

One essential part is building a tree structure of the paths in the compound path. The root paths are the ones that are not inside another path. They can have children, which are the paths inside the paths. The children can again have children, etc.

The reorientation of the tree will give alternate the orientation for each tree level.

And here is what I think needs to be done for the different boolean operations:

Unite

• Combine all sub paths of path1 and path2 and build the tree.
• Reorient the tree.

Subtract

• Reverse the orientation of path2.
• Combine all sub paths of path1 and path2 and build the tree.
• Delete all root paths (with ancestors) that have a counter clockwise winding
• Reorient the tree

Intersect

• Combine all sub paths of path1 and path2 and build the tree.
• Reorient the tree.
• Delete all root paths and make their children the new root paths.

Exclude

• Reverse the orientation of path2.
• Combine all sub paths of path1 and path2 and build the tree.
• Reverse all root paths (and their ancestors) that have a counter clockwise winding
• Reorient the tree

[lehni]

@iconexperience that's great! And by all means, do this in your own time : )

One thought: Is it possible to build this in a way that it could share code with the current reorient()? e.g. by sharing the same underlying private function that is parametrized differently? I'm trying to keep the total size of the library as low as possible, so if there are parallels here, it would be great to leverage those, and keep that in mind.

[iconexperience]

@lehni Yes, that was my intention. I may first implement it separately, but then it should be combined with reorient().

[lehni]

Sounds like a plan!

[iconexperience]

@lehni I have made a very first implementation as a Sketch (the example is at the end, where you can set the operation type).

The focus right now is making it work. Once it works, I will try to simplify the code.

[iconexperience]

And here is an updated version that supports 'intersect' and 'exclude', but 'exclude' probably does not work correctly, yet.

[iconexperience]

An another update to the code. 'exclude' now works for the example, but I have to do more testing to ensure that it works in all cases.

[lehni]

@iconexperience great news! The code looks good. At first sight, there is one thing you may want to consider when writing it, but we can add that in the end also: It sort of looks like the code calls setClockwise() more than once per node, with often changing orientation. If that is the case, then I think it would be better to keep the orientation information in a separate list, and only set it once at the end, because internally, patch.setClockwise() calls path.reverse() if the orientation changes.

[iconexperience]

@lehni Alright, I will think about that. Do you see any problem by using a mapping with the path-id as the key?

[lehni]

@iconexperience no, there is no problem with this mapping. Paths always have such a unique id, and I'm already using a similar lookup table in the current version of reorient().

[iconexperience]

One more update. This is more a backup for me, because I am programming directly in Sketch without version control (not the best idea). Next step will be adding the current "reorient" function.

[iconexperience]

Last update for today. If the second argument of the function is null and the operator is unite, the functionality should be the same as the reorient() function, except for the nonzero handling, but this functionality is already there. It only has the be used properly.

I was also able to reduce the code size a little bit and I am quite satisfied with the result so far.

[lehni]

Lokos great! Is reverseOrientation() used at all, or can that one actually go?

[iconexperience]

Well spotted, reverseOrientation() can go, normalizeOrientation() does this now.

Pretty compact, isn't it? :)

[lehni]

Yes, very cool! Shall I merge this in already? Or would you like to create a PR?

[iconexperience]

Yes, feel free to merge this in. Thanks!

[lehni]

@iconexperience I will be away for the next three days, will look into merging on Monday. If you feel like working more on it, or creating a PR, go ahead : )

[iconexperience]

OK, let's see what I can do. Enjoy your paper-free time :)

[iconexperience]

And here is another version. This one handles nonzero fill rule of the original paths correctly. I have created a new example for this.

[iconexperience]

The whole handling of nonzero fill rull is making my head spin again. Therefore I decided to test how some popular vector drawing programs handle these cases. As you will see, there is not much to learn from them :)

For testing I first created a compound path of two concentric rectangles, both with clockwise orientation:

When evenodd winding rule is used, the inner rectangle is a hole, with nonzero the inner rectangle is filled:

evenodd:

nonzero:

Then I create a simple path consiting of another rectangle concentric to the others. This rectangle has a clockwise orientation, too. This rectangle is united with the compound path. Here are the results:

Corel Draw X7: In the second (nonzero) case if the blue path is selected last

Inkscape 0.91

Unfortunately I do not have a license for a recent version of Illustrator, so I cannot test with it. But what I already see is that there is no consistent behavior, in the case of Corel Draw even within the same program. So this does not help much, we have to find our own way.

[iconexperience]

My personal opinion is that the filled area of the unite operation's result should be where at least one of the two original paths is filled. This is bascially the definition of the unite operation. Furthermore in standard cases, all parts of the paths that lie inside the resultin path get lost, therefore this should happen with these special cases, too.

Standard case (undisputedly correct result):

Paths with same orieantation and nonzero fill rule (IMHO correct result)

[lehni]

@iconexperience I completely agree with your observation. And it also appears to be how Illustrator handles it (you have the option to install a Trial version of it for a month, BTW)

[iconexperience]

@lehni Thanks for testing wilth Illustrator. The only thing that keeps me from finishing this is that you would like to have a "less destructive" algorithm for reorienting compound paths outside the boolean operations. I still have no idea how this could be achieved (but I do not rule out that there is a way to achieve this). Would it be sufficient for now if we used this reorientation algorithm only for boolean ops?

Also, I suggest that this algorithm should be used for cases without intersections, not only without crossings. In the example below there are no crossings, but my algorithm would create two rectangles in a unite operation, while the correct result is certainly one large rectangle.

[lehni]

Currently, when calling reorient(), the method always assume the even-odd fill-rule regardless of the path's style. We discussed this a couple of days ago. And I thought with even-odd, the sequence or amount of sub-paths doesn't change, only their orientation. That's non-destructive enough for me. Has something changed here?

As for the case of two touching paths: getCrossings() actually returns overlaps as its unclear if an overlap is a crossing or not without looking at neighboring intersections, and we don't do that right now. It gives the algorithm too many potential crossings, but the winding contribution will take care of that...

[iconexperience]

@lehni So we should try to keep the original sequence of the paths? With even-odd this should be possible. I will add a way to keep the sequence to the code.

Regarding the crossings/overlaps: Let's just give it a try :)

[lehni]

Yes, you can look at the current reorient() to see how I handle that. the lookup structure stores their original index, and inserts them back there. If the algorithm removes paths, then some indices remain empty, but that's not a problem because addChildren() handles those gaps. No sorting is required.

[iconexperience]

@lehni I finally found some time to work more on this. The implementation seems to work now, but there is still one test that fails.

This test is a bit special and I am not sure if the expected result is correct. It's test 300/54

Here are the two paths for which an intersect operation is performend (here is the sketch)

The expected result is a tiny path, while my algorithm also returns a path of area 0 that is connected to the tiny path:

I am not sure if the part of the result with zero area should really be removed, as in the exprected result. It is part of the original path 1, so maybe it should become part of the result, if it intersects with the other path.

So what do you think? Is the expected result correct, or the one currently returned by my implementation?

[iconexperience]

Here is my latest version. This one works with the two examples that I created for this issue and passes all tests except the one described above. After cleaning up and adding more comments I will post a PR.

[lehni]

@iconexperience these paths with area 0 are removed in normal boolean operations because they have no area and hence no change in winding...

[iconexperience]

@lehni Thanks. After using the existing createResult() function, which calls reduce({ simplify: true }), the test does not fail anymore. Looking good!

[iconexperience]

@lehni I have problems committing the file, because jshint does not like this part:

        var lookup = Base.each(children, function(path, i) {
            this[path._id] = {
                winding: path.isClockwise() ? 1 : -1,
                index: i
            };
        }, {}),

It says that lookup? is used before it is created (refering tothis` in the second line). This is the same code that is already being used. Do you have any advice?

[iconexperience]

@lehni solved the issue with jshint. This was a bit different than I had described.

[lehni]

@iconexperience before I merge your PR, could you clarify this part regarding the handling of the non-zero fill-rule:

If the second argument of the function is null and the operator is unite, the functionality should be the same as the reorient() function, except for the nonzero handling, but this functionality is already there. It only has the be used properly.

The functionality is already where?

[iconexperience]

@lehni I think the functionality of myPath.reorient(nonZero) can be achieved by calling computeNonCrossingBoolean(myPath, null, myPath, null, 'unite'), with the difference that a new CompoundPath will be created and nonZero will be set automatically by using the fill rule of myPath.

We can change the function to return only the children and handle them outside the function. In that case the function name should probably be changed, because it's behavior will not be consistent with computeBoolean() anymore. A nice effect would be that the function then only requires _path1 and _path2 and operation as arguments.

Additionally we can temporarily set the fill rule to nonzero if nonZero==true. reorient() would then look like this (untested):

reorient: function(nonZero) {
  var originalFillRule = this.fillRule;
  if (nonZero) this.fillRule = 'nonzero';
  var children = computeNonCrossingBoolean(this, null, 'unite');
  this.setChildren(paths);
  this.fillRule = originalFillRule;
  return this;
}