-
Notifications
You must be signed in to change notification settings - Fork 660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft PR: Half Grid Support for OpenVDB #1730
base: master
Are you sure you want to change the base?
Draft PR: Half Grid Support for OpenVDB #1730
Conversation
…alf. Signed-off-by: apradhana <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a great start to me. Very close to how I was expecting the implementation to look. A couple of things I see as important to resolve before getting this into the library:
- Static asserts in the methods that won't initially support half grid types. Fine to only support a subset, but I think we do want to block other methods being used initially.
- Can half grid types be level sets or just fog volumes?
- I think it makes sense to register the half grid type when
openvdb::initialize()
is called, but there is a question of whether it should be classed as a "RealGridType" or something else?
https://github.com/AcademySoftwareFoundation/openvdb/blob/master/openvdb/openvdb/openvdb.h#L91
An example of what the implication could be of doing that:
openvdb/openvdb/math/Vec3.h
Outdated
@@ -668,6 +669,21 @@ OPENVDB_IS_POD(Vec3ui) | |||
OPENVDB_IS_POD(Vec3s) | |||
OPENVDB_IS_POD(Vec3d) | |||
|
|||
// Half WIP | |||
template<> | |||
inline auto cwiseAdd(const Vec3<math::internal::half>& v, const float s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this use math::half
not math::internal::half
? Otherwise it won't work when compiling against an external half?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Thanks for catching this, Dan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed an updated version of this PR that moves this function to Types.h
because that's where we first define math::half
depending on the macro OPENVDB_USE_IMATH_HALF
.
@@ -68,7 +68,7 @@ include(GNUInstallDirs) | |||
option(OPENVDB_BUILD_CORE "Enable the core OpenVDB library. Both static and shared versions are enabled by default" ON) | |||
option(OPENVDB_BUILD_BINARIES "Enable the vdb binaries. Only vdb_print is enabled by default" ON) | |||
option(OPENVDB_BUILD_PYTHON_MODULE "Build the pyopenvdb Python module" OFF) | |||
option(OPENVDB_BUILD_UNITTESTS "Build the OpenVDB unit tests" OFF) | |||
option(OPENVDB_BUILD_UNITTESTS "Build the OpenVDB unit tests" ON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume some of these changes are just part of the draft PR and won't end up in the final PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly! I changed a few things in this Draft PR to highlight some of the processes I'm going through for the people from Autodesk.
template<typename T> | ||
struct ValueToComputeMap | ||
{ | ||
using Type = T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should probably pick one style and go with that? Either Type
or type
.
…er HalfGrid as RealGridTypes. Signed-off-by: apradhana <[email protected]>
Hi Dan. Thanks for looking at this PR and for all your comments. I'm trying to answer your questions here: Can half-grid types be level sets or just fog volumes? Yes, I think level sets can also be half-grid types. Thanks for pointing out how to do the HalfGrid registration on the Still TODO for me:
|
Great to see this PR in progress! This is functionality that is essential for Arnold renderer, to reduce the peak memory usage of scenes with large volumes. Just to note, we also need the facility to load from full-float grids on disk into half-float grids in memory, without needing to construct an intermediary float grid and convert to half. This needs to work when demand-loading also, i.e. have a full-float grid on disk be loaded/queried incrementally as a half-float grid in memory, on a per leaf-buffer basis as values are accessed. Is that something that is easy to add on top of this PR? |
Signed-off-by: apradhana <[email protected]>
A draft PR on supporting Half Grid in OpenVDB. The tools we are targeting are:
A few timings from VDB Render: