Support for hollow VDBs #1525

jmlait · 2022-11-18T21:23:24Z

From Tomas Skrivan comes this PR for providing hollow vdb support.

The volume-to-mesh gains an optional oracle to determine sign. Rebuild level set uses this to ensure the rebuilt levelset always matches sign with the input.

From Polygons SOP gains an option to use the winding number to determine sign, enabled in H20 where the winding number is exposed.

cases where the sign is known. LevelSetRebuild is changed to use the input grid as the oracle. Option to use GU_WindingNumber for future versions of Houdini. Signed-off-by: jlait <[email protected]>

Signed-off-by: jlait <[email protected]>

if. Signed-off-by: jlait <[email protected]>

jmlait · 2022-11-18T22:33:16Z

I don't like the unused parameter "warning" in the best of days, but in this case the problem is it is unused because we have a if constexpr which is causing it to not be used!

error: parameter 'interiorTest' set but not used [-Werror=unused-but-set-parameter]

Signed-off-by: jlait <[email protected]>

jmlait · 2022-11-18T23:00:31Z

Current failures seem to be explicit instantiation, which makes sense as I think Tomas added a template for the oracle.... Ideas of how to resolve this are appreciated.

danrbailey · 2022-11-19T00:28:40Z

Current failures seem to be explicit instantiation, which makes sense as I think Tomas added a template for the oracle.... Ideas of how to resolve this are appreciated.

The simplest resolution to this is to provide explicit values for the new arguments (ie nullptr, etc) here:

https://github.com/AcademySoftwareFoundation/openvdb/pull/1525/files#diff-cb3368eb16b1ce350d0e14c29b73757572b90b22b8e390f6ec2038d60a087d47R4449

That will fix the failures. However, by adding this new oracle, any time you provide a custom oracle, this pretty heavy method will need to be instantiated. A better solution might be to split this method up to call a few sub-methods where we could explicitly instantiate all but the oracle sub-method to minimize the impact on compile-times. I'm not sure how worth the effort that is, however this tool is one of the largest at ~4500 lines so it might still be worth doing this.

Do you have any unit tests you could add to this PR? Given how extensively used this tool is and how subtle some of the bugs in this area can be, it would be good to have some strong unit tests here.

jmlait · 2022-11-21T19:22:55Z

Yeah, the implications on explicit instantiation worry me. I'd like some feedback on the right way forward.

Note in particular this change has affected RebuildLevelSet. This does change behaviour even without hollow meshes - I think the new behaviour is better, but we still have to analyse the differences carefully.

when increasing resolution. We still include it because we want anyone who thinks of trying this approach in the future to have an example and some documentation of what goes wrong. Add explicit instantiation for oracle-less meshToVolume, which should allow the explicit template builds to work. Signed-off-by: Jeff Lait <[email protected]>

Signed-off-by: Jeff Lait <[email protected]>

jmlait · 2023-07-12T01:36:14Z

I've taking out the change to rebuildLevelSet as that doesn't work for resolution increases. I'd still like the oracle support, however, as it can at least fix the mesh case.

I've made it explicitly instantiate the std::null_ptr path, which I believe means any use of the oracle won't be explicitly instantiated. This is okay, I think, as if a user has their own oracle they can't use an instantiation anyways as we won't have their oracle. And in internal oracles, like if rebuildLevelSet were go through, they'll be instantiated when/if the containing rebuildLevelSet is explicitly instantiated.

kmuseth

Looks good, but I added two very minor comments

kmuseth · 2023-07-18T18:44:16Z

openvdb/openvdb/tools/LevelSetRebuild.h

+        return grid.transform().worldToIndex(xform->indexToWorld(coord));
+    };
+
+    auto interiorTest = [acc = grid.getConstAccessor(), &backToOldGrid, &xform](const Coord& coord) -> bool {


This looks sub-optimal to me; a ValueAccessor is created, which is only used once. Can this be improved or is it not a performance concern? Or is the sampler using it for all the stencil points?

If I understand properly, the
[acc = grid.getConstAccessor, ... ]
will add a new by-value const accessor to the capture of the interiorTest lambda. This is done at the time of creating this lambda, so this is not created on each call to the lambda, but instead on the creation.
Likewise, the interiorTest doesn't have to be thread safe so long as it is copy-constructable and copy-constructed versions of it can run in separate threads. So we should be doing copy-construction of the accessor before going to separate threads.

kmuseth · 2023-07-18T18:50:09Z

openvdb/openvdb/tools/MeshToVolume.h

+    std::vector<std::pair<Index, VoxelState>> offsetStack;
+    offsetStack.reserve(SIZE);
+
+    for (Index offset=0; offset<SIZE; offset++) {


I would have liked to see a short paragraph describing the high-level idea behind this new flood-filling algorithm

Additional comments have been added!

Signed-off-by: Jeff Lait <[email protected]>

apradhana · 2023-07-18T20:19:40Z

openvdb/openvdb/tools/MeshToVolume.h

+
+        // We do not assign anything for voxel close to the mesh
+        // This condition is aligned with the condition in traceVoxelLine
+        if (abs(value) <= 0.75) {


std::abs(value). But I'm also curious, is there a significance of the constant 0.75 here?

0.75 is a magic constant from traceVoxelLine.
// Boundary voxel check. (Voxel that intersects the surface)
if (!(dist > ValueType(0.75))) isOutside = false;

This has caught me out in the past too - we should really constexpr inline this constant in MeshToVolume.h and use a variable for this

apradhana · 2023-07-18T20:20:01Z

openvdb/openvdb/tools/MeshToVolume.h

+///
+/// Inside is set to positive and outside to negative. This is in reverse to the usual
+/// level set convention, but `meshToVolume` uses the opposite convention at certain point.
+///


Thank you for this comment :)

Signed-off-by: Jeff Lait <[email protected]>

Idclip · 2023-07-25T14:04:05Z

openvdb/openvdb/tools/MeshToVolume.h

+
+    std::vector<VoxelState> voxelState(SIZE, NOT_VISITED);
+
+    std::vector<std::pair<Index, VoxelState>> offsetStack;


Id consider using stack arrays for these

That's a good point, std::array could be used. I'm a bit nervous about making that change at this point so put it in the future enhancement category and try to include it in future workflows.

jmlait added 2 commits November 18, 2022 16:20

(skrivantomas): Support for an oracle in mesh to volume to handle

62c1fbf

cases where the sign is known. LevelSetRebuild is changed to use the input grid as the oracle. Option to use GU_WindingNumber for future versions of Houdini. Signed-off-by: jlait <[email protected]>

Include the UT_Version for the version checks.

729fcb0

Signed-off-by: jlait <[email protected]>

jmlait requested a review from kmuseth as a code owner November 18, 2022 21:23

jmlait requested review from danrbailey and Idclip November 18, 2022 21:23

jmlait added 2 commits November 18, 2022 17:04

Remove unused variables and explicitly cast DIM to signed.

cb1b8e9

Signed-off-by: jlait <[email protected]>

Force interiorTest to be considered used in other path of constexpr

dd4b675

if. Signed-off-by: jlait <[email protected]>

Require (void) so the use doesn't itself trigger a warning.

d4bdeb5

Signed-off-by: jlait <[email protected]>

jmlait added 2 commits July 11, 2023 13:52

Fix trailing whitespace.

321d2b3

Signed-off-by: Jeff Lait <[email protected]>

kmuseth approved these changes Jul 18, 2023

View reviewed changes

Add pendingchanges for this commit.

1bd822e

Signed-off-by: Jeff Lait <[email protected]>

jmlait requested review from apradhana and richhones as code owners July 18, 2023 19:53

jmlait added 2 commits July 18, 2023 16:07

Describe the floodfill algorithm.

57ec495

Signed-off-by: Jeff Lait <[email protected]>

Properly credit Tomas in pendingchanges.

95b1a43

Signed-off-by: Jeff Lait <[email protected]>

apradhana reviewed Jul 18, 2023

View reviewed changes

abs -> std::abs

bc96671

Signed-off-by: Jeff Lait <[email protected]>

Idclip reviewed Jul 25, 2023

View reviewed changes

apradhana approved these changes Jul 25, 2023

View reviewed changes

jmlait merged commit da409c2 into AcademySoftwareFoundation:master Jul 25, 2023
41 checks passed

jmlait deleted the send_upstream_hollowmesh branch July 25, 2023 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for hollow VDBs #1525

Support for hollow VDBs #1525

jmlait commented Nov 18, 2022

jmlait commented Nov 18, 2022

jmlait commented Nov 18, 2022

danrbailey commented Nov 19, 2022

jmlait commented Nov 21, 2022

jmlait commented Jul 12, 2023

kmuseth left a comment

kmuseth Jul 18, 2023

jmlait Jul 18, 2023

kmuseth Jul 18, 2023

jmlait Jul 18, 2023

apradhana Jul 18, 2023

jmlait Jul 24, 2023

Idclip Jul 25, 2023

apradhana Jul 18, 2023

Idclip Jul 25, 2023

jmlait Jul 25, 2023


		std::vector<VoxelState> voxelState(SIZE, NOT_VISITED);

		std::vector<std::pair<Index, VoxelState>> offsetStack;

Support for hollow VDBs #1525

Support for hollow VDBs #1525

Conversation

jmlait commented Nov 18, 2022

jmlait commented Nov 18, 2022

jmlait commented Nov 18, 2022

danrbailey commented Nov 19, 2022

jmlait commented Nov 21, 2022

jmlait commented Jul 12, 2023

kmuseth 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment