Geometry Node: Index of Nearest #104619
Merged
Jacques Lucke
merged 31 commits from 2023-04-22 13:12:03 +02:00
mod_moder/blender:index_of_nearest
into main
Reviewers
Request review
No reviewers
Labels
Clear labels
This issue affects/is about backward or forward compatibility
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports
Apply labels
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
This issue affects/is about backward or forward compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest: Wayland
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest: Wayland
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Set Project
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#104619
Reference in New Issue
There is no content yet.
Delete Branch "mod_moder/blender:index_of_nearest"
Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?
Geometry Node: Index of Nearest
A node to find the index of the closest element to itself.
See: #102387
Nice idea, and thanks for the clear design task. I think it would be interesting to align the naming a bit closer to the corresponding "Sample Nearest" node that has a geometry socket. Also the "Group ID to Search" name could probably be simplified as "Search ID".
Maybe we should talk a bit about naming in a sub-module meeting.
It's also a bit hard for me to be confident about the design without seeing alternative ideas explored, i.e. what are the "first principles" here and what abstractions can we use to solve the use cases.
Traversing neighbors in order, with a field callback for each i-other pair, and the ability to return an array of indices would be the most general solution. Though that would be a mega node given the need for various optimization techniques and support for arbitrary geometry/geometry in context.
About groups.
I see that the inverse option would be very useful. That is, don't look for the nearest one in group A, as the search is for group A, but look for all of non-A, for every A. But this only makes sense as a field. And this greatly increases the complexity of the node. May be added later.
@blender-bot package
Package build started. Download here when ready.
@ -107,1 +107,4 @@
}
template<typename Fn>
inline int BLI_kdtree_nd_(find_nearest_cb)(const KDTree *tree,
Might as well be consistent with the other function here and use the
_cpp
suffix too.@ -324,6 +324,7 @@ DefNode(GeometryNode, GEO_NODE_FLIP_FACES, 0, "FLIP_FACES", FlipFaces, "Flip Fac
DefNode(GeometryNode, GEO_NODE_GEOMETRY_TO_INSTANCE, 0, "GEOMETRY_TO_INSTANCE", GeometryToInstance, "Geometry to Instance", "Convert each input geometry into an instance, which can be much faster than the Join Geometry node when the inputs are large")
DefNode(GeometryNode, GEO_NODE_IMAGE_INFO, 0, "IMAGE_INFO", ImageInfo, "Image Info", "Retrieve information about an image")
DefNode(GeometryNode, GEO_NODE_IMAGE_TEXTURE, def_geo_image_texture, "IMAGE_TEXTURE", ImageTexture, "Image Texture", "Sample values from an image texture")
DefNode(GeometryNode, GEO_NODE_INDEX_OF_NEAREST, 0, "INDEX_OF_NEAREST", IndexOfNearest, "Index of Nearest", "Index of nearest element by groups condition")
For the description, I'd suggest:
Find the nearest element in the a group
Adding
. Similar to the \"Sample Nearest\" node
might be helpful too.@ -0,0 +8,4 @@
#include "BLI_multi_value_map.hh"
#include "BLI_task.hh"
#include "BKE_geometry_fields.hh"
BKE_geometry_fields.hh
is already included indirectly bynode_geometry_util.hh
@ -0,0 +12,4 @@
#include "node_geometry_util.hh"
#include "UI_interface.h"
I think these UI includes are unused
@ -0,0 +21,4 @@
{
b.add_input<decl::Vector>("Position").implicit_field(implicit_field_inputs::position);
b.add_input<decl::Int>("Self Group ID").supports_field().hide_value().default_value(0);
These identifiers/names are missing the translation macro
N_
@ -0,0 +22,4 @@
b.add_input<decl::Vector>("Position").implicit_field(implicit_field_inputs::position);
b.add_input<decl::Int>("Self Group ID").supports_field().hide_value().default_value(0);
b.add_input<decl::Int>("Group ID to Search").supports_field().hide_value().default_value(0);
We settled on
Self Group ID
andNearest Group ID
in the module meeting.@ -0,0 +68,4 @@
const VArray<int> group = evaluator.get_evaluated<int>(1);
const VArray<int> search_group = evaluator.get_evaluated<int>(2);
const bool group_use = !group.is_single();
I'd change these names to
use_group
anduse_search_group
for consistency with your other variable names and becauseuse
at front sounds more natural.@ -0,0 +95,4 @@
}
});
for (int key : in_group.keys()) {
int key
->const int key
@ -0,0 +162,4 @@
{
const Field<float3> position = params.extract_input<Field<float3>>("Position");
const Field<int> self_group = params.extract_input<Field<int>>("Self Group ID");
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.@ -0,0 +75,4 @@
MultiValueMap<int, int64_t> out_group;
Array<int> indices(mask.min_array_size());
threading::parallel_invoke((indices.size() > 512) && group_to_find_use && group_use,
This comment is a bit vague, but I'm finding it hard to keep track of the separate
group_use
andgroup_to_find_use
conditions. I wonder if the first condition could be handled with by splitting logic into a separate function, likeSampleCurveFunction
does with itssample_curve
lambda.This would be fantastic. Would love to use this to grow non-intersecting things like plants or rivers. Would be really awesome!
@ -0,0 +77,4 @@
for (const int key : in_group.keys()) {
/* Never empty. */
const IndexMask self_points(use_group ? IndexMask(in_group.lookup(key)) : mask);
No strong opinion, but using regular
=
assignment seems more consistent here.@ -0,0 +86,4 @@
continue;
}
KDTree_3d *tree = BLI_kdtree_3d_new(search_points.size());
It seems like creating a KD tree for every group ID will mean that the KD tree for the same set of search points will potentially be built multiple times. There should probably be some way to avoid building a tree multiple times for the same key.
Overall this area looks much cleaner than the last time I checked though!
After you point it out... it seems I made some kind of logical error in the implementation of switching groups when iterating over them. Or not .. i need to redo this part of the code
@ -0,0 +119,4 @@
/* The grain size should be larger as each tree gets smaller. */
const int avg_tree_size = group_ids.size() / group_mask_storage.size();
const int grain_size = std::max(8192 / avg_tree_size, 1);
Are
grain_size
is a power of 2?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.
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.
@ -0,0 +122,4 @@
const int grain_size = std::max(8192 / avg_tree_size, 1);
threading::parallel_for(tree_masks.index_range(), grain_size, [&](const IndexRange range) {
for (const int i : range) {
const IndexMask tree_mask = tree_masks[i];
Why
tree_masks[i]
andevaluate_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.
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.
@ -108,0 +110,4 @@
inline int BLI_kdtree_nd_(find_nearest_cb_cpp)(const KDTree *tree,
const float co[KD_DIMS],
KDTreeNearest *r_nearest,
const Fn &fn)
Can just use
Fn &&fn
, then you also don't need the const cast below I think.Should i also change one other _cpp wrapper above?
Just keep other code as is right now.
@ -0,0 +63,4 @@
MutableSpan<int> r_indices)
{
threading::parallel_for(mask.index_range(), 512, [&](const IndexRange range) {
mask.slice(range).foreach_index([&tree, positions, r_indices](const int index) {
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.@ -0,0 +108,4 @@
VectorSet<int> group_indexing;
Vector<Vector<int>> mask_indices;
Vector<Vector<int>> tree_indices;
Vector<KDTree_3d *> forest;
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
.If i do merge
parallel_for
s below, i can delete this vector.@ -0,0 +130,4 @@
}
}
threading::parallel_for(group_indexing.index_range(), 8, [&](const IndexRange range) {
Feels like this
parallel_for
loop and the one below can be combined into one. This could potentially improve multi-threaded performance.@ -0,0 +28,4 @@
return tree;
}
static KDTree_3d *build_kdtree(const Span<float3> &positions, const IndexRange range)
Pass spans by value
@ -0,0 +48,4 @@
Array<int> indices(mask.min_array_size());
const auto nearest_for = [this, &positions](const IndexMask mask, MutableSpan<int> r_indices) {
I don't think there's a benefit to specifying the capture in a case like this (instead of
[&]
)@ -0,0 +112,4 @@
group_indexing.add(group_id);
}
Vector<Vector<int>> mask_indices(group_indexing.size());
TBH I think it's better to just use
int64_t
here to avoid duplicating the functions above. Eventually whenIndexMask
is refactored, these could benefit from using that, and theint64_t
will make that more clear too.@ -0,0 +113,4 @@
}
Vector<Vector<int>> mask_indices(group_indexing.size());
Vector<Vector<int>> tree_indices(group_indexing.size());
Vector
->Array
here@ -0,0 +127,4 @@
threading::parallel_invoke(
mask.size() + domain_size > 1024,
[&]() { build_group_masks(mask, mask_indices); },
I do think one of these indices could be skipped if the selection is complete
@ -0,0 +201,4 @@
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. */
Oops, I forgot to remove this comment.
@ -0,0 +91,4 @@
group_indexing.add(group_id);
}
const bool mask_is_cheap = mask.size() < domain_size / 2;
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
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.
@ -0,0 +111,4 @@
};
threading::parallel_invoke(
domain_size > 1024 && mask_is_cheap,
I think this should be
!mask_is_cheap
I was isn't invented the best name,
mask_is_cheap
== true if computing of indices make sense and cheap mask is used.@ -0,0 +86,4 @@
}
VectorSet<int> group_indexing;
Remove empty line between
group_indexing
declaration and loop that creates it@ -0,0 +92,4 @@
group_indexing.add(group_id);
}
const bool use_cheap_mask = mask.size() < domain_size / 2;
Replace this with
const bool mask_is_full = mask.size() == domain_size;
@ -0,0 +102,4 @@
mask_indices.reinitialize(group_indexing.size());
}
else {
result.reinitialize(domain_size);
The result array should never need to be larger than
mask.min_array_size()
@ -0,0 +118,4 @@
threading::parallel_invoke(
domain_size > 1024 && use_cheap_mask,
[&]() {
domain_size > 1024 && use_cheap_mask
->domain_size > 1024 && !mask_is_full
@ -0,0 +127,4 @@
threading::parallel_for(group_indexing.index_range(), 256, [&](const IndexRange range) {
for (const int index : range) {
const Span<int64_t> mask_of_tree = tree_indices[index];
Use
IndexMask
instead ofSpan<int64_t>
for thesemask_of_tree
andmask
variables@ -0,0 +128,4 @@
threading::parallel_for(group_indexing.index_range(), 256, [&](const IndexRange range) {
for (const int index : range) {
const Span<int64_t> mask_of_tree = tree_indices[index];
KDTree_3d &tree = *build_kdtree(positions, mask_of_tree);
References typically indicate a lack of ownership, better to stick with a pointer here
Looks very nice now, thanks for going along with my requests/edits.
Reviewers