Added support for loading native object for openvdb#170
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #170 +/- ##
==========================================
- Coverage 90.03% 90.02% -0.02%
==========================================
Files 6 6
Lines 953 992 +39
Branches 126 132 +6
==========================================
+ Hits 858 893 +35
- Misses 58 61 +3
- Partials 37 38 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Is the objective of the PR to enable #162 for openvdb objects only or implement it for all available formats? It's totally ok to only do it for OpenVDB, I just want to know when reviewing the PR. |
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
Great that you're starting the work on #162 . The type-selection mechanism needs to be reworked.
We also need to have a more general architecture for Grid for (1) detecting the format, (2) dispatching to the correct loader. We can decide to do this in a separate PR if for right now we're only focusing on VDB.
| for key in self.vdb_grid.metadata: | ||
| try: | ||
| self.metadata[key] = self.vdb_grid[key] | ||
| except (TypeError, ValueError): |
There was a problem hiding this comment.
Under which conditions does that fail? I am always suspicious of try/except that passes.
There was a problem hiding this comment.
vec3f can be one of the example. But I can create a path for a warning to be raised for this
| self.origin = v_origin | ||
| self.delta = v_delta | ||
|
|
||
| dtype = np.dtype("float32") |
There was a problem hiding this comment.
I think it's cleaner to set the dtype to the default in an else block of the for loop:
for numpy_dtype, vdb_names in self._DATATYPES.items():
name_dtype = vdb_names[0]
...
if vdb_class_name == canonical_name:
dtype = numpy_dtype
break
else:
# could not find a matching dtype, use default
dtype = np.float32Or maybe should we use float64? @BradyAJohnston @PardhavMaradani do you have an opinion?
There was a problem hiding this comment.
You should also log a warning if you encounter an unknown type and use the default.
| canonical_name = ( | ||
| name_dtype.gridType | ||
| if isinstance(name_dtype, DownCastTo) | ||
| else name_dtype | ||
| ) | ||
|
|
||
| if vdb_class_name == canonical_name: | ||
| dtype = numpy_dtype | ||
| break |
There was a problem hiding this comment.
The logic is flawed. You are picking the first match to a value in the _DATATYPES dict and the relationship from vdb to np is many-to-one (eg FloatGrid can map back to int8, unint16, float32, ...) and the dict order is not defined so you might choose the wrong dtype.
Instead you must define an appropriate mapping from VDB to np.
There was a problem hiding this comment.
yep, I will try to redo that
| if bbox is None or bbox[0] == bbox[1]: | ||
| self.grid = np.zeros((0, 0, 0), dtype=dtype) | ||
| return |
There was a problem hiding this comment.
What happens here?
Possibly log a warning that you encountered an empty box with a single entry. (Is that what happened??)
There was a problem hiding this comment.
basically for VDB grid that has no active voxels
evalActiveVoxelBoundingBox() returns none
There was a problem hiding this comment.
Thanks for the explanation. Can you add a comment to the code that explains what happens?
And issue a warning.
| try: | ||
| # if a openvdb native grid is passed | ||
| import openvdb as vdb | ||
| if isinstance(grid, vdb.GridBase): | ||
| vdb_field = OpenVDB.OpenVDBField(grid=grid) | ||
| self.metadata = vdb_field.metadata | ||
| self._load( | ||
| grid=vdb_field.grid, | ||
| origin=vdb_field.origin, | ||
| delta=vdb_field.delta, | ||
| metadata=vdb_field.metadata, | ||
| ) | ||
| return | ||
| except ImportError: | ||
| pass |
There was a problem hiding this comment.
This is the right idea. This (together with the filename/array code below) is quite clunky. For each additional format we will need to add another try/except.
We need to come up with a more general solution, e.g., a dict-based dispatch lookup or a dispatcher function that calls the correct setup, based on the detected format.
| world = native_grid.transform.indexToWorld((0, 0, 0)) | ||
| assert_allclose([world[0], world[1], world[2]], g.origin, rtol=1e-5) | ||
|
|
||
| def test_extract_from_vdb_grid(self, grid345): |
There was a problem hiding this comment.
parameterize the test to try out all normally supported vdb gridtypes
| assert new_vdb_grid.metadata["name"] == "new_density" | ||
| assert new_vdb_grid.grid.dtype == np.dtype("float32") | ||
| assert_allclose(new_vdb_grid.grid, data, rtol=1e-5) | ||
|
|
There was a problem hiding this comment.
Also include a test that shows that file round-tripping works, e.g.,
- create
Grid - write OpenVDB
- read OpenVDB
- compare to
Grid
There was a problem hiding this comment.
Include a test that reads the test vdb file directly with OpenVDB first.
|
For right now I linked this PR to the OpenVDB-only issue #171 |
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
Documentation build overview
|
2 participants
Fixes #171
Changes -
_extract_from_vdb_gridthat gets the delta, origin, data type and metadata, if a native object is passed - OpenVDB.py_extract_from_vdb_grid- test_vdb.py@orbeckst please have a look at this when you are free and have time.
Also, made a new section in changelog, with enhancement in it