Geometry Nodes: Sort Elements node #114194

Merged
Hans Goudey merged 43 commits from mod_moder/blender:sort_elements into main 2024-01-12 14:30:44 +01:00
Iliya Katushenock added the
Interest
Geometry Nodes
label 2023-10-27 14:39:35 +02:00
Iliya Katushenock added 3 commits 2023-10-27 14:39:47 +02:00
Iliya Katushenock added this to the Nodes & Physics project 2023-10-27 14:39:51 +02:00
Iliya Katushenock added 1 commit 2023-10-27 19:40:00 +02:00
Iliya Katushenock added 2 commits 2023-10-29 15:21:40 +01:00
Iliya Katushenock requested review from Jacques Lucke 2023-10-29 15:37:24 +01:00
Iliya Katushenock requested review from Hans Goudey 2023-10-29 15:37:24 +01:00
Iliya Katushenock added 1 commit 2023-10-29 19:11:26 +01:00
Hans Goudey requested changes 2023-11-15 15:26:59 +01:00
Hans Goudey left a comment
Member

I think this should use the attribute API rather than CustomData. We're trying to move away from using CustomData in general, and working on a single attribute array at a time should give much better cache usage. It would be okay not to reuse the code from the debug randomization. Generating new geometries rather than modifying them in place would also be okay IMO.indices_are_range

I think this should use the attribute API rather than `CustomData`. We're trying to move away from using `CustomData` in general, and working on a single attribute array at a time should give much better cache usage. It would be okay not to reuse the code from the debug randomization. Generating new geometries rather than modifying them in place would also be okay IMO.indices_are_range
Iliya Katushenock added 1 commit 2023-11-17 12:11:24 +01:00
Iliya Katushenock added 1 commit 2023-11-17 16:41:56 +01:00
Iliya Katushenock requested review from Hans Goudey 2023-11-17 16:42:21 +01:00
Hans Goudey requested changes 2023-12-03 17:16:46 +01:00
Hans Goudey left a comment
Member

Didn't read through all of this yet, mostly just reorder.cc

Didn't read through all of this yet, mostly just `reorder.cc`
@ -0,0 +85,4 @@
MutableSpan<int> new_offsets)
{
Array<int> old_counts(old_offsets.size());
array_utils::copy(old_offsets, old_counts.as_mutable_span());
Member

How about adding an overload of gather_group_sizes that takes a Span<int> instead of an IndexMask? Then this would just look like:

gather_group_sizes(old_offsets, old_by_new_map, new_offsets);
accumulate_counts_to_offsets(new_offsets);
How about adding an overload of `gather_group_sizes` that takes a `Span<int>` instead of an `IndexMask`? Then this would just look like: ``` gather_group_sizes(old_offsets, old_by_new_map, new_offsets); accumulate_counts_to_offsets(new_offsets); ```
mod_moder marked this conversation as resolved
@ -0,0 +91,4 @@
offset_indices::accumulate_counts_to_offsets(new_offsets);
}
static void reorder_attributes(const bke::AttributeAccessor src_attributes,
Member

I think there's an overload of the existing gather_attributes function that does the same thing

I think there's an overload of the existing `gather_attributes` function that does the same thing
mod_moder marked this conversation as resolved
@ -0,0 +129,4 @@
return data;
}
static void reorder_customdata_groups(CustomData &data,
Member

This function seems to be unused

This function seems to be unused
mod_moder marked this conversation as resolved
@ -0,0 +158,4 @@
const Array<int> new_by_old_map = invert_permutation(old_by_new_map);
const auto old_index_to_new = [&](const int old_i) { return new_by_old_map[old_i]; };
parallel_transform(dst_mesh.edges_for_write().cast<int>(), 4098, old_index_to_new);
Member

I'm pretty sure these can be written with array_utils::gather rather than transform

I'm pretty sure these can be written with `array_utils::gather` rather than `transform`
Author
Member

Ooh, just after writing full comment why this is not correct, i realized what did you mean/
array_utils::gather(dst: edges, indices: edges, src: old_indices_to_new).

Ooh, just after writing full comment why this is not correct, i realized what did you mean/ `array_utils::gather(dst: edges, indices: edges, src: old_indices_to_new)`.
mod_moder marked this conversation as resolved
Iliya Katushenock requested review from Hans Goudey 2023-12-17 02:35:40 +01:00
Iliya Katushenock added 2 commits 2023-12-17 02:35:50 +01:00
Iliya Katushenock added 2 commits 2023-12-17 18:15:16 +01:00
Iliya Katushenock added 1 commit 2023-12-29 11:57:20 +01:00
Iliya Katushenock added 1 commit 2023-12-29 11:58:42 +01:00
Hans Goudey requested changes 2023-12-29 17:12:20 +01:00
Hans Goudey left a comment
Member

Overall structure looks good. Thanks for keeping this up to date.

Overall structure looks good. Thanks for keeping this up to date.
@ -156,2 +156,4 @@
void gather_group_sizes(OffsetIndices<int> offsets, const IndexMask &mask, MutableSpan<int> sizes);
void gather_group_sizes(OffsetIndices<int> offsets,
const Span<int> indices,
Member

const Span -> Span

`const Span` -> `Span`
mod_moder marked this conversation as resolved
@ -0,0 +4,4 @@
#include "DNA_curves_types.h"
#include "DNA_mesh_types.h"
#include "DNA_meshdata_types.h"
Member

DNA_meshdata_types is unnecessary

`DNA_meshdata_types` is unnecessary
Author
Member

What about Mesh declaration and access to its fields?

What about `Mesh` declaration and access to its fields?
Member

That's in DNA_mesh_types.h

That's in `DNA_mesh_types.h`
Author
Member

Ah, i mixup headers!

Ah, i mixup headers!
mod_moder marked this conversation as resolved
@ -0,0 +26,4 @@
&components_supported_reordering()
{
using namespace bke;
const static MultiValueMap<GeometryComponent::Type, AttrDomain> supported_types_and_domains =
Member

I'd probably skip the static variable unless it's helpful to the runtime of the calling code. AFAIK it has to be initialized on program startup otherwise.

I'd probably skip the static variable unless it's helpful to the runtime of the calling code. AFAIK it has to be initialized on program startup otherwise.
Author
Member

I just not sure how to define supported types. I mean, to validate input geometry and node settings, and to do not declare such internal things in definitely wrong place (in node witch is use this functions).
This might be something like bool is_supported(GeometryComponent::Type component, AttrDomain domain);..

Not sure about has to be initialized on program startup otherwise. It isn't is right now? Or you mean issues with guardedalloc?

I just not sure how to define supported types. I mean, to validate input geometry and node settings, and to do not declare such internal things in definitely wrong place (in node witch is use this functions). This might be something like `bool is_supported(GeometryComponent::Type component, AttrDomain domain);`.. Not sure about `has to be initialized on program startup otherwise`. It isn't is right now? Or you mean issues with guardedalloc?
Member

This might be something like bool is_supported(GeometryComponent::Type component, AttrDomain domain);..

That could work too. What I had in mind was just returning a newly constructed MultiValueMap every time. But either way is fine with me.

Not sure about has to be initialized on program startup otherwise. It isn't is right now? Or you mean issues with guardedalloc?

My thought wasn't about correctness, just about adding fewer static variables for performance reasons. But I forgot that if the static value is declared inside a function, it's just initialized the first time the function is called. So never mind about the whole comment! :)

> This might be something like `bool is_supported(GeometryComponent::Type component, AttrDomain domain);`.. That could work too. What I had in mind was just returning a newly constructed `MultiValueMap` every time. But either way is fine with me. > Not sure about `has to be initialized on program startup otherwise`. It isn't is right now? Or you mean issues with guardedalloc? My thought wasn't about correctness, just about adding fewer static variables for performance reasons. But I forgot that if the static value is declared inside a function, it's just initialized the first time the function is called. So never mind about the whole comment! :)
mod_moder marked this conversation as resolved
@ -0,0 +226,4 @@
array_utils::gather(old_transforms, old_by_new_map, new_transforms);
}
bke::GeometryComponent &reordered_component_copy(const bke::GeometryComponent &src_component,
Member

Think this should return a GeometryComponentPtr. That could be released by the caller.

Think this should return a `GeometryComponentPtr`. That could be released by the caller.
mod_moder marked this conversation as resolved
@ -462,6 +462,8 @@ DefNode(GeometryNode, GEO_NODE_INTERPOLATE_CURVES, 0, "INTERPOLATE_CURVES", Inte
DefNode(GeometryNode, GEO_NODE_POINTS_TO_CURVES, 0, "POINTS_TO_CURVES", PointsToCurves, "Points to Curves", "Split all points to curve by its group ID and reorder by weight")
DefNode(GeometryNode, GEO_NODE_INPUT_EDGE_SMOOTH, 0, "INPUT_EDGE_SMOOTH", InputEdgeSmooth, "Is Edge Smooth", "Retrieve whether each edge is marked for smooth or split normals")
DefNode(GeometryNode, GEO_NODE_SORT_ELEMENTS, 0, "SORT_ELEMENTS", SortElements, "Sort Elements", "")
Member

Suggestion: Rearrange geometry elements, changing their indices

Suggestion: `Rearrange geometry elements, changing their indices`
mod_moder marked this conversation as resolved
@ -0,0 +43,4 @@
node->custom1 = int(bke::AttrDomain::Point);
}
static bool indices_are_range(const Span<int> indices, const IndexRange range)
Member

The existing implementation of indices_are_range could move to BLI_array_utils.hh for this.

The existing implementation of `indices_are_range` could move to `BLI_array_utils.hh` for this.
mod_moder marked this conversation as resolved
@ -0,0 +83,4 @@
});
}
static void find_points_by_group_index(const Span<int> indices_of_curves,
Member

Guess this shouldn't mention curves anymore

Guess this shouldn't mention curves anymore
mod_moder marked this conversation as resolved
@ -0,0 +178,4 @@
const int total_groups = identifiers_to_indices(gathered_group_id);
Array<int> offsets_to_sort(total_groups + 1, 0);
find_points_by_group_index(gathered_group_id, offsets_to_sort, gathered_indices);
parallel_transform<int>(gathered_indices, 2048, [&](const int pos) { return mask[pos]; });
Member

Think this should be changed to avoid random access to the IndexMask.

Think this should be changed to avoid random access to the `IndexMask`.
Author
Member

Here is the same issue as in #115142, we have to have this log(n) complexity, or create tempolar array to realize indices from mask. I'd prefer to do not do this without additional benchmarks, at least.

Here is the same issue as in #115142, we have to have this log(n) complexity, or create tempolar array to realize indices from mask. I'd prefer to do not do this without additional benchmarks, at least.
Author
Member

I measured impact on performance by mask sampling (in #114194 (comment)). Temporary array would be some faster, but in just general impact on whole node is really small, and i not sure it is necessary to allocate so big tempolar array.

I measured impact on performance by mask sampling (in https://projects.blender.org/blender/blender/pulls/114194#issuecomment-1092985). Temporary array would be some faster, but in just general impact on whole node is really small, and i not sure it is necessary to allocate so big tempolar array.
@ -0,0 +201,4 @@
unselected.foreach_index_optimized<int>(GrainSize(2048),
[&](const int index) { indices[index] = index; });
mask.foreach_index_optimized<int>(GrainSize(2048), [&](const int index, const int pos) {
Member

Looks like this is array_utils::scatter

Looks like this is `array_utils::scatter`
mod_moder marked this conversation as resolved
@ -0,0 +256,4 @@
}
}
items.append({0, nullptr, 0, nullptr, nullptr});
return items.as_span();
Member

I'd just return the Vector, no real need to convert it to an array

I'd just return the `Vector`, no real need to convert it to an array
mod_moder marked this conversation as resolved
Iliya Katushenock added 5 commits 2023-12-29 20:30:14 +01:00
fede435753 fix
crash for selection / incorrect sort in groups
07f6f4c3b7 progress
move indices_are_range to bli
ec9c8ce43b cleanup
unused includes
Author
Member

I added example .blend file with combination of all supported geometry components/domains. Switching is happened manually, so be careful that attribute domain is the same in capturing and in sorting.

I'll have to make a benchmarks to deduce if direct sample of index mask should be replaced by creating tempolar array of mask indices though.

I added example .blend file with combination of all supported geometry components/domains. Switching is happened manually, so be careful that attribute domain is the same in capturing and in sorting. I'll have to make a benchmarks to deduce if direct sample of index mask should be replaced by creating tempolar array of mask indices though.
Iliya Katushenock added 1 commit 2023-12-29 20:39:07 +01:00
Author
Member

Here is a result of benchmark.
I did this measurements for this scope of code b9a539a76e/source/blender/nodes/geometry/nodes/node_geo_scale_elements.cc (L382) (rewrited version of Scale Element node), but this still applicable for current PR, and maybe all next.

Here is direct comparison of mask[i] and mask_indices[in] approach:
chart.png
Here is 3x speed improvements for mapping on really segmented mask. Usually there is better result though.

Here is more detailed exploration of mapping(to_indices) and it's part (to_indices):
chart (1).png
Real remapping is still slovest part of this scope, due to random memory-read i thought.
Array filling by to_indices is fast though.
For 0.5 random factor, this array is nearly to 3.000.000-+1.000.000 size. I think is a lot for this part of code:

This chart is about impact of both approach for one certain function (benchmarked node have other stages which simply didn't measured). Both target sub-parts is simply added under function timings:
chart (2).png

Here is a result of benchmark. I did this measurements for this scope of code https://projects.blender.org/blender/blender/src/commit/b9a539a76e6126f70447f36bc202182c447aea45/source/blender/nodes/geometry/nodes/node_geo_scale_elements.cc#L382 (rewrited version of Scale Element node), but this still applicable for current PR, and maybe all next. Here is direct comparison of `mask[i]` and `mask_indices[in]` approach: ![chart.png](/attachments/f0a8e42c-f867-4ff9-8081-014d2d0b8a48) Here is 3x speed improvements for mapping on really segmented mask. Usually there is better result though. Here is more detailed exploration of `mapping(to_indices)` and it's part (`to_indices`): ![chart (1).png](/attachments/20f257cd-aa8d-4e54-948e-7828ea54f5fb) Real remapping is still slovest part of this scope, due to random memory-read i thought. Array filling by `to_indices` is fast though. For `0.5` random factor, this array is nearly to `3.000.000`-+`1.000.000` size. I think is a lot for this part of code: This chart is about impact of both approach for one certain function (benchmarked node have other stages which simply didn't measured). Both target sub-parts is simply added under function timings: ![chart (2).png](/attachments/01f420b9-f927-4f1c-94df-fa4c3a455427)
Iliya Katushenock added 3 commits 2023-12-30 14:01:34 +01:00
56c396e5db cleanup
missed includs, unusef function
41dae4bd47 cleanup
const in header, clean unused attributes
Iliya Katushenock requested review from Hans Goudey 2023-12-30 14:01:44 +01:00
Hans Goudey reviewed 2024-01-06 02:01:14 +01:00
@ -0,0 +254,4 @@
clean_unused_attributes(propagation_info, *component);
if (const bke::MeshComponent *src_mesh_component = dynamic_cast<const bke::MeshComponent *>(
Member

Moving this component type switching to the geometry node (instead of this file) might allow exposing more reusable functionality: reorder_mesh, reorder_curves, etc.

Moving this component type switching to the geometry node (instead of this file) might allow exposing more reusable functionality: `reorder_mesh`, `reorder_curves`, etc.
Author
Member

It is component interface is not enough reusable already?

It is component interface is not enough reusable already?
Member

I think it's a bit messy to force users of the function to go to a different abstraction level just to use this code. If I just want to reorder vertices in a mesh, it would be better to call a function like reorder_mesh_verts than to have to create a geometry component. I'm thinking of the grease pencil operator as another user of this code currently.

I think it's a bit messy to force users of the function to go to a different abstraction level just to use this code. If I just want to reorder vertices in a mesh, it would be better to call a function like `reorder_mesh_verts` than to have to create a geometry component. I'm thinking of the grease pencil operator as another user of this code currently.
Author
Member

I had attempt to fix this, what do you think?

I had attempt to fix this, what do you think?
mod_moder marked this conversation as resolved
@ -0,0 +74,4 @@
offset_indices::build_reverse_offsets(indices, r_offsets);
Array<int> counts(r_offsets.size(), 0);
for (const int64_t index : indices.index_range()) {
Member

Is this not the same as reverse_indices_in_groups? Seems it's missing the multithreading from that implementation

Is this not the same as `reverse_indices_in_groups`? Seems it's missing the multithreading from that implementation
Author
Member

Right now here performance is not important. So i made this as simple as possible. Size of groups might be different, so general group deduce code have to be used. I might copy functions from #111081, as i did in #115142 though.

Right now here performance is not important. So i made this as simple as possible. Size of groups might be different, so general group deduce code have to be used. I might copy functions from #111081, as i did in #115142 though.
@ -0,0 +183,4 @@
Array<int> indices(domain_size);
array_utils::scatter<int>(gathered_indices, mask, indices);
unselected.foreach_index_optimized<int>(GrainSize(2048),
Member

index_mask::masked_fill

`index_mask::masked_fill`
Author
Member

This filling by indices, not by constants. This is required to check if result indices are range, and pass this non-gathered next, to copy data (And additional optimization could be to create a mask in copying stage to split copying data from random indices and the copying of memory segments).

This filling by indices, not by constants. This is required to check if result indices are range, and pass this non-gathered next, to copy data (And additional optimization could be to create a mask in copying stage to split copying data from random indices and the copying of memory segments).
mod_moder marked this conversation as resolved
Iliya Katushenock added 2 commits 2024-01-06 12:29:31 +01:00
Iliya Katushenock added 2 commits 2024-01-09 14:54:55 +01:00
Iliya Katushenock added 2 commits 2024-01-09 18:49:26 +01:00
Hans Goudey requested changes 2024-01-09 19:14:50 +01:00
Hans Goudey left a comment
Member
  • It might be nice to get a node warning if the geometry doesn't have any supported domain/type combinations
  • Not sure if I'm observing a bug here:
    image
- [x] It might be nice to get a node warning if the geometry doesn't have any supported domain/type combinations - [x] Not sure if I'm observing a bug here: ![image](/attachments/0fba47ad-340a-46b1-b70c-00bef693eb3c)
633 KiB
@ -85,1 +85,4 @@
template<typename T>
inline void scatter(const Span<T> src,
const IndexMask mask,
Member

const IndexMask mask -> const IndexMask &indices for consistency with other places in this file

`const IndexMask mask` -> `const IndexMask &indices` for consistency with other places in this file
mod_moder marked this conversation as resolved
@ -86,0 +92,4 @@
BLI_assert(mask.size() == src.size());
BLI_assert(mask.min_array_size() <= dst.size());
mask.foreach_index_optimized<int>(
GrainSize(grain_size), [&](const int index, const int pos) { dst[index] = src[pos]; });
Member

I don't think we can assume the size is less than 2 billion here. Using int64_t should be fine

I don't think we can assume the size is less than 2 billion here. Using `int64_t` should be fine
mod_moder marked this conversation as resolved
@ -0,0 +19,4 @@
#include "DNA_mesh_types.h"
#include "DNA_pointcloud_types.h"
namespace blender::geometry {
Member

Missing the GEO_reorder.hh include here

Missing the `GEO_reorder.hh` include here
mod_moder marked this conversation as resolved
@ -0,0 +136,4 @@
dst_mesh.attributes_for_write());
}
static void reorder_mesh_exec(const Mesh &src_mesh,
Member

I'd remove all of the _exec suffixes, I don't think they're necessary

I'd remove all of the `_exec` suffixes, I don't think they're necessary
Author
Member

Just to separate the two levels of functionality prepare/reorder.

Just to separate the two levels of functionality `prepare`/`reorder`.
Iliya Katushenock requested review from Hans Goudey 2024-01-09 20:31:13 +01:00
Iliya Katushenock added 6 commits 2024-01-09 20:31:21 +01:00
Hans Goudey requested changes 2024-01-09 21:38:13 +01:00
Hans Goudey left a comment
Member

Hmm, I didn't expect to see a warning in this case:
image

Hmm, I didn't expect to see a warning in this case: ![image](/attachments/bcde3ad6-01b5-469e-b08b-c134a4344a55)
Iliya Katushenock requested review from Hans Goudey 2024-01-10 12:01:46 +01:00
Iliya Katushenock added 2 commits 2024-01-10 12:01:57 +01:00
Iliya Katushenock added 1 commit 2024-01-10 12:19:25 +01:00
Hans Goudey approved these changes 2024-01-10 17:46:21 +01:00
Iliya Katushenock added 1 commit 2024-01-10 23:05:04 +01:00
Iliya Katushenock added 1 commit 2024-01-11 13:43:45 +01:00
Iliya Katushenock added 1 commit 2024-01-11 13:44:57 +01:00
Hans Goudey reviewed 2024-01-12 14:11:26 +01:00
@ -85,1 +85,4 @@
template<typename T>
inline void scatter(const Span<T> src,
const IndexMask indices,
Member

const IndexMask indices -> const IndexMask &indices

`const IndexMask indices` -> `const IndexMask &indices`
Author
Member

It is possible to mark copy constructor as an explicit?)

It is possible to mark copy constructor as an explicit?)
Member

Hmm, if possible that seems like a nice idea

Hmm, if possible that seems like a nice idea
mod_moder marked this conversation as resolved
Iliya Katushenock added 1 commit 2024-01-12 14:13:22 +01:00
Hans Goudey merged commit 37b2c12cfa into main 2024-01-12 14:30:44 +01:00
Iliya Katushenock deleted branch sort_elements 2024-01-12 15:05:13 +01:00
Sign in to join this conversation.
No reviewers
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
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
Overlay
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
Interest
Workbench
Interest: X11
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
No Milestone
No Assignees
2 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#114194
No description provided.