Geometry Nodes: Sample Volume node #107656

Closed
Erik Abrahamsson wants to merge 17 commits from erik85:volume-sample into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

This PR adds an experimental Sample Volume node that can sample
the value of a volume at a specified position or with a field.
It supports Float, Int32, Boolean and Vector grids.
The Grid input expects a named attribute input with the name of the grid.

bild

This PR adds an experimental `Sample Volume` node that can sample the value of a volume at a specified position or with a field. It supports Float, Int32, Boolean and Vector grids. The `Grid` input expects a named attribute input with the name of the grid. ![bild](/attachments/7de67bba-2290-4270-828f-2a4872f3a7ae)
402 KiB
Erik Abrahamsson added 1 commit 2023-05-05 17:19:27 +02:00
083ac5c0ad Geometry Nodes: Sample Volume node
This patch adds a `Sample Volume` node that can sample the value
of a volume at a specified position.
It supports Float, Int32, Boolean and Vector grids.
The `Grid` input expects a named attribute with the name of the grid.
Erik Abrahamsson added 1 commit 2023-05-05 17:24:32 +02:00
Erik Abrahamsson added this to the Nodes & Physics project 2023-05-05 17:24:46 +02:00
Erik Abrahamsson changed title from WIP: Geometry Nodes: Sample Volume node to Geometry Nodes: Sample Volume node 2023-05-05 17:25:18 +02:00
Iliya Katushenock added the
Interest
Geometry Nodes
label 2023-05-05 18:15:05 +02:00
Hans Goudey reviewed 2023-05-07 16:24:50 +02:00
Hans Goudey left a comment
Member

I think this will be useful! I'd expect this to be implemented as a multifunction similar to the other "sample" nodes though. An interpolation type dropdown might be nice to have as well, since I think OpenVDB makes a few available.

I think this will be useful! I'd expect this to be implemented as a multifunction similar to the other "sample" nodes though. An interpolation type dropdown might be nice to have as well, since I think OpenVDB makes a few available.
Erik Abrahamsson added 1 commit 2023-05-08 18:29:35 +02:00
Erik Abrahamsson requested review from Hans Goudey 2023-05-08 19:48:54 +02:00
Erik Abrahamsson added 2 commits 2023-05-08 21:51:18 +02:00
Author
Member

Potentially quite a few things here to consider I think. Pretty sure there are more than these I thought of:

  • Which grid types to support
  • Does it even make sense to support single values as a special case? It's fast as it is with the MultiFunction.
  • Handling of the case where not Point sampling is chosen for a Bool grid (now automatically uses Point).
Potentially quite a few things here to consider I think. Pretty sure there are more than these I thought of: * Which grid types to support * Does it even make sense to support single values as a special case? It's fast as it is with the MultiFunction. * Handling of the case where not Point sampling is chosen for a Bool grid (now automatically uses Point).
Erik Abrahamsson added 1 commit 2023-05-08 23:43:59 +02:00
Hans Goudey requested changes 2023-05-09 15:18:08 +02:00
Hans Goudey left a comment
Member
  • The grid types you have supported now look like a good place to start
  • I had to add typename in front of GridT::ConstAccessor everywhere to get this to compile here
- The grid types you have supported now look like a good place to start - I had to add `typename` in front of `GridT::ConstAccessor` everywhere to get this to compile here
@ -11004,0 +11013,4 @@
PropertyRNA *prop;
RNA_def_struct_sdna_from(srna, "NodeGeometrySampleVolume", "storage");
Member

The fact that the node is implemented with OpenVDB is an implementation detail that we don't need to expose to users. Something like "How to interpolate the values from neighboring voxels" might be more helpful

The fact that the node is implemented with OpenVDB is an implementation detail that we don't need to expose to users. Something like "How to interpolate the values from neighboring voxels" might be more helpful
erik85 marked this conversation as resolved
@ -11004,0 +11021,4 @@
prop = RNA_def_property(srna, "grid_type", PROP_ENUM, PROP_NONE);
RNA_def_property_enum_items(prop, rna_enum_attribute_type_items);
RNA_def_property_enum_funcs(
Member

Technically this doesn't change the data type stored in the grid, right? It's the type to use for sampling?

Technically this doesn't change the data type stored in the grid, right? It's the type to use for sampling?
erik85 marked this conversation as resolved
@ -0,0 +101,4 @@
{
StringRefNull grid_name;
if (field.node().field_inputs() != nullptr) {
Member

Does something like this work?

if (const auto *attribute_field_input = dynamic_cast<const AttributeFieldInput *>(field.node())) {
  return attribute_field_input->attribute_name();
}

Seems like it would be a bit more predictable

Does something like this work? ```cpp if (const auto *attribute_field_input = dynamic_cast<const AttributeFieldInput *>(field.node())) { return attribute_field_input->attribute_name(); } ``` Seems like it would be a bit more predictable
Member

I think this is simpler than looping through all the field inputs. Still worth doing IMO

I think this is simpler than looping through all the field inputs. Still worth doing IMO
erik85 marked this conversation as resolved
@ -0,0 +190,4 @@
{
const VArray<float3> &positions = params.readonly_single_input<float3>(0, "Position");
GMutableSpan dst = params.uninitialized_single_output(1, "Value");
const VolumeGridType grid_type = BKE_volume_grid_type_openvdb(*base_grid_);
Member

I think it would be fine to use VArraySpan here, since the input positions would probably almost always be a span anyway. That way the rest of the function doesn't need to use VArray

I think it would be fine to use `VArraySpan` here, since the input positions would probably almost always be a span anyway. That way the rest of the function doesn't need to use `VArray`
erik85 marked this conversation as resolved
@ -0,0 +195,4 @@
switch (grid_type) {
case VOLUME_GRID_FLOAT: {
sample_grid<openvdb::FloatGrid>(base_grid_, positions, mask, dst, sampling_mode_);
break;
Member

Nit-picky thing, but the braces seem to just take up unnecessary space here.

Nit-picky thing, but the braces seem to just take up unnecessary space here.
erik85 marked this conversation as resolved
@ -0,0 +320,4 @@
if (volume_grid == nullptr) {
char *message = BLI_sprintfN(TIP_("Grid not found in the volume: \"%s\""), grid_name.c_str());
params.error_message_add(NodeWarningType::Error, message);
params.set_default_remaining_outputs();
Member

Memory leak! But also typically we don't give errors for this. When an attribute doesn't exist, I think we usually just give default values.

Memory leak! But also typically we don't give errors for this. When an attribute doesn't exist, I think we usually just give default values.
erik85 marked this conversation as resolved
@ -0,0 +329,4 @@
/* Check that the input grid socket type matches the grid type. */
if (!((grid_type == VOLUME_GRID_FLOAT && input_grid_type == CD_PROP_FLOAT) ||
(grid_type == VOLUME_GRID_INT && input_grid_type == CD_PROP_INT32) ||
(grid_type == VOLUME_GRID_BOOLEAN && input_grid_type == CD_PROP_BOOL) ||
Member

Maybe a simpler and more reusable way to do this would be to have a function that takes a custom data type and returns a grid type (or vice versa).

Maybe a simpler and more reusable way to do this would be to have a function that takes a custom data type and returns a grid type (or vice versa).
erik85 marked this conversation as resolved
@ -0,0 +346,4 @@
NodeWarningType::Warning,
TIP_("Using the Point Sampler because of Boolean Grid type limitation"));
sampling_mode = GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_POINT;
}
Member

I think it's okay to just do this silently. It's not like users will really expect booleans to be interpolated.

I think it's okay to just do this silently. It's not like users will really expect booleans to be interpolated.
erik85 marked this conversation as resolved
@ -0,0 +351,4 @@
GField output_field;
if (position_value_or_field.is_field()) {
auto fn = std::make_shared<SampleVolumeFunction>(
std::move(base_grid), std::move(grid_field), sampling_mode);
Member

The geometry nodes evaluator will do this constant field optimization by itself. Typically the multi-functions shouldn't have to know about it (they'll just get a single-element span/mask).

The geometry nodes evaluator will do this constant field optimization by itself. Typically the multi-functions shouldn't have to know about it (they'll just get a single-element span/mask).
erik85 marked this conversation as resolved
Erik Abrahamsson added 1 commit 2023-05-09 19:32:47 +02:00
Erik Abrahamsson requested review from Hans Goudey 2023-05-09 19:44:56 +02:00
Iliya Katushenock reviewed 2023-05-09 20:22:34 +02:00
@ -0,0 +270,4 @@
}
BLI_assert_unreachable();
return std::nullopt;
}

It seems better to just agree that with above:
Default - unreachable
Return at the end - nothing

    case VOLUME_GRID_MASK:
      return CD_PROP_BOOL;
    default:
      BLI_assert_unreachable();
      break;
  }
  return std::nullopt;
}
It seems better to just agree that with above: Default - unreachable Return at the end - nothing ```Cpp case VOLUME_GRID_MASK: return CD_PROP_BOOL; default: BLI_assert_unreachable(); break; } return std::nullopt; } ```
erik85 marked this conversation as resolved
Iliya Katushenock reviewed 2023-05-09 20:22:36 +02:00
@ -0,0 +283,4 @@
}
const NodeGeometrySampleVolume &storage = node_storage(params.node());
const eCustomDataType input_grid_type = eCustomDataType(storage.grid_type);
auto interpolation_mode = (const GeometryNodeSampleVolumeInterpolationMode)

C-style cast there

C-style cast there
Author
Member

edit: Forget it, I'm babbling.

edit: Forget it, I'm babbling.

I mean (const GeometryNodeSampleVolumeInterpolationMode)(storage.interpolation_mode)
Really, not sure if it better.

I mean `(const GeometryNodeSampleVolumeInterpolationMode)(storage.interpolation_mode)` Really, not sure if it better.
mod_moder marked this conversation as resolved
Iliya Katushenock reviewed 2023-05-09 20:22:38 +02:00
@ -0,0 +329,4 @@
GField output_field = GField(std::move(op));
output_attribute_field(params, std::move(output_field));
params.set_default_remaining_outputs();

params.set_default_remaining_outputs(); is necessary there?

`params.set_default_remaining_outputs();` is necessary there?
erik85 marked this conversation as resolved
Erik Abrahamsson added 2 commits 2023-05-10 17:42:01 +02:00
Erik Abrahamsson added 1 commit 2023-05-10 22:22:01 +02:00
Hans Goudey requested changes 2023-05-12 17:49:25 +02:00
@ -0,0 +4,4 @@
#include "BKE_volume.h"
#include "BLI_virtual_array.hh"
#include "NOD_add_node_search.hh"
Member

Add newlines between includes from different modules

Add newlines between includes from different modules
erik85 marked this conversation as resolved
@ -0,0 +123,4 @@
{
using ValueT = typename GridT::ValueType;
using AccessorT = typename GridT::ConstAccessor;
const GridT::ConstPtr grid = openvdb::gridConstPtrCast<GridT>(base_grid);
Member

I also had to add typename before GridT::ConstPtr to get this to compile

I also had to add `typename` before `GridT::ConstPtr` to get this to compile
erik85 marked this conversation as resolved
@ -0,0 +231,4 @@
switch (data_type) {
case CD_PROP_FLOAT:
return params.extract_input<Field<float>>("Grid_Float");
case CD_PROP_FLOAT3:
Member

Braces can be removed for these switch cases

Braces can be removed for these switch cases
erik85 marked this conversation as resolved
@ -0,0 +310,4 @@
const VolumeComponent *component = geometry_set.get_component_for_read<VolumeComponent>();
const Volume *volume = component->get_for_read();
BKE_volume_load(volume, DEG_get_bmain(params.depsgraph()));
const VolumeGrid *volume_grid = BKE_volume_grid_find_for_read(volume, grid_name.c_str());
Member

Sampling the grid with its type (and only giving an error if the type is unsupported), then adding a conversion afterwards would probably be simpler.

Sampling the grid with its type (and only giving an error if the type is unsupported), then adding a conversion afterwards would probably be simpler.
Author
Member

Simpler in what way? And how do I add a conversion?

Simpler in what way? And how do I add a conversion?
Member

Simpler for the user, and simpler because you only need to check the grid type here. You can add a field conversion with get_implicit_type_conversions and fn::GField try_convert(fn::GField field, const CPPType &to_type) const;

Simpler for the user, and simpler because you only need to check the grid type here. You can add a field conversion with `get_implicit_type_conversions` and `fn::GField try_convert(fn::GField field, const CPPType &to_type) const;`
erik85 marked this conversation as resolved
Hans Goudey reviewed 2023-05-12 17:50:40 +02:00
@ -0,0 +64,4 @@
static void search_link_ops(GatherLinkSearchOpParams &params)
{
if (U.experimental.use_new_volume_nodes) {
blender::nodes::search_link_ops_for_basic_node(params);
Member

Since this node has faux multi-type sockets, it probably needs a search_link_ops implementation like the other sample nodes.

Since this node has faux multi-type sockets, it probably needs a `search_link_ops` implementation like the other sample nodes.
erik85 marked this conversation as resolved
Erik Abrahamsson added 2 commits 2023-05-14 23:07:55 +02:00
Erik Abrahamsson added 1 commit 2023-05-15 18:38:22 +02:00
Erik Abrahamsson added 1 commit 2023-05-15 18:44:21 +02:00
Hans Goudey requested changes 2023-05-15 23:31:51 +02:00
Hans Goudey left a comment
Member

Just a few small things! Looks good otherwise.

Just a few small things! Looks good otherwise.
@ -0,0 +114,4 @@
bNodeSocket *socket_value_boolean = socket_value_float->next;
bNodeSocket *socket_value_int32 = socket_value_boolean->next;
nodeSetSocketAvailability(ntree, socket_value_vector, grid_type == CD_PROP_FLOAT3);
Member

These need a bke:: prefix now

These need a `bke::` prefix now
erik85 marked this conversation as resolved
@ -0,0 +148,4 @@
return "";
}
static std::optional<eCustomDataType> vdb_grid_type_to_customdata_type(
Member

I guess this might as well be vdb_grid_type_to_cpp_type, that's how it's used anyway.

I guess this might as well be `vdb_grid_type_to_cpp_type`, that's how it's used anyway.
erik85 marked this conversation as resolved
@ -0,0 +179,4 @@
const GridT::ConstPtr grid = openvdb::gridConstPtrCast<GridT>(base_grid);
AccessorT accessor = grid->getConstAccessor();
auto copy_data = [&](auto sampler) {
Member

How about copy_data -> sample_data? It's not really a "copy" IMO

How about `copy_data` -> `sample_data`? It's not really a "copy" IMO
erik85 marked this conversation as resolved
@ -0,0 +356,4 @@
auto op = FieldOperation::Create(std::move(fn), {position_field});
GField output_field = GField(std::move(op));
if (grid_customdata_type.value() != output_field_type) {
Member

try_convert already does this check

`try_convert` already does this check
erik85 marked this conversation as resolved
Erik Abrahamsson added 1 commit 2023-05-16 02:45:24 +02:00
Hans Goudey approved these changes 2023-05-16 03:25:33 +02:00
Hans Goudey reviewed 2023-05-16 03:27:25 +02:00
@ -0,0 +134,4 @@
static const StringRefNull get_grid_name(GField &field)
{
if (field.node().field_inputs() != nullptr) {
Member

You marked this resolved twice, maybe I wasn't clear. I think you can replace this whole get_grid_name function with:

static const StringRefNull get_grid_name(const GField &field) {
  if (const auto *attribute_field_input = dynamic_cast<const AttributeFieldInput *>(field.node())) {
    return attribute_field_input->attribute_name();
  }
  return "";
}
You marked this resolved twice, maybe I wasn't clear. I think you can replace this whole `get_grid_name` function with: ```cpp static const StringRefNull get_grid_name(const GField &field) { if (const auto *attribute_field_input = dynamic_cast<const AttributeFieldInput *>(field.node())) { return attribute_field_input->attribute_name(); } return ""; } ```
Author
Member

Haha ok, I made some change the first time but missed the point and second comment.
I'm trying to think of a way it could fail, if there are other FieldInputs connected in some way. But I guess that wouldn't make any sense.

Haha ok, I made some change the first time but missed the point and second comment. I'm trying to think of a way it could fail, if there are other FieldInputs connected in some way. But I guess that wouldn't make any sense.

FieldInput is just base class of AttributeFieldInput, other nodes is class heirs too. So, dynamic_cast simple solve this.

`FieldInput` is just base class of `AttributeFieldInput`, other nodes is class heirs too. So, `dynamic_cast` simple solve this.
Author
Member

FieldInput is just base class of AttributeFieldInput, other nodes is class heirs too. So, dynamic_cast simple solve this.

I was mainly thinking that it could check the wrong node and miss the AttributeFieldInput, if it's not the only node connected to the input for some reason.

> `FieldInput` is just base class of `AttributeFieldInput`, other nodes is class heirs too. So, `dynamic_cast` simple solve this. I was mainly thinking that it could check the wrong node and miss the AttributeFieldInput, if it's not the only node connected to the input for some reason.
erik85 marked this conversation as resolved
Erik Abrahamsson force-pushed volume-sample from c5434396b7 to 70f312a954 2023-05-16 18:40:11 +02:00 Compare
Erik Abrahamsson closed this pull request 2023-05-16 19:10:41 +02:00
Erik Abrahamsson reopened this pull request 2023-05-16 19:10:59 +02:00
Erik Abrahamsson closed this pull request 2023-05-16 20:37:27 +02:00
Erik Abrahamsson deleted branch volume-sample 2023-05-16 20:37:41 +02:00

Pull request closed

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
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#107656
No description provided.