Fix properties #587
Fix properties #587
Conversation
src/manifold/src/boolean_result.cpp
Outdated
| @@ -567,7 +567,7 @@ void CreateProperties(Manifold::Impl &outR, const Vec<TriRef> &refPQ, | |||
| } | |||
| if (uvw[j] == 0) edge = j; | |||
| } | |||
| if (edge >= 0) { | |||
| if (edge >= 0) { // This misses some edges | |||
There was a problem hiding this comment.
This is the root cause of the problem: when an edge is collapsed (before this step) you can end up with verts that refer (through refPQ) to a triangle that they are now outside of, since they were merged with their neighboring triangle but the references were not updated. This would be fine, except here I'm assuming I can tell when a vert is on a retained edge by checking its barycentric coordinates - but I miss these ones. So I end up assigning a key with only the meshID and vert, which isn't enough when the other side of the retained edge might have different properties.
src/manifold/src/boolean_result.cpp
Outdated
| @@ -772,6 +772,7 @@ Manifold::Impl Boolean3::Result(OpType op) const { | |||
|
|
|||
| Vec<TriRef> refPQ = UpdateReference(outR, inP_, inQ_, invertQ); | |||
|
|
|||
| // This causes some verts to be outside of their refPQ triangle. | |||
| outR.SimplifyTopology(); | |||
There was a problem hiding this comment.
I tried just reversing the order - create the properties first, and then simplify the topology. However, in this case it's very difficult to keep from duplicating prop verts (CoplanarProp test does a good job catching this). This might not seem like a huge deal, but they tend to continue to multiply, until your final output going to the renderer is basically triangle soup, even if the geometry is manifold.
| @@ -396,7 +396,7 @@ void RelatedGL(const Manifold& out, const std::vector<MeshGL>& originals, | |||
| const float volumeP = | |||
| glm::dot(edgesP[0], glm::cross(edgesP[1], edgesP[2])); | |||
|
|
|||
| ASSERT_LE(volumeP, area * 100 * out.Precision()); | |||
| ASSERT_LE(volumeP, area * out.Precision()); | |||
There was a problem hiding this comment.
Our regular tests would have caught this bug if I hadn't put in this *100, which has the look of me just getting frustrated at some point.
There was a problem hiding this comment.
interesting, I think we should probably go over all tests to see if there are suspicious ones
|
|
||
| for (const int i : {0, 1, 2}) { | ||
| const int vert = outR.halfedge_[3 * tri + i].startVert; | ||
| const glm::vec3 &uvw = bary[3 * tri + i]; | ||
|
|
||
| glm::ivec4 key(PQ, idMissProp, -1, -1); | ||
| if (oldNumProp > 0) { | ||
| key[1] = vert; |
There was a problem hiding this comment.
This was the key fix: for anything close to a retained vert, only key on the propVert, since the separate verts are close together and will get collapsed anyway - this was causing us to duplicate propVerts unnecessarily.
|
|
||
| CreateProperties(outR, refPQ, inP_, inQ_); | ||
| outR.SimplifyTopology(); |
There was a problem hiding this comment.
No need for refPQ anymore since now everything happens in a logical order.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #587 +/- ##
==========================================
+ Coverage 91.36% 91.41% +0.04%
==========================================
Files 35 35
Lines 4494 4494
==========================================
+ Hits 4106 4108 +2
+ Misses 388 386 -2
☔ View full report in Codecov by Sentry. |
|
Thank you so much for all the effort put into fixing this @elalish! This is huge improvement. |
* fix testing * added test * add comments * tests pass * cleanup
Follow-up on #581
Fixes #579
Third time's the charm! Of course I had to write a hideous amount of code and get nowhere to finally realize the simple solution. This even allowed me to simplify our code a bit, since I removed a previous convoluted bit I had put in trying to fix related issues.