openvdb icon indicating copy to clipboard operation
openvdb copied to clipboard

Add a feature flag to the raw CsgDifferenceOp to immediately prune degenerate tiles.

Open jmlait opened this issue 3 years ago • 5 comments

When the same tile subtracted from itself a background tile is created. greatly reduces the generated ghost borders when similar objects are differenced.

Provided for discussion. Is a template parameter correct way for this? I want this in VDB Combine as a toggle parameter, so do we also add parameters to higher level functions? This also can extend to Union/Intersect.

And is this even a good thing?

jmlait avatar May 16 '22 19:05 jmlait

I don't see an ideal solution here as we can't easily modify the constructors due to the tag dispatch class. I think the simplest approach is to just add a runtime modifier such as:

CsgDifferenceOp::setPruneCancelledTiles(true);

then if we want to maintain the performance of the non-pruned leaf method (which we most likely do), then we either add a private member function that is templated to optimize out the value equality checking or for readability what I'd probably do in this case is to add a lambda for the aValue < bValue clause then live with a little code duplication for the rest of the loop. That at least avoids performing redundant template instantiation for the other methods that don't use this parameter.

This is a good idea.... I think, however, the correct solution is just to duplicate the loop. We have a 512:1 amortization rate with the leaf iteration, and it is just a few lines duplicated. I think setting up the template or other approach would generate more lines of code than we end up actually repeating here. I also like that the allequal stack variable can be skipped in the other loop making things clearer.

jmlait avatar May 17 '22 16:05 jmlait

I've added support for union/intersection, though it is somewhat difficult to setup these cases in practice. Two touching cubes tend to convert slightly differently so don't trigger it unless I round off distance values artificially.

Also switched to the member variable to avoid template explosion.

jmlait avatar May 18 '22 19:05 jmlait

This looks fine to me, though there is perhaps a little ambiguity on what whether prune=false will override prunecancelledtiles=true. There's a missing explicit template instantiation parameter that is causing the CI to fail. Also, minor quibble, I think we tend to prefer lower camel case for parameters (ie pruneCancelledTiles instead of prunecancelledtiles).

danrbailey avatar May 18 '22 21:05 danrbailey

Good point on ambiguity. I think it makes sense to only prune cancelled if pruning is enabled. So I've documented it to that effect & also made it act that way.

Fixed the case problems, thanks for calling that out.

Likewise, explicit instantiation should be fixed. All praise CI!

jmlait avatar May 19 '22 15:05 jmlait

I've added the SOP layer to match 19.5, if this is okay I'll merge as we are now past 9.1.

jmlait avatar Jun 10 '22 19:06 jmlait