Geometry Node: Index of Nearest #104619
@ -13,9 +13,7 @@ namespace blender::nodes::node_geo_index_of_nearest_cc {
|
|||||||
static void node_declare(NodeDeclarationBuilder &b)
|
static void node_declare(NodeDeclarationBuilder &b)
|
||||||
{
|
{
|
||||||
b.add_input<decl::Vector>(N_("Position")).implicit_field(implicit_field_inputs::position);
|
b.add_input<decl::Vector>(N_("Position")).implicit_field(implicit_field_inputs::position);
|
||||||
mod_moder marked this conversation as resolved
Outdated
|
|||||||
|
b.add_input<decl::Int>(N_("Group ID")).supports_field().hide_value();
|
||||||
b.add_input<decl::Int>(N_("Self Group ID")).supports_field().hide_value().default_value(0);
|
|
||||||
b.add_input<decl::Int>(N_("Nearest Group ID")).supports_field().hide_value().default_value(0);
|
|
||||||
|
|
||||||
b.add_output<decl::Int>(N_("Index")).field_source().description(N_("Index of nearest element"));
|
b.add_output<decl::Int>(N_("Index")).field_source().description(N_("Index of nearest element"));
|
||||||
b.add_output<decl::Bool>(N_("Has Neighbor")).field_source();
|
b.add_output<decl::Bool>(N_("Has Neighbor")).field_source();
|
||||||
@ -25,16 +23,12 @@ class IndexOfNearestFieldInput final : public bke::GeometryFieldInput {
|
|||||||
private:
|
private:
|
||||||
const Field<float3> positions_field_;
|
const Field<float3> positions_field_;
|
||||||
mod_moder marked this conversation as resolved
Outdated
Hans Goudey
commented
These identifiers/names are missing the translation macro These identifiers/names are missing the translation macro `N_`
|
|||||||
const Field<int> group_field_;
|
const Field<int> group_field_;
|
||||||
mod_moder marked this conversation as resolved
Outdated
Hans Goudey
commented
We settled on We settled on `Self Group ID` and `Nearest Group ID` in the module meeting.
|
|||||||
const Field<int> search_group_field_;
|
|
||||||
|
|
||||||
public:
|
public:
|
||||||
IndexOfNearestFieldInput(Field<float3> positions_field,
|
IndexOfNearestFieldInput(Field<float3> positions_field, Field<int> group_field)
|
||||||
Field<int> group_field,
|
|
||||||
Field<int> search_group_field)
|
|
||||||
: bke::GeometryFieldInput(CPPType::get<int>(), "Nearest to"),
|
: bke::GeometryFieldInput(CPPType::get<int>(), "Nearest to"),
|
||||||
positions_field_(std::move(positions_field)),
|
positions_field_(std::move(positions_field)),
|
||||||
group_field_(std::move(group_field)),
|
group_field_(std::move(group_field))
|
||||||
mod_moder marked this conversation as resolved
Outdated
Hans Goudey
commented
Pass spans by value Pass spans by value
|
|||||||
search_group_field_(std::move(search_group_field))
|
|
||||||
{
|
{
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -44,70 +38,47 @@ class IndexOfNearestFieldInput final : public bke::GeometryFieldInput {
|
|||||||
fn::FieldEvaluator evaluator{context, &mask};
|
fn::FieldEvaluator evaluator{context, &mask};
|
||||||
evaluator.add(positions_field_);
|
evaluator.add(positions_field_);
|
||||||
evaluator.add(group_field_);
|
evaluator.add(group_field_);
|
||||||
evaluator.add(search_group_field_);
|
|
||||||
evaluator.evaluate();
|
evaluator.evaluate();
|
||||||
|
|
||||||
const VArray<float3> &positions = evaluator.get_evaluated<float3>(0);
|
const VArray<float3> &positions = evaluator.get_evaluated<float3>(0);
|
||||||
const VArray<int> &group = evaluator.get_evaluated<int>(1);
|
const VArray<int> &group = evaluator.get_evaluated<int>(1);
|
||||||
const VArray<int> &search_group = evaluator.get_evaluated<int>(2);
|
|
||||||
|
|
||||||
const bool use_group = !group.is_single();
|
MultiValueMap<int, int64_t> group_masks;
|
||||||
const bool use_search_group = !search_group.is_single();
|
mask.foreach_index([&](const int index) { group_masks.add(group[index], index); });
|
||||||
|
|
||||||
MultiValueMap<int, int64_t> in_group;
|
|
||||||
MultiValueMap<int, int64_t> out_group;
|
|
||||||
Array<int> indices(mask.min_array_size());
|
Array<int> indices(mask.min_array_size());
|
||||||
|
|
||||||
threading::parallel_invoke(
|
const auto nearest_for = [this, &positions](const IndexMask mask, MutableSpan<int> r_indices) {
|
||||||
Hans Goudey
commented
I don't think there's a benefit to specifying the capture in a case like this (instead of I don't think there's a benefit to specifying the capture in a case like this (instead of `[&]`)
|
|||||||
(indices.size() > 512) && use_search_group && use_group,
|
devirtualize_varray(positions, [mask, r_indices, this](const auto positions) {
|
||||||
[&]() {
|
KDTree_3d *tree = BLI_kdtree_3d_new(mask.size());
|
||||||
if (use_group) {
|
mask.foreach_index([tree, positions](const int index) {
|
||||||
mask.foreach_index([&](const auto index) { in_group.add(group[index], index); });
|
BLI_kdtree_3d_insert(tree, index, positions[index]);
|
||||||
return;
|
|
||||||
}
|
|
||||||
const int group_key = group.get_internal_single();
|
|
||||||
in_group.add_multiple(group_key, {});
|
|
||||||
},
|
|
||||||
[&]() {
|
|
||||||
if (use_search_group) {
|
|
||||||
mask.foreach_index(
|
|
||||||
[&](const auto index) { out_group.add(search_group[index], index); });
|
|
||||||
}
|
|
||||||
});
|
});
|
||||||
|
|
||||||
for (const int key : in_group.keys()) {
|
BLI_kdtree_3d_balance(tree);
|
||||||
/* Never empty. */
|
|
||||||
const IndexMask self_points(use_group ? IndexMask(in_group.lookup(key)) : mask);
|
|
||||||
const IndexMask search_points(use_search_group ? IndexMask(out_group.lookup(key)) :
|
|
||||||
self_points);
|
|
||||||
|
|
||||||
if (search_points.is_empty()) {
|
threading::parallel_for(mask.index_range(), 512, [&](const IndexRange range) {
|
||||||
indices.as_mutable_span().fill_indices(self_points, -1);
|
mask.slice(range).foreach_index([&](const auto index) {
|
||||||
continue;
|
r_indices[index] = this->kdtree_find_neighboard(tree, positions[index], index);
|
||||||
}
|
});
|
||||||
|
});
|
||||||
|
|
||||||
KDTree_3d *tree = BLI_kdtree_3d_new(search_points.size());
|
BLI_kdtree_3d_free(tree);
|
||||||
Jacques Lucke
commented
Can just use a normal for loop here. The performance benefit of using Can just use a normal for loop here. The performance benefit of using `foreach_index` is negligible, because most time is spend in the kdtree traversal.
|
|||||||
|
|
||||||
for (const int64_t index : search_points) {
|
|
||||||
BLI_kdtree_3d_insert(tree, index, positions[index]);
|
|
||||||
}
|
|
||||||
|
|
||||||
BLI_kdtree_3d_balance(tree);
|
|
||||||
|
|
||||||
threading::parallel_for(self_points.index_range(), 512, [&](const IndexRange range) {
|
|
||||||
for (const int64_t index : self_points.slice(range)) {
|
|
||||||
indices[index] = this->kdtree_find_neighboard(tree, positions[index], index);
|
|
||||||
}
|
|
||||||
});
|
});
|
||||||
|
};
|
||||||
|
|
||||||
BLI_kdtree_3d_free(tree);
|
for (const Span<int64_t> mask_span : group_masks.values()) {
|
||||||
|
if (mask_span.size() == 1) {
|
||||||
mod_moder marked this conversation as resolved
Outdated
Hans Goudey
commented
I'd change these names to I'd change these names to `use_group` and `use_search_group` for consistency with your other variable names and because `use` at front sounds more natural.
|
|||||||
|
indices[mask_span.first()] = -1;
|
||||||
|
}
|
||||||
|
nearest_for(mask_span, indices);
|
||||||
}
|
}
|
||||||
|
|
||||||
return VArray<int>::ForContainer(std::move(indices));
|
return VArray<int>::ForContainer(std::move(indices));
|
||||||
}
|
}
|
||||||
Hans Goudey
commented
This comment is a bit vague, but I'm finding it hard to keep track of the separate This comment is a bit vague, but I'm finding it hard to keep track of the separate `group_use` and `group_to_find_use` conditions. I wonder if the first condition could be handled with by splitting logic into a separate function, like `SampleCurveFunction` does with its `sample_curve` lambda.
|
|||||||
|
|
||||||
protected:
|
protected:
|
||||||
mod_moder marked this conversation as resolved
Outdated
Hans Goudey
commented
No strong opinion, but using regular No strong opinion, but using regular `=` assignment seems more consistent here.
|
|||||||
int kdtree_find_neighboard(KDTree_3d *tree, const float3 &position, const int &index) const
|
static int kdtree_find_neighboard(KDTree_3d *tree, const float3 &position, const int &index)
|
||||||
{
|
{
|
||||||
return BLI_kdtree_3d_find_nearest_cb_cpp(
|
return BLI_kdtree_3d_find_nearest_cb_cpp(
|
||||||
tree,
|
tree,
|
||||||
@ -126,12 +97,11 @@ class IndexOfNearestFieldInput final : public bke::GeometryFieldInput {
|
|||||||
{
|
{
|
||||||
positions_field_.node().for_each_field_input_recursive(fn);
|
positions_field_.node().for_each_field_input_recursive(fn);
|
||||||
mod_moder marked this conversation as resolved
Outdated
Hans Goudey
commented
`int key` -> `const int key`
|
|||||||
group_field_.node().for_each_field_input_recursive(fn);
|
group_field_.node().for_each_field_input_recursive(fn);
|
||||||
search_group_field_.node().for_each_field_input_recursive(fn);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
uint64_t hash() const override
|
uint64_t hash() const override
|
||||||
{
|
{
|
||||||
return get_default_hash_3(positions_field_, group_field_, search_group_field_);
|
return get_default_hash_2(positions_field_, group_field_);
|
||||||
}
|
}
|
||||||
mod_moder marked this conversation as resolved
Outdated
Hans Goudey
commented
The result array should never need to be larger than The result array should never need to be larger than `mask.min_array_size()`
|
|||||||
|
|
||||||
bool is_equal_to(const fn::FieldNode &other) const override
|
bool is_equal_to(const fn::FieldNode &other) const override
|
||||||
@ -139,8 +109,7 @@ class IndexOfNearestFieldInput final : public bke::GeometryFieldInput {
|
|||||||
if (const IndexOfNearestFieldInput *other_field =
|
if (const IndexOfNearestFieldInput *other_field =
|
||||||
dynamic_cast<const IndexOfNearestFieldInput *>(&other)) {
|
dynamic_cast<const IndexOfNearestFieldInput *>(&other)) {
|
||||||
return positions_field_ == other_field->positions_field_ &&
|
return positions_field_ == other_field->positions_field_ &&
|
||||||
mod_moder marked this conversation as resolved
Outdated
Jacques Lucke
commented
While forest is used as a technical term for a graph containing multiple trees, I don't think the term should be used for a collection of multiple independent kd trees. Just use While forest is used as a technical term for a graph containing multiple trees, I don't think the term should be used for a collection of multiple independent kd trees. Just use `kdtrees`.
Iliya Katushenock
commented
If i do merge If i do merge `parallel_for`s below, i can delete this vector.
|
|||||||
group_field_ == other_field->group_field_ &&
|
group_field_ == other_field->group_field_;
|
||||||
search_group_field_ == other_field->search_group_field_;
|
|
||||||
}
|
}
|
||||||
return false;
|
return false;
|
||||||
mod_moder marked this conversation as resolved
Outdated
Hans Goudey
commented
I think this should be I think this should be `!mask_is_cheap`
Iliya Katushenock
commented
I was isn't invented the best name, I was isn't invented the best name, `mask_is_cheap` == true if computing of indices make sense and cheap mask is used.
|
|||||||
}
|
}
|
||||||
Hans Goudey
commented
TBH I think it's better to just use TBH I think it's better to just use `int64_t` here to avoid duplicating the functions above. Eventually when `IndexMask` is refactored, these could benefit from using that, and the `int64_t` will make that more clear too.
|
|||||||
@ -154,12 +123,10 @@ class IndexOfNearestFieldInput final : public bke::GeometryFieldInput {
|
|||||||
static void node_geo_exec(GeoNodeExecParams params)
|
static void node_geo_exec(GeoNodeExecParams params)
|
||||||
{
|
{
|
||||||
Field<float3> position_field = params.extract_input<Field<float3>>("Position");
|
Field<float3> position_field = params.extract_input<Field<float3>>("Position");
|
||||||
Iliya Katushenock
commented
Why It seems that trying to use a mask to multiply all user groups by groups from the mask leads to much more overhead in most cases. Why `tree_masks[i]` and `evaluate_masks[i]` is groups with the same id? this isn't sorted somether or protected, that mask isn't avoid all elements of some grouop and create offset for all other groups even all of that added in same order.
It seems that trying to use a mask to multiply all user groups by groups from the mask leads to much more overhead in most cases.
If you don't do some kind of analogue of a tree, then it's like doing a boolean through mask1 * mask2. When there is something like bvh. so I think it's easier to just calculate everything. I'm not sure there are many cases now where the mask can actually be a small part.
Hans Goudey
commented
Oh, great point! I think it's still worth having a separate evaluation mask. Even just the set position node with a small selection would benefit from it, I think kd tree lookups are fairly expensive. I guess both masks have to be created at the same time. Oh, great point! I think it's still worth having a separate evaluation mask. Even just the set position node with a small selection would benefit from it, I think kd tree lookups are fairly expensive. I guess both masks have to be created at the same time.
|
|||||||
|
Field<int> group_field = params.extract_input<Field<int>>("Group ID");
|
||||||
Field<int> self_group_field = params.extract_input<Field<int>>("Self Group ID");
|
|
||||||
Field<int> search_group_field = params.extract_input<Field<int>>("Nearest Group ID");
|
|
||||||
|
|
||||||
Field<int> index_of_nearest_field(std::make_shared<IndexOfNearestFieldInput>(
|
Field<int> index_of_nearest_field(std::make_shared<IndexOfNearestFieldInput>(
|
||||||
std::move(position_field), std::move(self_group_field), std::move(search_group_field)));
|
std::move(position_field), std::move(group_field)));
|
||||||
|
|
||||||
Hans Goudey
commented
I do think one of these indices could be skipped if the selection is complete I do think one of these indices could be skipped if the selection is complete
Hans Goudey
commented
Use Use `IndexMask` instead of `Span<int64_t>` for these `mask_of_tree` and `mask` variables
|
|||||||
if (params.output_is_required("Index")) {
|
if (params.output_is_required("Index")) {
|
||||||
mod_moder marked this conversation as resolved
Outdated
Hans Goudey
commented
References typically indicate a lack of ownership, better to stick with a pointer here References typically indicate a lack of ownership, better to stick with a pointer here
|
|||||||
static auto clamp_fn = mf::build::SI1_SO<int, int>(
|
static auto clamp_fn = mf::build::SI1_SO<int, int>(
|
||||||
|
I think these UI includes are unused