Added DisabledFaceTest #908
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #908 +/- ##
==========================================
- Coverage 91.84% 89.69% -2.15%
==========================================
Files 37 66 +29
Lines 4976 9775 +4799
Branches 0 1061 +1061
==========================================
+ Hits 4570 8768 +4198
- Misses 406 1007 +601 ☔ View full report in Codecov by Sentry. |
| ExportMesh("disabledFaceTest.glb", hull.GetMesh(), {}); | ||
| } | ||
| #endif | ||
| EXPECT_TRUE(isMeshConvex(hull)); |
There was a problem hiding this comment.
Does isMeshConvex return true for an empty mesh? If so, we should probably also expect !hull.isEmpty() too.
There was a problem hiding this comment.
Yeah it does. I have added that as well. Thanks for pointing that out.
| @@ -197,4 +197,29 @@ TEST(Hull, FailingTest2) { | |||
| } | |||
| #endif | |||
| EXPECT_TRUE(isMeshConvex(hull, 2.13966e-05)); | |||
| } | |||
|
|
|||
| TEST(Hull, DisabledFaceTest) { | |||
There was a problem hiding this comment.
When you say it has a disabled face, do you mean during the coarse of the algorithm it disables a face? Did you verify this test fails if you revert whatever the fix was?
There was a problem hiding this comment.
Yes, and that disabled face is linked to one of the halfEdge returned after the CreateConvexHalfedgeMesh function call. The algorithm doesn't remove them during computation to reduce time. But, we need to filter those at the end.
Yes, I verified that the test fails without the fix.
|
Thanks! Have you had a chance to look at |
|
To be honest I still don't understand why is that considered an error. Isn't it possible that it drops some verts that lie outside or on the surface of the hull? |
|
It's because it is two axis-aligned spheres. If you drop the n down to, say 12, and look at the result, I think it'll become clear. To be fair though, getting a repro of that windows problem is a higher priority. |
I added the narrowed down version (12 points) of the test case
101213.stl. It is a case where one of thehalfEdgein the list of halfEdges has a disabled face. This case caused a segfault before, and it's a good check to identify if disabled faces are handled correctly. Thanks, @elalish for suggesting this. Let me know if I should change anything.