update tolerance defaults #1007
Conversation
|
I believe this now also covers the TODO list from #997. However, in the process of adding the test, I found a serious problem with our simplification: |
| @@ -206,7 +206,7 @@ void Manifold::Impl::SimplifyTopology() { | |||
| { | |||
| ZoneScopedN("CollapseShortEdge"); | |||
| numFlagged = 0; | |||
| ShortEdge se{halfedge_, vertPos_, tolerance_}; | |||
| ShortEdge se{halfedge_, vertPos_, epsilon_}; | |||
There was a problem hiding this comment.
This is my quick fix - seems to be working okay. I need to come back and think about this more deeply sometime, but I think this will be okay for v3.0 - it's pretty close to what we were doing before.
There was a problem hiding this comment.
Yeah, I think we just need to get the APIs correct for v3.0, detailed implementation can be done a bit later.
test/properties_test.cpp
Outdated
|
|
||
| EXPECT_EQ(cylinder.NumTri(), 3996); | ||
| EXPECT_EQ(cylinder2.NumTri(), 3996); | ||
| EXPECT_EQ(cylinder3.NumTri(), 3996); |
There was a problem hiding this comment.
This is kind of a stupid test right now - the high res cylinder bends too much, so CreateFaces gives up and doesn't allow any simplification at all. This was true before too - our SimplifyTopology is not a real decimator; it's considerably simpler. However, we should probably come back someday and turn it into a proper decimator.
Meanwhile, I'll update this test to something more useful before we merge this PR.
| out.tolerance = | ||
| std::max(out.tolerance, | ||
| static_cast<Precision>(std::numeric_limits<float>::epsilon() * | ||
| impl.bBox_.Scale())); |
There was a problem hiding this comment.
Should we perform simplification after this?
There was a problem hiding this comment.
Huh, that's an interesting idea. It feels kind of wrong because you'd have to pay the cost of an extra simplification check on every mesh output, as well as paying the cost of extra faces in all of the boolean operations up to that point. It would be much better to set tolerance to the right level earlier instead. I feel like we should design our API to push people toward that golden path.
| * Returns the tolerance of this Manifold's vertices. | ||
| * Edges shorter than this tolerance value will be collapsed. | ||
| * Returns the tolerance value of this Manifold. Triangles that are coplanar | ||
| * within tolerance tend to be merged and edges shorter than tolerance tend to |
There was a problem hiding this comment.
I'm thinking if this will be true later when we implement better simplification. Not sure how to express this properly, but the final result should be between dilation and erosion by epsilon?
| @@ -206,7 +206,7 @@ void Manifold::Impl::SimplifyTopology() { | |||
| { | |||
| ZoneScopedN("CollapseShortEdge"); | |||
| numFlagged = 0; | |||
| ShortEdge se{halfedge_, vertPos_, tolerance_}; | |||
| ShortEdge se{halfedge_, vertPos_, epsilon_}; | |||
There was a problem h 8000 iding this comment.
Yeah, I think we just need to get the APIs correct for v3.0, detailed implementation can be done a bit later.
I doubt the code would be a good fit for Manifold as-is, but Alexis Naveros has done quite a bit of work on a liberally licensed mesh decimation implementation that might be a useful resource: https://github.com/BRL-CAD/mmesh/blob/main/src/meshdecimation.c (He tends to heavily optimize his codes for high performance, so there's a lot of logic in mmesh pertaining to that sort of optimization in addition to the decimation algorithms themselves.) We're currently using an older version of that code as one of the BRL-CAD options for decimation (still need to update to that newer mmesh version - ah, the never ending TODO list...) and it does pretty well in my experience at decimating without "ruining" the mesh shape. I tried wiring up the OpenMesh decimation as an alternative and had problems with the output from it not being manifold, so that's at least one data point. It might be simpler overall to grab a paper and start fresh, but I figured I might as well raise awareness just in case. |
|
Well, this is irritating - finally got some time to work on this again and without any changes on my side, suddenly my Mac is giving me a pile of Since I couldn't think of anything else to do, I went ahead and upgraded OS 14 to OS 15 and did a big |
|
Did you try a clean build? This sounds really weird... |
|
No, plenty of clean builds have had no effect. This is what it looks like - does anyone see anything weird about the command? It fails the same way on any file (fatal for whatever STL lib is included): |
|
By clean build, are you just trying ninja clean or did you remove the entire build directory? |
|
Removed the whole build directory. Very strange. |
|
is there an option to install libc++ (dev version)? sounds very weird |
|
I don't know - but I can't even build a simple C++ test project now. I think I got screwed by some background update. Super weird. |
|
Maybe try to use a docker environment for now? |
|
Yay, I managed to fix my build! My only guess is that somehow Clang's paths got messed up internally - I had to not only upgrade XCode and the command line tools, but actually build a C++ "hello world" program in XCode - which worked and somehow fixed the broken paths. Now my VSCode builds work too. So strange. |
|
|
||
| EXPECT_EQ(imperfect.NumTri(), 28); | ||
| EXPECT_EQ(imperfect2.NumTri(), 16); // TODO: should be 12 | ||
| EXPECT_EQ(imperfect3.NumTri(), 22); // TODO: should be 12 |
There was a problem hiding this comment.
Our simplification is obviously failing here, but I don't think it's due to the tolerance work in this PR (e.g. no change with a larger value). I think I'll merge this for now and come back and work on our simplification later.
|
I have been thinking about the mesh simplification thing. I think it is probably impossible to guarantee the result of applying several consecutive simplifications to be equivalent to the result of applying simplification once, considering the process is lossy. And this makes it hard to have guarantees on boolean results, e.g. |
|
Agreed that it's not exactly the same, but I think I feel okay as long as it's giving "good" results - along the lines of the volume and surface area are still close, or ideally some metric on maximum difference between the shapes (which we should probably implement someday). If you can find a situation where it's drifting way too much, please open a bug. It'll be helpful when I go through and overhaul the simplification. |
Now input
MeshGLs get a default tolerance of 1e-6 * scale, reflecting the uncertainty of their coordinates, and more closely aligning with the amount of default decimation we did before the update to double precision. The tolerance is likewise reduced for the floating point output ofMeshGL.