Repository navigation

Add File::readGrids() - #2356

Merged

danrbailey merged 6 commits into

AcademySoftwareFoundation:masterfrom

danrbailey:read_grids_by_name

Oct 16, 2026

Merged

Add File::readGrids()#2356
danrbailey merged 6 commits into
AcademySoftwareFoundation:masterfrom
danrbailey:read_grids_by_name

danrbailey commented Oct 2, 2026

Contributor

I believe this was functionality that was always intended to be added. In fact, there was a "@todo GridPtrVec readAllGrids(const Name&)" in the code which matches the method I am adding.

The problem is that when you have multiple grids with the same name - for example two grids called "density", the names get changed to "density[0]" and "density[1]" to disambiguate them, which requires the user to extract the names from the metadata and to parse these names and then iterate over all grids to which this applies and read them one-by-one. This is complicated workflow.

This new method File::readGrids(const Name&, ...) allows the user to call readGrids("density", ...) and it will read all grids with that name and return a populated GridPtrVecPtr. The grids are stored in the vector in the same order in which they appear in the file. I also added this to the Python bindings.

danrbailey requested a review from kmuseth as a code owner

October 2, 2026 22:45

danrbailey force-pushed the read_grids_by_name branch from 5cb2416 to 69410f9 Compare

October 2, 2026 23:18


          Add File::readGrids(const Name&, ...)

29dbb01

Signed-off-by: Dan Bailey <danbailey@ilm.com>

danrbailey force-pushed the read_grids_by_name branch from 69410f9 to 29dbb01 Compare

October 3, 2026 00:43

jmlait reviewed

View reviewed changes

openvdb/openvdb/io/File.h


          Improve documentation around the File API

2a7c3d7

Signed-off-by: Dan Bailey <danbailey@ilm.com>

jmlait approved these changes

View reviewed changes

kmuseth requested changes

View reviewed changes

kmuseth left a comment •

edited

Loading

Contributor

Looks good with a few exceptions:

See in-lined comment about missing "this->"

It seem to me that similarly named grids with shared trees are duplicated when calling readGrids. That is readGrids() loses instance sharing when converting streamed files. In File.cc (lines 548–551), each cached grid is resolved independently. For two same-name grids sharing a tree, conversion creates two separate trees.

getGrids(options) preserves tree sharing.
readGrids("density", options) breaks sharing for files written through io::Stream.
Files containing grid offsets preserve sharing correctly.

This contradicts the new API’s documented behavior and can multiply conversion work and memory consumption. Reusing the source-tree cache already implemented in getGrids() would address it.

openvdb/openvdb/io/File.cc Outdated

                   // (along with the entire contents of the file, because the file
                   // doesn't support random access), retrieve and return it.
                   GridBase::Ptr cachedGrid = retrieveCachedGrid(name);

kmuseth Oct 15, 2026

Contributor

Please add this-> here and to the new calls to member functions. I prefer this style because it makes calls on the current object immediately distinguishable from calls to free functions.

openvdb/openvdb/io/File.cc Outdated

-                  // Seek to and read in the grid from the file.
-                  const GridDescriptor& gd = it->second;
+                  return readGridFromDescriptor(it->second, readOptions);

kmuseth Oct 15, 2026

Contributor

Please add this-> here and to the new calls to member functions. I prefer this style because it makes calls on the current object immediately distinguishable from calls to free functions.

openvdb/openvdb/io/File.cc Outdated

+                  } else if (readOptions.clipBBox.isSorted()) {
+                      // A clip depends on each grid's own transform, so instances do not share trees.
+                      for (NameMapCIter it = range.first; it != range.second; ++it) {
+                          ret->push_back(readGridFromDescriptor(it->second, readOptions));

kmuseth Oct 15, 2026

Contributor

Please add this-> here and to the new calls to member functions. I prefer this style because it makes calls on the current object immediately distinguishable from calls to free functions.

openvdb/openvdb/io/File.cc Outdated


		const std::pair<NameMapCIter, NameMapCIter> range = mGridDescriptors.equal_range(name);

		if (!inputHasGridOffsets()) {

kmuseth Oct 15, 2026

Contributor

Please add this-> here and to the new calls to member functions. I prefer this style because it makes calls on the current object immediately distinguishable from calls to free functions.

openvdb/openvdb/io/File.cc Outdated

+                      for (NameMapCIter it = range.first; it != range.second; ++it) {
+                          const auto cachedIt = mNamedGrids.find(it->second.uniqueName());
+                          if (cachedIt == mNamedGrids.end()) continue;
+                          ret->push_back(resolveCachedGrid(cachedIt->second, readOptions, mReadDiagnostics));

kmuseth Oct 15, 2026

Contributor

Please add this-> here and to the new calls to member functions. I prefer this style because it makes calls on the current object immediately distinguishable from calls to free functions.

openvdb/openvdb/io/File.cc Outdated

+                      for (NameMapCIter it = range.first; it != range.second; ++it) {
+                          const GridDescriptor& gd = it->second;
+                          gd.seekToGrid(inputStream());

kmuseth Oct 15, 2026

Contributor

Please add this-> here and to the new calls to member functions. I prefer this style because it makes calls on the current object immediately distinguishable from calls to free functions.

openvdb/openvdb/io/File.cc Outdated

+                      for (NameMapCIter it = range.first; it != range.second; ++it) {
+                          const GridDescriptor& gd = it->second;
+                          gd.seekToGrid(inputStream());
+                          GridBase::Ptr grid = Archive::readGrid(gd, inputStream(), readOptions, mReadDiagnostics);

kmuseth Oct 15, 2026

Contributor

Please add this-> here and to the new calls to member functions. I prefer this style because it makes calls on the current object immediately distinguishable from calls to free functions.

openvdb/openvdb/io/File.cc Outdated

+                                  << "\" for grid " << GridDescriptor::nameAsString(gd.uniqueName())
+                                  << " in file " << mFilename);
+                          }
+                          parentIt->second.seekToGrid(inputStream());

kmuseth Oct 15, 2026

Contributor

Please add this-> here and to the new calls to member functions. I prefer this style because it makes calls on the current object immediately distinguishable from calls to free functions.

openvdb/openvdb/io/File.cc Outdated

+                          }
+                          parentIt->second.seekToGrid(inputStream());
+                          namedGrids[gd.instanceParentName()] =
+                              Archive::readGrid(parentIt->second, inputStream(), readOptions, mReadDiagnostics);

kmuseth Oct 15, 2026

Contributor

Please add this-> here and to the new calls to member functions. I prefer this style because it makes calls on the current object immediately distinguishable from calls to free functions.

openvdb/openvdb/io/File.cc Outdated

+                  // Seek to and read in the grid from the file.
                   gd.seekToGrid(inputStream());
-                  grid = Archive::readGrid(gd, inputStream(), readOptions, mReadDiagnostics);
+                  GridBase::Ptr grid = Archive::readGrid(gd, inputStream(), readOptions, mReadDiagnostics);

kmuseth Oct 15, 2026

Contributor

Please add this-> here and to the new calls to member functions. I prefer this style because it makes calls on the current object immediately distinguishable from calls to free functions.

danrbailey added 3 commits

October 15, 2026 11:58


          Address feedback regarding instanced grids

224e537

Signed-off-by: Dan Bailey <danbailey@ilm.com>


          De-instances grids when applying clipping through getGrids()

c2c652c

Signed-off-by: Dan Bailey <danbailey@ilm.com>


          Add this-> in modified lines

e5178cd

Signed-off-by: Dan Bailey <danbailey@ilm.com>

danrbailey commented Oct 16, 2026

Contributor Author

Looks good with a few exceptions:

See in-lined comment about missing "this->"

It seem to me that similarly named grids with shared trees are duplicated when calling readGrids. That is readGrids() loses instance sharing when converting streamed files. In File.cc (lines 548–551), each cached grid is resolved independently. For two same-name grids sharing a tree, conversion creates two separate trees.

getGrids(options) preserves tree sharing.

readGrids("density", options) breaks sharing for files written through io::Stream.

Files containing grid offsets preserve sharing correctly.

This contradicts the new API’s documented behavior and can multiply conversion work and memory consumption. Reusing the source-tree cache already implemented in getGrids() would address it.

I think you're right. readGrids() previously de-instanced grids for files without grid offsets when a read mode or clip was used. I moved the sharing logic out of getGrids() into a private File::resolveCachedGrids() and both methods call this now so they will stay in sync. There's a new unit test too.

I thought I'd go over the other instance cases to confirm there was nothing missing and I also found a bug in getGrids(readOptions). When you clip on a file with grid offsets, it incorrectly used the parent transform and shared that with the other instances. I fixed this by clipping each grid with its own transform - this matches readGrid(name, bbox) and readGrids() now. Note that this only affects the ReadOptions overload which is new functionality I added recently, getGrids() with no clip has not changed.

One more thing to highlight - for all clipping methods used with files with grid offsets, the grids get de-instanced. This has always been the case, for example readGrid(name, bbox) has always de-instanced when you clip (and I'm not even sure how we go about preserving the instancing). Out of scope for this change I think.

Files with and without grid offsets adds so much complexity, I am glad we are moving away from this in the new file format. :)

Finally, I made the changes you requested to add this->, but I don't think we're that consistent with that in this class (I only did it in the cases you raised).

kmuseth approved these changes

View reviewed changes

kmuseth left a comment

Contributor

Thanks for addressing my comments - I'll approve


          Change notes

ead31c2

Signed-off-by: Dan Bailey <danbailey@ilm.com>

danrbailey merged commit 802689a into AcademySoftwareFoundation:master

42 of 45 checks passed

danrbailey deleted the read_grids_by_name branch

October 16, 2026 20:51

danrbailey added the io label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

io