Geometry Nodes: initial Volume Grid socket support #115270

Merged
Jacques Lucke merged 193 commits from LukasTonne/blender:volume-grid-sockets into main 2023-12-20 22:33:26 +01:00
Member

This is the initial implementation for the volume grid socket that has been discussed during the November 2023 geometry nodes workshop.

It adds initial support for passing volume grids around in sockets. Furthermore, it adds two new nodes. Both are initially hidden under the "New Volume Nodes" experimental option until we have a few mode nodes.

  • Get Named Grid: Gets or extracts a volume grid from a volume geometry based on the grid's name.
  • Store Named Grid: Puts a volume grid back into a volume with a name.

image

SocketValueVariant is extended to support grids besides single values and fields.

Next steps:

  • Implement grid socket shape and inferencing (currently, they just look like single values).
  • Add implicit conversions between grid types.
  • Implement nodes that operate on the grids (#116021).
  • Improved spreadsheet and viewer support.
This is the initial implementation for the [volume grid socket](https://devtalk.blender.org/t/volumes-in-geometry-nodes-proposal/31917) that has been discussed during the [November 2023 geometry nodes workshop](https://devtalk.blender.org/t/2023-11-06-geometry-nodes-workshop-notes/32007#volumes-3). It adds initial support for passing volume grids around in sockets. Furthermore, it adds two new nodes. Both are initially hidden under the "New Volume Nodes" experimental option until we have a few mode nodes. * **Get Named Grid**: Gets or extracts a volume grid from a volume geometry based on the grid's name. * **Store Named Grid**: Puts a volume grid back into a volume with a name. ![image](/attachments/f099c8dd-61fa-4e5f-9ad1-21918ff5499b) `SocketValueVariant` is extended to support grids besides single values and fields. Next steps: * Implement grid socket shape and inferencing (currently, they just look like single values). * Add implicit conversions between grid types. * Implement nodes that operate on the grids (#116021). * Improved spreadsheet and viewer support.
Lukas Tönne added 25 commits 2023-11-22 15:15:23 +01:00
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.
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.
9f45d22858 Use implicit sharing to create deep copies of grids when needed.
This allows branching from a node socket to multiple inputs.
Lukas Tönne added 4 commits 2023-11-22 16:06:11 +01:00
Lukas Tönne added 4 commits 2023-11-22 18:07:56 +01:00
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.
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.
Lukas Tönne added 1 commit 2023-11-23 10:02:27 +01:00
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.
Lukas Tönne added 1 commit 2023-11-23 11:11:45 +01:00
Lukas Tönne added 1 commit 2023-11-23 11:29:13 +01:00
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.
Lukas Tönne added 1 commit 2023-11-23 12:41:38 +01:00
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.
Lukas Tönne added 1 commit 2023-11-23 15:00:36 +01:00
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.
Lukas Tönne added 1 commit 2023-11-23 15:51:49 +01:00
Lukas Tönne added 1 commit 2023-11-23 15:58:43 +01:00
b2c9503996 Rename VolumeGrid type alias to GVolumeGrid.
This is to avoid name conflicts once the FieldValueGrid gets renamed.
Lukas Tönne added 1 commit 2023-11-23 16:12:09 +01:00
Lukas Tönne added 5 commits 2023-11-24 16:12:07 +01:00
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.
Lukas Tönne added 1 commit 2023-11-24 16:14:21 +01:00
Lukas Tönne added 1 commit 2023-11-24 18:04:16 +01:00
Lukas Tönne added 1 commit 2023-11-25 17:16:01 +01:00
Lukas Tönne added 2 commits 2023-11-26 12:07:12 +01:00
Lukas Tönne added 1 commit 2023-11-27 10:24:12 +01:00
Lukas Tönne added 1 commit 2023-11-27 11:22:13 +01:00
Lukas Tönne added 1 commit 2023-11-27 11:51:16 +01:00
Lukas Tönne force-pushed volume-grid-sockets from 7009f8cb21 to f34032ca03 2023-11-27 12:23:22 +01:00 Compare
Lukas Tönne added 1 commit 2023-11-27 12:32:23 +01:00
Lukas Tönne added 1 commit 2023-11-27 13:09:42 +01:00
Lukas Tönne added 1 commit 2023-11-27 15:54:33 +01:00
Lukas Tönne added a new dependency 2023-11-27 17:34:37 +01:00
Lukas Tönne added 1 commit 2023-11-27 18:25:39 +01:00
Lukas Tönne added 1 commit 2023-11-27 19:40:22 +01:00
Lukas Tönne added 1 commit 2023-11-28 10:36:15 +01:00
Lukas Tönne added 1 commit 2023-11-29 12:06:37 +01:00
Lukas Tönne added 1 commit 2023-11-29 12:16:26 +01:00
Lukas Tönne added 1 commit 2023-11-29 12:37:43 +01:00
Lukas Tönne changed title from WIP: Volume Grid Sockets to Volume Grid Sockets 2023-11-29 12:38:30 +01:00
Lukas Tönne requested review from Jacques Lucke 2023-11-29 13:02:46 +01:00
Lukas Tönne requested review from Hans Goudey 2023-11-29 13:02:53 +01:00
Lukas Tönne added this to the Nodes & Physics project 2023-11-29 13:02:59 +01:00
Lukas Tönne added 1 commit 2023-11-29 14:35:39 +01:00
Lukas Tönne added 1 commit 2023-11-29 16:18:44 +01:00
Lukas Tönne added 1 commit 2023-11-29 16:41:00 +01:00
Lukas Tönne added 1 commit 2023-11-29 21:58:19 +01:00
Lukas Tönne added 1 commit 2023-11-30 08:21:41 +01:00
Lukas Tönne added 1 commit 2023-11-30 12:24:47 +01:00
Hans Goudey reviewed 2023-12-01 06:24:57 +01:00
Hans Goudey left a comment
Member

I didn't look at the node implementations yet, or compile and test this. But from what I read, the code looks great. I got the feeling the conversion code could be a bit simpler, but I'd probably have to spend more time looking at that too. For now I just left a few small comments.

I didn't look at the node implementations yet, or compile and test this. But from what I read, the code looks great. I got the feeling the conversion code could be a bit simpler, but I'd probably have to spend more time looking at that too. For now I just left a few small comments.
@ -0,0 +59,4 @@
};
template<typename FieldValueType, typename GridValueType, typename LeafBufferValueType>
struct GridConverter_CopyConstruct {
Member

This seems to be unused. Anyway, I'm a bit unsure about the goals here or if this is used in this PR. I'd hope this could be structured a bit more similarly to our attribute_convert.hh header, which is a relatively common pattern when converting to formats from other libraries.

FieldValue (assuming it's referring to fn::Field) seems like a bit of a misnomer, there is no type system specific to fields.

This seems to be unused. Anyway, I'm a bit unsure about the goals here or if this is used in this PR. I'd hope this could be structured a bit more similarly to our `attribute_convert.hh` header, which is a relatively common pattern when converting to formats from other libraries. `FieldValue` (assuming it's referring to `fn::Field`) seems like a bit of a misnomer, there is no type system specific to fields.
Author
Member

I've cleaned this up a bit now (these converters have gone through multiple iterations in different branches).
Converters are in blender::bke::grids namespace now to avoid possible conflicts with the existing AttributeConverter. I have two separate converter classes for the Blender->OpenVDB (grids::AttributeConverter) and the OpenVDB->Blender direction (grids::GridConverter). I'm using "AttributeType" here to refer to Blender types (CPPType, customdata, sockets) even though grids aren't actually attributes. Have to call it something ...

I've cleaned this up a bit now (these converters have gone through multiple iterations in different branches). Converters are in `blender::bke::grids` namespace now to avoid possible conflicts with the existing `AttributeConverter`. I have two separate converter classes for the Blender->OpenVDB (`grids::AttributeConverter`) and the OpenVDB->Blender direction (`grids::GridConverter`). I'm using "AttributeType" here to refer to Blender types (CPPType, customdata, sockets) even though grids aren't actually attributes. Have to call it _something_ ...
LukasTonne marked this conversation as resolved
@ -0,0 +275,4 @@
namespace grid_utils {
template<typename T> bool get_background_value(const VolumeGridPtr<T> &grid, T &r_value)
Member

std::optional<T>?

`std::optional<T>`?
LukasTonne marked this conversation as resolved
@ -0,0 +8,4 @@
* \ingroup bli
*/
enum VolumeGridType : int8_t {
Member

Maybe BKE_volume_enums.hh makes more sense. "Types" seems a bit redundant with BKE_volume_grid.hh

Maybe `BKE_volume_enums.hh` makes more sense. "Types" seems a bit redundant with `BKE_volume_grid.hh`
LukasTonne marked this conversation as resolved
@ -412,6 +415,7 @@ DefNode(GeometryNode, GEO_NODE_RESAMPLE_CURVE, 0, "RESAMPLE_CURVE", ResampleCurv
DefNode(GeometryNode, GEO_NODE_REVERSE_CURVE, 0, "REVERSE_CURVE", ReverseCurve, "Reverse Curve", "Change the direction of curves by swapping their start and end data")
DefNode(GeometryNode, GEO_NODE_ROTATE_INSTANCES, 0, "ROTATE_INSTANCES", RotateInstances, "Rotate Instances", "Rotate geometry instances in local or global space")
DefNode(GeometryNode, GEO_NODE_SAMPLE_CURVE, def_geo_curve_sample, "SAMPLE_CURVE", SampleCurve, "Sample Curve", "Retrieve data from a point on a curve at a certain distance from its start")
DefNode(GeometryNode, GEO_NODE_SAMPLE_GRID, 0, "SAMPLE_GRID", SampleGrid, "Sample Grid", "Sample a volume grid value")
Member

Sample values from a volume grid at positions is a bit more specific I suppose

`Sample values from a volume grid at positions` is a bit more specific I suppose
LukasTonne marked this conversation as resolved
Jacques Lucke requested changes 2023-12-01 10:19:39 +01:00
Jacques Lucke left a comment
Member
  • Trying to insert the Dilate Grid node here results in a crash in try_dilate_grid at BLI_assert(grid);.
    image
* [x] Trying to insert the `Dilate Grid` node here results in a crash in `try_dilate_grid` at `BLI_assert(grid);`. ![image](/attachments/0b1a55e7-b4f8-4270-97c0-f02065e6c6e9)
@ -51,2 +70,4 @@
return fn::evaluate_constant_field(this->field);
}
if (this->grid) {
/* Returns the grid background value. */
Member

I think we can just always return the default value (0) here when there is a grid. This function mainly exists because sometimes single values are stored as constant fields, but we shouldn't do that with grids.

I think we can just always return the default value (`0`) here when there is a grid. This function mainly exists because sometimes single values are stored as constant fields, but we shouldn't do that with grids.
LukasTonne marked this conversation as resolved
@ -73,2 +80,2 @@
VolumeGrid *BKE_volume_grid_get_for_write(Volume *volume, int grid_index);
const VolumeGrid *BKE_volume_grid_active_get_for_read(const Volume *volume);
GVolumeGridPtr BKE_volume_grid_get_for_read(const Volume *volume, int grid_index);
GVolumeGridPtr BKE_volume_grid_get_for_write(Volume *volume, int grid_index);
Member

I think this will probably have to keep returning VolumeGrid * or VolumeGrid &. Returning GVolumeGridPtr is problematic, because that increases the user count (unless I'm missing something), which would make the grid immutable.
Note how e.g. GeometrySet::get_component returns a raw pointer instead of a GeometryComponentPtr.

I think this will probably have to keep returning `VolumeGrid *` or `VolumeGrid &`. Returning `GVolumeGridPtr` is problematic, because that increases the user count (unless I'm missing something), which would make the grid immutable. Note how e.g. `GeometrySet::get_component` returns a raw pointer instead of a `GeometryComponentPtr`.
Author
Member

API is back to returning VolumeGrid *.

API is back to returning `VolumeGrid *`.
LukasTonne marked this conversation as resolved
@ -0,0 +45,4 @@
* Procedurally generated grids are not.
* \{ */
struct GVolumeGridPtr : public VolumeGridPtrCommon {
Member

I'm not entirely convinced by the need for GVolumeGridPtr and VolumeGridPtr yet, instead of just using ImplicitSharingPtr<VolumeGrid>.

I'm not entirely convinced by the need for `GVolumeGridPtr` and `VolumeGridPtr` yet, instead of just using `ImplicitSharingPtr<VolumeGrid>`.
Author
Member

We could have a single (untemplated) VolumeGridPtr and then only access the internal OpenVDB grid as typed:
get_grid() -> shared_ptr<openvdb::GridBase>
get_grid<T>() -> shared_ptr<VolumeGridType<T>>

However, having a typed wrapper is quite useful since we are storing it inside ValueOrField<T> with a known type. The field is also a typed wrapper around a generic shared_ptr<FieldNode>, with the generic GField variant.

We could have a single (untemplated) `VolumeGridPtr` and then only access the internal OpenVDB grid as typed: `get_grid() -> shared_ptr<openvdb::GridBase>` `get_grid<T>() -> shared_ptr<VolumeGridType<T>>` However, having a typed wrapper is quite useful since we are storing it inside `ValueOrField<T>` with a known type. The `field` is also a typed wrapper around a generic `shared_ptr<FieldNode>`, with the generic `GField` variant.
Member

I think the thin typed wrapper we have no is ok.

I think the thin typed wrapper we have no is ok.
JacquesLucke marked this conversation as resolved
@ -0,0 +72,4 @@
}
}
params.set_default_remaining_outputs();
Member

The behavior isn't quite right here. The output geometry should always be the same as the input geometry, except that it might have one grid less.

The behavior isn't quite right here. The output geometry should always be the same as the input geometry, except that it might have one grid less.
LukasTonne marked this conversation as resolved
@ -0,0 +27,4 @@
b.add_input<decl::Geometry>("Volume");
b.add_input<decl::String>("Name");
grids::declare_grid_type_input(b, eCustomDataType(node->custom1), "Grid");
Member

Use hide_value for the grid input. This input only works when you actually connect a grid.

Use `hide_value` for the grid input. This input only works when you actually connect a grid.
LukasTonne marked this conversation as resolved
@ -0,0 +68,4 @@
if (volume) {
bke::GVolumeGridPtr grid = grids::extract_grid_input(params, "Grid", data_type);
if (!grid) {
return;
Member

This forgets to set the output.

This forgets to set the output.
LukasTonne marked this conversation as resolved
@ -0,0 +82,4 @@
return;
}
params.set_default_remaining_outputs();
Member

Note that here we also want that the output is generally the same as the input, but potentially with one extra/replaced grid.

Note that here we also want that the output is generally the same as the input, but potentially with one extra/replaced grid.
LukasTonne marked this conversation as resolved
Lukas Tönne added 2 commits 2023-12-01 11:39:43 +01:00
Lukas Tönne added 1 commit 2023-12-01 11:48:51 +01:00
Lukas Tönne added 1 commit 2023-12-01 11:53:03 +01:00
Lukas Tönne added 1 commit 2023-12-01 11:54:19 +01:00
Lukas Tönne added 1 commit 2023-12-01 12:18:23 +01:00
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.
Lukas Tönne added 3 commits 2023-12-01 15:46:05 +01:00
Lukas Tönne added 1 commit 2023-12-03 09:31:35 +01:00
Lukas Tönne added 1 commit 2023-12-03 09:52:27 +01:00
Lukas Tönne added 1 commit 2023-12-03 10:12:00 +01:00
Lukas Tönne added 1 commit 2023-12-03 13:23:22 +01:00
Lukas Tönne added 1 commit 2023-12-04 11:13:27 +01:00
Lukas Tönne added 1 commit 2023-12-04 12:09:57 +01:00
Lukas Tönne added 3 commits 2023-12-04 12:38:20 +01:00
Lukas Tönne added 2 commits 2023-12-04 12:39:44 +01:00
Jacques Lucke requested changes 2023-12-04 13:47:40 +01:00
@ -0,0 +9,4 @@
*/
#include <mutex>
#include <unordered_set>
Member

Seems unused.

Seems unused.
LukasTonne marked this conversation as resolved
@ -0,0 +39,4 @@
#ifdef WITH_OPENVDB
namespace grids {
template<typename T> struct AttributeConverter {
Member

Maybe call this TypeConverter with two functions: to_blender and to_openvdb. That might make the purpose more clear.

I was confused at first that there is AttributeConverter and GridConverter. It wasn't obvious that those are just opposites of each other.

Maybe call this `TypeConverter` with two functions: `to_blender` and `to_openvdb`. That might make the purpose more clear. I was confused at first that there is `AttributeConverter` and `GridConverter`. It wasn't obvious that those are just opposites of each other.
Author
Member

Yeah that's what i had before

Yeah that's what i had before
LukasTonne marked this conversation as resolved
@ -0,0 +147,4 @@
VolumeGrid(const char *template_file_path,
const GridBasePtr &template_grid,
int simplify_level = 0);
VolumeGrid(const VolumeGrid &other);
Member

This copy constructor seems confusing, since it's not actually making a copy of the grid.

This copy constructor seems confusing, since it's not actually making a copy of the grid.
Author
Member

Leftover from old code ... VolumeGrid is actually non-copyable now, so trying to use this constructor would cause an error anyway. Removed.

Leftover from old code ... `VolumeGrid` is actually non-copyable now, so trying to use this constructor would cause an error anyway. Removed.
LukasTonne marked this conversation as resolved
@ -0,0 +150,4 @@
VolumeGrid(const VolumeGrid &other);
~VolumeGrid();
const char *name() const;
Member

Seems ok for now, but eventually we shouldn't rely on this anymore because it hinders reusability of grids.

Seems ok for now, but eventually we shouldn't rely on this anymore because it hinders reusability of grids.
Author
Member

Old code, i suggest we clean this up when we know how to handle the cache and simplified grids etc.

Old code, i suggest we clean this up when we know how to handle the cache and simplified grids etc.
Author
Member

it hinders reusability of grids

Can you explain this some more? The code comment says it's just a wrapper around grid metadata to avoid a string copy. It's only used for logging and when loading a vdb file. That won't work if the grid name does not exist in the file it's associated with, so renaming a grid after loading can "break" in that sense. But i don't see how re-using the grid itself would be an issue.

IIRC we wanted to (eventually) store a name/pointer map in Volume and only use the grid name for debugging purposes.

> it hinders reusability of grids Can you explain this some more? The code comment says it's just a wrapper around grid metadata to avoid a string copy. It's only used for logging and when loading a vdb file. That won't work if the grid name does not exist in the file it's associated with, so renaming a grid after loading can "break" in that sense. But i don't see how re-using the grid itself would be an issue. IIRC we wanted to (eventually) store a name/pointer map in Volume and only use the grid name for debugging purposes.
Member

IIRC we wanted to (eventually) store a name/pointer map in Volume and only use the grid name for debugging purposes.

That's exactly the point I wanted to make. No need to change anything here now.

> IIRC we wanted to (eventually) store a name/pointer map in Volume and only use the grid name for debugging purposes. That's exactly the point I wanted to make. No need to change anything here now.
JacquesLucke marked this conversation as resolved
@ -0,0 +152,4 @@
const char *name() const;
const char *error_message() const;
Member

This could use some comment.

This could use some comment.
JacquesLucke marked this conversation as resolved
@ -0,0 +156,4 @@
bool grid_is_loaded() const;
void load(const char *volume_name, const char *filepath) const;
Member

Feels a bit weird that this takes the filepath but is still const. Not sure how this is used currently.

Feels a bit weird that this takes the filepath but is still `const`. Not sure how this is used currently.
Author
Member

This is basically an internal function, it only gets called by other API functions BKE_volume_grid_load and BKE_volume_grid_openvdb_for_write (basically a "make local" that loads the grid and then makes a deep copy in duplicate_reference). The filepath argument is always the grids.filepath passed down from the container.

The function can be const because it only affects the cache entry and the mutable is_loaded_ flag. Should probably make this protected with a friend class or something, and/or maybe move the actual loading to the cache entry.

This is basically an internal function, it only gets called by other API functions `BKE_volume_grid_load` and `BKE_volume_grid_openvdb_for_write` (basically a "make local" that loads the grid and then makes a deep copy in `duplicate_reference`). The `filepath` argument is always the `grids.filepath` passed down from the container. The function can be const because it only affects the cache entry and the mutable `is_loaded_` flag. Should probably make this protected with a friend class or something, and/or maybe move the actual loading to the cache entry.
Member

Just mentioning that in a comment for now is fine.

Just mentioning that in a comment for now is fine.
JacquesLucke marked this conversation as resolved
@ -0,0 +161,4 @@
void set_simplify_level(int simplify_level);
void clear_reference(const char *volume_name);
Member

This stuff shouldn't really be necessary with implicit-sharing anymore.

This stuff shouldn't really be necessary with implicit-sharing anymore.
Author
Member

Agreed.

Currently:

  • In VolumeGridVector we use implicit sharing with VolumeGrid. Each openvdb::GridBase::Ptr (std::shared_ptr) should always have exactly 1 user. Except ...
  • VolumeGrid also potentially has a cache entry, which uses the same openvdb::GridBase::Ptr. So volumes in bmain can have multiple users of the shared_ptr, while transient GN output grids never have a file cache and never more than 1 user.

What could work:

  • VolumeFileCacheEntry points to VolumeGrid (implicit sharing data) instead of the openvdb::GridBase::Ptr.
  • VolumeFileCacheEntry only adds a weak user to the VolumeGrid. This ensures that cache entries can be freed when nothing uses the grid any more.
  • Grids in a Volume in bmain can now be shared with other Volumes when loading the same file.

The explicit user counting in VolumeFileCacheEntry could then be removed, relying on the implicit sharing user count instead. However, the cache entry distinguishes between "metadata users" and "tree users". I'm not sure how important this distinction is and if it ever becomes relevant in practice.

Agreed. Currently: - In `VolumeGridVector` we use implicit sharing with `VolumeGrid`. Each `openvdb::GridBase::Ptr` (`std::shared_ptr`) should always have exactly 1 user. _Except ..._ - `VolumeGrid` also potentially has a cache entry, which uses the same `openvdb::GridBase::Ptr`. So volumes in bmain can have multiple users of the `shared_ptr`, while transient GN output grids never have a file cache and never more than 1 user. What could work: - `VolumeFileCacheEntry` points to `VolumeGrid` (implicit sharing data) instead of the `openvdb::GridBase::Ptr`. - `VolumeFileCacheEntry` only adds a __weak user__ to the `VolumeGrid`. This ensures that cache entries can be freed when nothing uses the grid any more. - Grids in a Volume in bmain can now be shared with other Volumes when loading the same file. The explicit user counting in `VolumeFileCacheEntry` could then be removed, relying on the implicit sharing user count instead. However, the cache entry distinguishes between "metadata users" and "tree users". I'm not sure how important this distinction is and if it ever becomes relevant in practice.
Author
Member

It looks like tree and metadata users are mutually exclusive, so this can probably be simplified (confirmed with Brecht)

  • Creating a cache entry (from VolumeGrid constructors) starts out with 1 metadata user.
  • Destroying a VolumeGrid removes a tree user if the grid is loaded and a metadata user otherwise.
  • copy_user is unused (cache entries are never actually copied).
  • Loading a cache entry changes from metadata user to tree user.
  • Unloading a cache entry changes from tree user to metadata user.

If a cached grid is fully unloaded (zero tree users) but still has un-loaded grids referencing it (1+ metadata users), the openvdb grid is replaced with an empty grid.

It gets a bit weird now because the entry is referenced by the VolumeGrid in Volume but also references the VolumeGrid in turn, rather than both sharing the lower-level openvdb grid pointer.

It looks like tree and metadata users are mutually exclusive, so this can probably be simplified (confirmed with Brecht) - Creating a cache entry (from `VolumeGrid` constructors) starts out with 1 metadata user. - Destroying a `VolumeGrid` removes a tree user _if the grid is loaded_ and a metadata user otherwise. - `copy_user` is unused (cache entries are never actually copied). - Loading a cache entry changes from metadata user to tree user. - Unloading a cache entry changes from tree user to metadata user. If a cached grid is fully unloaded (zero tree users) but still has un-loaded grids referencing it (1+ metadata users), the openvdb grid is replaced with an empty grid. It gets a bit weird now because the entry is referenced by the `VolumeGrid` in `Volume` but also references the `VolumeGrid` in turn, rather than both sharing the lower-level openvdb grid pointer.
Author
Member

I think the VolumeGrid::entry_ pointer isn't really necessary, it's just set in the constructor and never changed. We could just do a lookup whenever the cache entry associated with a VolumeGrid is required, without the VolumeGrid itself having a dependency. The functions accessing the cache entry are only called infrequently.

I think the `VolumeGrid::entry_` pointer isn't really necessary, it's just set in the constructor and never changed. We could just do a lookup whenever the cache entry associated with a `VolumeGrid` is required, without the `VolumeGrid` itself having a dependency. The functions accessing the cache entry are only called infrequently.
Member

Currently, it feels like the global cache for meta data and actual voxel data could be separate. I'm not even sure if the meta data really needs a cache at all (I'd expect it to be very cheap to load compared to other stuff that is loaded).. Or it could be cached independently.

VolumeFileCacheEntry only adds a weak user to the VolumeGrid.

In theory, that's a nice idea. Unfortunately, weak users don't quite work the same way with implicit sharing as with std::shared_ptr as is mentioned in the ImplicitSharingInfo::add_weak_user() comment. A weak user can not be turned into a strong user and it also does not protect against changes to the underlying data.

Currently, it feels like that additional logic would be necessary in remove_user_and_delete_if_last and the *_for_write methods would be necessary to get the same behavior from before.

To me personally, it still seems like it would be easier to design the caching mechanism from scratch given the new environment and to start out with a clean state.

Currently, it feels like the global cache for meta data and actual voxel data could be separate. I'm not even sure if the meta data really needs a cache at all (I'd expect it to be very cheap to load compared to other stuff that is loaded).. Or it could be cached independently. > `VolumeFileCacheEntry` only adds a weak user to the `VolumeGrid`. In theory, that's a nice idea. Unfortunately, weak users don't quite work the same way with implicit sharing as with `std::shared_ptr` as is mentioned in the `ImplicitSharingInfo::add_weak_user()` comment. A weak user can not be turned into a strong user and it also does not protect against changes to the underlying data. Currently, it feels like that additional logic would be necessary in `remove_user_and_delete_if_last` and the `*_for_write` methods would be necessary to get the same behavior from before. To me personally, it still seems like it would be easier to design the caching mechanism from scratch given the new environment and to start out with a clean state.
Author
Member

Ok, to summarize the discussion:

  • VolumeGrid is the shared data, previously VolumeFileCacheEntry had this job.
  • To have per-instance loading state and simplification level we would need a VolumeGridInstance struct, but want to avoid this complication.
  • Global vdb file cache VolumeFileCache also stores VolumeGrid but uses the implicit sharing refcount instead of the tree_users and metadata_users.
  • All instances of a volume will share the same loading state: once a grid is loaded all instances will use the tree data.
  • Automatic unloading of grids is removed. This would require reference-counting of loaded grid instances, each instance would have to keep tracking of it "loaded" state.
  • Simplification levels can be cached in VolumeGrid, but selecting the level happens on-the-fly using scene setting. This is a drawing/rendering feature so should not be relevant to generated grids.
Ok, to summarize the discussion: - `VolumeGrid` is the shared data, previously `VolumeFileCacheEntry` had this job. - To have per-instance loading state and simplification level we would need a `VolumeGridInstance` struct, but want to avoid this complication. - Global vdb file cache `VolumeFileCache` also stores `VolumeGrid` but uses the implicit sharing refcount instead of the `tree_users` and `metadata_users`. - All instances of a volume will share the same loading state: once a grid is loaded all instances will use the tree data. - Automatic unloading of grids is removed. This would require reference-counting of loaded grid instances, each instance would have to keep tracking of it "loaded" state. - Simplification levels can be cached in `VolumeGrid`, but selecting the level happens on-the-fly using scene setting. This is a drawing/rendering feature so should not be relevant to generated grids.
JacquesLucke marked this conversation as resolved
@ -0,0 +23,4 @@
/* -------------------------------------------------------------------- */
/** \name Common Grid Wrapper
*
* Base class for both generic and typed grid wrapper classes.
Member

Base class for #GVolumeGridPtr and #VolumeGridPtr.

`Base class for #GVolumeGridPtr and #VolumeGridPtr.`
LukasTonne marked this conversation as resolved
@ -0,0 +28,4 @@
struct VolumeGridPtrCommon : ImplicitSharingPtr<VolumeGrid> {
#ifdef WITH_OPENVDB
using GridPtr = std::shared_ptr<openvdb::GridBase>;
Member

Both are unused I think.

Both are unused I think.
LukasTonne marked this conversation as resolved
@ -0,0 +37,4 @@
/* Enable implicit conversion from nullptr. */
VolumeGridPtrCommon(std::nullptr_t) : ImplicitSharingPtr<VolumeGrid>(nullptr) {}
using ImplicitSharingPtr<VolumeGrid>::ImplicitSharingPtr;
virtual ~VolumeGridPtrCommon();
Member

This class doesn't need a vtable afaik.

This class doesn't need a vtable afaik.
LukasTonne marked this conversation as resolved
@ -0,0 +45,4 @@
/* -------------------------------------------------------------------- */
/** \name Generic Volume Grid
*
* Wrapper around a generic OpenVDB grid.
Member

A slightly better one-sentence-description might be: Owning pointer to a #VolumeGrid.

A slightly better one-sentence-description might be: `Owning pointer to a #VolumeGrid.`
LukasTonne marked this conversation as resolved
@ -0,0 +46,4 @@
/** \name Generic Volume Grid
*
* Wrapper around a generic OpenVDB grid.
* Grids loaded from OpenVDB files are always stored in the global cache.
Member

This information does not seem to belong here.

This information does not seem to belong here.
LukasTonne marked this conversation as resolved
@ -0,0 +142,4 @@
};
/* Stub class for string attributes, not supported. */
template<> struct VolumeGridTraits<std::string> {
using TreeType = openvdb::MaskTree;
Member

Should probably use void instead of openvdb::MaskTree. Other code has to handle this case in some way anyway.

Should probably use `void` instead of `openvdb::MaskTree`. Other code has to handle this case in some way anyway.
LukasTonne marked this conversation as resolved
@ -0,0 +159,4 @@
using GridPtr = std::shared_ptr<GridType>;
using GridConstPtr = std::shared_ptr<const GridType>;
VolumeGridPtr(const GVolumeGridPtr &other) : VolumeGridPtrCommon(other.get()) {}
Member

Is this constructor necessary?
It also seems to have reference count issue: it creates a new owning pointer without increasing the user count.

Is this constructor necessary? It also seems to have reference count issue: it creates a new owning pointer without increasing the user count.
Author
Member

This allows casting a type VolumeGridPtr<T> to GVolumeGridPtr. VArray/GVArray works the same way, while Field/GField uses inheritance (Field is a GField). Probably both could work here, although i like to not use inheritance when the conversion is the only thing needed here. refcount already fixed.

This allows casting a type `VolumeGridPtr<T>` to `GVolumeGridPtr`. VArray/GVArray works the same way, while Field/GField uses inheritance (Field<T> is a GField). Probably both could work here, although i like to not use inheritance when the conversion is the only thing needed here. refcount already fixed.
Member

Ah ok, that's fine then. I still don't see how you fixed the refcount though.

Ah ok, that's fine then. I still don't see how you fixed the refcount though.
Author
Member
   if (*this) {
      this->get()->add_user();
    }
    return VolumeGridPtr<T>(*this);
``` if (*this) { this->get()->add_user(); } return VolumeGridPtr<T>(*this); ```
Member

I see, I think that's the wrong solution. This constructor should add the user instead, otherwise the automatic reference counting is kinda broken.

This should not result in a memory bug, because one is just dealing with managed pointers. Haven't tried, but right now this probably results in a use-after-free on destruction.

GVolumeGridPtr grid = get_grid(...);
VolumeGridPtr<float> float_grid = grid;
I see, I think that's the wrong solution. This constructor should add the user instead, otherwise the automatic reference counting is kinda broken. This should not result in a memory bug, because one is just dealing with managed pointers. Haven't tried, but right now this probably results in a use-after-free on destruction. ``` GVolumeGridPtr grid = get_grid(...); VolumeGridPtr<float> float_grid = grid; ```
Author
Member

Not sure what you mean, the snippet above is in the constructor.

Not sure what you mean, the snippet above is in the constructor.
Member

Not sure if I'm missing something. It looks like it is in the typed() method and not in the constructor, but I think it should be in the constructor. Maybe the constructor should also be private so that one is forced to call typed()?

Not sure if I'm missing something. It looks like it is in the `typed()` method and not in the constructor, but I think it should be in the constructor. Maybe the constructor should also be private so that one is forced to call `typed()`?
Author
Member

Oh sorry, i was getting confused by the cast direction. Moved this to the constructor.

Maybe the constructor should also be private so that one is forced to call typed()?

Yes the constructor should be private, mostly because there is no good way to handle errors in case of type mismatch. The typed<T>() function can return null at least.

Oh sorry, i was getting confused by the cast direction. Moved this to the constructor. > Maybe the constructor should also be private so that one is forced to call typed()? Yes the constructor should be private, mostly because there is no good way to handle errors in case of type mismatch. The `typed<T>()` function can return null at least.
LukasTonne marked this conversation as resolved
@ -0,0 +170,4 @@
GridPtr grid_for_write() const
{
const VolumeGrid *data = this->get();
if (data->is_mutable()) {
Member

Call tag_ensured_mutable. In theory that could be part of is_mutable, but it's not because I don't want it to be called by e.g. asserts that check mutability.

Call `tag_ensured_mutable`. In theory that could be part of `is_mutable`, but it's not because I don't want it to be called by e.g. asserts that check mutability.
LukasTonne marked this conversation as resolved
@ -0,0 +173,4 @@
if (data->is_mutable()) {
return openvdb::GridBase::grid<GridType>(const_cast<VolumeGrid *>(data)->grid_for_write());
}
return openvdb::GridBase::grid<GridType>(data->grid()->deepCopyGrid());
Member

I think it would be expected that this creates a new VolumeGrid and changes this to point to the new grid. This is how the *_for_write methods work elsewhere.
This also applies to the untyped version of this function.

I think it would be expected that this creates a new `VolumeGrid` and changes `this` to point to the new grid. This is how the `*_for_write` methods work elsewhere. This also applies to the untyped version of this function.
Author
Member

That would be the case for getting a VolumeGrid from Volume, but not for accessing the openvdb::GridBase from VolumeGrid. I don't know of other cases of implicit sharing data changing their data in this way. This function would be comparable to e.g. CurveComponent::get_for_write, where it asserts that the ImplicitSharingData itself is mutable and then returns a mutable pointer to the internals.

56be404b0f/source/blender/blenkernel/intern/geometry_component_curves.cc (L84-L97)

That would be the case for getting a `VolumeGrid` from `Volume`, but not for accessing the `openvdb::GridBase` from `VolumeGrid`. I don't know of other cases of implicit sharing data changing their data in this way. This function would be comparable to e.g. `CurveComponent::get_for_write`, where it _asserts_ that the ImplicitSharingData itself is mutable and then returns a mutable pointer to the internals. https://projects.blender.org/blender/blender/src/commit/56be404b0f7f8b9296d72b7cce6c7a415f1d7321/source/blender/blenkernel/intern/geometry_component_curves.cc#L84-L97
Member
  • VolumeGrid::grid_for_write() is more like Curves *CurveComponent::get_for_write():
    • In a non-const method of implicitly shared data, you can assert that the caller made sure that the data is actually mutable.
  • GridPtr VolumeGridPtr::grid_for_write() const is more like Curves *GeometrySet::get_curves_for_write() or Mesh::vert_positions_for_write():
    • A *_for_write method on something that owns implicitly shared data might update the internal data pointer if a copy has to be made.

Never really thought about this in this explicit way, but I hope this comparison helps.

* `VolumeGrid::grid_for_write()` is more like `Curves *CurveComponent::get_for_write()`: * In a non-const method of implicitly shared data, you can assert that the caller made sure that the data is actually mutable. * `GridPtr VolumeGridPtr::grid_for_write() const` is more like `Curves *GeometrySet::get_curves_for_write()` or `Mesh::vert_positions_for_write()`: * A `*_for_write` method on something that owns implicitly shared data might update the internal data pointer if a copy has to be made. Never really thought about this in this explicit way, but I hope this comparison helps.
LukasTonne marked this conversation as resolved
@ -0,0 +203,4 @@
template<typename T> std::optional<T> get_background_value(const VolumeGridPtr<T> &grid)
{
#ifdef WITH_OPENVDB
if constexpr (std::is_same_v<T, std::string>) {
Member

Would be nice if we could check for the string type at a higher level, so that we don't have to have special cases for it in volume handling.

Would be nice if we could check for the string type at a higher level, so that we don't have to have special cases for it in volume handling.
Author
Member

Removed, it's not needed at this point.

This distinction becomes necessary when using a static dispatch "apply" function where the set of possible types includes strings. For now the set of supported attribute types includes only float and float3. Handling all socket types and possibly type-less MaskGrid is a future problem.

Removed, it's not needed at this point. This distinction becomes necessary when using a static dispatch "apply" function where the set of possible types includes strings. For now the set of supported attribute types includes only `float` and `float3`. Handling all socket types and possibly type-less `MaskGrid` is a future problem.
LukasTonne marked this conversation as resolved
@ -1241,2 +860,3 @@
for (GVolumeGridPtr &grid : grids) {
if (grid_index-- == 0) {
return &grid;
return const_cast<VolumeGrid *>(grid.get());
Member

This isn't actually ensuring that the grid is mutable. Should probably use GVolumeGridPtr.grid_for_write().

This isn't actually ensuring that the grid is mutable. Should probably use `GVolumeGridPtr.grid_for_write()`.
Author
Member

Should be fixed now. The BKE_volume_grid_get_for_write function makes sure the data is mutable and otherwise creates a copy. Accessing OpenVDB grids asserts that the data is mutable before returning the pointer, since we don't know if data mutable just from the type (depends on runtime user count).

Will need a cleanup pass to ensure nodes and utility functions aren't making unnecessary copies now.

Should be fixed now. The `BKE_volume_grid_get_for_write` function makes sure the data is mutable and otherwise creates a copy. Accessing OpenVDB grids asserts that the data is mutable before returning the pointer, since we don't know if data mutable just from the type (depends on runtime user count). Will need a cleanup pass to ensure nodes and utility functions aren't making unnecessary copies now.
LukasTonne marked this conversation as resolved
@ -133,2 +134,4 @@
};
template<typename T, typename... Args>
inline ImplicitSharingPtr<T> make_implicit_shared(Args &&...args)
Member

Looks like this is currently unused.

Looks like this is currently unused.
LukasTonne marked this conversation as resolved
@ -0,0 +69,4 @@
params.set_output("Volume", geometry_set);
if (remove_grid) {
BKE_volume_grid_remove(volume, grid);
Member

Remove the grid before calling set_output.

Remove the grid before calling `set_output`.
LukasTonne marked this conversation as resolved
@ -0,0 +75,4 @@
}
}
grids::set_output_grid(params, "Grid", data_type, nullptr);
Member

Currently crashes if the grid name is not found (e.g. when just inserting a new Get Named Grid node after a Mesh to Volume node).

Currently crashes if the grid name is not found (e.g. when just inserting a new `Get Named Grid` node after a `Mesh to Volume` node).
LukasTonne marked this conversation as resolved
@ -158,6 +158,10 @@ static void transform_volume(GeoNodeExecParams &params,
const int grids_num = BKE_volume_num_grids(&volume);
for (const int i : IndexRange(grids_num)) {
VolumeGrid *volume_grid = BKE_volume_grid_get_for_write(&volume, i);
if (!volume_grid->is_mutable()) {
Member

A grid returned from BKE_volume_grid_get_for_write should always be mutable (or null).

A grid returned from `BKE_volume_grid_get_for_write` should always be mutable (or null).
LukasTonne marked this conversation as resolved
Lukas Tönne added 2 commits 2023-12-04 14:36:53 +01:00
Lukas Tönne added 9 commits 2023-12-04 15:35:15 +01:00
Lukas Tönne added 1 commit 2023-12-04 16:31:54 +01:00
Lukas Tönne added 1 commit 2023-12-04 17:19:04 +01:00
Lukas Tönne added 1 commit 2023-12-04 17:23:59 +01:00
Author
Member

@blender-bot build

@blender-bot build
Lukas Tönne added 5 commits 2023-12-05 10:14:40 +01:00
Lukas Tönne added 2 commits 2023-12-05 13:56:51 +01:00
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.
Lukas Tönne added 1 commit 2023-12-07 09:54:43 +01:00
Lukas Tönne added 4 commits 2023-12-07 10:07:46 +01:00
Lukas Tönne reviewed 2023-12-07 16:23:03 +01:00
@ -0,0 +66,4 @@
}
#ifdef WITH_OPENVDB
GridConstPtr grid() const;
Author
Member

Note to self: remove these, they don't copy on write and are shortcuts to the same functions in VolumeGrid.

Note to self: remove these, they don't copy on write and are shortcuts to the same functions in VolumeGrid.
JacquesLucke marked this conversation as resolved
Lukas Tönne added 1 commit 2023-12-09 11:24:50 +01:00
Lukas Tönne added a new dependency 2023-12-11 08:27:24 +01:00
Lukas Tönne added 1 commit 2023-12-11 09:52:56 +01:00
Lukas Tönne added 2 commits 2023-12-11 10:36:35 +01:00
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.
Author
Member

It's a bit confusing how there is a BKE_volume_load (loads metadata for all grids) and a BKE_volume_grid_load (loads actual tree data) function. I'd like to make naming a bit more explicit:

  • BKE_volume_load -> BKE_volume_ensure_loaded
    This function is called in many places before accessing the volume, similar to how we use ensure_xxx functions for other lazy-loaded/generated data.
    The current implementation will only load the metadata. Either the name should indicate that (BKE_volume_ensure_metadata_loaded) or there should be an option to also load tree data (default off) so it's clear what this does.
  • BKE_volume_grid_load -> BKE_volume_grid_ensure_tree_loaded

There may be nicer options for naming ...

It's a bit confusing how there is a `BKE_volume_load` (loads metadata for all grids) and a `BKE_volume_grid_load` (loads actual tree data) function. I'd like to make naming a bit more explicit: - `BKE_volume_load` -> `BKE_volume_ensure_loaded` This function is called in many places before accessing the volume, similar to how we use `ensure_xxx` functions for other lazy-loaded/generated data. The current implementation will only load the metadata. Either the name should indicate that (`BKE_volume_ensure_metadata_loaded`) or there should be an option to also load tree data (default off) so it's clear what this does. - `BKE_volume_grid_load` -> `BKE_volume_grid_ensure_tree_loaded` There may be nicer options for naming ...
Member

It feels like we don't really need BKE_volume_grid_ensure_tree_loaded if we do the load lazily when accessing VolumeGrid::grid().

Technically, I'd expect BKE_volume_load to automatically run for all volume data blocks on file load. Maybe we can just hide this loading behind an access API as well? So when some code first tries to access the grid meta data, it's loaded.

It feels like we don't really need `BKE_volume_grid_ensure_tree_loaded` if we do the load lazily when accessing `VolumeGrid::grid()`. Technically, I'd expect `BKE_volume_load` to automatically run for all volume data blocks on file load. Maybe we can just hide this loading behind an access API as well? So when some code first tries to access the grid meta data, it's loaded.
Author
Member

It feels like we don't really need BKE_volume_grid_ensure_tree_loaded if we do the load lazily when accessing VolumeGrid::grid()

That could work, it's what the C api does now too: 089383a53a/source/blender/blenkernel/intern/volume.cc (L1632)

Technically, I'd expect BKE_volume_load to automatically run for all volume data blocks on file load.

I'm a bit surprised this doesn't seem to happen currently (have to confirm on main). But i guess if any code accessing grid info calls BKE_volume_load anyway before using the API there isn't any need to load metadata in advance.

Maybe we can just hide this loading behind an access API as well?

Possibly, but i think it's still good to have an explicit function to load file metadata upfront, rather than deferring file I/O to some unknown point inside another function. This would apply to any function accessing grid info:

  • BKE_volume_num_grids
  • BKE_volume_grid_get_for_read/write
  • BKE_volume_grid_find_for_read/write
  • BKE_volume_grid_name
  • BKE_volume_grid_type
  • BKE_volume_grid_channels
  • BKE_volume_grid_transform_matrix[_set]
  • BKE_volume_grid_add (since adding a grid has to check for name conflicts)
> It feels like we don't really need `BKE_volume_grid_ensure_tree_loaded` if we do the load lazily when accessing `VolumeGrid::grid()` That could work, it's what the C api does now too: https://projects.blender.org/blender/blender/src/commit/089383a53a05baf80501a9610d0c66dfb0729c96/source/blender/blenkernel/intern/volume.cc#L1632 > Technically, I'd expect BKE_volume_load to automatically run for all volume data blocks on file load. I'm a bit surprised this doesn't seem to happen currently (have to confirm on main). But i guess if any code accessing grid info calls `BKE_volume_load` anyway before using the API there isn't any need to load metadata in advance. > Maybe we can just hide this loading behind an access API as well? Possibly, but i think it's still good to have an explicit function to load file metadata upfront, rather than deferring file I/O to some unknown point inside another function. This would apply to any function accessing grid info: - BKE_volume_num_grids - BKE_volume_grid_get_for_read/write - BKE_volume_grid_find_for_read/write - BKE_volume_grid_name - BKE_volume_grid_type - BKE_volume_grid_channels - BKE_volume_grid_transform_matrix[_set] - BKE_volume_grid_add (since adding a grid has to check for name conflicts)
Brecht Van Lommel removed a dependency 2023-12-11 12:18:35 +01:00
Member

It mainly feels weird to always have to call BKE_volume_load before doing anything with a volume. It may be more obvious now, because we don't support storing grids directly in .blend files yet. It feels like it will become a common issue to forget calling BKE_volume_load. If you want to ensure some data is loaded at a specific point in time, then you can also just "touch" the data (e.g. calling BKE_volume_num_grids without actually needing it right now). That feels like the more scalable approach to me.

I'm fine with some more internal BKE_volume_ensure_loaded function.

It mainly feels weird to always have to call `BKE_volume_load` before doing anything with a volume. It may be more obvious now, because we don't support storing grids directly in .blend files yet. It feels like it will become a common issue to forget calling `BKE_volume_load`. If you want to ensure some data is loaded at a specific point in time, then you can also just "touch" the data (e.g. calling `BKE_volume_num_grids` without actually needing it right now). That feels like the more scalable approach to me. I'm fine with some more internal `BKE_volume_ensure_loaded` function.
Lukas Tönne added 1 commit 2023-12-12 09:29:12 +01:00
Lukas Tönne added 1 commit 2023-12-12 13:01:09 +01:00
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.
Jacques Lucke reviewed 2023-12-12 13:38:39 +01:00
@ -0,0 +142,4 @@
/* Source of the tree data stored in the grid. */
VolumeTreeSource tree_source() const;
/* Make sure the tree data has been loaded if the grid has a file source. */
bool ensure_tree_loaded(
Member

It still seems somewhat weird that the filepath is passed into a const method. I'd expect all the data required for lazy-loading the grid to be passed into the constructor of VolumeGrid.

It still seems somewhat weird that the filepath is passed into a `const` method. I'd expect all the data required for lazy-loading the grid to be passed into the constructor of `VolumeGrid`.
JacquesLucke marked this conversation as resolved
@ -0,0 +145,4 @@
bool ensure_tree_loaded(
StringRef filepath, FunctionRef<void(StringRef)> error_fn = [](StringRef) {}) const;
/* Unload the tree data but keep metadata. */
void unload_tree() const;
Member

I don't think it's possible for this to be const. We can't unload the tree when some other user of VolumeGrid is using the underlying grid currently.

I don't think it's possible for this to be `const`. We can't unload the tree when some other user of `VolumeGrid` is using the underlying grid currently.
JacquesLucke marked this conversation as resolved
@ -0,0 +148,4 @@
void unload_tree() const;
protected:
GridBasePtr main_grid() const;
Member

Can be removed now.

Can be removed now.
JacquesLucke marked this conversation as resolved
@ -0,0 +20,4 @@
bool VolumeFileCache::has_grid(const VolumeFileCacheKey &key) const
{
return file_grids_.contains(key);
Member

Every access to file_grids_ needs to be protected by the mutex.

Every access to `file_grids_` needs to be protected by the mutex.
JacquesLucke marked this conversation as resolved
@ -0,0 +48,4 @@
}
/* Returns the original grid or a simplified version depending on the given #simplify_level. */
GVolumeGridPtr VolumeFileCache::get_simplified_grid(const VolumeFileCacheKey &key,
Member

I find this signature misleading because VolumeFileCacheKey already contains the simplify_level. Based on the signature I'd expect that the simplify level is applied on top of the simplify level stored in the key.

I think the lookup of simplified grids could just use the normal lookup function as well. volume_update_simplify_level can set the simplify_level in the key.

I find this signature misleading because `VolumeFileCacheKey` already contains the `simplify_level`. Based on the signature I'd expect that the simplify level is applied on top of the simplify level stored in the `key`. I think the lookup of simplified grids could just use the normal `lookup` function as well. `volume_update_simplify_level` can set the `simplify_level` in the key.
JacquesLucke marked this conversation as resolved
Lukas Tönne added 1 commit 2023-12-12 13:55:08 +01:00
Lukas Tönne added 1 commit 2023-12-12 16:18:00 +01:00
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.
Lukas Tönne added 1 commit 2023-12-12 16:43:27 +01:00
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.
Lukas Tönne added 1 commit 2023-12-12 17:31:50 +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-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.
Author
Member

Regarding the GVolumeGridPtr/VolumeGridPtr<T>:
IIRC we briefly touched on removing the typed pointer variant and just use a generic ImplicitSharingPtr. Such pointers will always return a un-typed const VolumeGrid *. The caller then has to:

  1. Check for mutability and do a const_cast or grid_ptr->copy(); to get a mutable VolumeGrid *
  2. Use VolumeGrid::get or VolumeGrid::get_for_write functions to get a openvdb::GridBase::Ptr
    OR use type-casting functions VolumeGrid::get<T> or VolumeGrid::get_for_write<T> to get a VolumeGridType<T> (an openvdb::Grid type based on the Blender type T)

I think this would be more in line with the intended use of ImplicitSharingData. I don't expect use cases to get more complex by this change. The BKE_volume_grid_ptr.hh header would become unnecessary.

Regarding the `GVolumeGridPtr`/`VolumeGridPtr<T>`: IIRC we briefly touched on removing the typed pointer variant and just use a generic `ImplicitSharingPtr`. Such pointers will always return a un-typed `const VolumeGrid *`. The caller then has to: 1. Check for mutability and do a `const_cast` or `grid_ptr->copy();` to get a mutable `VolumeGrid *` 2. Use `VolumeGrid::get` or `VolumeGrid::get_for_write` functions to get a `openvdb::GridBase::Ptr` __OR__ use type-casting functions `VolumeGrid::get<T>` or `VolumeGrid::get_for_write<T>` to get a `VolumeGridType<T>` (an `openvdb::Grid` type based on the Blender type `T`) I think this would be more in line with the intended use of `ImplicitSharingData`. I don't expect use cases to get more complex by this change. The `BKE_volume_grid_ptr.hh` header would become unnecessary.
Author
Member

@blender-bot build

@blender-bot build
Lukas Tönne added 1 commit 2023-12-13 10:15:08 +01:00
Jacques Lucke added 1 commit 2023-12-13 13:19:06 +01:00
Jacques Lucke added 1 commit 2023-12-13 13:44:38 +01:00
Jacques Lucke added 1 commit 2023-12-13 13:48:33 +01:00
Jacques Lucke added 54 commits 2023-12-18 22:30:57 +01:00
Jacques Lucke added 1 commit 2023-12-18 22:37:42 +01:00
Jacques Lucke added 1 commit 2023-12-18 22:46:59 +01:00
Jacques Lucke added 3 commits 2023-12-20 16:05:37 +01:00
Jacques Lucke added 1 commit 2023-12-20 16:08:24 +01:00
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
da01246e8f
avoid empty submenus in new volume nodes are disabled
Jacques Lucke changed title from Volume Grid Sockets to Geometry Nodes: initial Volume Grid socket support 2023-12-20 16:08:43 +01:00
Member

@blender-bot build

@blender-bot build
Hans Goudey approved these changes 2023-12-20 16:52:51 +01:00
@ -332,6 +332,7 @@ DefNode(GeometryNode, GEO_NODE_FILL_CURVE, 0, "FILL_CURVE", FillCurve, "Fill Cur
DefNode(GeometryNode, GEO_NODE_FILLET_CURVE, 0, "FILLET_CURVE", FilletCurve, "Fillet Curve", "Round corners by generating circular arcs on each control point")
DefNode(GeometryNode, GEO_NODE_FLIP_FACES, 0, "FLIP_FACES", FlipFaces, "Flip Faces", "Reverse the order of the vertices and edges of selected faces, flipping their normal direction")
DefNode(GeometryNode, GEO_NODE_GEOMETRY_TO_INSTANCE, 0, "GEOMETRY_TO_INSTANCE", GeometryToInstance, "Geometry to Instance", "Convert each input geometry into an instance, which can be much faster than the Join Geometry node when the inputs are large")
DefNode(GeometryNode, GEO_NODE_GET_NAMED_GRID, 0, "GET_NAMED_GRID", GetNamedGrid, "Get Named Grid", "Get grid data from a volume geometry with the specified name")
Member

Get grid data -> Get volume grid data

Same below

`Get grid data` -> `Get volume grid data` Same below
JacquesLucke marked this conversation as resolved
@ -0,0 +55,4 @@
static void try_store_grid(GeoNodeExecParams params, Volume *volume)
{
BLI_assert(volume);
Member

Pass a reference instead of asserting the pointer is non-null

Pass a reference instead of asserting the pointer is non-null
JacquesLucke marked this conversation as resolved
Jacques Lucke added 3 commits 2023-12-20 22:09:31 +01:00
Jacques Lucke approved these changes 2023-12-20 22:11:56 +01:00
Jacques Lucke added 1 commit 2023-12-20 22:30:28 +01:00
Jacques Lucke merged commit e470edf3e1 into main 2023-12-20 22:33:26 +01:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Blocks
Reference: blender/blender#115270
No description provided.