Misc changes #1162
Misc changes #1162
Conversation
|
@elalish FYI this is the bad polygon causing the triangulation error: There are two simple polygons here, both are winding in CCW direction. Probably a bug in our boolean. import numpy as np
import matplotlib.pyplot as plt
array = np.array
show = lambda x: plt.plot(*np.hsplit(np.append(x, x[0, :].reshape((1, 2)), axis=0), 2), marker='.')
show(array([
[3.49327, 8.14635],
[3.18819, 7.67361],
[3.2871, 7.67361],
[3.2871, 7.67361],
[3.30402, 7.68686],
[3.49327, 8.14635],
[3.18819, 7.67361],
[3.49327, 8.14635],
[3.49327, 8.14635],
[3.49327, 8.14635],
[3.49327, 8.14635],
[3.49327, 8.14635],
[3.49327, 8.14635],
[3.58896, 7.67361],
[3.49327, 8.14635],
[3.47349, 7.69348],
[3.49006, 7.67361],
[3.49006, 7.67361],
[3.58896, 7.67361],
]))
show(array([
[3.4011, 7.73014],
[3.4197, 7.72455],
[3.49327, 8.14635],
[3.38002, 7.72455],
[3.4011, 7.73014],
]))
plt.show() |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1162 +/- ##
==========================================
+ Coverage 91.69% 91.70% +0.01%
==========================================
Files 30 32 +2
Lines 5959 5980 +21
==========================================
+ Hits 5464 5484 +20
- Misses 495 496 +1 ☔ View full report in Codecov by Sentry. |
|
btw, should we keep both |
👍 Let's unify them. |
| Manifold result = | ||
| Manifold::Cube().Rotate(-0.10000000000000001, 0.10000000000000001, -1.) + | ||
| Manifold::Cube() - | ||
| Manifold::Cube().Rotate(-0.10000000000000001, -0.10000000000066571, -1.); | ||
| EXPECT_EQ(result.Status(), Manifold::Error::NoError); | ||
| ManifoldParams().intermediateChecks = false; | ||
| ManifoldParams().processOverlaps = true; |
There was a problem hiding this comment.
What's the trouble with this test?
There was a problem hiding this comment.
It produces self-intersecting results (more than epsilon):
| TEST(Samples, CondensedMatter16) { | ||
| // FIXME: Triangulation can be invalid | ||
| bool old = PolygonParams().processOverlaps; | ||
| PolygonParams().processOverlaps = true; |
There was a problem hiding this comment.
As for the bad triangulation - did you output that poly with full precision? It says they exactly share a vertex position, but I'm guessing they actually overlap slightly? Because otherwise I think the triangulator should work. When you find polys like this and they turn out to actually be epsilon-valid, go ahead and add them to our polygon test corpus.
There was a problem hiding this comment.
Just updated, but it is indeed sharing the exact same vertex position:
Polygon 0 2.306422737411849e-11 2
18
3.493267459305203 8.14635
3.188192877073223 7.673607122926777
3.287098975966871 7.673607122926777
3.287098975966871 7.673607122926777
3.304015665365264 7.686855769635112
3.493267459305203 8.14635
3.188192877073223 7.673607122926777
3.493267459305203 8.14635
3.493267459305203 8.14635
3.493267459305203 8.14635
3.493267459305203 8.14635
3.493267459305203 8.14635
3.493267459305203 8.14635
3.5889649365369 7.673607122926777
3.493267459305203 8.14635
3.47348551191873 7.693480092989279
3.49005883764325 7.673607122926777
3.5889649365369 7.673607122926777
5
3.401099121578296 7.730144750127856
3.419703476450267 7.724547521102497
3.493267459305203 8.14635
3.380015762439546 7.724547521102497
3.401099121578296 7.730144750127856
# ...
show(array([
[3.493267459305203, 8.14635],
[3.188192877073223, 7.673607122926777],
[3.287098975966871, 7.673607122926777],
[3.287098975966871, 7.673607122926777],
[3.304015665365264, 7.686855769635112],
[3.493267459305203, 8.14635],
[3.188192877073223, 7.673607122926777],
[3.493267459305203, 8.14635],
[3.493267459305203, 8.14635],
[3.493267459305203, 8.14635],
[3.493267459305203, 8.14635],
[3.493267459305203, 8.14635],
[3.493267459305203, 8.14635],
[3.5889649365369, 7.673607122926777],
[3.493267459305203, 8.14635],
[3.47348551191873, 7.693480092989279],
[3.49005883764325, 7.673607122926777],
[3.5889649365369, 7.673607122926777],
]))
show(array([
[3.401099121578296, 7.730144750127856],
[3.419703476450267, 7.724547521102497],
[3.493267459305203, 8.14635],
[3.380015762439546, 7.724547521102497],
[3.401099121578296, 7.730144750127856],
]))
There was a problem hiding this comment.
That looks like a good test for the corpus. I need to come back and debug that one.
| @@ -55,6 +55,7 @@ jobs: | |||
| url: https://pypi.org/p/manifold3d | |||
| permissions: | |||
| id-token: write # IMPORTANT: this permission is mandatory for trusted publishing | |||
| if: ${{ github.event_name == 'release' }} | |||
There was a problem hiding this comment.
Want to add instructions to our release checklist?
Hmm, looking at it, we want to enable intermediate checks for polygons but not manifold. Maybe we can make a And is this considered a breaking change? I think some users may use these params and removing I think I will do that in another PR. |
|
Yeah, good call on deprecation, though honestly I hope no one is changing those values in production. |
Samples.CondensedMatter16test with triangulation check disabled. It can produce bad triangulation, but the output looks kind of fine.To test 1, we need to merge this PR first because manually triggering CI uses the file in the master branch iirc.