-
Notifications
You must be signed in to change notification settings - Fork 86
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
Earclip #564
Conversation
weird. maybe you can ignore this for now and fix other failures first. hopefully their causes are the same. |
Took a good 20 minutes, but I got all your openscad tests to run, @pca006132. They all appear to have passed for me. How's it looking for you? I still have some ideas to further improve robustness of this triangulator, but I think it's better than it was before at least. |
It seems that the CCW check is not enabled when
but it only throws an error when I set
|
Ah, indeed, |
I thought that the ccw check is enabled regardless of the value of processOverlaps, because iirc the result may not be manifold if the triangulation is not all ccw? |
No, manifoldness should never be affected by geometrically invalid results. We did have that happen because of a bug in the decimator I just fixed, which is why that had me so concerned. |
By the way, if you happen to find a way to change that openscad test so that it will terminate a given test when the triangulator throws, but still proceed to the next test, that would be very helpful. These shell scripts are making me really appreciate our GTest framework... |
removing the exit on error flag for bash should do the trick. will change it when I have access to my computer later today |
i.e. diff --git a/test b/test
index ec163b2..a6bca8a 100755
--- a/test
+++ b/test
@@ -4,7 +4,7 @@
#
# ./bench foo.scad
#
-set -euo pipefail
+# set -euo pipefail
RUNS=${RUNS:-1}
export OUTPUT_DIR=${OUTPUT_DIR:-$PWD/out} |
it seems that the mesh produced is fine even though it throws error when |
As in, when you set |
To your point, I'm now getting only one failure in your openscad-test - box.scad is failing here:
But I verified that this polygon is actually self-intersecting at the specified tolerance (though a bit more and the whole thing is just a degenerate line), so this result is correct. This is consistent with seeing a resulting object that looks right as well. Do you have any other OpenSCAD tests you want to verify this triangulator on? If you think it's better than our current triangulator, then I think I'm ready to merge it. |
I think we can merge this now. This is definitely more robust! |
}; | ||
|
||
TEST(Polygon, Eberly) { | ||
Polygons polys; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long time, no see @davideberly! I've extended your earclipping algorithm to handle ε-valid polygons (polygons that may self-intersect, but for which a perturbation of each vertex by less than ε exists for which the polygon does not self-intersect). It's taken a lot of work, but I'm pleased with the results.
In the process I found a small error in your paper. It turns out Mark Spoor was wrong and your original minimize-the-angle-approach is correct. Here is a counter-example polygon for finding a mutually visible vertex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for letting me know. I am in the process of dealing with another issue for the ear-clipping algorithm and will resolve that soon I hope. I will restore the previous algorithm.
@pca006132 Sorry, I did another significant refactor - I promise this is the last! I believe it's significantly more robust now and there are no disabled tests anymore, as well as several new ones added. If you wouldn't mind trying it out on whatever OpenSCAD models we had before that were failing, I'd appreciate it. Of course the reduced polygon test cases all pass, but those reductions doesn't really apply anymore now that the algorithm has changed. |
This is nice. I don't have more openscad models to check, I think it would be fine if the models I mentioned previously works. |
* earclip compiles * some tests passing * 46 tests passing * refactor * cleanup * new cost * cleanup * fixed TouchingHole * more fixes * only 5 tests failing * fixed short sides * fixed Sponge2 * better cost * improved keyholing * Moved Rect to public * added top/bottom check * fixed bBox * Polygon tests pass * fixed Sponge3a * another fix, another test * keyhole refactor * Added ClipDegenerates * Refactor FindStarts * fixed bugs * fixed Sponge4 * refactor clipped * another bug, another test * another bug, another test * Sponge4 passing * all tests passing * removed old triangulator * fix isfinite * debug checks * remove 2-manifold check * added comments * fixed missing point bug * another test, another fix * two tests added and fixed * fix build * fixed CoplanarProp * fixed seg fault * reenable 2-manifold check * refactor for polygon area * factored out loop * tests passing * another test, another fix * another test, another fix * fixed dedupe segfault * ignore duplicated verts * re-enabled turn180 * re-enabled duplicate tests * another test, another fix * refactor keyholing * more precise area computation * disabled a test * added test * another test, another test * fixed bridge bound * fewer short edges * replaced intersection sorting with InsideEdge * another test, another fix * allow a few degenerates in Sponge4
Fixes #530
Fixes #535
Well, this turned into a big one. I spent a week or two trying to refactor my monotone triangulator to be more robust given the new test cases, and finally decided it was just too convoluted, mostly because it's a sweep-line approach, which doesn't handle the concept of epsilon perturbations well because there's not a great concept of certainty in a sorted list.
Instead, I went back to the drawing board and remembered years ago when I first wrote this triangulator that I was on the fence between using monotone triangulation or ear clipping. Either way I needed to write my own, since none existed for dealing with epsilon-valid polygons. At the time I went with monotones because it is O(nlogn), while ear clipping is O(n^2). This time, I decided that the simplicity associated with ear clipping makes it worth the extra time, especially since our polygon edges n is generally a lot smaller than our triangles n.
It's taken 2 or 3 weeks to build and robustify an ear clipping algorithm, but I must say that beats the many months worth of effort that went into the monotone triangulator. Ear clipping is also significantly less code, even with all my added precision-related code.
@pca006132 your minimize test case function was invaluable! It cut 5,000 point polygons down to 12 point test cases - you can see I've added quite a few.
Please help me kick the tires on this - the real goal is to get all our OpenSCAD problem models working properly.