Define a test for simple offset booleans #998

starseeker · 2024-10-18T18:36:13Z

This is intended to check for performance regressions - the BRL-CAD logic performing this operation using v2.4.5 ran considerably faster.

Note - removing in-loop assertions for triangle count > 0 with the intermediate forms greatly increases the speed, but they are placed there deliberately to simulate what BRL-CAD does to validate the output of the booleans on an incremental basis. In the event of a boolean failure or invalid output being produced for any reason, we want to be able to capture the exact inputs responsible - which means we need to check after each boolean operation to validate the results and catch any bad intermediate states at the time they are produced.

This is intended to check for performance regressions - the BRL-CAD logic performing this operation using v2.4.5 ran considerably faster. Note - removing in-loop assertions for triangle count > 0 with the intermediate forms greatly increases the speed, but they are placed there deliberately to simulate what BRL-CAD does to validate the output of the booleans on an incremental basis. In the event of a boolean failure or invalid output being produced for any reason, we want to be able to capture the *exact* inputs responsible - which means we need to check after each boolean operation to validate the results and catch any bad intermediate states at the time they are produced.

starseeker · 2024-10-18T18:37:18Z

@elalish I haven't back-ported to v2.4.5 yet to confirm it runs faster there in this form, but that should be the general idea.

codecov · 2024-10-18T18:47:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.81%. Comparing base (d437097) to head (e08cb1c).
Report is 125 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #998      +/-   ##
==========================================
- Coverage   91.84%   85.81%   -6.03%     
==========================================
  Files          37       62      +25     
  Lines        4976     9616    +4640     
  Branches        0     1049    +1049     
==========================================
+ Hits         4570     8252    +3682     
- Misses        406     1364     +958

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

elalish

Thanks, looks like it takes about 10 seconds to run on the CI currently. We should definitely be able to get that back down.

test/boolean_complex_test.cpp

starseeker · 2024-10-18T20:43:21Z

Uh... that failure is unexpected. A triangulation failure?? Did I break the logic somehow?

elalish · 2024-10-18T20:58:59Z

Uh... that failure is unexpected. A triangulation failure?? Did I break the logic somehow?

Actually, this is why my Offset PR never got merged. I'm guessing I hit a case where the symbolic perturbation did something unexpected, created some extremely degenerate polygons that compounded enough until the triangulor gave up. However, I never found the time to properly debug it. Interesting that you only saw this error once you adopted my technique - perhaps my technique is flawed. It also means you might just have an algorithm to contribute for an Offset method, which would be awesome!

Always fun when one bug leads to another - they're all important, so please follow up on any/all that interest you. But let's try to keep them in separate PRs.

elalish · 2024-10-18T21:03:15Z

Hmm, actually I take it back, this didn't start when you switched to Cylinder, it started when you allowed our CSG tree to reorder. That's very interesting. @pca006132 could this be a bug in the CSG optimization somehow? It would be amazing if we could cut this down to find exactly the Boolean op that's failing, but for that I think we'll have to put some debug code into the CSG tree itself.

test/boolean_complex_test.cpp

starseeker · 2024-10-18T22:45:50Z

Um. Again not what I was expecting...

Getting CI failure, see if this had an impact somehow... This reverts commit 55ca9a9.

Still not passing?? Revert another commit. This reverts commit 89065dd.

OK, starting from a passing state, see if this works. This reverts commit ba6027e.

This reverts commit c137a48.

This reverts commit 35f0eb0.

starseeker · 2024-10-19T03:33:39Z

New data point - the triangulation failure only triggers with tbb enabled. I was wondering why I wasn't seeing it locally - it's because I normally build without tbb. When I do enable it here, I get the failure.

elalish · 2024-10-19T03:53:50Z

Thanks for all the debug - we're getting closer. Since this test is passing, I suppose we might as well merge it. That should make it easier to test various ideas for fixes, not to mention we can modify it to repro the other bug you found.

elalish

Thanks for the perf test!

starseeker · 2024-10-19T03:58:36Z

I don't know if this matters or helps, but I was experimenting with doing a number of iterations on the triangle potion of the unioning before calling Status(). At least on my machine here, if I run 15 iterations before calling Status(), I get intermittent success and failure - things aren't reproducible/deterministic from run to run. If I do more than 15 iterations it seems to reliably fail, and less than 15 I'm not seeing a failure.

starseeker · 2024-10-19T04:21:52Z

Sorry, I need to correct my terms - it's not iterations, it's number of faces processed. So unioning in 14 triangle-generated solids and then calling Status() seems to work, trying to do 16 fails, and 15 sometimes works and sometimes doesn't. I'm not iterating over all the triangles multiple times trying to union them in again and again (that would just be evil.)

elalish · 2024-10-19T05:12:52Z

Can you give me any background on what this model is supposed to be? I'm curious why it's 3,000 meters tall and offset by 1 meter? And is this cutout in the top intentional?

starseeker · 2024-10-19T11:45:29Z

It's a piece of the FAA's GenericTwin plane model. They model thin volumes with just a surface mesh (hence why we need the offset for boolean ops) and that just happened to be the piece I grabbed for testing - there's lots more like it. The ones that look more like "airplane" are considerably larger, and this one was one of the smaller ones - just picked at random.

As far as I can tell, the cutout is intentional.

pca006132 · 2024-10-20T06:37:14Z

Hmm, actually I take it back, this didn't start when you switched to Cylinder, it started when you allowed our CSG tree to reorder. That's very interesting. @pca006132 could this be a bug in the CSG optimization somehow? It would be amazing if we could cut this down to find exactly the Boolean op that's failing, but for that I think we'll have to put some debug code into the CSG tree itself.

Not sure, csg reordering can give different results, and if things were close to triggering an issue, csg reordering can push it to actually trigger the issue I guess.

But yeah, some mechanism to dump the offending boolean operation will be nice to catch this kind of error. I actually did that for #970, forgot why I did not make a PR for it.

starseeker added 2 commits October 18, 2024 14:49

formatting

333c22c

More formatting

a021134

elalish requested changes Oct 18, 2024

View reviewed changes

test/boolean_complex_test.cpp Outdated Show resolved Hide resolved

test/boolean_complex_test.cpp Show resolved Hide resolved

test/boolean_complex_test.cpp Outdated Show resolved Hide resolved

test/boolean_complex_test.cpp Outdated Show resolved Hide resolved

starseeker added 2 commits October 18, 2024 15:44

Per suggestion from elalish, simplify edge pair setup

51d9179

Switch to a Status() check - appears to have the same slowing effect.

03186b5

starseeker mentioned this pull request Oct 18, 2024

Performance regression with many multiple sequential union operations #993

Closed

Move the checks out of the loops

1f47f80

Use initializer lists for better compactness

89065dd

elalish reviewed Oct 18, 2024

View reviewed changes

test/boolean_complex_test.cpp Outdated Show resolved Hide resolved

starseeker added 2 commits October 18, 2024 18:36

Restore in-loop checks with explanation

fe9e220

Simplify with += syntax.

55ca9a9

starseeker added 11 commits October 18, 2024 18:47

Revert "Simplify with += syntax."

ba6027e

Getting CI failure, see if this had an impact somehow... This reverts commit 55ca9a9.

Revert "Use initializer lists for better compactness"

f215cbd

Still not passing?? Revert another commit. This reverts commit 89065dd.

Reapply "Simplify with += syntax."

9a3dba3

OK, starting from a passing state, see if this works. This reverts commit ba6027e.

Break the initialization changes down - start with faces array.

f928ef8

Initialize pts array

3feecbb

Ah ha. There it is. Indexing problem with second half of pnts setup.

7ffb940

formatting

c44e456

Test

c137a48

Revert "Test"

2d9fff3

This reverts commit c137a48.

See if the failure triggers if we only check once at the end.

35f0eb0

Revert "See if the failure triggers if we only check once at the end."

e08cb1c

This reverts commit 35f0eb0.

elalish approved these changes Oct 19, 2024

View reviewed changes

elalish merged commit 0a51ae2 into elalish:master Oct 19, 2024
21 checks passed

starseeker deleted the perf_check branch October 19, 2024 04:22

starseeker mentioned this pull request Oct 22, 2024

Triangulation failure with TBB enabled in SimpleOffset test #1006

Open

elalish mentioned this pull request Nov 5, 2024

v3.0 #1024

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define a test for simple offset booleans #998

Define a test for simple offset booleans #998

starseeker commented Oct 18, 2024

starseeker commented Oct 18, 2024

codecov bot commented Oct 18, 2024 •

edited

Loading

elalish left a comment

starseeker commented Oct 18, 2024

elalish commented Oct 18, 2024

elalish commented Oct 18, 2024

starseeker commented Oct 18, 2024 •

edited

Loading

starseeker commented Oct 19, 2024

elalish commented Oct 19, 2024

elalish left a comment

starseeker commented Oct 19, 2024

starseeker commented Oct 19, 2024

elalish commented Oct 19, 2024

starseeker commented Oct 19, 2024 •

edited

Loading

pca006132 commented Oct 20, 2024

Define a test for simple offset booleans #998

Define a test for simple offset booleans #998

Conversation

starseeker commented Oct 18, 2024

starseeker commented Oct 18, 2024

codecov bot commented Oct 18, 2024 • edited Loading

Codecov Report

elalish left a comment

Choose a reason for hiding this comment

starseeker commented Oct 18, 2024

elalish commented Oct 18, 2024

elalish commented Oct 18, 2024

starseeker commented Oct 18, 2024 • edited Loading

starseeker commented Oct 19, 2024

elalish commented Oct 19, 2024

elalish left a comment

Choose a reason for hiding this comment

starseeker commented Oct 19, 2024

starseeker commented Oct 19, 2024

elalish commented Oct 19, 2024

starseeker commented Oct 19, 2024 • edited Loading

pca006132 commented Oct 20, 2024

codecov bot commented Oct 18, 2024 •

edited

Loading

starseeker commented Oct 18, 2024 •

edited

Loading

starseeker commented Oct 19, 2024 •

edited

Loading