Page MenuHome

WIP patch for OpenVDB integration in Blender and Cycles
Changes PlannedPublic

Authored by Kévin Dietrich (kevindietrich) on May 18 2015, 5:23 PM.

Details

Reviewers
Lukas Toenne (lukastoenne)
Thomas Dinges (dingto)
Sergey Sharybin (sergey)
Commits
rB5ca5009e1592: Merge branch 'master' into openvdb_smoke
rBe6517c1fd74d: Various cleanup.
rBeeb5b5411f96: Quick fix for data corruption happening when changing scene frame in separate…
rBdb695688e9dd: Quick fix for openvdb::ArithmeticError being thrown on frame 1.
rBac22acaec347: Quiet compile warning when WITH_OPENVDB is off.
rB68ec6efa768c: Fix memory leak when creating new sockets.
rBb82b176f9b61: Move node socket creation to the C-code.
rBac131ea55138: Relink outputs when opening a file after one was already open in the node.
rBcdbca9c018d1: Small fix for previous commit.
rB4ce8a1bf253d: Ensure filename is absolute in the vdb node.
rBbd30d25e4076: Only load the array in memory when needed.
rB4e9d5d1727ca: Merge branch 'master' into openvdb_smoke
rBca2f5cc466b6: Merge branch 'master' into openvdb_smoke
rB2beb654d69b6: Fix compile issue when WITH_OPENVDB
rB0d51aadbfed6: Cleanup: use madd_v3_v3fl to avoid unnecessary steps and local variable.
rBf456cc75f200: Fix grid not being in the proper place (transform).
rB543ee7961bb5: Cycles: fix typos (crashers) in vector grid allocation and sampling code.
rB7135e17df91d: Cleanup: unnecessary includes.
rBe669eefae0b1: Fix check to see if a frame is out of range.
rB7e03c74c5330: Add OPENVDB_USE_BLOSC definition.
rBedf73ab96ba7: Simplify matrix computation.
rBa2e3b7d48a48: Remove dead code.
rB8c8b54b60187: Import grids for playback now works.
rB2a040825cbbb: Fix typos in metadata reading.
rB8d6e1f313935: Set caches' reader and writer to NULL when reading a file.
rBe189fe46e1b9: Set vector type when converting a grid.
rB17fe79b2287b: Cleanup/fix: put 'using namespace' inside ifdef block.
rB1fd5f9f00442: Fix grids' names mismatch when reading a file.
rBd5551729bf65: Wrap volume sampling call in a macro.
rBfdc051b3ac47: Merge branch 'master' into openvdb_smoke
rB6a0854e04a1d: Get rid of FluidDomainDescription struct.
rB29fa4b913861: Move some files and functions around.
rB788ecf29a7fb: Don't construct default samplers, rather just store pointers to accessors and…
rBce2b57ea91de: VolumeManager: put OpenVDB related routines in their own functions.
rB2016c92ef183: Fix segault when closing Blender.
rB76b5f3d1f743: Avoid recalculating the object's data when a new cache is added.
rB74ecf0432b20: Merge branch 'master' into openvdb_smoke
rBf01afd7e2e19: Fix for cache not being set as current when deleting the first in the list.
rBa11e61a3c75f: Cleanup: indent
rB46d4cb86b64c: Move source/blender/openvdb folder to intern/openvdb
rB3e4d053a167a: Cycles: style cleanup.
rBe656d07cbc64: Implementation of VDB ray intersectors.
rBdabc9d1ee9bf: Cycles: rename OpenVDBManager -> VolumeManager, et simili.
rBf6fe6759d010: Cycles: subclass the grid samplers and make them thread-safe.
rB344de9ca95f7: Fix typos in smoke exporter.
rB09da26d720a3: Expose file compression types.
rBcd301995cc18: A new function to split a vector grid into three scalar grids.
rB6c2727e08eff: Initialize the VDB writer when creating a new cache.
rB41f4ea44548b: A new function to create a sparse vector grid from three scalar dense grids.
rBe0504bf9bbb3: Move fluid matrices to SmokeDomainSettings.
rB1b1566af9861: Redesign the VDB API a bit.
rB1341109c70f6: Tackle some of the review points.
rB091ef60e48b5: Initial support for rendering vdb file sequences.
rB9514191b0cd6: Cycles: add support for transforming the grid (pos, rot, scale).
rBcc220c34ad00: Various cleanups.
rBddd36a24a973: Cleanup.
rBa1bb117dacf0: Cycles, OpenVDBManager: split routines into separate functions.
rB5e82b837d96f: Cycles: add OpenVDBManager::delete_sampler convenience function.
rB9e892c6035be: Cycles: use ccl_fetch to get the shading point position.
rB8366d1de7041: Cycles: move routine to find an exiting grid slot to its own function.
rB997f2009c062: Export: take adaptive domain into account (still a bit off but seems to work)
rB2cf7efff36b2: WIP patch for OpenVDB integration in Blender and Cycles
rBc465f9f7290f: Cycles: add grid memory usage to logging.
rB46341783966d: Correction to some CMakeLists.txt (missed those in some previous commit)
rBbd65b1239804: Support compilation with SCons (only tested on Linux).
rB579d88551cf9: Improvements to compilation with CMake: set flags to be able to build with or…
rB96054b23ec4f: Modification to the node's UI:
rB9d1bdd964775: Add OpenVDB version to the system info file
rB7b6deb2a4663: Make OpenVDB cache properties its own struct/linked list, including some…
rBd5c17b2c92d9: Properly set the fluid transformation matrix.
rB3b9b59a9db52: Quiet warning.
rB02f307baf69d: Cleanup: unessecary use of Object in get_fluid_description, also make it static.
rB4eb69ba90f51: Cycles: missing break in GridDescription loop.
rBbdaf88765431: Cycles: remove grid name print.
rBfc49fc69d3e5: Correct update in RNA and send notifier.
rBfb71b93b43d6: Fix crash happening when exporting the simulation right after resetting it…
rB0cd3e5e72cf1: Cycles: early exit add_volume if we already have a sampler allocated.
rB2d5bd5638fa6: Cycles: tag OpenVDBManager as needing an update when connecting node sockets…
rBe810dabf739a: Fix compile issue.
rBd758a73a7474: Fluid transform cleanup (avoid reinventing the wheel).
rB9a9b2f5b9851: Cleanup: grid naming, use CFRA, quiet warning, unused include.
rB51dceb32a804: Cycles: add some quick logging.
rBdf9d4f4bff50: Add an operator to update the transformation matrices of the grids.
rB9d1652e84339: Merge branch 'master' into openvdb_smoke
rB9fe8f6e2913b: Dynamically add sockets to the node based on the grids available in the file.
rBd6d7297b746c: Fix missing object drawing update when done exporting.
rBb464dd48627b: Fix fluid transformation matrix.
rB8066346267e7: Cycles: only store a single float for scalar outputs.
rBabaa8617d7ca: Move openvdb utilities into their own files/directory.
rBa75a5d3987d2: Cycles OpenVDB node: add output sockets based on Blender's node outputs.
rBbd96249455f6: Apply patch in a new branch, to clean the history a bit
Summary

As requested by a handfull, here's some code :)

you'll need to disable compilation with OSL and install OpenVDB separately.

Some docs:
Smoke import/export
Cycles rendering

Diff Detail

Repository
rB Blender
Branch
openvdb

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Martijn Berger (juicyfruit) added inline comments.
CMakeLists.txt
1155

Should we split this out ?

Separate finding of tbb is nicer

intern/cycles/kernel/svm/svm_openvdb.h
24

should be ccl_fetch(sd, P)

source/blender/blenkernel/intern/smoke.c
2867

I expect this is not finished ?

Kévin Dietrich (kevindietrich) marked an inline comment as done.May 18 2015, 8:32 PM
Kévin Dietrich (kevindietrich) added inline comments.
CMakeLists.txt
1155

Yes, I think it can be good to separate this, maybe add a FindTBB.cmake module as well?

source/blender/blenkernel/intern/smoke.c
2867

Yes, the whole import side need some consideration. Also I remember having a few issues here, like it would try to import the data when doing an export and this can't work as the smoke data isn't available yet, so I commented this out.

Sergey Sharybin (sergey) requested changes to this revision.

For now only mainly looked into Cycles part. All the rest is more about workflow definition and that part doesn't feel really finished -- it just hooks OpenVDB as some sort of side appendix without solving memory usage of simulation/viewport unless i'm missing something.

From general notes:

  • Don't silence exception unless it's _REALLY_ needed, and surely don't silence generic exception. You're just burring hell of a lot of dead cats doing that.
  • I don't see why source/blender/openvdb need to be there. To be it more belongs to intern/openvdb.
intern/cycles/CMakeLists.txt
215

Should be wrapped with if(WITH_OPENVDB).

intern/cycles/kernel/kernel_globals.h
44

Think it's better to stick to an array of pointer of here. We can't support infinite amount of samples anyway.

46

In Cycles terminology it's not vec3, it's float3.

intern/cycles/kernel/svm/svm_openvdb.h
28

Don't think we need this, __OPENVDB__ is disabled for GPU anyway.

32

No need to store a default value.

34

this is to be wrapped into kernel_etx_voxel_* or so. Plus i would suggest subclassing the samplers and implement cycles-like API in them, so explicit conversion from float3 to Vec3 in the kernel is not needed.

intern/cycles/kernel/svm/svm_types.h
129

Comma.

356

NODE_VDB_FLOAT3

intern/cycles/render/CMakeLists.txt
15

if(WITH_OPENVDB).

intern/cycles/render/nodes.cpp
4580

Not sure why this is to be ifdefed. API could still be available while sampling/storage will be ifdefed.

intern/cycles/render/nodes.h
27

More i look into code more i think it should be VoxelManager or VolumeManager, which then will decide if it's OpenVDB to be used for sampling or something else.

intern/cycles/render/openvdb.cpp
50 ↗(On Diff #4252)

This is bad practice. Unless there's way to get specific exception from OpenVDB would prefer to just crash here instead of silencing potential bugs.

159 ↗(On Diff #4252)

Again, would prefer to not be so much generic and only check particular exception class of calls where it's needed.

223 ↗(On Diff #4252)

With subclassed samplers we can avoid having such a duplication. Or alternatively particular functions bodies can be ifdefed. Would also help in cases if/when we'll want to support something else than OpenVDB for volume storage/sampling.

232 ↗(On Diff #4252)

C++ way to deal with unused arguments it to comment name of variable in the function definition.

intern/cycles/render/scene.h
122

Dead code is forbidden.

intern/cycles/util/CMakeLists.txt
9

if(WITH_OPENVDB).

I also had impression you did includes in a parent's CMakeLists.txt?

intern/cycles/util/util_openvdb.h
10 ↗(On Diff #4252)

This totally duplicates code from util_types.h.

14 ↗(On Diff #4252)

Why this is even needed?

22 ↗(On Diff #4252)

enum can be declared even with OpenVDB disabled.

source/blender/blenkernel/intern/smoke.c
416

Prefer to have iterator initialized in the body.

source/blender/editors/object/CMakeLists.txt
37

if(WITH_SMOKE).

This revision now requires changes to proceed.May 20 2015, 12:16 PM
Kévin Dietrich (kevindietrich) marked 12 inline comments as done.May 20 2015, 4:20 PM

@Sergey Sharybin (sergey) it's cool to get a review early on, but, as you somewhat noted, this patch is definitely not near to be finished, or near its final state. Actually, in the past days few I added a couple of stuff and made some changes... I just uploaded it here on requests from @Martijn Berger (juicyfruit) and @Thomas Dinges (dingto). In the case of Cycles, I'd like to make use of OpenVDB's own ray intersectors to skip empty space and those need to be per thread (because they are not thread safe), is it possible to do so (having per thread instances)? The same goes for the samplers, for now they use the VDB tree itself but for faster sampling I'd suggest using ValueAccessors (and therefore will need to be per thread).

But then, if we can ensure thread safety, it is not clear to me how to plug in the ray intersectors. I mean, they would need to be created in the "OpenVDBManager" class (or whatever it will be called) from the grids, just like the samplers, then we'll have an array of those (also in KernelGlobals?) but how to check the right intersector? We could iterate over the whole array in scene_intersect(), but we'll need the right one for the actual ray marching later on.

Maybe we could use a single object to subclass the samplers and the ray intersectors? What approach would you take here?


For now only mainly looked into Cycles part. All the rest is more about workflow definition and that part doesn't feel really finished -- it just hooks OpenVDB as some sort of side appendix without solving memory usage of simulation/viewport unless i'm missing something.

I thought the goal (suggested by @Lukas Toenne (lukastoenne)) was to use OpenVDB as a caching system here. Also I'm not sure what you mean by "solving memory usage of simulation/viewport", do you want me to reimplement the fluid solver using OpenVDB and rework the drawing code to use OpenVDB grids? (For the drawing code I have no real idea on how to properly send an OpenVDB grid, or its information, to the GPU). If so, then those are not goals of this patch, unless I'm missing something too.

From general notes:

  • Don't silence exception unless it's _REALLY_ needed, and surely don't silence generic exception. You're just burring hell of a lot of dead cats doing that.
  • I don't see why source/blender/openvdb need to be there. To be it more belongs to intern/openvdb.

About source/blender/openvdb, I feel like OpenVDB can be used in more ways than what is done in this here patch, I've done the same for the particle mesher (D1008). Well I feel like it could be close to where it is used. For instance, I am using a FluidDomainDescr struct to encode data on the simultion itself, the only difference between this struct and SmokeDomainSettings are two matrices, that could be added to SmokeDomainSettings and send this directly. That's my end of it, but if you intend to use VDB for e.g. hair stuff (creating a hair system from the volume of a mesh, using VDB as the grid definition, solving the simulation...), you might not want to go through the hassle of having to write and maintain an API for every single function present in VDB. I'm cool with both front (intern/ or source/), we just need to take possible future usage in consideration, and take a decision. I also feel like having some {DNA|BKE|rna}_openvdb* to put openvdb related stuff in (at least for the caching system for now), so it's more integrated and won't feel like a "side appendix". Finally I put it here, because I couldn't decide where else (like imbuf/intern/openexr/, or alembic directly included in source/blender/pointcache, at least in the dev branch).

In all, I consider that code to be more like a proof of concept, and needs proper rewrite or integration (for lack of better terms).

I'll update the patch shortly with some of the review points taken care of (the quick ones ;) ) and my latest changes (quick support to load file for the current frame, and ability to manipulate the coordinates of the shading point in Cycles node tree).

intern/cycles/CMakeLists.txt
215

For some reason I have yet to figure, wrapping it with if(WITH_OPENVDB) make compilation fail due to 'implicit float to double conversions' happening in OpenVDB. It's on my todo list though.

intern/cycles/kernel/svm/svm_openvdb.h
34

Will do in the coming days.

intern/cycles/render/nodes.cpp
4580

This was a quick hack to circumvent the fact that I ifdef-ed the API. I pretty much done the whole ifdef part of the patch in one go to check that everything compiles when WITH_OPENVDB is not defined, and it was faster to put an ifdef here than recompiling Cycles.

intern/cycles/render/nodes.h
27

I was thinking about the same.

intern/cycles/render/openvdb.cpp
50 ↗(On Diff #4252)

In this case I'd only catch openvdb::IoError, as we may not have a file for the current frame or so.

159 ↗(On Diff #4252)

This is just a code de-duplication technique to avoid maintaining different catch blocks provided that this here function will probably not be the only one to have to catch exceptions. If an exception is thown, and catch_exception() defined above does not catch it, it will crash.

232 ↗(On Diff #4252)

Already did, but haven't updated this differential.

intern/cycles/util/CMakeLists.txt
9

Was probably from an early draft, removed.

intern/cycles/util/util_openvdb.h
14 ↗(On Diff #4252)

Was just from the quick copy paste to solve build issues, can clean this up.

Kévin Dietrich (kevindietrich) marked 6 inline comments as done.
Kévin Dietrich (kevindietrich) updated this revision to Diff 4302.

Most of the early review points were taken of, I believe some minor ones remain though.

I update this patch as a checkpoint and also to settle on the general design of the Cycles part. Basically, I put the samplers in two different classes, for float and vector types. Those classes also hold the ray intersecting and marching routines. I made them derive from a virtual base class. The rationale is to have some sort of generecity, so other voxel storage library can be used in the future without much changes. It's also made to isolate the OpenVDB code a bit, so maybe RTTI symbols in OpenVDB won't be an issue.

Though, it is not clear to me at the moment how to further proceed on the implementation (I'll need to study the code a bit more), so I'd rather get a checkpoint here.

For the Blender side, I just started rewriting the API, at least to make it usable in more cases than the smoke simulation, and the code altogether was moved to intern/openvdb.

Also here's some rendering tests, including render times:


(The size/placement of the VDB grid is a bit off, I have yet to take some time to find where the error lays, it's on the Blender side though.)

The dense one was rendered with cubic interpolation, whilst the VDB version was using the box sampler, this makes me think that I could/should look into using the interpolation menu from the material properties panel instead of having a separate one in the node.

CMakeLists.txt
228

I'm not sure about this one. Blosc isn't a dependency as far as OpenVDB is concerned, so it's an option there too. The only advantage blosc offers is multi-threaded compression, size and quality are similar to zip. But I don't know if we should make it a build option in Blender as well, as we can also make a run-time check, for now this option just disables an option in the UI...

Kévin Dietrich (kevindietrich) updated this revision to Diff 4321.
  • Various cleanups and fixes.
  • Set vector type when converting a grid.
  • Add OPENVDB_USE_BLOSC definition, we have to define this in Blender if using blosc due to a bug in OpenVDB which is resolved in the current development version. When next stable release is out those defines will be removed.
  • Fix local grid offset. Basically the smoke simulator uses the MAC grid approach where values are stored cell centered in the voxels whereas VDB is storing values node centered, so we have to offset the grid's position (in the transform matrix) by half a voxel to compensate.
  • Merge branch 'master' into openvdb_smoke
  • Only load the array in memory when needed.
  • Small fix for previous commit.
  • Ensure filename is absolute in the vdb node.
  • Relink outputs when opening a file after one was already open in the
  • Move node socket creation to the C-code.
  • Fix memory leak when creating new sockets.
  • Quiet compile warning when WITH_OPENVDB is off.
  • Quick fix for openvdb::ArithmeticError being thrown on frame 1.
  • Quick fix for data corruption happening when changing scene frame in
  • Various cleanup.
  • Merge branch 'master' into openvdb_smoke

Hi Kevin,
The patch applies properly here but could you give some instructions for building with your patch? How and where to build OpenVDB so "python scons/scons.py" finds it in the compilation process?

@mathieu menuet (bliblubli) building OpenVDB is very straighforward, you should already have all its dependencies, I'll link to my Makefile for OpenVDB. Then to build in Blender with Scons simply add to your user-config.py:

WITH_BF_OPENVDB = True
BF_OPENVDB = '/opt/lib/openvdb'  # change this if needed

Here's the makefile for VDB: http://www.pasteall.org/58808/text

Adapt it to suit your needs, I'm on Linux here so I used the paths used by the install_deps.sh script (/opt/lib for libraries). Once this makefile is setup on your side simply use make and make install to compile and install the lib.

well, that may be the problem then, I'm on windows and it seems to be not as straight forward as on linux :( If someone manage to make a build for windows, I'll be happy.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.

The main concern I have with the caching side is when loading, naming the handling of the exported flag (this is mostly me not paying much attention), and only loading/reading what's necessary for rendering both viewport and other render engines.

For Cycles, I left the code here to get another round of review on the sampling and some feedback on the direction of the patch regarding ray marching/intersection. IMO, some of the latter should be handled in parrallel (most likely prior) to the BVH testing. Also it recquires a patched version of OpenVDB which I'm trying to get in the upstream version, as well as the most recent version of Boost.

Kévin Dietrich (kevindietrich) planned changes to this revision.Aug 3 2015, 11:40 AM

Tagging as planned changes for now, I made some changes in the branch which are not in this here patch, and of course those changes broke a few things. Stay tuned for an update in the coming days.


Anyone (users or testers) paying attention is of course welcome to stress test the branch.

  • Several improvement from user feedback and some code cleanups

Please continue this awesome patch :)

Hello
can someone create script and share it ?

Hi seghier, what do you mean with "create script"?
Download raw.diff, patch Blender source code, recompile (if the patch still apply).

Cheers, mib

Thanks Wolfgang Faehnle
i have no idea about recompile ; why they didn't simply create plugin and users install it ?

Guys, this is a developer differential, not a user discussion. Test builds will be available, as soon as the feature is ready for testing.

so we wait blender 2.8

I have just opened a new differential for the cache system, reason being that it's different than what is in this patch, and some Cycles related reviews already happened here (I'll reuse this differential when the Cycles code is ready). For the few non-developers paying attention here, if you want to test it, please grab the patch from D1721. And again, please avoid posting random non development related comments, this creates noise :)

Kévin Dietrich (kevindietrich) planned changes to this revision.Jan 28 2016, 8:46 AM

Marking this as planned changes so it doesn't stay on people's "to review" stack. (Also it's fairly outdated.)