avoid recursion in csg tree

[pca006132]

Fixes #989.

This refactors the code into a single loop that does dfs on the csg tree, and perform flattening on-the-fly.

There are still some failures, I guess this is due to shared nodes being evaluated in a weird way... Will try to fix it soon. But for now I think the logic is mostly there and should be ready for review.

For comparison:

// before
n = 20
nTri = 91814, time = 2.28608 sec
n = 30
nTri = 279334, time = 20.6878 sec

// after
n = 20
nTri = 91814, time = 1.18475 sec
n = 30
nTri = 279334, time = 5.67487 sec

[elalish]

Thanks! Looks like a good direction, though a bit out of my wheelhouse. I'll be curious to understand where the bug is...

[pca006132]

CondensedMatter16 hits #970 after this PR. I guess this is due to the code now performs optimization more aggressively, and there are more things that got feed into BatchUnion...

[codecov]

Codecov Report

Attention: Patch coverage is 91.05691% with 11 lines in your changes missing coverage. Please review.

Project coverage is 91.87%. Comparing base (11387fb) to head (a87c439).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
src/csg_tree.cpp	90.98%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1086      +/-   ##
==========================================
+ Coverage   91.72%   91.87%   +0.14%     
==========================================
  Files          30       30              
  Lines        5910     5882      -28     
==========================================
- Hits         5421     5404      -17     
+ Misses        489      478      -11     

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

[pca006132]

This slows down the overall test slightly, from 26180ms to 26877ms, ~2.5%. This is due to CondensedMatter64 being slower, which is due to BatchUnioning a lot of meshes (1021) being slower than unioning them in batches...

If I tune the BatchUnion threshold from 1000 to 100, it is now taking 25641ms, ~2% faster. Not sure if this is general enough though.

[pca006132]

Will add documentation tmr, a bit tired today.

[pca006132]

Anyway, decided to write the documentation now :)

About the original bug: The idea is that the flattening takes O(n) time in the old implementation, which makes it problematic because in the worst case we can have O(n) flattening operations as well, which gives us O(n^2) time. As an example, consider the following:

Union(a, Union(b, Union(c, Union(d)))
=> Union(a, Union(b, Union(c, d)))       // 1 copy
=> Union(a, Union(b, c, d))              // 2 copy
=> Union(a, b, c, d)                     // 3 copy

The implementation here doesn't attempt to do flattening when we construct the node. In fact, it is impossible to avoid this quadratic behavior if we perform (all) flattening when we construct the nodes, because there will be O(n^2) nodes if we do that. The algorithm only does one pass over the tree, and do flattening on-the-fly. Each node is moved only once. This makes it O(n).

[elalish]

I don't quite follow this logic: either we add the node to the set of children, or we add a frame to the stack that contains this node and all the children? Do the children belong to this node or not (when the function is called)?

[pca006132]

You can view adding a stack frame as "we will call this node recursively".
Maybe I should add a recursion pseudocode to make things clearer, this code is quite indirect...

[pca006132]

See if it is better with the recursive pseudocode...

[elalish]

Thanks, that's much clearer!

[pca006132]

maybe wait for me to merge it, I just thought of some minor changes.

[pca006132]

After thinking about it, the fast path for testing cache_ is probably moot, as it requires the exact same node with the exact same transformation, which users will probably never write (why would they do boolean operations on two things that are exactly the same? Not about the mesh result, but the input expressions are the same). Instead, I just test for single children case, which indicates this node is probably shared and is already evaluated, this allows for node sharing with different transforms, which is a legitimate usecase.