Page MenuHome
Differential D13520 Diff 45844 source/blender/nodes/geometry/nodes/node_geo_attribute_statistic.cc

Changeset View

Standalone View

View Options

source/blender/nodes/geometry/nodes/node_geo_attribute_statistic.cc

Show All 25 Lines
namespace blender::nodes::node_geo_attribute_statistic_cc { namespace blender::nodes::node_geo_attribute_statistic_cc {
static void node_declare(NodeDeclarationBuilder &b) static void node_declare(NodeDeclarationBuilder &b)
{ {
b.add_input<decl::Geometry>(N_("Geometry")); b.add_input<decl::Geometry>(N_("Geometry"));
b.add_input<decl::Float>(N_("Attribute")).hide_value().supports_field(); b.add_input<decl::Float>(N_("Attribute")).hide_value().supports_field();
b.add_input<decl::Vector>(N_("Attribute"), "Attribute_001").hide_value().supports_field(); b.add_input<decl::Vector>(N_("Attribute"), "Attribute_001").hide_value().supports_field();
b.add_input<decl::Bool>(N_("Selection")).default_value(true).supports_field().hide_value();
HooglyBooglyUnsubmitted
Done
Inline Actions

Selection inputs should go below the geometry input.

HooglyBoogly: Selection inputs should go below the geometry input.
b.add_output<decl::Float>(N_("Mean")); b.add_output<decl::Float>(N_("Mean"));
b.add_output<decl::Float>(N_("Median")); b.add_output<decl::Float>(N_("Median"));
b.add_output<decl::Float>(N_("Sum")); b.add_output<decl::Float>(N_("Sum"));
b.add_output<decl::Float>(N_("Min")); b.add_output<decl::Float>(N_("Min"));
b.add_output<decl::Float>(N_("Max")); b.add_output<decl::Float>(N_("Max"));
b.add_output<decl::Float>(N_("Range")); b.add_output<decl::Float>(N_("Range"));
b.add_output<decl::Float>(N_("Standard Deviation")); b.add_output<decl::Float>(N_("Standard Deviation"));
▲ Show 20 Lines • Show All 109 Lines • ▼ Show 20 Linesstatic void node_geo_exec(GeoNodeExecParams params)
const bNode &node = params.node(); const bNode &node = params.node();
const CustomDataType data_type = static_cast<CustomDataType>(node.custom1); const CustomDataType data_type = static_cast<CustomDataType>(node.custom1);
const AttributeDomain domain = static_cast<AttributeDomain>(node.custom2); const AttributeDomain domain = static_cast<AttributeDomain>(node.custom2);
int64_t total_size = 0; int64_t total_size = 0;
Vector<const GeometryComponent *> components = geometry_set.get_components_for_read(); Vector<const GeometryComponent *> components = geometry_set.get_components_for_read();
for (const GeometryComponent *component : components) { for (const GeometryComponent *component : components) {
if (component->attribute_domain_supported(domain)) { if (component->attribute_domain_supported(domain)) {
total_size += component->attribute_domain_size(domain); total_size += component->attribute_domain_size(domain);
} }
} }
if (total_size == 0) { if (total_size == 0) {
params.set_default_remaining_outputs(); params.set_default_remaining_outputs();
return; return;
} }
const Field<bool> selection_field = params.get_input<Field<bool>>("Selection");
HooglyBooglyUnsubmitted
Done
Inline Actions

Maximum size is never used, so this calculation could be completely removed.

HooglyBoogly: Maximum size is never used, so this calculation could be completely removed.
switch (data_type) { switch (data_type) {
case CD_PROP_FLOAT: { case CD_PROP_FLOAT: {
const Field<float> input_field = params.get_input<Field<float>>("Attribute"); const Field<float> input_field = params.get_input<Field<float>>("Attribute");
Array<float> data = Array<float>(total_size);
int offset = 0; Vector<float> data(total_size, 0.0f);
total_size = 0;
HooglyBooglyUnsubmitted
Done
Inline Actions

I don't think there's any need to initialize the values.

HooglyBoogly: I don't think there's any need to initialize the values.
for (const GeometryComponent *component : components) { for (const GeometryComponent *component : components) {
if (component->attribute_domain_supported(domain)) { if (component->attribute_domain_supported(domain)) {
GeometryComponentFieldContext field_context{*component, domain}; GeometryComponentFieldContext field_context{*component, domain};
const int domain_size = component->attribute_domain_size(domain); const int domain_size = component->attribute_domain_size(domain);
HooglyBooglyUnsubmitted
Done
Inline Actions

No need for a temporary array here if it's just going to be copied into the data vector after.

HooglyBoogly: No need for a temporary array here if it's just going to be copied into the `data` vector after.
fn::FieldEvaluator selection_evaluator{field_context, domain_size};
selection_evaluator.add(selection_field);
selection_evaluator.evaluate();
const IndexMask selection = selection_evaluator.get_evaluated_as_mask(0);
Array<float> data_in(domain_size);
fn::FieldEvaluator data_evaluator{field_context, domain_size}; fn::FieldEvaluator data_evaluator{field_context, domain_size};
MutableSpan<float> component_result = data.as_mutable_span().slice(offset, domain_size); MutableSpan<float> component_result = data_in.as_mutable_span();
data_evaluator.add_with_destination(input_field, component_result); data_evaluator.add_with_destination(input_field, component_result);
data_evaluator.evaluate(); data_evaluator.evaluate();
HooglyBooglyUnsubmitted
Done
Inline Actions

Moving from the selection field won't work if there is more than one component

HooglyBoogly: Moving from the selection field won't work if there is more than one component
offset += domain_size;
int position = 0;
for (const int i : selection) {
data[total_size + position++] = component_result[i];
HooglyBooglyUnsubmitted
Done
Inline Actions

I think these could be simplified by slicing data beforehand, then iterating over selection.index_range() rather than keeping track of a "position"

HooglyBoogly: I think these could be simplified by slicing `data` beforehand, then iterating over `selection.
HooglyBooglyUnsubmitted
Done
Inline Actions

I still think this would be an improvement.

HooglyBoogly: I still think this would be an improvement.
HooglyBooglyUnsubmitted
Not Done
Inline Actions

const (same with the other)

HooglyBoogly: `const` (same with the other)
} }
HooglyBooglyUnsubmitted
Done
Inline Actions

I doesn't seem correct to subtract one here, what's the reasoning for that?

HooglyBoogly: I doesn't seem correct to subtract one here, what's the reasoning for that?
guitargeekAuthorUnsubmitted
Done
Inline Actions

just a severe case of copy-and-paste-itis

guitargeek: just a severe case of copy-and-paste-itis
total_size += selection.size();
} }
}
data.resize(total_size);
HooglyBooglyUnsubmitted
Done
Inline Actions

I'm not sure how resizing this *after* adding the data to it works, I think data could just be an array instead, since the size is known beforehand.

HooglyBoogly: I'm not sure how resizing this *after* adding the data to it works, I think `data` could just…
guitargeekAuthorUnsubmitted
Done
Inline Actions

Maybe I'm misunderstanding something then. Here was my logic:

  • Make a temp array large enough for all the source data
  • Evaluate the source data into the temp array
  • get the mask
  • use the mask to only copy the selected source data into the Vector that will have calculations done on it.
  • Since there may now be room at the end of this vector, chop it off to the right length
  • Move along to the stats.
guitargeek: Maybe I'm misunderstanding something then. Here was my logic: - Make a temp array large…
HooglyBooglyUnsubmitted
Done
Inline Actions

That works, but there's no need to evaluate into an array directly, you can just use const VArray<float3> &component_data = data_evaluator.get_evaluated<float3>()

And you resized the vector to total_size earlier in the function, there shouldn't be any extra room at the end. (You never call append)

HooglyBoogly: That works, but there's no need to evaluate into an array directly, you can just use `const…
float mean = 0.0f; float mean = 0.0f;
float median = 0.0f; float median = 0.0f;
float sum = 0.0f; float sum = 0.0f;
float min = 0.0f; float min = 0.0f;
float max = 0.0f; float max = 0.0f;
float range = 0.0f; float range = 0.0f;
float standard_deviation = 0.0f; float standard_deviation = 0.0f;
▲ Show 20 Lines • Show All 41 Lines • ▼ Show 20 Lines case CD_PROP_FLOAT: {
params.set_output("Standard Deviation", standard_deviation); params.set_output("Standard Deviation", standard_deviation);
params.set_output("Variance", variance); params.set_output("Variance", variance);
} }
break; break;
} }
case CD_PROP_FLOAT3: { case CD_PROP_FLOAT3: {
const Field<float3> input_field = params.get_input<Field<float3>>("Attribute_001"); const Field<float3> input_field = params.get_input<Field<float3>>("Attribute_001");
Array<float3> data = Array<float3>(total_size); Vector<float3> data(total_size);
int offset = 0; total_size = 0;
HooglyBooglyUnsubmitted
Done
Inline Actions

Don't reuse a variable and give it a different meaning.

HooglyBoogly: Don't reuse a variable and give it a different meaning.
for (const GeometryComponent *component : components) { for (const GeometryComponent *component : components) {
if (component->attribute_domain_supported(domain)) { if (component->attribute_domain_supported(domain)) {
GeometryComponentFieldContext field_context{*component, domain}; GeometryComponentFieldContext field_context{*component, domain};
const int domain_size = component->attribute_domain_size(domain); const int domain_size = component->attribute_domain_size(domain);
fn::FieldEvaluator selection_evaluator{field_context, domain_size};
selection_evaluator.add(selection_field);
selection_evaluator.evaluate();
const IndexMask selection = selection_evaluator.get_evaluated_as_mask(0);
Array<float3> data_in(domain_size);
fn::FieldEvaluator data_evaluator{field_context, domain_size}; fn::FieldEvaluator data_evaluator{field_context, domain_size};
MutableSpan<float3> component_result = data.as_mutable_span().slice(offset, domain_size); MutableSpan<float3> component_result = data_in.as_mutable_span();
data_evaluator.add_with_destination(input_field, component_result); data_evaluator.add_with_destination(input_field, component_result);
data_evaluator.evaluate(); data_evaluator.evaluate();
offset += domain_size;
int position = 0;
for (const int i : selection) {
data[total_size + position++] = component_result[i];
}
total_size += selection.size();
} }
} }
data.resize(total_size);
HooglyBooglyUnsubmitted
Done
Inline Actions

It looks simpler to just skip this variable and call data.size() below.

HooglyBoogly: It looks simpler to just skip this variable and call `data.size()` below.
float3 median{0}; float3 median{0};
float3 min{0}; float3 min{0};
float3 max{0}; float3 max{0};
float3 range{0}; float3 range{0};
float3 sum{0}; float3 sum{0};
float3 mean{0}; float3 mean{0};
float3 variance{0}; float3 variance{0};
▲ Show 20 Lines • Show All 93 Lines • Show Last 20 Lines