Geometry Node: Index of Nearest #104619
|
@ -2,8 +2,6 @@
|
|||
|
||||
#include "BKE_attribute_math.hh"
|
||||
|
||||
#include "NOD_socket_search_link.hh"
|
||||
|
||||
#include "BLI_kdtree.h"
|
||||
#include "BLI_multi_value_map.hh"
|
||||
#include "BLI_task.hh"
|
||||
|
@ -25,18 +23,18 @@ static void node_declare(NodeDeclarationBuilder &b)
|
|||
|
||||
class IndexOfNearestFieldInput final : public bke::GeometryFieldInput {
|
||||
mod_moder marked this conversation as resolved
Outdated
|
||||
private:
|
||||
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<float3> positions_;
|
||||
const Field<int> group_;
|
||||
const Field<int> search_group_;
|
||||
const Field<float3> positions_field_;
|
||||
const Field<int> group_field_;
|
||||
const Field<int> search_group_field_;
|
||||
|
||||
public:
|
||||
IndexOfNearestFieldInput(const Field<float3> positions,
|
||||
const Field<int> group,
|
||||
const Field<int> search_group)
|
||||
IndexOfNearestFieldInput(const Field<float3> positions_field,
|
||||
mod_moder marked this conversation as resolved
Outdated
Hans Goudey
commented
Pass spans by value Pass spans by value
|
||||
const Field<int> group_field,
|
||||
const Field<int> search_group_field)
|
||||
: bke::GeometryFieldInput(CPPType::get<int>(), "Nearest to"),
|
||||
positions_(std::move(positions)),
|
||||
group_(std::move(group)),
|
||||
search_group_(std::move(search_group))
|
||||
positions_field_(std::move(positions_field)),
|
||||
group_field_(std::move(group_field)),
|
||||
search_group_field_(std::move(search_group_field))
|
||||
{
|
||||
}
|
||||
|
||||
|
@ -44,9 +42,9 @@ class IndexOfNearestFieldInput final : public bke::GeometryFieldInput {
|
|||
IndexMask mask) const final
|
||||
{
|
||||
fn::FieldEvaluator evaluator{context, &mask};
|
||||
evaluator.add(positions_);
|
||||
evaluator.add(group_);
|
||||
evaluator.add(search_group_);
|
||||
evaluator.add(positions_field_);
|
||||
evaluator.add(group_field_);
|
||||
evaluator.add(search_group_field_);
|
||||
evaluator.evaluate();
|
||||
|
||||
const VArray<float3> &positions = evaluator.get_evaluated<float3>(0);
|
||||
|
@ -109,13 +107,13 @@ class IndexOfNearestFieldInput final : public bke::GeometryFieldInput {
|
|||
}
|
||||
|
||||
protected:
|
||||
int kdtree_find_neighboard(KDTree_3d *tree, const float3 &position, int index) const
|
||||
int kdtree_find_neighboard(KDTree_3d *tree, const float3 &position, const int index) const
|
||||
{
|
||||
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.
|
||||
return BLI_kdtree_3d_find_nearest_cb_cpp(
|
||||
tree,
|
||||
position,
|
||||
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.
|
||||
0,
|
||||
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.
|
||||
[index](const int other_new_i, const float * /*co*/, float /*dist_sq*/) {
|
||||
[index](const int other_new_i, const float * /*co*/, const float /*dist_sq*/) {
|
||||
mod_moder marked this conversation as resolved
Outdated
Hans Goudey
commented
`Vector` -> `Array` here
|
||||
if (index == other_new_i) {
|
||||
return 0;
|
||||
}
|
||||
|
@ -126,29 +124,30 @@ class IndexOfNearestFieldInput final : public bke::GeometryFieldInput {
|
|||
public:
|
||||
void for_each_field_input_recursive(FunctionRef<void(const FieldInput &)> fn) const
|
||||
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.
|
||||
{
|
||||
positions_.node().for_each_field_input_recursive(fn);
|
||||
group_.node().for_each_field_input_recursive(fn);
|
||||
search_group_.node().for_each_field_input_recursive(fn);
|
||||
positions_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);
|
||||
}
|
||||
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
|
||||
|
||||
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
|
||||
uint64_t hash() const override
|
||||
{
|
||||
Jacques Lucke
commented
Feels like this Feels like this `parallel_for` loop and the one below can be combined into one. This could potentially improve multi-threaded performance.
|
||||
return get_default_hash_3(positions_, group_, search_group_);
|
||||
return get_default_hash_3(positions_field_, group_field_, search_group_field_);
|
||||
}
|
||||
|
||||
bool is_equal_to(const fn::FieldNode &other) const override
|
||||
{
|
||||
if (const IndexOfNearestFieldInput *other_field =
|
||||
dynamic_cast<const IndexOfNearestFieldInput *>(&other)) {
|
||||
return positions_ == other_field->positions_ && group_ == other_field->group_ &&
|
||||
search_group_ == other_field->search_group_;
|
||||
return positions_field_ == other_field->positions_field_ &&
|
||||
group_field_ == other_field->group_field_ &&
|
||||
search_group_field_ == other_field->search_group_field_;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
std::optional<eAttrDomain> preferred_domain(const GeometryComponent &component) const override
|
||||
{
|
||||
return bke::try_detect_field_domain(component, positions_);
|
||||
return bke::try_detect_field_domain(component, positions_field_);
|
||||
}
|
||||
};
|
||||
|
||||
|
@ -164,9 +163,9 @@ static void node_geo_exec(GeoNodeExecParams params)
|
|||
|
||||
if (params.output_is_required("Index")) {
|
||||
static auto clamp_fn = mf::build::SI1_SO<int, int>(
|
||||
mod_moder marked this conversation as resolved
Outdated
Hans Goudey
commented
If using I think it would be fine to retrieve them directly in If using `std::move` with these variables, don't declare them const.
I think it would be fine to retrieve them directly in `make_shared` though, rather than having temporary variables.
|
||||
"Clamp",
|
||||
"Index Clamping",
|
||||
[](const int index) { return math::max(0, index); },
|
||||
mf::build::exec_presets::AllSpanOrSingle());
|
||||
mf::build::exec_presets::Materialized());
|
||||
auto clamp_op = std::make_shared<FieldOperation>(
|
||||
FieldOperation(std::move(clamp_fn), {index_of_nearest_field}));
|
||||
params.set_output("Index", Field<int>(clamp_op, 0));
|
||||
|
@ -174,10 +173,9 @@ static void node_geo_exec(GeoNodeExecParams params)
|
|||
|
||||
if (params.output_is_required("Valid")) {
|
||||
static auto valid_fn = mf::build::SI1_SO<int, bool>(
|
||||
"Valid Index",
|
||||
"Index Validating",
|
||||
[](const int index) { return index != -1; },
|
||||
mf::build::exec_presets::AllSpanOrSingle());
|
||||
|
||||
mf::build::exec_presets::Materialized());
|
||||
auto valid_op = std::make_shared<FieldOperation>(
|
||||
FieldOperation(std::move(valid_fn), {std::move(index_of_nearest_field)}));
|
||||
params.set_output("Valid", Field<bool>(valid_op, 0));
|
||||
|
|
These identifiers/names are missing the translation macro
N_