Massive improvements to vdb tool - #2241
Conversation
|
Very high level comment, the design of the kernel and forallvalues (and other iterators) actions looks like it provide a similar interface to what you might do with VDB AX, can you not chain the use of vdb_tool through the vdb_ax cli to provide the same functionality? I can see that some things that are missing in current AX functionality are supported (user-def functions for example) but would love to get efforts towards getting these working in AX instead of making a tool that overlaps in functionality. |
Great observation @richhones, and that has definitely crossed my mind. In fact, that is the goal, but there are two reasons why it shouldn't block this PR. First, this PR is carefully crafted to introduce a lot of new features without any new mandatory dependencies. As you know, AX requires LLVM as a major new dependency. Second, while this implementation of stream processing does conceptually overlap with AX, it is a lightweight stand-in for fast prototyping. It has no JIT compilation—so it's presumably much slower—and is by no means a replacement for AX. So, yes, I completely agree AX should be integrated into vdb_tool moving forward, but I'm not convinced it should block this already massive PR. However, I'm happy to prioritize AX integration as the next improvement (though the memory management between scoped variables in AX and the vdb_tool will be nontrivial) |
80de1d6 to
cdf3a40
Compare
|
CI is failing on the Windows build, but strangely it problem appears to be unrelated to changed made in this PR (vdb_view doesn't compile due so some GL issues). Any ideas? |
|
@richhones - can you let me know what you think about my response (to your comment) above? More to the point, do you want me to add AX support to the AX tool in this PR or later? I don't mind either way, but it might make the review harder :) |
|
@richhones - never mind - it turned out to be pretty straight forward to integrate AX into this tool, which I did in the latest commit. As expected it is faster that the existing -forOnValues action, but also requires LLVM - so I made it optional. |
There was a problem hiding this comment.
I now think the shrinkwrap script can fit well as part of a broader Python examples effort.
The most valuable part of the addition is that convertPolygonSoupToLevelSet() is the real library feature. It is a low-level binding: polygon soup arrays go in, level-set grids come out. That belongs in the OpenVDB Python API.
shrink_wrap.py is different. It is a workflow example. It shows how someone might use the binding in a real USD pipeline: read a USD file, collect meshes, bake transforms, triangulate faces, convert to a level set, optionally convert back to mesh, and write output files. That is useful, but it should not look like an installed or officially supported OpenVDB app unless the project intends to test and maintain it that way.
Ideally, in the light of agentic coding, we add a vdb_examples folder sitting at the same level of openvdb_cmd folder. We then add vdb_examples/python/shrink_wrap.py in that folder along with other examples that uses both the Openvdb-python-binding and Nanovdb-python-binding. I'd go one step further and scope it out as collecting examples that uses Open/NanoVDB-python-binding + USD + Numpy workflows. A good source of such examples is the Houdini toolkit example hip file contributed by Jeff Budsberg listed in https://www.openvdb.org/download/. This is useful because we can add this into the CI and users can point their agents to this folder as examples workflow when they use VDB in Python. However, this is a tall order.
As a middle ground, I would prefer moving it from python/app into something like:
openvdb/openvdb/python/examples/shrink_wrap/
with a clear README saying:
This is example code for learning and experimentation. It is not installed as part of the OpenVDB Python package and is not a stable API.
That makes the script valuable without quietly expanding what OpenVDB maintainers are responsible for. The binding stays library-shaped, while the shrinkwrap script becomes a practical example.
There was a problem hiding this comment.
Fwiw, in updating NanoVDB python bindings in those PRs #2219 #2225, I've been pushing out a bunch of example python scripts (and updating the ones that exist) as I go along (into nanovdb/nanovdb/python/examples. Would love to get your review on how helpful those are… the goal was to either mirror the utility in the C++ examples (to show how you'd implement the same things in Python) or demonstrate the usage of the novel Python API surfaces.
I think one PR that's key for docs is getting #2217 merged because right now code like the NanoVDB HelloWorld up on the website/docs won't compile… that code has drifted from the actual APIs. #2217 also includes a test for Doxygen warnings/failures and changes how we pull example code into docs by referencing from live code that gets compiled and tested in CI to hopefully prevent drift and keep docs accurate.
There was a problem hiding this comment.
@apradhana - let's talk. I didn't add this python script - I suspect @swahtz did. More to the point, I'm not sure what you're asking me to change in this PR :)
There was a problem hiding this comment.
Mmmm, nope I don't recall writing that, I don't believe I've worked on this branch.
There was a problem hiding this comment.
interesting ... I wonder who ... maybe Rohan? Let me check
There was a problem hiding this comment.
After talking to Andre, we decided to simply leave out all files in openvdb/python/app for now. They will be moved to another location in another PR. @swahtz, are you okay with that? Rohan added them, with my blessing, but I forgot, so it's my fault that it ended up in this PR by mistake. The problem is it breaks with the lower-level Python bindings we have at this location (it's more of an App), so I agree with Andre that it belongs elsewhere. I'll talk to Rohan, and the two of you (@apradhana and @swahtz) should coordinate on the low-level Python bindings. I've removed these files in this commit.
There was a problem hiding this comment.
Thank you for removing the usd dependency from this PR, @kmuseth ! I will work with @rohan-sawhney and @swahtz to include the example and more python bindings in separate PRs.
There was a problem hiding this comment.
Oh ok, great. Removing them is fine with me, happy to keep this PR more focused, and fitting them into a separate python focused PR of examples/apps is all good.
| OPENVDB_THROW(TypeError, "polySoupToLevelSet: supported only for scalar floating-point grids"); | ||
| } | ||
| if (!mPoly.bbox) mPoly.bbox = PolySoupToLevelSet::getBBox(mPoly.vtx); | ||
| const float maxLength = mPoly.bbox.extents()[mPoly.bbox.maxExtent()]; |
There was a problem hiding this comment.
Please consider sanitizing with OPENVDB_THROW for inputs violating:
- halfWidth is positive
- bbox is non-empty and has positive extent
- computed mMinVoxelSize / mMaxVoxelSize are finite and positive
There was a problem hiding this comment.
This is non-blocking for me.
| OPENVDB_THROW(TypeError, "polySoupToLevelSet: supported only for scalar floating-point grids"); | ||
| } | ||
| if (!mPoly.bbox) mPoly.bbox = PolySoupToLevelSet::getBBox(mPoly.vtx); | ||
| const float maxLength = mPoly.bbox.extents()[mPoly.bbox.maxExtent()]; |
There was a problem hiding this comment.
Same as above, please consider sanitizing bbox is non-empty.
There was a problem hiding this comment.
This is non-blocking for me.
|
Thanks for addressing the reviews. The rest of the comments I made are non-blocking. As long as DCO is passing, we can merge this PR. There are two ways to address this: The attributions for the contributions can be preserved with: |
swahtz
left a comment
There was a problem hiding this comment.
Thanks for addressing my notes, lgtm!
|
I certify this contribution under the DCO and authorize Ken to add my Signed-off-by line. |
1 similar comment
|
I certify this contribution under the DCO and authorize Ken to add my Signed-off-by line. |
|
I certify this contribution under the DCO and authorize @kmuseth to add my Signed-off-by line. |
Co-authored-by: Andrew <areidmeyer@users.noreply.github.com> Co-authored-by: Eftychios Sifakis <esifakis@nvidia.com> Co-authored-by: rohan-sawhney <sawhney_rohan@yahoo.co.in> Signed-off-by: Andrew <areidmeyer@users.noreply.github.com> Signed-off-by: Eftychios Sifakis <esifakis@nvidia.com> Signed-off-by: rohan-sawhney <sawhney_rohan@yahoo.co.in> Signed-off-by: Ken Museth <ken.museth@gmail.com>
dd018b7 to
2bc11df
Compare
fb4516e
into
AcademySoftwareFoundation:master
New Features:
Improvements:
Fixes:
Fixed vdb_tool::uuid so it generates a 36-character RFC 4122 v4 UUID.