Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simple subtract operation throws open path error message #835

Closed
iconexperience opened this issue Nov 25, 2015 · 4 comments
Closed

Simple subtract operation throws open path error message #835

iconexperience opened this issue Nov 25, 2015 · 4 comments

Comments

@iconexperience
Copy link
Contributor

While working on path offsetting I found that very simple subtract operations on CompoundPath objects result in an open path error message:

// compound path
var cp = new CompoundPath();
var p1 = new Path.Rectangle(100, 100, 100, 100);
cp.addChild(p1);
cp.strokeColor = "green";
// path
var p2 = new Path.Circle(180, 180, 50);
p2.strokeColor = "red";
// subtract
var c = cp.subtract(p2);

The message is Boolean operation resulted in open path segments = 2 length = 65.8395631458437

The problem does not occur if p2 is subtracted directly from p1 (var c = p1.subtract(p2)).

Note that I cannot test against the develop branch at the moment, but the bug occurs in 0.9.25. If this is fixed in the new branch already please just close this issue.

@iconexperience
Copy link
Contributor Author

This is really strange. At the beginning of computeBoolean() we reduce both paths, so cp is replaced by a simple Path, which should be identical to p1. But obviously something is different.

@iconexperience
Copy link
Contributor Author

I found out that when we use the CompoundPathas path1 in computeBoolean(), its reduced clone _path1 unexpectedly has a parent (which is the 'CompoundPath). This happens, becausereduce()is called afterclone(), andreduce()` does not remove the resulting path if the original compound path has no parent. Here is a Sketch that illustrates this behaviour.

Maybe in CompoundPath.reduce() Item.reduce() the line

path.insertAbove(this);

should be changed to

this.parent ? path.insertAbove(this) : path.remove();
if (this.parent) child.insertAbove(this); else child.remove();

@iconexperience
Copy link
Contributor Author

As an alternative I thought about changing the behaviour of insertAbove(), but I think the current behaviour is correct: If there is no parent, insertAbove() should fail and null should be returned.

So here is my summary:

  • If reduce() is called on an item, the resulting item should have the same parent as the original item. This implies that if the original item has no parent, the result should have no parent, too.
  • Currently if the original item has no parent, the resulting item can have a parent, because insertAbove() does nothing if there is no parent, so the child that will replace the original item will keep its parent.
  • Since the behaviour of insertAbove() is correct, the reduce() function has to make sure that the parent is set to null if the original item has no parent.

CompoundPath.reduce() does not need to be changed, because if the CompoundPath is replaced by an empty Path, the new Pathobject initially has no parent.

Only Item.reduce() needs to be changed. I will create a pull request.

@lehni
Copy link
Member

lehni commented Dec 26, 2015

Well spotted! Shall merge this in now.

lehni added a commit that referenced this issue Dec 26, 2015
Make sure reduced item has no parent if original item had no parent.
Fix for #835
lehni added a commit that referenced this issue Dec 28, 2015
Make sure reduced item has no parent if original item had no parent.
Fix for #835
lehni added a commit that referenced this issue Dec 30, 2015
Make sure reduced item has no parent if original item had no parent.
Fix for #835
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants