Add tolerance value #997

pca006132 · 2024-10-18T18:05:27Z

Implements #994.

Hide precision from public API and change its name.
Add tolerance value API and logic.
Read the field from MeshGL (somehow forgot that)
Fix documentation.
Test for using tolerance.
Test for mesh simplification with increased tolerance.

elalish

Good start; I think we need to be pretty careful about where we use tolerance vs uncertainty. I think uncertainty is the epsilon in epsilon-valid, while tolerance is our simplication parameter.

src/manifold.cpp

src/properties.cpp

elalish · 2024-10-18T19:23:43Z

test/properties_test.cpp

@@ -37,27 +37,26 @@ TEST(Properties, GetProperties) {

 TEST(Properties, Precision) {


Update test names

src/boolean_result.cpp

src/face_op.cpp

pca006132

Thinking about this, I guess I should just go for the stricter one and use epsilon whenever I am not sure.

src/edge_op.cpp

src/impl.cpp

elalish · 2024-10-21T04:36:01Z

src/impl.cpp


    const double volume = std::abs(la::dot(normal, pairVec));
    // Only operate on coplanar triangles
-    if (volume > std::max(area, areaPair) * precision) return;
+    if (volume > std::max(area, areaPair) * epsilon) return;


I think tolerance is probably natural for coplanarity.

elalish · 2024-10-21T04:36:31Z

src/impl.cpp

@@ -171,7 +170,7 @@ struct CheckCoplanarity {
      // If any component vertex is not coplanar with the component's reference
      // triangle, unmark the entire component so that none of its triangles are
      // marked coplanar.
-      if (std::abs(la::dot(normal, vert - origin)) > precision) {
+      if (std::abs(la::dot(normal, vert - origin)) > epsilon) {


tolerance again.

elalish · 2024-10-21T04:40:08Z

src/manifold.cpp

+  auto impl = std::make_shared<Impl>(*GetCsgLeafNode().GetImpl());
+  auto oldTolerance = impl->tolerance_;
+  impl->SetEpsilon(epsilon);
+  if (impl->tolerance_ > oldTolerance) impl->SimplifyTopology();


If we moved this logic into the Impl functions could we reduce duplication by having SetEpsilon call SetTolerance?

elalish · 2024-10-21T04:43:13Z

test/test_main.cpp

+
+  float epsilon = std::max(static_cast<float>(out.GetEpsilon()),
+                           std::numeric_limits<float>::epsilon() *
+                               static_cast<float>(out.BoundingBox().Scale()));


Why is this necessary?

This is getting MeshGL, which uses single precision. The epsilon value we track is for double precision, which is too small here.

Hmm, that sounds like a bug we should fix at the MeshGL level. Also, I thought I already had?

Tolerance value is set before the mesh is created, when SimplfiyTopology cannot be called.

pca006132 · 2024-10-21T08:11:00Z

For the test naming, I think I will leave it here as we should fix a bunch of other documentations and naming as well. Should we change RefineToPrecision to RefineToTolerance, for example?

pca006132 · 2024-10-21T09:10:31Z

@starseeker can you check if this helps?
You can just set the tolerance field in the MeshGL struct, or use the SetTolerance method after creating the primitives.

starseeker · 2024-10-21T14:01:19Z

@pca006132 Trying various settings in this context doesn't seem to change the timing: BRL-CAD/brlcad@9a82ed3

starseeker · 2024-10-21T14:02:24Z

(I know I need to clean up the code to not do the intermediate checks for performance, but for the moment I'm leaving it as it was for testing.)

elalish · 2024-10-21T15:01:43Z

@starseeker How certain are you that it was faster before? Manifold can be tricky to time properly - based on the operations you're doing, I'd be a bit surprised if it was really finishing orders of magnitude faster than now. Can you check out a particular commit, add your test and show the timing difference, for instance?

starseeker · 2024-10-21T15:24:58Z

@starseeker How certain are you that it was faster before? Manifold can be tricky to time properly - based on the operations you're doing, I'd be a bit surprised if it was really finishing orders of magnitude faster than now. Can you check out a particular commit, add your test and show the timing difference, for instance?

It's possible I'm not doing something right, but If I go back to the stand-alone test that got boiled down into the boolean_complex case, I did have that working in both v2.4.5 and current:

manifold/build (perf_test_v245) $ time ./extras/offsetTest ~/Generic_Twin_91.1.t0.glb
...
Face 264: 915 vertices, 1830 triangles
Face 265: 907 vertices, 1810 triangles
Processing 266 triangles... done.

real	0m3.488s
user	0m3.436s
sys	0m0.052s

Putting the same offsetTest into the tolerance branch, pulling in the RotateUp fix and setting the tolerance in the input mesh:

MeshGL input;
input.tolerance = std::numeric_limits<float>::min();
input = ImportMesh(filename);

then running the same input file:

manifold/build (tolerance) $ time ./extras/offsetTest ~/Generic_Twin_91.1.t0.glb
...
Face 264: 13244 vertices, 26488 triangles
Face 265: 13251 vertices, 26498 triangles
Processing 266 triangles... done.

real	1m54.895s
user	1m54.496s
sys	0m0.397s

starseeker · 2024-10-21T15:33:11Z

The v2.4.5 version: master...starseeker:manifold:perf_test_v245
The tolerance version: starseeker@60ea167

starseeker · 2024-10-21T15:46:12Z

Ah, sorry, that sha1 for the tolerance is one behind - this is the file that compiles: https://github.com/starseeker/manifold/blob/tolerance/extras/offset_test.cpp

starseeker · 2024-10-21T16:15:59Z

A quick backport of the new boolean_complex test also shows similar timing differences:

master...starseeker:manifold:test_backport

manifold/build (test_backport) $ time ./test/manifold_test --gtest_filter="*SimpleOffset*"
Note: Google Test filter = *SimpleOffset*
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from BooleanComplex
[ RUN      ] BooleanComplex.SimpleOffset
[       OK ] BooleanComplex.SimpleOffset (1417 ms)
[----------] 1 test from BooleanComplex (1417 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1417 ms total)
[  PASSED  ] 1 test.

real	0m1.428s
user	0m1.410s
sys	0m0.017s

tolerance branch in starseeker/manifold:

manifold/build (tolerance) $ time ./test/manifold_test --gtest_filter="*SimpleOffset*"
Note: Google Test filter = *SimpleOffset*
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from BooleanComplex
[ RUN      ] BooleanComplex.SimpleOffset
[       OK ] BooleanComplex.SimpleOffset (51897 ms)
[----------] 1 test from BooleanComplex (51897 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (51897 ms total)
[  PASSED  ] 1 test.

real	0m51.967s
user	0m51.938s
sys	0m0.028s

starseeker · 2024-10-21T16:23:06Z

tolerance branch in starseeker/manifold with tolerance settings per 74f2fe4:

manifold/build (tolerance) $ time ./test/manifold_test --gtest_filter="*SimpleOffset*"
Note: Google Test filter = *SimpleOffset*
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from BooleanComplex
[ RUN      ] BooleanComplex.SimpleOffset
[       OK ] BooleanComplex.SimpleOffset (51735 ms)
[----------] 1 test from BooleanComplex (51735 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (51735 ms total)
[  PASSED  ] 1 test.

real	0m51.810s
user	0m51.779s
sys	0m0.031s

starseeker · 2024-10-21T18:41:21Z

1810 tris in the old vs 4708 in the new, looks like.

starseeker · 2024-10-21T19:03:06Z

Correction - per the test_backport branch, I'm seeing 3004 triangles exported in the final output mesh.

elalish · 2024-10-21T19:11:41Z

Okay, thanks - that definitely doesn't seem like enough to be responsible for most the slowdown. Anyway, this is all a long tangent from the question @pca006132 was asking regarding this PR. I believe this testing shows that indeed, this PR's change is good and working as intended, even though the problem it's fixing is not the whole regression identified by the SimpleOffset test.

elalish

This looks great, thanks!

pca006132 · 2024-10-22T01:08:49Z

In combination with #989, increasing the number of faces may cause a significant slowdown. But should do a detailed profiling to see the reason.

Will finish the remaining of this PR later when I have time, hopefully this week.

pca006132 · 2024-10-22T05:56:23Z

@starseeker and you can try to use BatchBoolean instead of doing += repeatedly. It should help a bit, even though the older version should also have this performance issue regarding +=...

starseeker · 2024-10-22T13:32:42Z

I don't know if this is helpful, but here are Linux perf recordings of the test_backport and current master running the simple offset test case: perf record ./test/manifold_test --gtest_filter="*SimpleOffset*" (only saved 0.40% and up)

https://brlcad.org/~starseeker/perf_test_backport.txt
https://brlcad.org/~starseeker/perf_master.txt

starseeker · 2024-10-22T13:39:04Z

Interestingly, SimplifyTopology in master wound up below the 0.40% cutoff:

0.12% manifold_test libmanifold.so.2.5.1 [.] manifold::Manifold::Impl::SimplifyTopology()

starseeker · 2024-10-22T13:50:09Z

Actually, the call graph version of master might be more helpful:

Samples: 185K of event 'cycles:P', Event count (approx.): 207025003327
  Children      Self  Command        Shared Object         Symbol
-   99.07%     0.00%  manifold_test  libmanifold.so.2.5.1  [.] manifold::CsgOpNode::BatchBoolean(manifold::OpType, st
   - manifold::CsgOpNode::BatchBoolean(manifold::OpType, std::vector<std::shared_ptr<manifold::Manifold::Impl const>,
      - 86.85% manifold::Boolean3::Result(manifold::OpType) const
         - 40.08% manifold::Manifold::Impl::SimplifyTopology()
            - 26.49% void manifold::for_each_n<manifold::CountingIterator<unsigned long>, manifold::Manifold::Impl::S
               - void manifold::for_each<manifold::CountingIterator<unsigned long>, manifold::Manifold::Impl::Simplif
                  - 26.43% manifold::Manifold::Impl::SimplifyTopology()::{lambda(unsigned long)#3} std::for_each<mani
                     - 26.31% manifold::Manifold::Impl::SimplifyTopology()::{lambda(unsigned long)#3}::operator()(uns
                        - 25.95% (anonymous namespace)::SwappableEdge::operator()(int) const
                           + 15.56% auto linalg::operator*<linalg::mat<double, 2, 3>, linalg::vec<double, 3> >(linalg
                           + 5.06% manifold::CCW(linalg::vec<double, 2>, linalg::vec<double, 2>, linalg::vec<double,
                           + 3.20% manifold::GetAxisAlignedProjection(linalg::vec<double, 3>)
                             0.63% (anonymous namespace)::TriOf(int)
            + 7.24% manifold::Manifold::Impl::CleanupTopology()
            + 2.74% void manifold::for_each_n<manifold::CountingIterator<unsigned long>, manifold::Manifold::Impl::Si
            + 2.42% void manifold::for_each_n<manifold::CountingIterator<unsigned long>, manifold::Manifold::Impl::Si
              0.70% manifold::Manifold::Impl::CollapseEdge(int, std::vector<int, std::allocator<int> >&)
         - 25.70% manifold::Manifold::Impl::Finish()
            - 10.38% manifold::Manifold::Impl::CalculateNormals()
               - 9.96% void manifold::for_each_n<manifold::CountingIterator<unsigned long>, (anonymous namespace)::As
                  - 9.96% void manifold::for_each<manifold::CountingIterator<unsigned long>, (anonymous namespace)::A
                     - 9.94% (anonymous namespace)::AssignNormals<false> std::for_each<manifold::CountingIterator<uns
                        - 9.41% (anonymous namespace)::AssignNormals<false>::operator()(int)
                           + 2.85% (anonymous namespace)::AtomicAddVec3(linalg::vec<double, 3>&, linalg::vec<double,
                           + 2.04% linalg::vec<double, 3> linalg::normalize<double, 3>(linalg::vec<double, 3> const&)
                           + 0.83% double linalg::dot<double, 3>(linalg::vec<double, 3> const&, linalg::vec<double, 3
                           + 0.77% auto linalg::operator*<double, linalg::vec<double, 3> >(double const&, linalg::vec
                           + 0.71% linalg::detail::apply<linalg::detail::op_sub, void, linalg::vec<double, 3>, linalg
            + 5.87% manifold::Manifold::Impl::GetFaceBoxMorton(manifold::Vec<manifold::Box>&, manifold::Vec<unsigned
            + 4.36% manifold::Collider::Collider(manifold::VecView<manifold::Box const> const&, manifold::VecView<uns
            + 2.66% manifold::Manifold::Impl::SortFaces(manifold::Vec<manifold::Box>&, manifold::Vec<unsigned int>&)
            + 2.02% manifold::Manifold::Impl::SortVerts()
         - 17.16% manifold::Manifold::Impl::Face2Tri(manifold::Vec<int> const&, manifold::Vec<manifold::TriRef> const
            - 10.23% manifold::Manifold::Impl::CreateHalfedges(manifold::Vec<linalg::vec<int, 3> > const&)
               - 7.42% void manifold::stable_sort<int*, int, manifold::Manifold::Impl::CreateHalfedges(manifold::Vec<
                    void manifold::stable_sort<int*, int, manifold::Manifold::Impl::CreateHalfedges(manifold::Vec<lin
                    void std::stable_sort<int*, manifold::Manifold::Impl::CreateHalfedges(manifold::Vec<linalg::vec<i
                    void std::__stable_sort<int*, __gnu_cxx::__ops::_Iter_comp_iter<manifold::Manifold::Impl::CreateH
                  - void std::__stable_sort_adaptive<int*, int*, __gnu_cxx::__ops::_Iter_comp_iter<manifold::Manifold
                     - 7.01% void std::__merge_sort_with_buffer<int*, int*, __gnu_cxx::__ops::_Iter_comp_iter<manifol
                        - 5.49% void std::__merge_sort_loop<int*, int*, long, __gnu_cxx::__ops::_Iter_comp_iter<manif
                           + 5.11% int* std::__move_merge<int*, int*, __gnu_cxx::__ops::_Iter_comp_iter<manifold::Man
                        + 1.51% void std::__chunk_insertion_sort<int*, long, __gnu_cxx::__ops::_Iter_comp_iter<manifo
               + 1.18% void manifold::for_each_n<manifold::CountingIterator<unsigned long>, manifold::Manifold::Impl:
                 0.59% manifold::Manifold::Impl::RemoveUnreferencedVerts()
            + 6.79% void std::_Bind<manifold::Manifold::Impl::Face2Tri(manifold::Vec<int> const&, manifold::Vec<manif
         + 1.21% (anonymous namespace)::AppendWholeEdges(manifold::Manifold::Impl&, manifold::Vec<int>&, manifold::Ve
           1.17% (anonymous namespace)::SizeOutput(manifold::Manifold::Impl&, manifold::Manifold::Impl const&, manifo
      - 12.21% manifold::Boolean3::Boolean3(manifold::Manifold::Impl const&, manifold::Manifold::Impl const&, manifol
         + 3.77% manifold::Manifold::Impl::EdgeCollisions(manifold::Manifold::Impl const&, bool) const
         + 3.26% (anonymous namespace)::Shadow11(manifold::SparseIndices&, manifold::Manifold::Impl const&, manifold:
         + 2.16% (anonymous namespace)::Intersect12(manifold::Manifold::Impl const&, manifold::Manifold::Impl const&,
         + 1.67% (anonymous namespace)::Filter11(manifold::Manifold::Impl const&, manifold::Manifold::Impl const&, ma
         + 0.66% (anonymous namespace)::Shadow02(manifold::Manifold::Impl const&, manifold::Manifold::Impl const&, ma

starseeker · 2024-10-22T14:05:06Z

Urk. Wait a minute. If I explicitly specify CMAKE_BUILD_TYPE=Release for configure, I'm getting an execution time in master of 5143 ms. Maybe it's as simple as the v2.4.5 build getting some build flags by default without having to specify the CMake build type.

pca006132 · 2024-10-22T14:16:20Z

Hmm, interesting. But we do have default build type as configured in

manifold/CMakeLists.txt

Lines 115 to 125 in e647093

    
           if(TRACY_ENABLE) 
        
             option(CMAKE_BUILD_TYPE "Build type" RelWithDebInfo) 
        
             if(TRACY_MEMORY_USAGE) 
        
               list(APPEND MANIFOLD_FLAGS -DTRACY_MEMORY_USAGE) 
        
             endif() 
        
             if(NOT MSVC) 
        
               set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-omit-frame-pointer") 
        
             endif() 
        
           else() 
        
             option(CMAKE_BUILD_TYPE "Build type" Release) 
        
           endif()

Also, I guess with this PR, the problem is essentially solved now? If you use the correct build type.

starseeker · 2024-10-22T14:17:22Z

Bingo. Old build has "-O3" in the compile lines even when I don't specify a build type, and the master build does not. Looks like I somehow managed to drop those in the CMake refactor. Good grief. Pull request imminent.

pca006132 · 2024-10-22T14:18:50Z

To be fair, I think it is fine to drop the -O3 flags, or at least only do it when we do release/relwithdebinfo build? It feels weird to do that with debug build.

starseeker · 2024-10-22T14:19:50Z

Well... Hmm. That's a point - the perf reports do actually illustrate why you might not always want the O3 flag on. @elalish any thoughts on that one?

pca006132 · 2024-10-22T14:21:22Z

And I think it is normal for SimplifyTopology to run slower now, see #675.

pca006132 · 2024-10-22T14:33:19Z

Ah, and another issue is that because you are calling c.Status() for every iteration, this will prohibit some of our CSG reordering, so things will be slower (but you also avoids #989).

starseeker · 2024-10-22T14:37:56Z

@pca006132 Eventually I'd like to fully stomp on the gas - enable tbb, remove all the Status() and exports mid-execution, etc. - but that triangulation failure that only seems to manifest with tbb enabled in particular has me a little gun-shy. Slower and reliable wins over faster and occasionally failing (well, within reason) in my particular use case.

pca006132 · 2024-10-22T14:43:37Z

I see. I would suggest you to try the fast way and fallback to the slower way in case something failed, because the timing difference is huge. I managed to cut the time down from 5450ms to 220ms just by changing these.

We will add some debug info in csg tree to report back the offending boolean pair when something bad happens, so it should be easier to debug later.

pca006132 · 2024-10-22T14:45:03Z

Anyway, probably too much discussion about things tangential to this PR. We should continue the discussion in #993 if needed. Wish github can just allow me to select and move some of the discussions to another issue...

elalish · 2024-10-22T15:11:09Z

Good teamwork on the debugging - I'm glad this is solved! @pca006132 should we just go ahead and merge this and do the docs & tests in a follow-on PR?

pca006132 · 2024-10-22T15:18:46Z

sounds good to me

pca006132 · 2024-10-22T18:07:37Z

For the test naming, I think I will leave it here as we should fix a bunch of other documentations and naming as well. Should we change RefineToPrecision to RefineToTolerance, for example?

@elalish so what do you think about the naming?

elalish · 2024-10-22T19:48:26Z

Yeah, I agree. I'm working on PRs now to update these names and to remove the need for that epsilon setting in RelatedGL.

starseeker · 2024-10-22T20:50:29Z

Just to check - three of the original checkboxes in the initial PR ended up unchecked - are they still needed?

pca006132 · 2024-10-22T21:02:10Z

Yes, we will do it later

pca006132 added 2 commits October 19, 2024 02:05

renaming precision to uncertainty, and hiding

766ea58

add tolerance

4393132

pca006132 requested a review from elalish October 18, 2024 18:05

elalish requested changes Oct 18, 2024

View reviewed changes

pca006132 added 8 commits October 20, 2024 13:36

rename uncertainty to epsilon

823847f

use epsilon instead of tolerance

cf1c296

guard simplify topology for SetEpsilon

faec2f9

Merge remote-tracking branch 'upstream/master' into tolerance

638a989

format

7a46316

remove conflict markers

cdee4e9

fix builds

e3ceb1a

use epsilon for barycentric

72a7433

pca006132 commented Oct 20, 2024

View reviewed changes

src/edge_op.cpp Outdated Show resolved Hide resolved

src/impl.cpp Outdated Show resolved Hide resolved

pca006132 added 2 commits October 20, 2024 14:00

prefer using epsilon

8ccbeb7

update python bindings

6cdf345

elalish reviewed Oct 21, 2024

View reviewed changes

pca006132 added 3 commits October 21, 2024 14:56

address comments

5efe098

format

ce0938f

remove SetTolerance

c20fad0

Tolerance value is set before the mesh is created, when SimplfiyTopology cannot be called.

read/write MeshGL tolerance field

72ea845

elalish approved these changes Oct 21, 2024

View reviewed changes

elalish merged commit 5ba3c24 into master Oct 22, 2024
19 checks passed

elalish deleted the tolerance branch October 22, 2024 15:29

elalish mentioned this pull request Oct 22, 2024

Performance regression with many multiple sequential union operations #993

Closed

elalish changed the title ~~[WIP] Add tolerance value~~ Add tolerance value Oct 22, 2024

elalish mentioned this pull request Oct 22, 2024

update tolerance defaults #1007

Open

		@@ -37,27 +37,26 @@ TEST(Properties, GetProperties) {

		TEST(Properties, Precision) {

Add tolerance value #997

Add tolerance value #997

Conversation

pca006132 commented Oct 18, 2024 • edited Loading

elalish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pca006132 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pca006132 commented Oct 21, 2024

pca006132 commented Oct 21, 2024 • edited Loading

starseeker commented Oct 21, 2024

starseeker commented Oct 21, 2024

elalish commented Oct 21, 2024

starseeker commented Oct 21, 2024 • edited Loading

starseeker commented Oct 21, 2024

starseeker commented Oct 21, 2024

starseeker commented Oct 21, 2024

starseeker commented Oct 21, 2024

starseeker commented Oct 21, 2024

starseeker commented Oct 21, 2024

elalish commented Oct 21, 2024

elalish left a comment

Choose a reason for hiding this comment

pca006132 commented Oct 22, 2024

pca006132 commented Oct 22, 2024

starseeker commented Oct 22, 2024 • edited Loading

starseeker commented Oct 22, 2024

starseeker commented Oct 22, 2024

starseeker commented Oct 22, 2024

pca006132 commented Oct 22, 2024

starseeker commented Oct 22, 2024

pca006132 commented Oct 22, 2024

starseeker commented Oct 22, 2024

pca006132 commented Oct 22, 2024

pca006132 commented Oct 22, 2024

starseeker commented Oct 22, 2024

pca006132 commented Oct 22, 2024

pca006132 commented Oct 22, 2024

elalish commented Oct 22, 2024

pca006132 commented Oct 22, 2024

pca006132 commented Oct 22, 2024

elalish commented Oct 22, 2024

starseeker commented Oct 22, 2024

pca006132 commented Oct 22, 2024

pca006132 commented Oct 18, 2024 •

edited

Loading

pca006132 commented Oct 21, 2024 •

edited

Loading

starseeker commented Oct 21, 2024 •

edited

Loading

starseeker commented Oct 22, 2024 •

edited

Loading