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.
3 changed files with 90 additions and 138 deletions
Showing only changes of commit 45c17f6c19 - Show all commits

View File

@ -1637,8 +1637,8 @@ typedef struct NodeGeometryDistributePointsInVolume {
typedef struct NodeGeometrySampleVolume {
/* eCustomDataType. */
int8_t grid_type;
/* GeometryNodeSampleVolumeSamplingMode */
int8_t sampling_mode;
/* GeometryNodeSampleVolumeInterpolationMode */
int8_t interpolation_mode;
} NodeGeometrySampleVolume;
typedef struct NodeFunctionCompare {
@ -2445,11 +2445,11 @@ typedef enum GeometryNodeScaleElementsMode {
GEO_NODE_SCALE_ELEMENTS_SINGLE_AXIS = 1,
} GeometryNodeScaleElementsMode;
typedef enum GeometryNodeSampleVolumeSamplingMode {
GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_POINT = 0,
GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_BOX = 1,
GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_QUADRATIC = 2,
} GeometryNodeSampleVolumeSamplingMode;
typedef enum GeometryNodeSampleVolumeInterpolationMode {
GEO_NODE_SAMPLE_VOLUME_INTERPOLATION_MODE_NEAREST = 0,
GEO_NODE_SAMPLE_VOLUME_INTERPOLATION_MODE_TRILINEAR = 1,
GEO_NODE_SAMPLE_VOLUME_INTERPOLATION_MODE_TRIQUADRATIC = 2,
} GeometryNodeSampleVolumeInterpolationMode;
typedef enum NodeCombSepColorMode {
NODE_COMBSEP_COLOR_RGB = 0,

View File

@ -11003,10 +11003,22 @@ static void def_geo_attribute_capture(StructRNA *srna)
static void def_geo_sample_volume(StructRNA *srna)
{
static const EnumPropertyItem sampler_items[] = {
{GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_POINT, "POINT", 0, "Point", ""},
{GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_BOX, "BOX", 0, "Box", ""},
{GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_QUADRATIC, "QUADRATIC", 0, "Quadratic", ""},
static const EnumPropertyItem interpolation_mode_items[] = {
{GEO_NODE_SAMPLE_VOLUME_INTERPOLATION_MODE_NEAREST, "NEAREST", 0, "Nearest Neighbor", ""},
{GEO_NODE_SAMPLE_VOLUME_INTERPOLATION_MODE_TRILINEAR, "TRILINEAR", 0, "Trilinear", ""},
{GEO_NODE_SAMPLE_VOLUME_INTERPOLATION_MODE_TRIQUADRATIC,
"TRIQUADRATIC",
0,
"Triquadratic",
""},
{0, NULL, 0, NULL, NULL},
};
erik85 marked this conversation as resolved Outdated

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
static const EnumPropertyItem grid_type_items[] = {
{CD_PROP_FLOAT, "FLOAT", 0, "Float", "Floating-point value"},
{CD_PROP_FLOAT3, "FLOAT_VECTOR", 0, "Vector", "3D vector with floating-point values"},
{CD_PROP_INT32, "INT", 0, "Integer", "32-bit integer"},
{CD_PROP_BOOL, "BOOLEAN", 0, "Boolean", "True or false"},
{0, NULL, 0, NULL, NULL},
};
erik85 marked this conversation as resolved Outdated

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?
@ -11014,17 +11026,16 @@ static void def_geo_sample_volume(StructRNA *srna)
RNA_def_struct_sdna_from(srna, "NodeGeometrySampleVolume", "storage");
prop = RNA_def_property(srna, "sampling_mode", PROP_ENUM, PROP_NONE);
RNA_def_property_enum_items(prop, sampler_items);
RNA_def_property_ui_text(prop, "Sampling Mode", "Which OpenVDB sampler to use");
prop = RNA_def_property(srna, "interpolation_mode", PROP_ENUM, PROP_NONE);
RNA_def_property_enum_items(prop, interpolation_mode_items);
RNA_def_property_ui_text(
prop, "Interpolation Mode", "How to interpolate the values from neighboring voxels");
RNA_def_property_update(prop, NC_NODE | NA_EDITED, "rna_Node_update");
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(
prop, NULL, NULL, "rna_GeometryNodeAttributeType_type_with_socket_itemf");
RNA_def_property_enum_items(prop, grid_type_items);
RNA_def_property_enum_default(prop, CD_PROP_FLOAT);
RNA_def_property_ui_text(prop, "Grid Type", "Type of data stored in the input grid");
RNA_def_property_ui_text(prop, "Grid Type", "Type of grid to sample data from");
RNA_def_property_update(prop, NC_NODE | NA_EDITED, "rna_GeometryNode_socket_update");
}

View File

@ -3,6 +3,7 @@
#include "DEG_depsgraph_query.h"
#include "BKE_volume.h"
#include "BLI_virtual_array.hh"
#include "NOD_add_node_search.hh"
erik85 marked this conversation as resolved Outdated

Add newlines between includes from different modules

Add newlines between includes from different modules
#include "NOD_socket_search_link.hh"
#include "node_geometry_util.hh"
@ -57,14 +58,14 @@ static void node_layout(uiLayout *layout, bContext * /*C*/, PointerRNA *ptr)
uiLayoutSetPropSep(layout, true);
uiLayoutSetPropDecorate(layout, false);
uiItemR(layout, ptr, "grid_type", 0, "", ICON_NONE);
uiItemR(layout, ptr, "sampling_mode", 0, "", ICON_NONE);
uiItemR(layout, ptr, "interpolation_mode", 0, "", ICON_NONE);
}
static void node_init(bNodeTree * /*tree*/, bNode *node)
{
NodeGeometrySampleVolume *data = MEM_cnew<NodeGeometrySampleVolume>(__func__);
data->grid_type = CD_PROP_FLOAT;
erik85 marked this conversation as resolved
Review

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.
data->sampling_mode = GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_BOX;
data->interpolation_mode = GEO_NODE_SAMPLE_VOLUME_INTERPOLATION_MODE_TRILINEAR;
node->storage = data;
}
@ -99,8 +100,6 @@ static void node_update(bNodeTree *ntree, bNode *node)
static const StringRefNull get_grid_name(GField &field)
{
StringRefNull grid_name;
if (field.node().field_inputs() != nullptr) {
const std::shared_ptr<const fn::FieldInputs> &field_inputs = field.node().field_inputs();
erik85 marked this conversation as resolved
Review

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
Review

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
@ -108,24 +107,24 @@ static const StringRefNull get_grid_name(GField &field)
if (const auto *attribute_field_input = dynamic_cast<const AttributeFieldInput *>(
&field_input))
{
grid_name = attribute_field_input->attribute_name();
break;
return attribute_field_input->attribute_name();
}
}
}
return grid_name;
return "";
}
template<typename GridT>
erik85 marked this conversation as resolved Outdated

These need a bke:: prefix now

These need a `bke::` prefix now
void sample_grid(openvdb::GridBase::ConstPtr base_grid,
const VArray<float3> &positions,
const Span<float3> positions,
const IndexMask mask,
GMutableSpan dst,
const GeometryNodeSampleVolumeSamplingMode sampling_mode)
const GeometryNodeSampleVolumeInterpolationMode interpolation_mode)
{
using ValueT = typename GridT::ValueType;
using AccessorT = typename GridT::ConstAccessor;
const GridT::ConstPtr grid = openvdb::gridConstPtrCast<GridT>(base_grid);
erik85 marked this conversation as resolved Outdated

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
GridT::ConstAccessor accessor = grid->getConstAccessor();
AccessorT accessor = grid->getConstAccessor();
auto copy_data = [&](auto sampler) {
mask.foreach_index([&](const int64_t i) {
@ -143,22 +142,22 @@ void sample_grid(openvdb::GridBase::ConstPtr base_grid,
});
};
switch (sampling_mode) {
case GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_BOX: {
openvdb::tools::GridSampler<GridT::ConstAccessor, openvdb::tools::BoxSampler> sampler(
switch (interpolation_mode) {
case GEO_NODE_SAMPLE_VOLUME_INTERPOLATION_MODE_TRILINEAR: {
openvdb::tools::GridSampler<AccessorT, openvdb::tools::BoxSampler> sampler(
accessor, grid->transform());
copy_data(sampler);
break;
}
erik85 marked this conversation as resolved Outdated

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.
case GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_QUADRATIC: {
openvdb::tools::GridSampler<GridT::ConstAccessor, openvdb::tools::QuadraticSampler> sampler(
case GEO_NODE_SAMPLE_VOLUME_INTERPOLATION_MODE_TRIQUADRATIC: {
openvdb::tools::GridSampler<AccessorT, openvdb::tools::QuadraticSampler> sampler(
accessor, grid->transform());
copy_data(sampler);
break;
}
case GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_POINT:
case GEO_NODE_SAMPLE_VOLUME_INTERPOLATION_MODE_NEAREST:
default: {
openvdb::tools::GridSampler<GridT::ConstAccessor, openvdb::tools::PointSampler> sampler(
openvdb::tools::GridSampler<AccessorT, openvdb::tools::PointSampler> sampler(
accessor, grid->transform());
copy_data(sampler);
break;
@ -169,16 +168,16 @@ void sample_grid(openvdb::GridBase::ConstPtr base_grid,
class SampleVolumeFunction : public mf::MultiFunction {
openvdb::GridBase::ConstPtr base_grid_;
GField grid_field_;
GeometryNodeSampleVolumeSamplingMode sampling_mode_;
GeometryNodeSampleVolumeInterpolationMode interpolation_mode_;
mf::Signature signature_;
public:
SampleVolumeFunction(openvdb::GridBase::ConstPtr base_grid,
GField grid_field,
GeometryNodeSampleVolumeSamplingMode sampling_mode)
GeometryNodeSampleVolumeInterpolationMode interpolation_mode)
: base_grid_(std::move(base_grid)),
grid_field_(std::move(grid_field)),
sampling_mode_(sampling_mode)
interpolation_mode_(interpolation_mode)
{
mf::SignatureBuilder builder{"Sample Volume", signature_};
erik85 marked this conversation as resolved Outdated

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
builder.single_input<float3>("Position");
@ -188,65 +187,30 @@ class SampleVolumeFunction : public mf::MultiFunction {
void call(IndexMask mask, mf::Params params, mf::Context /*context*/) const override
{
const VArray<float3> &positions = params.readonly_single_input<float3>(0, "Position");
const VArraySpan<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_);
erik85 marked this conversation as resolved Outdated

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`
switch (grid_type) {
case VOLUME_GRID_FLOAT: {
sample_grid<openvdb::FloatGrid>(base_grid_, positions, mask, dst, sampling_mode_);
case VOLUME_GRID_FLOAT:
sample_grid<openvdb::FloatGrid>(base_grid_, positions, mask, dst, interpolation_mode_);
break;
}
case VOLUME_GRID_INT: {
sample_grid<openvdb::Int32Grid>(base_grid_, positions, mask, dst, sampling_mode_);
case VOLUME_GRID_INT:
erik85 marked this conversation as resolved Outdated

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.
sample_grid<openvdb::Int32Grid>(base_grid_, positions, mask, dst, interpolation_mode_);
break;
}
case VOLUME_GRID_BOOLEAN: {
sample_grid<openvdb::BoolGrid>(base_grid_, positions, mask, dst, sampling_mode_);
case VOLUME_GRID_BOOLEAN:
sample_grid<openvdb::BoolGrid>(base_grid_, positions, mask, dst, interpolation_mode_);
break;
}
case VOLUME_GRID_VECTOR_FLOAT: {
sample_grid<openvdb::VectorGrid>(base_grid_, positions, mask, dst, sampling_mode_);
case VOLUME_GRID_VECTOR_FLOAT:
sample_grid<openvdb::VectorGrid>(base_grid_, positions, mask, dst, interpolation_mode_);
break;
}
default: {
default:
BLI_assert_unreachable();
break;
}
}
}
};
template<typename GridT>
typename GridT::ValueType sample_grid_single(
openvdb::GridBase::ConstPtr &base_grid,
float3 &position,
const GeometryNodeSampleVolumeSamplingMode sampling_mode)
{
using ValueT = typename GridT::ValueType;
const GridT::ConstPtr grid = openvdb::gridConstPtrCast<GridT>(base_grid);
GridT::ConstAccessor accessor = grid->getConstAccessor();
switch (sampling_mode) {
case GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_BOX: {
openvdb::tools::GridSampler<GridT::ConstAccessor, openvdb::tools::BoxSampler> sampler(
accessor, grid->transform());
return sampler.wsSample(openvdb::Vec3R(position.x, position.y, position.z));
}
case GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_QUADRATIC: {
openvdb::tools::GridSampler<GridT::ConstAccessor, openvdb::tools::QuadraticSampler> sampler(
accessor, grid->transform());
return sampler.wsSample(openvdb::Vec3R(position.x, position.y, position.z));
}
case GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_POINT:
default: {
openvdb::tools::GridSampler<GridT::ConstAccessor, openvdb::tools::PointSampler> sampler(
accessor, grid->transform());
return sampler.wsSample(openvdb::Vec3R(position.x, position.y, position.z));
}
}
}
static GField get_input_attribute_field(GeoNodeExecParams &params, const eCustomDataType data_type)
{
switch (data_type) {
@ -288,6 +252,25 @@ static void output_attribute_field(GeoNodeExecParams &params, GField field)
}
}
static std::optional<eCustomDataType> openvdb_grid_type_to_customdata_type(
const VolumeGridType grid_type)
{
switch (grid_type) {
case VOLUME_GRID_FLOAT:
return CD_PROP_FLOAT;
case VOLUME_GRID_VECTOR_FLOAT:
return CD_PROP_FLOAT3;
case VOLUME_GRID_INT:
return CD_PROP_INT32;
case VOLUME_GRID_BOOLEAN:
case VOLUME_GRID_MASK:
return CD_PROP_BOOL;
default:
break;
}
BLI_assert_unreachable();
return std::nullopt;
}
erik85 marked this conversation as resolved Outdated

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; } ```
#endif /* WITH_OPENVDB */
static void node_geo_exec(GeoNodeExecParams params)
@ -300,26 +283,22 @@ static void node_geo_exec(GeoNodeExecParams params)
}
const NodeGeometrySampleVolume &storage = node_storage(params.node());
const eCustomDataType input_grid_type = eCustomDataType(storage.grid_type);
auto sampling_mode = (const GeometryNodeSampleVolumeSamplingMode)storage.sampling_mode;
auto interpolation_mode = (const GeometryNodeSampleVolumeInterpolationMode)
mod_moder marked this conversation as resolved Outdated

C-style cast there

C-style cast there

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.
storage.interpolation_mode;
GField grid_field = get_input_attribute_field(params, input_grid_type);
const StringRefNull grid_name = get_grid_name(grid_field);
if (grid_name == nullptr) {
if (grid_name == "") {
params.error_message_add(NodeWarningType::Error, TIP_("Grid name needs to be specified"));
params.set_default_remaining_outputs();
return;
}
ValueOrField<float3> position_value_or_field = params.extract_input<ValueOrField<float3>>(
"Position");
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());
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();
return;
}
@ -327,11 +306,8 @@ static void node_geo_exec(GeoNodeExecParams params)
const VolumeGridType grid_type = BKE_volume_grid_type_openvdb(*base_grid);
/* 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) ||
(grid_type == VOLUME_GRID_VECTOR_FLOAT && input_grid_type == CD_PROP_FLOAT3)))
{
if (openvdb_grid_type_to_customdata_type(grid_type).value() != input_grid_type) {
params.set_default_remaining_outputs();
params.error_message_add(
NodeWarningType::Error,
erik85 marked this conversation as resolved Outdated

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.

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

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

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;`
@ -339,53 +315,18 @@ static void node_geo_exec(GeoNodeExecParams params)
return;
}
/* Switch to the Nearest Neighbor sampler for Bool grids (no interpolation). */
if (grid_type == VOLUME_GRID_BOOLEAN &&
sampling_mode != GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_POINT)
interpolation_mode != GEO_NODE_SAMPLE_VOLUME_INTERPOLATION_MODE_NEAREST)
{
params.error_message_add(
NodeWarningType::Warning,
TIP_("Using the Point Sampler because of Boolean Grid type limitation"));
sampling_mode = GEO_NODE_SAMPLE_VOLUME_SAMPLING_MODE_POINT;
interpolation_mode = GEO_NODE_SAMPLE_VOLUME_INTERPOLATION_MODE_NEAREST;
}
erik85 marked this conversation as resolved Outdated

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.
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);
auto op = FieldOperation::Create(std::move(fn), {position_value_or_field.as_field()});
output_field = GField(std::move(op));
}
else {
/* Evaluate single value here if possible to avoid the overhead of the MultiFunction. */
float3 position = position_value_or_field.as_value();
switch (grid_type) {
case VOLUME_GRID_FLOAT: {
output_field = fn::make_constant_field<float>(
sample_grid_single<openvdb::FloatGrid>(base_grid, position, sampling_mode));
break;
}
case VOLUME_GRID_INT: {
output_field = fn::make_constant_field<int>(
sample_grid_single<openvdb::Int32Grid>(base_grid, position, sampling_mode));
break;
}
case VOLUME_GRID_BOOLEAN: {
output_field = fn::make_constant_field<bool>(
sample_grid_single<openvdb::BoolGrid>(base_grid, position, sampling_mode));
break;
}
case VOLUME_GRID_VECTOR_FLOAT: {
openvdb::Vec3f vector = sample_grid_single<openvdb::VectorGrid>(
base_grid, position, sampling_mode);
output_field = fn::make_constant_field<float3>(vector.asV());
break;
}
default:
BLI_assert_unreachable();
break;
}
}
Field<float3> position_field = params.extract_input<Field<float3>>("Position");
auto fn = std::make_shared<SampleVolumeFunction>(
std::move(base_grid), std::move(grid_field), interpolation_mode);
auto op = FieldOperation::Create(std::move(fn), {position_field});
GField output_field = GField(std::move(op));
output_attribute_field(params, std::move(output_field));
params.set_default_remaining_outputs();
erik85 marked this conversation as resolved Outdated

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).

params.set_default_remaining_outputs(); is necessary there?

`params.set_default_remaining_outputs();` is necessary there?