OpenVDB and Alembic reader #102

sosh · 2022-03-12T19:15:28Z

Adding OpenVDB point reader contributed by Roman Zulak and Alembic point reader contributed by Lucas Miller.

…iller)

lgritz · 2022-04-20T00:05:45Z

Ping

davvid · 2022-04-20T03:57:02Z

This is super cool. I'll file some internal issues so that we can have our partio maintainer chime in as well. I'll leave a few review notes here so we can push this forward.

davvid · 2022-04-20T03:58:28Z

src/lib/CMakeLists.txt

+set(TBB_INCLUDE_DIRS "$ENV{TBB_ROOT}/include")
+set(CMAKE_MODULE_PATH "$ENV{OpenVDB_ROOT}/lib/cmake/OpenVDB"
+    "${OpenVDB_ROOT}/lib/cmake/OpenVDB"
+    ${CMAKE_MODULE_PATH})


CMAKE_MODULE_PATH usually shouldn't need to be modified. If you set CMAKE_PREFIX_PATH to include the OpenVDB_ROOT then in theory find_package(OpenVDB) should work without needing to special-case openvdb here.

davvid · 2022-04-20T04:01:57Z

src/lib/CMakeLists.txt

+
+target_link_libraries(partio PRIVATE Alembic::Alembic OpenVDB::openvdb)
+target_include_directories(partio PRIVATE ${TBB_INCLUDE_DIRS})
+


Right now partio has a really minimal set of dependencies.

Since these are new dependencies it would be good to ease into them. Can we please make these opt-in and not built by default?

I would suggest adding some option(...) variables that default to OFF. Maybe call them something like PARTIO_ENABLE_ALEMBIC and PARTIO_ENABLE_OPENVDB so that we can opt-in to these settings.

TBB is needed for openvb so we can bundle those settings alongside PARTIO_ENABLE_OPENVDB.

davvid · 2022-04-20T04:15:13Z

src/lib/io/ABC.cpp

+{
+    // partio looks for the extension .abc to decide to call readABC
+    // and we want to be able to accept a time to read (right now in seconds)
+    // so we will pass in /tmp/[email protected]


Oh, bummer. That's really unfortunate. Would it help at all if we let you pass in something that looked like a URL? eg. foo.abc?time=42.5. Just thinking out loud for the future..

If we start to support this construct now then we will have to forever support it, so we should think about it.

davvid · 2022-04-20T04:15:47Z

src/lib/io/ABC.cpp

+            name = name.substr(0, lastAt) + ".abc";
+        }
+        catch(...)
+        {}


We should be explicit about what we're catching here. It looks like the idea is that if it's not something that converts via stod then we'll ignore the exception it throws?

davvid · 2022-04-20T04:16:45Z

src/lib/io/ABC.cpp

+            *errorStream << "Partio: Unable to open file Alembic: " << name << std::endl;
+            *errorStream << "(" << filename << ")" << std::endl;
+        }
+        return 0;


sug: prefer nullptr over 0

davvid · 2022-04-20T04:19:29Z

src/lib/io/ParticleIO.cpp

@@ -54,6 +54,8 @@ readers()
    static bool initialized=false;
    if(!initialized){
 	initializationMutex.lock();


if we make these plugins opt-in then we'll probably need #defines that match them so that we can conditionally compile these lines below.

davvid · 2022-04-20T04:24:16Z

src/lib/io/VDB.cpp

+    }
+    tbb::task_scheduler_init schedulerInit((numThreads == 0) ? tbb::task_scheduler_init::automatic : numThreads);
+
+    initLibrary();


initLibrary() and all this TBB scheduler initialization stuff seems like it's main()'s responsibility rather than the responsibility of a plugin.

CUE_THREADS is rather specific and seeing openvb::uninitialize() makes me a bit nervous because I'm assuming that it's touching some global state.

I think this would be better mentioned in the readme with some example code showing what users need to do in order to use the library rather than taking on these responsibilities inside library code.

Try to keep these parts generic and reusable with as little assumptions as possible.

davvid · 2022-04-20T04:26:07Z

src/lib/io/VDB.cpp

+    struct AttributeCache {
+        std::vector<AttributeIndex> indices;
+        ParticleAttribute           partio;    // partio.type contains the type that partio will store / allocate
+        unsigned                    tupleSize;


I dunno if it's just me, but I prefer seeing unsigned int over unsigned even though I know it's implied.

davvid

This looks great, thanks again for getting this working.

We'll see if others have any notes but this looks like a good place to start refining. I'm not opposed to merging this in if this feature can be made optional / opt-in and the implementation issues are be resolved.

davvid · 2022-04-20T04:38:20Z

src/lib/io/VDB.cpp

+            cache.indices.clear();
+            cache.partio.type = NONE;
+            continue;
+        }


note to self: the style above with the if (..) { opening brace on the same line as the if/while/switch/for control statements is preferred. We should probably share our internal clang-format config in this repo so that no one has to think about it. We haven't applied it to this repo yet.

davvid · 2022-04-20T04:48:46Z

src/lib/io/VDB.cpp

+                                convertScalar<float>(*float32, *indexIter, particles, cache.partio, static_cast<size_t>(offset++));
+                            else
+                                convertScalar<float>(*float64, *indexIter, particles, cache.partio, static_cast<size_t>(offset++));
+                        }


not a big deal, but it's unfortunate for performance that we end up in these situations with a loop followed by a conditional.

It's clunky to flip the loop around as well since we end up having to repeat that for (...) expression just to get rid of the extra branching. The branch predictor probably deals with these alright since float32 is a loop invariant, though.

davvid · 2022-04-20T04:55:44Z

src/lib/io/VDB.cpp

+                        if (precision == Precision32)
+                            float32.reset(new openvdb::points::AttributeHandle<float>(*vdbAttr));
+                        else
+                            float64.reset(new openvdb::points::AttributeHandle<double>(*vdbAttr));


Ahh.. I see. calling new in a loop is probably something that should be avoided. It's slow and it looks like we can avoid the heap altogether.

Can we keep uninitialized handles on the stack instead? If that's tricky because the types are different then that might be another vote towards restructuring this loop below to instead have a top-level conditional for Precision32 and then dispatching from there rather than pushing the float32 vs float64 stuff down into the loop bodies.

One way to do that without having to have too much duplicate code just for AttributeHandle<double> vs <float> would be to add a template function to handle the loop body and then we can dispatch from this point down.

That would make it much faster too. What do you think?

davvid · 2022-04-20T04:58:46Z

src/lib/io/VDB.cpp

+                        if (precision == Precision32)
+                            int32.reset(new openvdb::points::AttributeHandle<openvdb::Int32>(*vdbAttr));
+                        else
+                            int64.reset(new openvdb::points::AttributeHandle<openvdb::Int64>(*vdbAttr));


Ditto -- same as above. Try to avoid new whenever possible, especially inside loops.

davvid · 2022-04-20T05:03:20Z

src/lib/io/ABC.cpp

+                            oAttrMap[keyName] = ioData->addAttribute(
+                                propName.c_str(), INT, extent);
+                        }
+                    }


does this need break; to avoid fallthrough?

davvid · 2022-04-20T05:03:51Z

src/lib/io/ABC.cpp

+                            oAttrMap[keyName] = ioData->addAttribute(
+                                propName.c_str(), ptype, extent);
+                        }
+                    }


ditto, do we need break above and below?

davvid · 2022-04-20T05:09:01Z

src/lib/io/readers.h

@@ -36,6 +36,8 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGES.
 #define _READERS_h_

 namespace Partio{
+ParticlesDataMutable* readABC(  const char* filename,const bool headersOnly,std::ostream* errorStream);
+ParticlesDataMutable* readVDB(  const char* filename,const bool headersOnly,std::ostream* errorStream);
 ParticlesDataMutable* readBGEO(	const char* filename,const bool headersOnly,std::ostream* errorStream);


note: this part would also nee some special ifdef handling if we made openvb + alembic optional.

davvid · 2022-04-20T05:12:38Z

src/lib/io/ABC.cpp

+
+        std::map<std::string, ParticleAttribute>::const_iterator it;
+        for (it = iAttrMap.begin(); it != iAttrMap.end(); ++it)
+        {


Tiny style sug ~ map iterators are one of those areas where auto is kind of nice. Could we even go one step further and use a new-style loop here?

for (const auto& entry : iAttrMap) { // entry.first, entry.second }

Adding OpenVDB reader (By Roman Zulak) and Alembic reader (By Lucas M…

e2aadab

…iller)

davvid reviewed Apr 20, 2022

View reviewed changes

davvid requested changes Apr 20, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenVDB and Alembic reader #102

OpenVDB and Alembic reader #102

sosh commented Mar 12, 2022

lgritz commented Apr 20, 2022

davvid commented Apr 20, 2022

davvid Apr 20, 2022

davvid Apr 20, 2022

davvid Apr 20, 2022

davvid Apr 20, 2022

davvid Apr 20, 2022

davvid Apr 20, 2022

davvid Apr 20, 2022

davvid Apr 20, 2022

davvid left a comment

davvid Apr 20, 2022

davvid Apr 20, 2022

davvid Apr 20, 2022

davvid Apr 20, 2022

davvid Apr 20, 2022

davvid Apr 20, 2022

davvid Apr 20, 2022

davvid Apr 20, 2022


		target_link_libraries(partio PRIVATE Alembic::Alembic OpenVDB::openvdb)
		target_include_directories(partio PRIVATE ${TBB_INCLUDE_DIRS})

OpenVDB and Alembic reader #102

Are you sure you want to change the base?

OpenVDB and Alembic reader #102

Conversation

sosh commented Mar 12, 2022

lgritz commented Apr 20, 2022

davvid commented Apr 20, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davvid left a 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

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