Basic OpenVDB read support #2010

marsupial · 2018-09-19T00:20:21Z

Description

Add OpenVDB reader to OIIO

Tests

Not yet, no

Checklist:

I have read the contribution guidelines.
If this is more extensive than a small change to existing code, I
have previously submitted a Contributor License Agreement
(individual, and if there is any way my
employers might think my programming belongs to them, then also
corporate).
I have updated the documentation, if applicable.
I have ensured that the change is tested somewhere in the testsuite
(adding new test cases if necessary).
My code follows the prevailing code style of this project.

lgritz · 2018-09-19T00:23:53Z

Makefile

+endif
+
+ifneq (${OPENVDB_HOME},)
+MY_CMAKE_FLAGS += -DOPENVDB_ROOT:STRING=${OPENVDB_ROOT}


OPENVDB_HOME or OPENVDB_ROOT?

OPENVDB_ROOT will do more interesting things...my Makefile knowledge pretty much starts and ends with these changes so probably wise to be thorough there.

Oh, I just meant to make the names consistent. If the important thing on the cmake side is OPENVDB_ROOT, let's use the same name on the makefile wrapper side.

Yes I got that, but wasn't the original just wrong as well:
make OPENVDB_HOME=/somewhere would have done nothing useful to MY_CMAKE_FLAGS

Yeah, I think it's all fine as long as it's always ROOT

lgritz

This is looking good.

TBB -- is that a strict requirement of OpenVDB? Is it just a link time dependency, or do we need include files?

I'd really like you to add a testsuite case for this. It can be somewhat like the field3d case. Does not have to exercise every feature, just a simple (small!) file to be sure it's able to open and read vdb.

CMakeLists.txt

site/spi/Makefile-bits-arnold

lgritz · 2018-09-21T18:34:39Z

src/cmake/externalpackages.cmake

@@ -7,6 +7,7 @@ if (NOT VERBOSE)
    set (DCMTK_FIND_QUIETLY true)
    set (FFmpeg_FIND_QUIETLY true)
    set (Field3D_FIND_QUIETLY true)
+    set (OpenVDB_FIND_QUIETLY true)


Also TBB_FIND_QUIETLY

marsupial · 2018-09-21T22:57:50Z

As I remember TBB was required as somewhere deep in VDB headers it was being pulled in.
But I'll investigate and make sure it wasn't from inclusion of something not included here.

marsupial · 2018-09-24T18:46:12Z

TBB indeed needs to be visible for compile & link.
Split out TBB to be a separate CMake object.
Added cached variables OPENVDB_LOCATION for TBB_LOCATION.

Might still need to be an update to travis & appveyor to run the test.

src/cmake/externalpackages.cmake

marsupial · 2018-09-24T18:59:07Z

CI seems to show not having VDB doesn't break the build, is it worth trying to get it supported there?

lgritz · 2018-09-24T19:13:14Z

LGTM, except that I tried to check on my laptop and it failed to find TBB. Offline, Roman and I discussed and he's going to substitute a different FindTBB.cmake, the one from USD, which seems to work better.

lgritz · 2018-09-24T19:14:31Z

Correct, not having VDB should not break the build.

I'll go back later and adjust the travis files to make sure openvdb is installed (at least on Mac, where it's easy thanks to Homebrew) so that this feature is exercised. You can try that, too, if you want, but no big deal if you want to move on and I'll take care of it separately.

marsupial · 2018-09-24T21:05:54Z

Seems good here, probably worth trying to make sure it works as expected on OS X.
Next travis build should give it a try..

lgritz · 2018-09-24T23:30:52Z

On OSX (both on my laptop and Travis), its finding TBB but not finding OpenVDB. Not sure why.

marsupial · 2018-09-25T02:17:05Z

Could NO_DEFAULT_PATH in FindOpenVDB.cmake:66 be the culprit? Seems I stripped that from the header portion but not the library.

lgritz · 2018-09-25T04:26:35Z

I tried messing with that, and it didn't help on my laptop.

I did find another FindOpenVDB file, however: https://github.com/dfelinto/blender/blob/master/build_files/cmake/Modules/FindOpenVDB.cmake

Using this simple one from Blender (which says it's BSD license) seems to do the trick to build it, if you also change the appropriate variables in externalpackages.cmake and openvdb.imageio/CMakeLists.txt.

I do still fail the openvdb unit test, getting errors like this:

ERROR: Could not open 'src/sphereCd.vdb': IoError: Blosc decoding is not supported

Do you know what that's about?

marsupial · 2018-09-25T05:22:32Z

An older version of VDB, or one compiled without blosc support? SideFx forum also mentioned possible incompatible versions of the blosc lib itself causing issues. I'll see if there's a way to convert or output from an older version of Houdini.

marsupial · 2018-09-25T18:31:43Z

Updated test to not use blosc, are you going to push something on top with the working CMake or is that something to be done here?

lgritz · 2018-09-25T19:17:11Z

I'm happy to do it (since I already made the changes on my end to test it).
Should I merge what you have and then send a separate PR for that? Or should I push back to your branch to make it all one merge (you need to have checked the box that lets me push to your branch, for that to work)?

marsupial · 2018-09-25T21:02:41Z

I'll push the button..

…cludes globally

lgritz · 2018-09-25T21:11:37Z

Builds on OSX and passes the unit test!
I'm going to add some touch-ups and then push back so you can see and approve.

The original FindOpenVDB.cmake was unreliable. Replaced with one found elsewhere on the net (from Blender, bearing a BSD license).

marsupial · 2018-09-25T22:20:41Z

Great thanks...Out of curiosity the VDB library was 4 or 3?

lgritz · 2018-09-25T23:01:54Z

5

* Makefile wrapper: directly use OPENVDB_ROOT_DIR * Update INSTALL.md and oiiointro.tex to document the new optional dependencies and how we incorporated those two new Find*.cmake files from other projects (with appropriate licenses). * Simplification of the TBB and OpenVDB sections of externalpackages.cmake, they didn't need to be quite so verbose, didn't need to -UUSE_BLAH if not found, did not need to set BLAH_FOUND (done by the FindBlah itself), and I added minimum versions of both packages based on this year's VFX Platform guidelines (as a forward-looking new feature in master, I don't feel the need to verify support of older versions). * Update oiio docs chapter on supported formats to include OpenVDB. * Some extremely minor formatting and whitespace fixes in openvdbinput.cpp to conform to prevailing project style. * Simplification of the SPI-specific Makefile-bits. * Minor touch of field3d to add "worldtolocal" metadata to conform to the new convention we're using with vdb.

lgritz · 2018-09-25T23:29:33Z

OK, I pushed some minor fixes on top of yours. A lot was just minor cleanup I would have done at some later time as I touched up the docs or prepared for a release.

The original FindOpenVDB.cmake was unreliable. Replaced with one found elsewhere on the net (from Blender, bearing a BSD license).
Makefile wrapper: directly use OPENVDB_ROOT_DIR
Update INSTALL.md and oiiointro.tex to document the new optional dependencies and how we incorporated those two new Find*.cmake files from other projects (with appropriate licenses).
Simplification of the TBB and OpenVDB sections of externalpackages.cmake, they didn't need to be quite so verbose, didn't need to -UUSE_BLAH if not found, did not need to set BLAH_FOUND (done by the FindBlah itself), and I added minimum versions of both packages based on this year's VFX Platform guidelines (as a forward-looking new feature in master, I don't feel the need to verify support of older versions).
Update oiio docs chapter on supported formats to include OpenVDB.
Some extremely minor formatting and whitespace fixes in openvdbinput.cpp to conform to prevailing project style.
Simplification of the SPI-specific Makefile-bits.
Minor touch of field3d to add "worldtolocal" metadata to conform to the new convention we're using with vdb.

marsupial · 2018-09-25T23:56:05Z

src/openvdb.imageio/openvdbinput.cpp

 bool
 OpenVDBInput::open (const std::string &filename, ImageSpec &newspec)
 {
    if (m_input)
        close();

    auto file = openVDB(filename, this);
-    if (!file)
+    if (!file || !file->isOpen())


The ASSERT removed was because the contract from the openVDB function is not to return a io::File unless isOpen has returned true already. In the scheme of what's about to be done the additional check likely adds nothing, but I did like the ASSERTION of that promise.

I see. I was just trying to reduce the number of cases where we couldn't recover. From the point of an app, I think it's preferable to return "I can't open this file" whether it be that it's malformed, or OpenVDB is somehow broken, then to terminate the whole process.

How about if I use DASSERT, which will do the hard crash/error when in debug mode? But for release build, the app will get a failure state rather than terminate?

Yeah, I'm fine with how you had it. Changing back.

marsupial · 2018-09-25T23:58:28Z

src/cmake/externalpackages.cmake

-        set (oiio_vdb_why ", could not find Intel TBB")
-    endif ()
-    message (STATUS "OpenVDB will not be used${oiio_vdb_why}")
+    message (STATUS "OpenVDB will not be used, could not find Intel TBB")


Won't this spew the message when invoked with -DUSE_OPENVDB=OFF

I think you're right, I will fix it.

marsupial · 2018-09-26T00:02:25Z

Everything seems to work as was, so I'd say good to go.
Left two little comments above, but totally fine if you want to show them to the waste-bin.

lgritz · 2018-09-26T15:24:33Z

Merged.
Great work, Roman!

lgritz reviewed Sep 19, 2018

View reviewed changes

lgritz requested changes Sep 21, 2018

View reviewed changes

marsupial force-pushed the openvdb branch from c34338f to b63c46f Compare September 24, 2018 06:49

lgritz reviewed Sep 24, 2018

View reviewed changes

src/cmake/externalpackages.cmake Outdated Show resolved Hide resolved

rzulak added 9 commits September 25, 2018 14:06

[OpenVDB] Add build system support.

3c423d7

[OpenVDB] Add reader support

b549a2f

[OpenVDB] cell-centered lookup

dd6c060

[CMake] Expose OPENVDB_LOCATION & TBB_LOCATION as CMake cached variables

2448871

[OpenVDB] Remove unneccessary rpath

7780703

[OpenVDB] Add test

80acb80

[OpenVDB] Swap out FindTBB with one from USD & don't add VDB & TBB in…

a0fd188

…cludes globally

[Travis] Add openvdb to OS X testing

837e4fc

[OpenVDB] Don't use blosc compression in tests

f71e1b6

Fix builds on OSX

803ece5

The original FindOpenVDB.cmake was unreliable. Replaced with one found elsewhere on the net (from Blender, bearing a BSD license).

lgritz force-pushed the openvdb branch from 28452ea to ae4afea Compare September 25, 2018 23:26

marsupial commented Sep 25, 2018

View reviewed changes

lgritz added 2 commits September 25, 2018 17:21

Fixes per Roman's comments

b1bf9e9

Fixes per Roman's comments

8559cef

lgritz merged commit 47e9a4c into AcademySoftwareFoundation:master Sep 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic OpenVDB read support #2010

Basic OpenVDB read support #2010

marsupial commented Sep 19, 2018

lgritz Sep 19, 2018

marsupial Sep 19, 2018

lgritz Sep 21, 2018

marsupial Sep 21, 2018

lgritz Sep 21, 2018

lgritz left a comment

lgritz Sep 21, 2018

marsupial commented Sep 21, 2018

marsupial commented Sep 24, 2018

marsupial commented Sep 24, 2018

lgritz commented Sep 24, 2018

lgritz commented Sep 24, 2018

marsupial commented Sep 24, 2018

lgritz commented Sep 24, 2018

marsupial commented Sep 25, 2018

lgritz commented Sep 25, 2018

marsupial commented Sep 25, 2018

marsupial commented Sep 25, 2018

lgritz commented Sep 25, 2018

marsupial commented Sep 25, 2018

lgritz commented Sep 25, 2018

marsupial commented Sep 25, 2018

lgritz commented Sep 25, 2018

lgritz commented Sep 25, 2018

marsupial Sep 25, 2018

lgritz Sep 26, 2018

lgritz Sep 26, 2018

marsupial Sep 25, 2018

lgritz Sep 26, 2018

marsupial commented Sep 26, 2018

lgritz commented Sep 26, 2018

Basic OpenVDB read support #2010

Basic OpenVDB read support #2010

Conversation

marsupial commented Sep 19, 2018

Description

Tests

Checklist:

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

lgritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marsupial commented Sep 21, 2018

marsupial commented Sep 24, 2018

marsupial commented Sep 24, 2018

lgritz commented Sep 24, 2018

lgritz commented Sep 24, 2018

marsupial commented Sep 24, 2018

lgritz commented Sep 24, 2018

marsupial commented Sep 25, 2018

lgritz commented Sep 25, 2018

marsupial commented Sep 25, 2018

marsupial commented Sep 25, 2018

lgritz commented Sep 25, 2018

marsupial commented Sep 25, 2018

lgritz commented Sep 25, 2018

marsupial commented Sep 25, 2018

lgritz commented Sep 25, 2018

lgritz commented Sep 25, 2018

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

marsupial commented Sep 26, 2018

lgritz commented Sep 26, 2018