Skip to content
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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

apradhana
Copy link
Contributor

@apradhana apradhana commented Dec 14, 2023

A draft PR on supporting Half Grid in OpenVDB. The tools we are targeting are:

  • morphology
  • interpolation
  • ray tracing

A few timings from VDB Render:

Case Active Voxel Count Timing Float (s) Timing Half (s)
Bunny Fog Small 19,210,271 2.7 1.15
Bunny Fog Big 176,493,009 4.83 2.47
Bunny SDF Small 5,513,993 0.0818 0.0863
Crawler 181,196,266 0.542 0.341

Copy link
Contributor

@danrbailey danrbailey left a 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:

https://github.com/AcademySoftwareFoundation/openvdb/blob/master/openvdb_houdini/openvdb_houdini/SOP_OpenVDB_Resample.cc#L577

@@ -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)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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)
Copy link
Contributor

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?

Copy link
Contributor Author

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;
Copy link
Contributor

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]>
@apradhana
Copy link
Contributor Author

apradhana commented Dec 20, 2023

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 RealGridTypes. I pushed an update on the PR that does the registration this way, but I'm leaving some money on the table. I did not test how grid.apply would work with RealGridTypes as you pointed out. Based on our conversation with Ken and Jeff, there can be cases where we want to do arithmetic using float when modifying a HalfGrid. So I need to double-check that there is a way for grid.apply to work when we 'specialize' an operator for HalfGrid type to use a promoted type. Do you agree with this?

Still TODO for me:

  • static asserts in the methods that won't initially support half grid types. Presumably, all the methods that work for FloatGrid and DoubleGrid, but not HalfGrid.
  • Come up with a unit to for grid.apply for HalfGrid.

@portsmouth
Copy link

portsmouth commented Feb 26, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants