-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Unite operation results in empty path #1261
Comments
What I find irritating here is that the intersections at the bottom corner actually results in splitting the curves there, although the distance of the points there is tiny ( |
Interesting indeed! Here the sketch with intersections marked More analysis soon. |
And here the updated sketch keeping the circle size independent of the zoom level With this you can see what the problem is: On the right hand side, the green path appears to cross the red one, but when you zoom in, you can see that it doesn't seem to. The left part of the right "beam" is not crossing at all, and the right part appears to barely touch (no matter how much I zoom in, I can't see the two lines separately, before the rendering goes crazy because of too much zooming). So why is this not an overlap? It may just be another case of not finding an overlap. I would guess that this is the reason why it's not doing the right thing. |
I found a way to adjust this and make the code not see it as a crossing, but the unite operation still fails. Could it be that this is a problem of the new winding code? I have to bring back my boolean debug code to look into the involved winding numbers, but it could be the source of the problem. |
Here is the sketch using the "segment" form. I find this more useful for fiddling around. Regarding the "not found overlap": I do not think this is the problem. If I move the green corner down a bit as in this sketch the problem persists. Regarding your idea with the winding code: I think this is not the problem, either (but not sure), because I am using another version of the winding code and still see the same problem. |
Maybe we are doing it still wrong when collecting the intersections. First we should look if end points of curves are close, using geometrical epsilon. If so, these intersections should get |
If I put the following code snipplet into var c1p0 = c1.point1,
c1p3 = c1.point2,
c2p0 = c2.point1,
c2p3 = c2.point2;
if (c1p0.isClose(c2p0, epsilon))
addLocation(locations, include, c1, 0, c1p0, c2, 0, c2p0);
if (c1p0.isClose(c2p3, epsilon))
addLocation(locations, include, c1, 0, c1p0, c2, 1, c2p3);
if (c1p3.isClose(c2p0, epsilon))
addLocation(locations, include, c1, 1, c1p3, c2, 0, c2p0);
if (c1p3.isClose(c2p3, epsilon))
addLocation(locations, include, c1, 1, c1p3, c2, 1, c2p3); |
There is a reason why we don't look at these points first: Remember when we had a case where a line-line case would produce 2 intersections instead of only one, because one pair of start-/ end-points where closer to each other than BTW, in the code you're proposing, which values is And finally the question: Is there a reason for you to still use your own version of the winding code? Does the one in the library not work as well? And if not, could you provide cases where it fails? |
@iconexperience I just tried your suggestion of adding the code snippet, but it doesn't change the behavior at my end (and the same code is already performed after Did you also change the |
Oh, and #1174 is the issue that breaks if I add this code and increase the epsilon... But the sketch above still fails for me on |
@iconexperience the strange corner is not at the bottom, btw, it is at the top right. The distance between the found intersection and the actual corner is |
Oh, btw: The problem is that the |
I now found out that my |
Sorry for again another useless proposal. I am currently working on so many things, so I tried to find a quick solution, but doing fundamental things in a hurry is just a bad idea. And I noticed later that the strange corner is at the top right, too. Anyway, your observation is really interesting. It sounds like this could really be the breakthrough. |
@iconexperience I have just pushed all the changes that fix this issue here. But it also introduced some new failures with unit tests. After analyzing the first problem, it turned out that this error was caused by the reintroduction of the following snippet (see #1073 (comment)): if (onPath && !windingL && !windingR) {
// If the point is on the path and the windings canceled
// each other, we treat the point as if it was inside the
// path. A point inside a path has a winding of [+1,-1]
// for clockwise and [-1,+1] for counter-clockwise paths.
// If the ray is cast in y direction (dir == 1), the
// windings always have opposite sign.
var add = path.isClockwise(closed) ^ dir ? 1 : -1;
windingL += add;
windingR += add;
} I have then removed it, but now another test case is failing, and I am not sure why yet. I am fairly certain that the snipped above is wrong though, as it leads to wrong winding numbers. I am tapping a bit in the dark with all this though, perhaps you can help me out? Also, I am still waiting for feedback on the winding changes outlined in #1073... |
Here the newly failing sketch. Note that it works alright on |
When zooming in on the bottom part o Firefox (all other browsers crap out due to very high zoom level), you can see the situation there: What's interesting is that if I scale the active layer up by factor 10, everything works. Looks like winding imprecisions to me, but it's really hard to debug: project.activeLayer.scale(10); |
this address the issue outlined in #1261 (comment)
I just found that it appears to be OK to reduce |
I started looking at our winding code and already found one, albeit unrelated part that can be improved. In |
One part of the code that I find suspicious is this line in
I have my doubts that this only handles thoses cases that it is supposed to handle. It may handle other cases, too, which could cause trouble. I will have to think this though again. |
Yes, I am pretty sure that part of } else if (a0 != a3Prev) {
// A horizontal curve is between current and previous
// non-horizontal curve
if (a3Prev < paR && a > paR) {
// Right winding was not added before, so add it now
windingR += winding;
onPath = true;
} else if (a3Prev > paL && a < paL) {
// Left winding was not added before, so add it now
windingL += winding;
onPath = true;
}
// TODO: Determine how to handle quality when curve is crossed
// at starting point. Do we always need to set to 0?
} I have not tested if this fixes any of the remaining issues, but at least it passes all unit tests. |
Here is an explanation of the code above. It is supposed to handle cases in which the winding of the path does not change (from upwards to downwards or vice versa) and there is a horizontal curve between two non-horizontal curves. The blue area is the range between In case 1 the winding of the previous curve was not added, so we should add the winding of the current curve. In case 2 the winding of the previous curve was added, but it is on the opposite side of the blue range, therefore we should add the winding of the new curve. In case 3 the winding of the previous curve was added. The start point of the new curve is in the blue range, therefore the winding of the new curve will not be added. In cases 4 and 5 the winding of the previous curve was added. In order to not add the same winding twice, the winding of the new curve will not be added. |
@iconexperience great, thanks! That's very useful. I will look into it shortly and get back. |
@iconexperience this looks reasonable. But we still need to figure out what to do with |
@lehni The correct approach for the quality factor would be like this: In all these special cases we have two abscissa values, |
@iconexperience thanks for the PR! This doesn't address the proposed way to handle quality yet, correct? |
Unfortunately not. This only addresses the simple stuff. And the addressed cases are quite rare, therefore we should not see any changes at all, but it had to be done. Sorry for being late on the quality handling, I still have some other things to do first. |
Another question: Should we use Curve.solveCubic(v, io, po, roots, 0, 1) === 1
? roots[0]
: 0.5, |
If I see it correctly, if we arrive there something has gone wrong in |
Or maybe we should use Curve.solveCubic(v, io, po, roots, 0, 1) > 0
? roots[0]
: 0.5, But honestly, I do not think this makes any difference. |
Here is an example of a unite operation that results in an empty path:
Here is the sketch, but note that in the sketch everything works as expected.
The text was updated successfully, but these errors were encountered: