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

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

Restructured Volume node menu like other geometry menus. 5ef682bf67

Stub nodes for testing new grid socket. 12bd80f507

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

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.

Move ValueOrField into BKE. 999b08ca3a

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.

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

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

Added a Grid pointer to the ValueOrField struct. e9d78c65b0

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.

Use grid background value as the field value. 7f78bdfbd0

Grid wrapper class that works as ImplicitSharindMixin. 18b72fd4f3

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

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

Fixed user count cleanup when releasing the FieldValueGrid. 71375eec57

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

Use ImplicitSharingPtr for the field value grid. 8c9ad29d46

Constructor fix for ValueOrField. c837056e45

Fix linker errors due to missing FieldValueGrid CPPType definition. e0b324c959

This shouldn't be necessary.

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

Implementation of Store Grid node. a484bcd99d

Support storing grids in empty volumes. 91f0b8e20f

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

Added dilate and erode nodes. 8e1988c91b

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

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

This allows branching from a node socket to multiple inputs.

Sanity check before accessing internal grid pointer. 8cc6f785b5

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

Cleanup: Single namespace statement. a19915b67b

Cleanup: Removed unused static dispatch utility function. 02016f813e

Cleanup: Move FieldValueTreeImpl into the detail namespace. 68e5c1257d

Cleanup: Moved VolumeGrid into BKE_volume_grid.hh. bca4dffe22

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.

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

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

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

This reverts commit 021121382b.

Temp: Move VolumeGrid into bke namespace. 2f35ca5a91

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.

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

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.

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

Make VolumeGridCommon an ImplicitSharingMixing class. 0858c6992b

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.

Store ImplicitSharingPtr to grids in VolumeGridVector. b5741095cb

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

Fix unexpected data destruction by making constructor explicit. 3b3cc31141

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.

Make FieldValueGrid based on VolumeGridCommon. da5f3b725b

Rename VolumeGrid type alias to GVolumeGrid. b2c9503996

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

Renamed the FieldValueGrid and related types to VolumeGrid etc. cd9f6363ab

Another wrapper type around openvdb grid pointers. d5a98e0079

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.

Revert "Store ImplicitSharingPtr to grids in VolumeGridVector." d123afbd31

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.

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

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.

Renamed structs to VolumeGrid, VolumeGridPtr, GVolumeGridPtr. 1f5526b6b4

Cleanup: renamed leftover instances of VolumeGridSharedData. 189dfd9669

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

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

Build fixes for Volume API changes. 81f57efdc2

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

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

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

Removed redundant WITH_OPENVDB section from makesrna cmake file. c0164c1d44

Link TBB and OpenVDB against makesrna. f34032ca03

Fix circular header include for VolumeGrid. a968855a28

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

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

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

Sanity check when trying to store named grids. a1424cc7c2

More utility functions for handling grid sockets. 48391544b8

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

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

Hide new nodes behind the experimental flag for now. ab047c04d4

Fixed build errors with USD enabled. 5be0a53c86

Added some asserts to VolumeGridPtr constructors. 6de20875f9

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

Fix mismatching storage type in the Sample Grid node. a1c879a0c8

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

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

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

Simplified converter templates for grid values. c2e55e2950

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

Renamed BKE_volume_types.hh to BKE_volume_enums.hh. d180cf731a

Update description of sample grid node. 9a0aa23729

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

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.

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

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

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

Fixed empty grid constructor. 88f30df8b6

Handle empty volume bounds return value in drawing code. f1566e278b

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

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

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

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

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

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

Hide values on grid input sockets. bc591770c4

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

Cleanup: formatting. 37eac000e6

Move some internal includes to the source file. d4315578c2

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

Remove copy constructor from non-copyable VolumeGrid. df0a15212b

Mention concrete subclasses of VolumeGridPtrCommon. bad819c4d6

Remove virtual destructor for VolumeGridPtrCommon. 22875e3b05

Updated description. a10bdafb71

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

Call tag_ensured_mutable after checking for mutable grid data. 6caba1d201

Removed the unused make_implicit_sharing_ptr utility function. 6a4b095f33

Set output values before removing grid from volume. 2b62f77202

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

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

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

Remove unnecessary check after BKE_volume_grid_get_for_write.

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

Details

4ea0196b6f

Add back missing destructor for VolumeGridPtrCommon. 1068df47e9

Fix Store Named Grid node with mutable VolumeGrid. 0bc33f3f88

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.

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

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

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

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

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

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

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

grid_for_write accessor in VolumeGridPtr can be const. 348a469b88

Change friend class declaration to struct. 6818919bb2

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

Cleanup: formatting. 868bc66fd6

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

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

Remove potential grid copy in VolumeGridPtr accessors. 7af2717b1c

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.

Return grid pointer directly when extracting grid inputs. 0c788585ca

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.

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

Reimplemented the file cache for volume grids. 869cf6bb30

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.

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

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

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

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

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.

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

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

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.

Merge branch 'main' into volume-grid-sockets 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

extract grid type traits to separate file 0f9331fb48

initial new api c6bee66e40

add initial file cache 4bcf283c26

remove old api 9c2d95b3eb

progress 605c350a61

support preloading meta-data 04e59efd84

cache grids in file d29656f7ae

progress 7f2ae9d938

support unloading tree data 873ca6db97

progress 07864a1561

progress c790ba1e64

fix e2d8611b0d

add forward declaration header f145c5b8c2

progress 43a07718d3

first successful compilation e7480faeff

fixes fdf11e4809

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

fixes for new socket storage dcf458dfcf

move some functions to forward declaration header 9152f1e296

cleanup dealing with grid transforms 6125554b50

progress 6776ac7866

cleanup naming a5e486a4f0

cleanup 69d1908b97

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

remove include ba49aebbbe

fix leak 82e7725ceb

don't unload tree after computing bounding box 1a5e1e12d1

remove outdated comment 11bb535631

cleanup 1e0c5742cb

fix wrong transform with simplified grids cd14f8b1d4

improve file name 4a42a6d858

unload unused grids when volume is deleted 4e41558182

use simplify level in first evaluation after blend file load db0c019b11

comments 6312123261

cleanup 14ade7fcf8

comments cd01dbad0d

progress 3ef227f98f

comments 635d5f9a73

cleanup naming 9a176d5764

comments 1107cf0034

comment a8aea749d3

progress 7dfa5a1a54

progress d2a8a1d98a

cleanup socket declaration e486c48053

progress d84d7be6bc

progress b648c4144d

progress cfee633887

progress a21457cb6f

hide value on grid sockets e1df224b08

fixes 7fa26a9f05

fix compilation without openvdb 4d6dc189aa

add tests 94c4899ef1

lazy load test ed25bbebfa

cleanup ace4ef1191

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

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

geometry nodes specific parts 79ee59efc6

cleanup

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

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

fix unit test with asserts enabled

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

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

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

various smaller cleanups

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

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