blender
/
blender
136
407
Fork 589
Code Issues 6.3k Pull Requests 768 Projects 19 Wiki Activity

Geometry Node: Index of Nearest #104619

Merged
Jacques Lucke merged 31 commits from mod_moder/blender:index_of_nearest into main 2023-04-22 13:12:03 +02:00
Conversation 6 Commits 31 Files Changed 8 +21 -33
1 changed files with 21 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 590a9c0781 - Show all commits

54
source/blender/nodes/geometry/nodes/node_geo_index_of_nearest.cc
View File

@ -1,9 +1,9 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
#include "BLI_array.hh"
#include "BLI_kdtree.h"
#include "BLI_map.hh"
#include "BLI_task.hh"
#include "BLI_vector.hh"
#include "node_geometry_util.hh"
@ -18,20 +18,10 @@ static void node_declare(NodeDeclarationBuilder &b)
b.add_output<decl::Bool>(N_("Has Neighbor")).field_source();
}
static KDTree_3d *build_kdtree(const Span<float3> &positions, const Span<int> indices)
static KDTree_3d *build_kdtree(const Span<float3> positions, const IndexMask mask)
{
KDTree_3d *tree = BLI_kdtree_3d_new(indices.size());
for (const int index : indices) {
BLI_kdtree_3d_insert(tree, index, positions[index]);
}
BLI_kdtree_3d_balance(tree);
return tree;
}
static KDTree_3d *build_kdtree(const Span<float3> &positions, const IndexRange range)
{
KDTree_3d *tree = BLI_kdtree_3d_new(range.size());
for (const int index : range) {
KDTree_3d *tree = BLI_kdtree_3d_new(mask.size());
for (const int index : mask) {
mod_moder marked this conversation as resolved Outdated
Hans Goudey commented 2023-04-04 23:24:09 +02:00
Outdated
Review
Copy Link

These identifiers/names are missing the translation macro N_

These identifiers/names are missing the translation macro `N_`
BLI_kdtree_3d_insert(tree, index, positions[index]);
mod_moder marked this conversation as resolved Outdated
Hans Goudey commented 2023-04-04 23:27:53 +02:00
Outdated
Review
Copy Link

We settled on Self Group ID and Nearest Group ID in the module meeting.

We settled on `Self Group ID` and `Nearest Group ID` in the module meeting.
}
BLI_kdtree_3d_balance(tree);
@ -46,17 +36,6 @@ static int find_nearest_non_self(const KDTree_3d &tree, const float3 &position,
});
}
static void find_neighbors(const KDTree_3d &tree,
const Span<float3> positions,
const Span<int> mask,
MutableSpan<int> r_indices)
{
threading::parallel_for(mask.index_range(), 512, [&](const IndexRange range) {
for (const int index : mask.slice(range)) {
r_indices[index] = find_nearest_non_self(tree, positions[index], index);
}
});
}
static void find_neighbors(const KDTree_3d &tree,
const Span<float3> positions,
const IndexMask mask,
@ -112,10 +91,16 @@ class IndexOfNearestFieldInput final : public bke::GeometryFieldInput {
group_indexing.add(group_id);
}
Vector<Vector<int>> mask_indices(group_indexing.size());
Vector<Vector<int>> tree_indices(group_indexing.size());
const bool mask_is_cheap = mask.size() < domain_size / 2;
mod_moder marked this conversation as resolved Outdated
Hans Goudey commented 2023-04-21 14:54:01 +02:00
Outdated
Review
Copy Link

We can't selectively ignore the mask actually, it may be incorrect to write the output to non-selected indices. So this check really has to be mask.size() == domain_size

We can't selectively ignore the mask actually, it may be incorrect to write the output to non-selected indices. So this check really has to be `mask.size() == domain_size`
Iliya Katushenock commented 2023-04-21 15:00:25 +02:00
Outdated
Review
Copy Link

I see.
Just if domain size is 10000, mask is 9999, is no much sense to compute the cheap mask. I think, is better to just allocate all elements if cheap mask is used.

I see. Just if domain size is 10000, mask is 9999, is no much sense to compute the cheap mask. I think, is better to just allocate all elements if cheap mask is used.
mod_moder marked this conversation as resolved Outdated
Hans Goudey commented 2023-04-21 15:31:33 +02:00
Outdated
Review
Copy Link

Replace this with const bool mask_is_full = mask.size() == domain_size;

Replace this with `const bool mask_is_full = mask.size() == domain_size;`
const auto build_group_masks = [&](const IndexMask mask, MutableSpan<Vector<int>> r_groups) {
Array<Vector<int64_t>> tree_indices(group_indexing.size());
Array<Vector<int64_t>> mask_indices;
if (mask_is_cheap) {
mod_moder marked this conversation as resolved Outdated
Hans Goudey commented 2023-04-04 23:41:29 +02:00
Outdated
Review
Copy Link

int key -> const int key

`int key` -> `const int key`
mask_indices.reinitialize(group_indexing.size());
}
const auto build_group_masks = [&](const IndexMask mask,
MutableSpan<Vector<int64_t>> r_groups) {
for (const int index : mask) {
const int group_id = group[index];
mod_moder marked this conversation as resolved Outdated
Hans Goudey commented 2023-04-21 15:36:53 +02:00
Outdated
Review
Copy Link

The result array should never need to be larger than mask.min_array_size()

The result array should never need to be larger than `mask.min_array_size()`
const int index_of_group = group_indexing.index_of_try(group_id);
@ -126,15 +111,19 @@ class IndexOfNearestFieldInput final : public bke::GeometryFieldInput {
};
mod_moder marked this conversation as resolved Outdated
Jacques Lucke commented 2023-04-21 13:15:33 +02:00
Outdated
Review
Copy Link

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.

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`.
👍 1
Iliya Katushenock commented 2023-04-21 13:39:08 +02:00
Outdated
Review
Copy Link

If i do merge parallel_fors below, i can delete this vector.

If i do merge `parallel_for`s below, i can delete this vector.
threading::parallel_invoke(
mask.size() + domain_size > 1024,
[&]() { build_group_masks(mask, mask_indices); },
domain_size > 1024 && mask_is_cheap,
mod_moder marked this conversation as resolved Outdated
Hans Goudey commented 2023-04-21 14:54:21 +02:00
Outdated
Review
Copy Link

I think this should be !mask_is_cheap

I think this should be `!mask_is_cheap`
Iliya Katushenock commented 2023-04-21 14:58:40 +02:00
Outdated
Review
Copy Link

I was isn't invented the best name, mask_is_cheap == true if computing of indices make sense and cheap mask is used.

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 2023-04-21 14:21:21 +02:00
Outdated
Review
Copy Link

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.

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.
if (mask_is_cheap) {
mod_moder marked this conversation as resolved Outdated
Hans Goudey commented 2023-04-21 14:18:39 +02:00
Outdated
Review
Copy Link

Vector -> Array here

`Vector` -> `Array` here
😆 1
build_group_masks(mask, mask_indices);
}
},
[&]() { build_group_masks(IndexMask(domain_size), tree_indices); });
Hans Goudey commented 2023-04-21 15:50:24 +02:00
Outdated
Review
Copy Link

domain_size > 1024 && use_cheap_mask -> domain_size > 1024 && !mask_is_full

`domain_size > 1024 && use_cheap_mask` -> `domain_size > 1024 && !mask_is_full`
threading::parallel_for(group_indexing.index_range(), 256, [&](const IndexRange range) {
Iliya Katushenock commented 2023-04-20 21:25:01 +02:00
Outdated
Review
Copy Link

Are grain_size is a power of 2?

Are `grain_size` is a power of 2?
Hans Goudey commented 2023-04-20 21:32:11 +02:00
Outdated
Review
Copy Link

They don't need to be powers of two, though I think it can be slightly beneficial. Since these numbers are a bit arbitrary anyway I started using powers of two.

They don't need to be powers of two, though I think it can be slightly beneficial. Since these numbers are a bit arbitrary anyway I started using powers of two.
Iliya Katushenock commented 2023-04-20 21:34:12 +02:00
Outdated
Review
Copy Link

I remember fixing a bug, only related to the fact that there was no power of 2x.
For the blur node, I made the search function for the top bit part in the threading namespace.

I remember fixing a bug, only related to the fact that there was no power of 2x. For the blur node, I made the search function for the top bit part in the threading namespace.
for (const int index : range) {
const Span<int> mask_of_tree = tree_indices[index];
const Span<int> mask = mask_indices[index];
const Span<int64_t> mask_of_tree = tree_indices[index];
KDTree_3d &tree = *build_kdtree(positions, mask_of_tree);
Iliya Katushenock commented 2023-04-20 21:48:04 +02:00
Outdated
Review
Copy Link

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.

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 2023-04-20 21:56:57 +02:00
Outdated
Review
Copy Link

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.
const Span<int64_t> mask = mask_is_cheap ? mask_indices[index].as_span() : mask_of_tree;
find_neighbors(tree, positions, mask, result);
BLI_kdtree_3d_free(&tree);
}
@ -201,7 +190,6 @@ class HasNeighborFieldInput final : public bke::GeometryFieldInput {
return VArray<bool>::ForSingle(true, mask.min_array_size());
}
/* When a group ID is contained in the set, it means there is only one element with that ID. */
Map<int, int> counts;
const VArraySpan<int> group_span(group);
mask.foreach_index([&](const int i) {