Volumes: refactor volume grid storage #116315

Jacques Lucke · 2023-12-18T23:10:16+01:00

Jacques Lucke commented

2023-12-18 23:10:16 +01:00

This is preparation for #115270.

This refactors how volume grids are stored with the following new goals in mind:

Get a stand-alone volume grid data structure that can be used by geometry nodes. Previously, the VolumeGrid data structure was tightly coupled with the Volume data block.
Support implicit sharing of grids and trees. Previously, it was possible to share data when multiple Volume data blocks loaded grids from the same .vdb files but this was not flexible enough.
Get a safe API for lazy-loading and unloading of grids without requiring explicit calls to some "load" function all the time.
Get a safe API for caching grids from files that is not coupled to the Volume data block.
Get a tiered API for different levels of openvdb involvement:
- No OpenVDB: Since WITH_OPENVDB is optional, it's helpful to have parts of the API that still work in this case. This makes it possible to write high level code for volumes that does not require #ifdef WITH_OPENVDB checks everywhere. This is in BKE_volume_grid_fwd.hh.
- Shallow OpenVDB: Code using this API requires WITH_OPENVDB checks. However, care is taken to not include the expensive parts of OpenVDB and to use forward declarations as much as possible. This is in BKE_volume_grid.hh and uses openvdb_fwd.hh.
- "Full" OpenVDB: This API requires more heavy OpenVDB includes. Fortunately, it turned out to be not necessary for the common API. So this is only used for task specific APIs.

At the core of the new API is the VolumeGridData type. It's a wrapper around an openvdb::Grid and adds some features on top like implicit sharing, lazy-loading and unloading. Then there are GVolumeGrid and VolumeGrid which are containers for a volume grid. Semantically, each VolumeGrid has its own independent grid, but this is cheap due to implicit sharing. At highest level we currently have the Volume data-block which contains a list of VolumeGrid.

flowchart LR
  Volume --> VolumeGrid --> VolumeGridData --> openvdb::Grid

The loading of .vdb files is abstracted away behind the volume file cache API. This API makes it easy to load and reuse entire files and individual grids from disk. It also supports caching simplify levels for grids on disk.

An important new concept are the "tree access tokens". Whenever some code wants to work with an openvdb tree, it has to retrieve an access token from the corresponding VolumeGridData. This access token has to be kept alive for as long as the code works with the grid data. The same token is valid for read and write access. The purpose of these access tokens is to make it possible to detect when some code is currently working with the openvdb tree. This allows freeing it if it's possible to reload it later on (e.g. from disk). It's possible to free a tree that is referenced by multiple owners, but only no one is actively working with. In some sense, this is similar to the existing ImageUser concept.

The most important files to read are BKE_volume_grid.hh and BKE_volume_grid_file_cache.hh. Most other changes are updates to existing code to use the new API.

This is preparation for #115270. This refactors how volume grids are stored with the following new goals in mind: * Get a **stand-alone volume grid** data structure that can be used by geometry nodes. Previously, the `VolumeGrid` data structure was tightly coupled with the `Volume` data block. * Support **implicit sharing of grids and trees**. Previously, it was possible to share data when multiple `Volume` data blocks loaded grids from the same `.vdb` files but this was not flexible enough. * Get a safe API for **lazy-loading and unloading** of grids without requiring explicit calls to some "load" function all the time. * Get a safe API for **caching grids from files** that is not coupled to the `Volume` data block. * Get a **tiered API** for different levels of `openvdb` involvement: * No `OpenVDB`: Since `WITH_OPENVDB` is optional, it's helpful to have parts of the API that still work in this case. This makes it possible to write high level code for volumes that does not require `#ifdef WITH_OPENVDB` checks everywhere. This is in `BKE_volume_grid_fwd.hh`. * Shallow `OpenVDB`: Code using this API requires `WITH_OPENVDB` checks. However, care is taken to not include the expensive parts of `OpenVDB` and to use forward declarations as much as possible. This is in `BKE_volume_grid.hh` and uses `openvdb_fwd.hh`. * "Full" `OpenVDB`: This API requires more heavy `OpenVDB` includes. Fortunately, it turned out to be not necessary for the common API. So this is only used for task specific APIs. At the core of the new API is the `VolumeGridData` type. It's a wrapper around an `openvdb::Grid` and adds some features on top like implicit sharing, lazy-loading and unloading. Then there are `GVolumeGrid` and `VolumeGrid` which are containers for a volume grid. Semantically, each `VolumeGrid` has its own independent grid, but this is cheap due to implicit sharing. At highest level we currently have the `Volume` data-block which contains a list of `VolumeGrid`. ```mermaid flowchart LR Volume --> VolumeGrid --> VolumeGridData --> openvdb::Grid ``` The loading of `.vdb` files is abstracted away behind the volume file cache API. This API makes it easy to load and reuse entire files and individual grids from disk. It also supports caching simplify levels for grids on disk. An important new concept are the "tree access tokens". Whenever some code wants to work with an openvdb tree, it has to retrieve an access token from the corresponding `VolumeGridData`. This access token has to be kept alive for as long as the code works with the grid data. The same token is valid for read and write access. The purpose of these access tokens is to make it possible to detect when some code is currently working with the openvdb tree. This allows freeing it if it's possible to reload it later on (e.g. from disk). It's possible to free a tree that is referenced by multiple owners, but only no one is actively working with. In some sense, this is similar to the existing `ImageUser` concept. The most important files to read are `BKE_volume_grid.hh` and `BKE_volume_grid_file_cache.hh`. Most other changes are updates to existing code to use the new API.

❤️ 5

Jacques Lucke added 187 commits 2023-12-18 23:10:26 +01:00

5bed953f85 Utility BKE header for forward-declaring OpenVDB grid types.

5ef682bf67 Restructured Volume node menu like other geometry menus.

12bd80f507 Stub nodes for testing new grid socket.

7c87888c4a Use regular float/vector/etc. sockets to represent grids.

The socket value will be extended from ValueOrField to
"ValueOrFieldOrGrid" (with a nicer name). The data type is the same,
and the socket shape will be updated later when inferencing supports
grid data.

999b08ca3a Move ValueOrField into BKE.

This struct is used for geometry nodes sockets only and only used by
the geometry nodes module (with an indirect dependency via simulation
zone baking). It currently lives in the functions namespace, which is
problematic when adding grids in future, which should not be part of
the functions module.

869bfe44fe Merge branch 'main' into volume-grid-sockets

4cec91d7d2 Merge branch 'main' into volume-grid-sockets

e9d78c65b0 Added a Grid pointer to the ValueOrField struct.

This will be used for grid sockets to pass grid data at runtime.
The GridType is a forward-declared OpenVDB grid class, with a template
parameter that matches Blender math types and gets swapped with a
matching OpenVDB type for the actual grid.

7f78bdfbd0 Use grid background value as the field value.

18b72fd4f3 Grid wrapper class that works as ImplicitSharindMixin.

dfd006e92f Implementation of "get grid" node that writes to ValueOrField.

b73a2aaf96 Temp: clear user count of the grid pointer when releasing ValueOrField.

71375eec57 Fixed user count cleanup when releasing the FieldValueGrid.

c404eadd44 Merge branch 'main' into volume-grid-sockets

8c9ad29d46 Use ImplicitSharingPtr for the field value grid.

c837056e45 Constructor fix for ValueOrField.

e0b324c959 Fix linker errors due to missing FieldValueGrid CPPType definition.

This shouldn't be necessary.

625590a08c Implemented the "remove" feature when getting a grid from a volume.

a484bcd99d Implementation of Store Grid node.

91f0b8e20f Support storing grids in empty volumes.

6026941ab4 Merge branch 'main' into volume-grid-sockets

8e1988c91b Added dilate and erode nodes.

af34235642 Merge branch 'main' into volume-grid-sockets

9f45d22858 Use implicit sharing to create deep copies of grids when needed.

This allows branching from a node socket to multiple inputs.

8cc6f785b5 Sanity check before accessing internal grid pointer.

c1f100c58e Cleanup: moved OpenVDB forward declarations to a dedicated file.

a19915b67b Cleanup: Single namespace statement.

02016f813e Cleanup: Removed unused static dispatch utility function.

68e5c1257d Cleanup: Move FieldValueTreeImpl into the detail namespace.

bca4dffe22 Cleanup: Moved VolumeGrid into BKE_volume_grid.hh.

The BKE_grid_types header becomes BKE_volume_grid. The VolumeGrid struct
will be the main wrapper around openvdb::Grid. GVolumeGrid will be a
generic base class and VolumeGrid<T> the type variants.

021121382b Renamed VolumeGrid to GVolumeGrid as a generic grid wrapper.

VolumeGrid<T> will be a template for typed grid wrappers.

cd5a15d5c1 Revert "Renamed VolumeGrid to GVolumeGrid as a generic grid wrapper."

This reverts commit 021121382b.

2f35ca5a91 Temp: Move VolumeGrid into bke namespace.

VolumeGrid -> blender::bke::GVolumeGrid

A type alias is added to existing volume code outside of bke working
with the existing type name. Unfortunately this does not work for RNA
where a plain C struct in the global namespace is needed.

cb88e6c113 Added dummy struct in RNA to act as a placeholder for GVolumeGrid.

DummyVolumeGrid is used as a stand-in for the actual VolumeGrid struct.
RNA callback wrappers around `rna_VolumeGrid_load` and
`rna_VolumeGrid_unload` need a C struct as the main "self" argument.
The struct does not have to be an actual DNA struct.
- VolumeGrid is a type alias ("using") that cannot be used directly for
  this purpose.
- blender::bke::GVolumeGrid is the actual struct but C++ namespaces also
  don't work.
This dummy struct is used as a placeholder for the callbacks and
reinterpreted as the actual VolumeGrid type.

cc8a86ac8c Move file cache properties of VolumeGrid into a common base class.

0858c6992b Make VolumeGridCommon an ImplicitSharingMixing class.

This currently breaks storage of grids in Volume, because the std::list
will allocate and decallocate grids, which does not respect the user
count of implicit sharing. Have to change the container to use pointers.

b5741095cb Store ImplicitSharingPtr to grids in VolumeGridVector.

Implicit sharing means that grids should not be allocated and freed
by the grid vector, otherwise they will not get deallocated properly.

3b3cc31141 Fix unexpected data destruction by making constructor explicit.

The ImplicitSharingPtr had an implicit constructor for raw pointers.
This had unintended effects when comparing an ImplicitSharingPtr to a
raw pointer: The raw pointer was implicitly converted to the shared
pointer (without change in refcount) and when going out of scope would
decrement user count, eventually freeing the data.

Conversion from raw pointer to shared pointer should not happen
implicitly. The constructor is made explicit now. This requires a little
more boilerplate when constructing a sharing pointer. A special
constructor for the nullptr is added so comparison with nullptr can
still happen without writing out a constructor.

da5f3b725b Make FieldValueGrid based on VolumeGridCommon.

b2c9503996 Rename VolumeGrid type alias to GVolumeGrid.

This is to avoid name conflicts once the FieldValueGrid gets renamed.

cd9f6363ab Renamed the FieldValueGrid and related types to VolumeGrid etc.

d5a98e0079 Another wrapper type around openvdb grid pointers.

The type structure for VolumeGrid needs to change: Casting from a
generic GVolumeGrid to a typed VolumeGrid<T> and vice versa would mean
constructing a new instance, which means deep copies of data because the
VolumeGrid is not just a reference but the ImplicitSharingData itself.

To fix this the structure should be:
1. `SharedGrid<GridType>`: A wrapper around some openvdb::Grid (can be
    GridBase).
2. `VolumeGrid<T>`/`GVolumeGrid`: Stores a `ImplicitSharingPtr` to a
    `SharedGrid`.
3. `VolumeGridVector` goes back to storing the `VolumeGrid` themselves,
    no need for another indirection here.

d123afbd31 Revert "Store ImplicitSharingPtr to grids in VolumeGridVector."

This reverts commit b5741095cb.

VolumeGridVector now stores plain GVolumeGrid instances again instead of
ImplicitSharingPtr. The shared pointers will get moved inside the
VolumeGrid struct, so that they can be casted as lightweight wrappers
without having to do deep copies of the data.

1127ac2e61 Restructured volume grid data types to separate shared data and pointers.

The VolumeGrid class will become the primary ImplicitSharingData, while
the VolumeGridPtr<T> and GVolumeGridPtr are ImplicitSharingPtrs.
Casing only happens on the pointer side, otherwise the casting would
create a deep copy of the data each time.

1f5526b6b4 Renamed structs to VolumeGrid, VolumeGridPtr, GVolumeGridPtr.

189dfd9669 Cleanup: renamed leftover instances of VolumeGridSharedData.

4b67f5edfc Merge branch 'main' into volume-grid-sockets

c0421fced0 Temp: Return shared ptrs from volume API to avoid unintentional copies.

81f57efdc2 Build fixes for Volume API changes.

f75280098b Merge branch 'main' into volume-grid-sockets

5e5c990582 Fix linking issues in makesrna caused by missing TBB linking.

e345cd77ce Merge branch 'main' into volume-grid-sockets

c0164c1d44 Removed redundant WITH_OPENVDB section from makesrna cmake file.

f34032ca03 Link TBB and OpenVDB against makesrna.

a968855a28 Fix circular header include for VolumeGrid.

7d9783c727 Make the "local" grid protected to enforce cached access.

080fe7f997 Merge branch 'main' into volume-grid-sockets

6fc068802e Sanity check for bounding box when creating texspace for volumes.

a1424cc7c2 Sanity check when trying to store named grids.

48391544b8 More utility functions for handling grid sockets.

4fb9c33074 New sampling node that works directly with grids instead of volumes.

6a2919c91e Merge branch 'main' into volume-grid-sockets

ab047c04d4 Hide new nodes behind the experimental flag for now.

5be0a53c86 Fixed build errors with USD enabled.

6de20875f9 Added some asserts to VolumeGridPtr constructors.

c0f112a70a Merge branch 'main' into volume-grid-sockets

a1c879a0c8 Fix mismatching storage type in the Sample Grid node.

ff9030b7f0 Merge branch 'main' into volume-grid-sockets

d70ad119a0 Merge branch 'main' into volume-grid-sockets

c9a99c2602 Merge branch 'main' into volume-grid-sockets

c2e55e2950 Simplified converter templates for grid values.

221b4abc88 Return a std::optional value for the grid background.

d180cf731a Renamed BKE_volume_types.hh to BKE_volume_enums.hh.

9a0aa23729 Update description of sample grid node.

0336ed9273 Fixed copy-on-write grids in dilate/erode nodes.

If the grid type is correct, `grid_for_write` should always return a
valid grid. Also have to make sure to output the new grid pointer
instead of just copying the existing input.

2f98d49922 Merge branch 'main' into volume-grid-sockets

5c4e58d5a2 Fixed build issues due to ImplicitSharingPtr always storing const data now.

e46a918533 Partial: Make ImplicitSharingPtr a base class of the volume gri pointers.

88f30df8b6 Fixed empty grid constructor.

f1566e278b Handle empty volume bounds return value in drawing code.

0a1cc3fb5d Fix outputs when getting grid from volume is not found.

3d2f8001a3 Partial: revert volume API changes and return plain VolumeGrid pointers.

821d98d0f1 Added a dedicated constructor function for making volume grid pointers.

56eb0045c7 Fix refcount when creating a typed copy of a grid pointer.

365e7eea6c Return the default value if grid sockets are used as values.

64fca45ab7 Ensure all outputs are written in the grid store node.

bc591770c4 Hide values on grid input sockets.

b3666bea3a Merge branch 'main' into volume-grid-sockets

37eac000e6 Cleanup: formatting.

d4315578c2 Move some internal includes to the source file.

1e83478304 Use a single Converter template with two methods instead of separate converters.

df0a15212b Remove copy constructor from non-copyable VolumeGrid.

bad819c4d6 Mention concrete subclasses of VolumeGridPtrCommon.

22875e3b05 Remove virtual destructor for VolumeGridPtrCommon.

a10bdafb71 Updated description.

898cf929d4 Use `void` instead of a generic MaskTree to indicate that string attributes are unsupported.

6caba1d201 Call tag_ensured_mutable after checking for mutable grid data.

6a4b095f33 Removed the unused make_implicit_sharing_ptr utility function.

2b62f77202 Set output values before removing grid from volume.

b7e781d133 Don't try to increment user count when the pointer is null.

16edee566a Comment explaining the user count of the openvdb shared_ptr.

b802677898 Make sure volume grid API functions create mutable data copies when needed.

buildbot/vexp-code-patch-coordinator Build done. Details

4ea0196b6f Remove unnecessary check after BKE_volume_grid_get_for_write.

1068df47e9 Add back missing destructor for VolumeGridPtrCommon.

0bc33f3f88 Fix Store Named Grid node with mutable VolumeGrid.

Volume API now has a "move" function for inserting an existing
VolumeGrid into the Volume. Store Named Grid node now creates a correct
mutable grid to store.

0ce7d5dc18 Fix VolumeGrid copy function: make an actual deep grid copy.

5ee032a2f7 Sanity check in drawing code trying to access optional volume bounds.

163bab64c0 Merge branch 'main' into volume-grid-sockets

3d5b151366 Increment user count in cast constructor instead of `typed()` method.

The constructor also becomes private since it shouldn't be called
outside of the downcast function.

1b311a30a3 Added some basic unit tests for volumes and pointer behavior.

ad248cea56 Merge branch 'main' into volume-grid-sockets

348a469b88 grid_for_write accessor in VolumeGridPtr can be const.

6818919bb2 Change friend class declaration to struct.

870c11fa58 Default return value if the "apply" operator returns a value.

868bc66fd6 Cleanup: formatting.

ace3f99d02 Merge branch 'main' into volume-grid-sockets

d1ea5b7164 Merge branch 'main' into volume-grid-sockets

7af2717b1c Remove potential grid copy in VolumeGridPtr accessors.

The `grid`/`grid_for_write` getters are shortcuts to the internal
VolumeGrid data. They are useful because VolumeGrid is always un-typed,
so getting a typed openvdb::Grid from volume grid requires a second cast
to the type already known by VolumeGridPtr<T>. But the grid accessors
should work the same ways as accessing the openvdb::Grid from VolumeGrid
and leave copy-on-write to higher level methods.

VolumeGridPtr might be better off untyped, with typed grid accessors
inside VolumeGrid. But that is for a later change.

0c788585ca Return grid pointer directly when extracting grid inputs.

The `as_grid` function will construct an empty grid if none is present.
That is not usually what we want when extracting an input, since a
nullptr grid is a valid pointer and can be used as the default input.

For example, using a mask grid can be optional, and the input should be
a nullptr when no mask is used. With the as_grid method the result would
always be a (empty) grid rather than nullptr.

e834b3498a Merge branch 'main' into volume-grid-sockets

869cf6bb30 Reimplemented the file cache for volume grids.

The file cache stores VolumeGrid implicit sharing data now. It uses
a combination of file path, grid name, and the simplification leve as
the key. That way simplified grids can be handled as cached data without
complicated internal refcounting. Generated grids will not have cached
simplification, but that is expected and in line with other caching
systems in Blender.

The `is_loaded` flag has been replaced with a more explicit enum
`VolumeTreeSource` to make more transparent decisions about what should
happen to grids on load/unload, simplification, copy, etc.

7d84a84d2f Merge branch 'main' into volume-grid-sockets

6d7effb088 Unnamed grids are valid, ignore grid names in file cache keys.

This is to fix the blendfile_versioning test which loads a vdb file
with an unnamed grid.

2f2a784fbd Fix cycles test: Replace the grid when unloading instead of new tree.

Cycles "steals" the OpenVDB grid from the VolumeGrid when constructing
the scene and then unloads the grid again. This unloading was replacing
the tree pointer inside the grid, breaking cycles volume rendering.

Do what the old cache did and replace the entire OpenVDB grid in case
some code holds a reference to the pointer. This is not great but it
keeps things working.

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

6c8211baf2 Fix for USD volume bounds calculation, have to load the grid first.

This pattern is not at all ideal but currently common: Any code that
wants to access grid data has to call BKE_volume_grid_load first to
ensure that the potential grid placeholder gets the full tree data.

Like in other places (Cycles, viewport drawing) the code first checks
if the volume grid is a placeholder, and in that case unloads the grid
again after getting the bounds.

Eventually grids should automatically be loaded when accessing the
internal OpenVDB grid data. This would remove the need for
BKE_volume_grid_load, but it does not cover the case of automatic
unloading after use. Some transient struct that unloads a placeholder
grid after going out of scope might handle this.

fc60f30b4f Merge branch 'main' into volume-grid-sockets

1c329e5964 Merge branch 'main' into volume-grid-sockets

b59075471e Merge branch 'main' into volume-grid-sockets

a93a7ac874 Merge branch 'main' into volume-grid-sockets

0f9331fb48 extract grid type traits to separate file

c6bee66e40 initial new api

4bcf283c26 add initial file cache

9c2d95b3eb remove old api

605c350a61 progress

04e59efd84 support preloading meta-data

d29656f7ae cache grids in file

7f2ae9d938 progress

873ca6db97 support unloading tree data

07864a1561 progress

c790ba1e64 progress

e2d8611b0d fix

f145c5b8c2 add forward declaration header

43a07718d3 progress

e7480faeff first successful compilation

fdf11e4809 fixes

71ca47cba7 Merge branch 'main' into volume-grid-sockets2

dcf458dfcf fixes for new socket storage

9152f1e296 move some functions to forward declaration header

6125554b50 cleanup dealing with grid transforms

6776ac7866 progress

a5e486a4f0 cleanup naming

69d1908b97 cleanup

2085ee862d Merge branch 'main' into volume-grid-sockets2

ba49aebbbe remove include

82e7725ceb fix leak

1a5e1e12d1 don't unload tree after computing bounding box

11bb535631 remove outdated comment

1e0c5742cb cleanup

cd14f8b1d4 fix wrong transform with simplified grids

4a42a6d858 improve file name

4e41558182 unload unused grids when volume is deleted

db0c019b11 use simplify level in first evaluation after blend file load

6312123261 comments

14ade7fcf8 cleanup

cd01dbad0d comments

3ef227f98f progress

635d5f9a73 comments

9a176d5764 cleanup naming

1107cf0034 comments

a8aea749d3 comment

7dfa5a1a54 progress

d2a8a1d98a progress

e486c48053 cleanup socket declaration

d84d7be6bc progress

b648c4144d progress

cfee633887 progress

a21457cb6f progress

e1df224b08 hide value on grid sockets

7fa26a9f05 fixes

4d6dc189aa fix compilation without openvdb

94c4899ef1 add tests

ed25bbebfa lazy load test

ace4ef1191 cleanup

63bdefe94d Merge branch 'main' into volume-grid-sockets

2059966ad6 Merge branch 'main' into volume-grid-sockets

79ee59efc6 geometry nodes specific parts

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

50cc5f5404 cleanup

Jacques Lucke commented

2023-12-18 23:48:31 +01:00

@blender-bot build

Jacques Lucke requested review from Lukas Tönne 2023-12-18 23:52:09 +01:00

Jacques Lucke requested review from Hans Goudey 2023-12-18 23:52:10 +01:00

Jacques Lucke added 1 commit 2023-12-19 00:28:36 +01:00

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

9e43698d73 fix unit test with asserts enabled

Jacques Lucke commented

2023-12-19 00:28:37 +01:00

@blender-bot build

Hans Goudey reviewed 2023-12-19 16:12:02 +01:00

Hans Goudey left a comment

Overall this seems like a nice API, and I really appreciate the documentation in the headers and the PR description.

The part I understand least now is why it's worth having implicit sharing at the VolumeGridData level when it's already implemented at the grid level with tree_sharing_info_. VolumeGridData seems very light in comparison-- not really a problem to copy. But maybe it is if we're passing it around as socket data.

Overall this seems like a nice API, and I really appreciate the documentation in the headers and the PR description. The part I understand least now is why it's worth having implicit sharing at the `VolumeGridData` level when it's already implemented at the grid level with `tree_sharing_info_`. `VolumeGridData` seems very light in comparison-- not really a problem to copy. But maybe it is if we're passing it around as socket data.

source/blender/blenkernel/BKE_volume_grid.hh Outdated

						
				@ -0,0 +37,4 @@

				 *

				 * Features:

				 * - Implicit sharing of the #VolumeGridData: This makes it cheap to copy e.g. a #VolumeGrid<T>,

				 *   because it just increases the number of users. On actual copy is only done, when the grid is

Hans Goudey commented

2023-12-19 01:41:45 +01:00

On actual copy is only done, when -> An actual copy is only done when

`On actual copy is only done, when` -> `An actual copy is only done when`

JacquesLucke marked this conversation as resolved

source/blender/blenkernel/BKE_volume_grid.hh Outdated

						
				@ -0,0 +105,4 @@

				   * not. If this variable is the only owner of the `shared_ptr`, no one else has access to the

				   * tree.

				   */

				  std::shared_ptr<AccessToken> tree_access_token_;

Hans Goudey commented

2023-12-19 01:47:35 +01:00

I guess a shared pointer is just a simple way of keeping a use count?

Jacques Lucke commented

2023-12-19 23:50:30 +01:00

Correct. Will extend the comment.

source/blender/blenkernel/BKE_volume_grid_file_cache.hh Outdated

						
				@ -0,0 +23,4 @@

				namespace blender::bke::volume_grid::file_cache {

				/**

				 * Get the volume grid identified by the parameters from a cache. This does load the tree data in

Hans Goudey commented

2023-12-19 01:33:44 +01:00

This does load -> This does not load?

`This does load` -> `This does not load`?

JacquesLucke marked this conversation as resolved

source/blender/blenkernel/BKE_volume_grid_file_cache.hh

						
				@ -0,0 +26,4 @@

				 * Get the volume grid identified by the parameters from a cache. This does load the tree data in

				 * grid because that is done on demand when it is accessed.

				 */

				GVolumeGrid get_grid_from_file(StringRef file_path, StringRef grid_name, int simplify_level = 0);

Hans Goudey commented

2023-12-19 01:34:35 +01:00

I guess it might be nice if this function could give an error message too, at least optionally?

EDIT: Looks like the error is stored in the grid data-- that's how it already is in main. Seems a bit odd but not a big deal.

I guess it might be nice if this function could give an error message too, at least optionally? EDIT: Looks like the error is stored in the grid data-- that's how it already is in `main`. Seems a bit odd but not a big deal.

Jacques Lucke commented

2023-12-19 23:53:50 +01:00

Yeah, one issue is that you can't really reliably know whether there is a load error in this function, because it doesn't actually load the grid.

source/blender/blenkernel/BKE_volume_grid_fwd.hh Outdated

						
				@ -0,0 +34,4 @@

				class GVolumeGrid;

				/**

				 * Same as #GVolumeGrid but makes sure that the contained grid is of a specific type.

Hans Goudey commented

2023-12-19 01:37:21 +01:00

I'd say "means" or something instead of "makes sure" which implies some sort of action (like implicit conversion or something). Even if that happened, this wouldn't be the place to mention it IMO.

JacquesLucke marked this conversation as resolved

source/blender/blenkernel/BKE_volume_render.hh Outdated

						
				@ -29,3 +29,3 @@

				bool BKE_volume_grid_dense_floats(const Volume *volume,

				                                  const VolumeGrid *volume_grid,

				                                  const blender::bke::VolumeGridData *volume_grid,

Hans Goudey commented

2023-12-19 01:52:35 +01:00

Missing forward declarations here

JacquesLucke marked this conversation as resolved

Jacques Lucke commented

2023-12-19 16:24:04 +01:00

The part I understand least now is why it's worth having implicit sharing at the VolumeGridData level when it's already implemented at the grid level with tree_sharing_info_. VolumeGridData seems very light in comparison-- not really a problem to copy. But maybe it is if we're passing it around as socket data.

It's a good point. It's definitely light in comparison to the tree, but it's also "copied" more often. I see little reason not to use implicit sharing here. A more important reason may be that it is more natural to share the grid that's loaded from a file than it is to just share the tree. Also, I don't actually know if it's common to put lots of meta data into a grid or not. There are a fair amount of allocations involved when copying a grid, even if the tree is shared (it also wouldn't fit into SocketValueVariant).

That said, almost everything is light compared to a tree haha

> The part I understand least now is why it's worth having implicit sharing at the `VolumeGridData` level when it's already implemented at the grid level with `tree_sharing_info_.` `VolumeGridData` seems very light in comparison-- not really a problem to copy. But maybe it is if we're passing it around as socket data. It's a good point. It's definitely light in comparison to the tree, but it's also "copied" more often. I see little reason not to use implicit sharing here. A more important reason may be that it is more natural to share the grid that's loaded from a file than it is to just share the tree. Also, I don't actually know if it's common to put lots of meta data into a grid or not. There are a fair amount of allocations involved when copying a grid, even if the tree is shared (it also wouldn't fit into `SocketValueVariant`). That said, almost everything is light compared to a tree haha

Hans Goudey commented

2023-12-19 16:31:47 +01:00

Okay, interesting. Sounds a bit like "since we have to allocate it anyway, and it does have some relatively non-trivial data in it, might as well make it implicitly shared". I can get behind that, since it seems like a nice goal for most allocated Blender data in general. OTOH, it would probably simplify understanding the design if it wasn't shared, but that's fine.

Jacques Lucke added 2 commits 2023-12-20 00:02:19 +01:00

67d8e320c3 Merge branch 'main' into volume-grid-refactor

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

4946e7c574 various smaller cleanups

Hans Goudey approved these changes 2023-12-20 00:05:43 +01:00

Jacques Lucke commented

2023-12-20 00:08:07 +01:00

OTOH, it would probably simplify understanding the design if it wasn't shared, but that's fine.

Maybe it would, not 100% sure. I'm especially not sure if this might become a problem with lazy-loading if we don't have this intermediate level.

That aside, I think the "since we have to allocate it anyway, and it does have some relatively non-trivial data in it, might as well make it implicitly shared" argument isn't too bad in itself either. At least as long as the API used to work with the data is safe and generally hides the sharing if possible, which I think is the case here. It's not really something one has to think about when implementing nodes for example: https://projects.blender.org/blender/blender/pulls/115270/files#diff-b343e08adb4ec2030715a70d36bd88ea641779d6

> OTOH, it would probably simplify understanding the design if it wasn't shared, but that's fine. Maybe it would, not 100% sure. I'm especially not sure if this might become a problem with lazy-loading if we don't have this intermediate level. That aside, I think the "since we have to allocate it anyway, and it does have some relatively non-trivial data in it, might as well make it implicitly shared" argument isn't too bad in itself either. At least as long as the API used to work with the data is safe and generally hides the sharing if possible, which I think is the case here. It's not really something one has to think about when implementing nodes for example: https://projects.blender.org/blender/blender/pulls/115270/files#diff-b343e08adb4ec2030715a70d36bd88ea641779d6

Jacques Lucke commented

2023-12-20 00:32:52 +01:00

@blender-bot build

Jacques Lucke merged commit a72e7a220d into main

2023-12-20 15:33:09 +01:00

Jacques Lucke referenced this issue from a commit

2023-12-20 15:33:10 +01:00

Volumes: refactor volume grid storage

Jacques Lucke deleted branch volume-grid-refactor

2023-12-20 15:33:11 +01:00

Guillermo Venegas referenced this issue from a commit

2023-12-26 22:06:35 +01:00