Details

Reviewers

Jacques Lucke (JacquesLucke)
Hans Goudey (HooglyBoogly)

Commits

rB05f3f11d553e: Geometry Nodes: Attribute Statistic Node

Summary

Attribute Statistic node

Status: The implementation is ready and the speed is definitely usable. However currently all outputs are calculated even when the some of the sockets are not connected. For most operations this does not really matter because they're cheap, but finding median is relatively expensive, so it would be good to calculate it only when the socket is connected. Also in certain cases, the materialization of the attributes could then be removed.

Description

Current UI:

Initial UI (for reference):

Design

The possible data_types:

Float (float)
Vector (float3)

Conversion to the data_type happens before calculating the statistic and is handled by create_implicit_conversions.

A few notes:

all statistics are element-wise for Vectors
product was not added because the it would very easily overflow
it was decided to not add the + operator etc, to Color4f (see: https://developer.blender.org/D10269#inline-82590), so I've also removed support for the color data type in this patch too.

Benchmarks

Node evaluation time breakdown for a float attribute:

Node evaluation time breakdown for a vector attribute:

Note about the full sort: Sorting the entire span is only slightly slower that std::nth_element in a single thread, but sorting seems to scale better with amount of threads than std::nth_element. For certain spans e.g. the UVMap span, std::nth_element is terribly slow when executing multithreaded, and just sorting the entire array is much faster. Not sure what the cause could be, it looks like some kind of "soft deadlock" is happening, where the threads are competing for the same data during the median calculation.

File used for the benchmarks:

attribute_statistic_stress_test.blend849 KBDownload

Diff Detail

Repository

rB Blender

Branch

geometry-nodes-attribute-statistics (branched from master)

Build Status

Buildable 14070
Build 14070: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@Jacques Lucke (JacquesLucke) the copy is needed for the median calculation (both std::sort and std::nth_element modify the order of the underlying data). I'll remove the redundant copy into a span though. Do you know how I could easily materialize the mesh_attribute and pointcloud_attribute directly into a Vector?

Regarding the parallelism, I guess the implementation of the threading depends on which implementation of the C++ standard library is used. I found this line in the release notes of gcc 9:

Parallel algorithms and <execution> (requires Thread Building Blocks 2018 or newer).

I assume the standard library implementation on windows also uses tbb, but I haven't found a source for that yet.

Also, is ASAN something that should run automatically or do I have to install it and run it manually?

Removed redundant copy into span

Harbormaster completed remote builds in B14447: Diff 36894.May 8 2021, 5:20 PM

merge master

Harbormaster completed remote builds in B14448: Diff 36895.May 8 2021, 5:35 PM

Victor-Louis De Gusseme (victorlouis) edited the summary of this revision. (Show Details)May 8 2021, 5:37 PM

Victor-Louis De Gusseme (victorlouis) edited the summary of this revision. (Show Details)May 8 2021, 5:44 PM

fixed segfault

Harbormaster completed remote builds in B14478: Diff 36949.May 10 2021, 10:45 PM

Victor-Louis De Gusseme (victorlouis) edited the summary of this revision. (Show Details)May 10 2021, 10:49 PM

Merge branch 'master' into geometry-nodes-attribute-statistics

Harbormaster completed remote builds in B14479: Diff 36950.May 10 2021, 10:56 PM

Victor-Louis De Gusseme (victorlouis) marked 3 inline comments as done.May 10 2021, 11:01 PM

applied arc patch
merge master

Harbormaster completed remote builds in B14499: Diff 36981.May 11 2021, 1:16 PM

I think it would be a good idea to have a way to mask/exclude some vertices in the node itself, normally for this kind of thing you can always just multiply by 0 or something else along that line, but in this case you would straight up get the wrong answer with things like mean/standard deviation etc as opposed to excluding the vertices entirely from the calculations i.e pretending like they don't exist. Plus it would probably be faster , but then again branching is slow so it might just end up being slower on small/medium sized inputs.

In D10202#288122, @StrictlyIncreasing (StrictlyIncreasing) wrote:

I think it would be a good idea to have a way to mask/exclude some vertices in the node itself, normally for this kind of thing you can always just multiply by 0 or something else along that line, but in this case you would straight up get the wrong answer with things like mean/standard deviation etc as opposed to excluding the vertices entirely from the calculations i.e pretending like they don't exist. Plus it would probably be faster , but then again branching is slow so it might just end up being slower on small/medium sized inputs.

You can use a point separate node before the attribute statistic node to only use some of the vertices. There is also D10748: Geometry Nodes: Add a new Delete Geometry node., which is more versatile. I'm not sure in terms of performance if that would actually be faster, as these deleting nodes have to copy over all the attributes.

I'll update this patch to use the new feature of the node graph evaluator to only compute connected outputs.

Ray (DerivedC) awarded a token.Jun 15 2021, 5:36 PM

Aneesh Nazar (aneeshnazar) added a subscriber: Aneesh Nazar (aneeshnazar).Jun 16 2021, 8:44 AM

Wilsimar (wilBr) awarded a token.Jun 24 2021, 7:38 PM

Johnny Matthews (guitargeek) mentioned this in D12119: Geometry Nodes: Attribute Statistics.Aug 3 2021, 8:38 PM

Johnny Matthews (guitargeek) added a child revision: D12119: Geometry Nodes: Attribute Statistics.

Merge branch 'master' into arcpatch-D10202
Add Conditional Execution based on outputs

Harbormaster completed remote builds in B16198: Diff 40283.Aug 3 2021, 8:52 PM

Good to have this closer to the finish line. Thanks for working on it Johnny.

We should remove the timers and the extra braces
It should retrieve an attribute from the curve component as well.

source/blender/nodes/NOD_geometry.h
67	Merge error here.
source/blender/nodes/geometry/nodes/node_geo_attribute_statistic.cc
269–275	for (const int i : attributes.index_range()) { attributes_x[i] = attributes[i].x; attributes_y[i] = attributes[i].y; attributes_z[i] = attributes[i].z; }
287	These should be initialized, for cases when the domain size is zero (or the attribute is empty)
289–291	I think it's a bit cleaner to initialize these like this: Span<float> x{attributes_x}; Span<float> y{attributes_y}; Span<float> z{attributes_z}; It says something move like "make a span out of the array" rather than "the span is the array"
299–301	These assignments look unnecessary.
308–318	This can be simplified a bit: if (!x.is_empty()) { min = float3(x.first(), y.first(), z.first()); max = float3(x.last(), y.last(), z.last()); }

This revision now requires changes to proceed.Aug 6 2021, 3:52 AM

Merge branch 'master' into arcpatch-D10202
Merge branch 'master' into arcpatch-D10202
Alphabetize
Remove timers, cleanup a few bits.

Harbormaster completed remote builds in B16256: Diff 40393.Aug 6 2021, 5:17 AM

Thanks for picking this up @Johnny Matthews (guitargeek) :) I wasn't finding the time and energy to finish this.

In D10202#314257, @Victor-Louis De Gusseme (victorlouis) wrote:

Thanks for picking this up @Johnny Matthews (guitargeek) :) I wasn't finding the time and energy to finish this.

You're welcome! You put a lot of great work into it!

I was thinking about starting a node like this and got pointed to your patch by @Hans Goudey (HooglyBoogly) and he suggested I try to get it over the finish line.

It is one that is gonna get used a lot I think!

Various cleanups:

Simplify required outputs logic, group by which work is necessary.
Don't output results that haven't been computed (for socket inspection tooltips)
Avoid unecessary spans
Avoid unecessary copying to arrays when calculating just the sum or mean of float3
"Un-template" functions that only really supported floats
Use the attribute functions that don't specify a domain instead of ATTR_DOMAIN_AUTO

Harbormaster completed remote builds in B16287: Diff 40454.Aug 9 2021, 5:38 AM

Hans Goudey (HooglyBoogly) accepted this revision.Aug 9 2021, 5:38 AM

Well, now this node needs a couple updates:

Attribute input should be a field instead
The node should not realize instances

Those changes should be pretty simple though.

This revision now requires changes to proceed.Sep 19 2021, 8:13 PM

Merge branch 'master' into arcpatch-D10202
Update patch for new socket API and make it fields aware.

Harbormaster completed remote builds in B17130: Diff 42059.Sep 20 2021, 1:06 AM

Fix Naming

Harbormaster completed remote builds in B17131: Diff 42060.Sep 20 2021, 1:20 AM

Pretty close, just a comment about the field evaluation.

source/blender/nodes/geometry/nodes/node_geo_attribute_statistic.cc
202	In which case was the input field empty? I'm not sure that should happen, since even when it's not connected it should just be constant.
212	Misleading variable name here.
213–218	This should be much more efficient: MutableSpan component_result<float> = data.as_mutable_span().slice(offset, domain_size); evaluator.add_with_destination(input_field, component_result); evaluator.evaluate(); Avoid copying the evaluation result into the final array Avoids a virtual varray call for every index Gives the evaluator a chunk of memory to work with and reuse

This revision now requires changes to proceed.Sep 20 2021, 3:14 AM

Better variable names, remove useless check, better allocation for field data retrieval.

Harbormaster completed remote builds in B17134: Diff 42063.Sep 20 2021, 4:01 AM

Looks good now!

Paul Larson (GeorgiaPacific) added a subscriber: Paul Larson (GeorgiaPacific).Sep 20 2021, 5:30 PM

Looks mostly fine, just two notes.

source/blender/nodes/geometry/nodes/node_geo_attribute_statistic.cc

[690/1724] Building CXX object source/blender/nodes/CMakeFiles/bf_nodes.dir/geometry/nodes/node_geo_attribute_statistic.cc.o
In file included from /usr/include/c++/11.1.0/pstl/parallel_backend_tbb.h:26,
                 from /usr/include/c++/11.1.0/pstl/parallel_backend.h:20,
                 from /usr/include/c++/11.1.0/pstl/algorithm_impl.h:22,
                 from /usr/include/c++/11.1.0/pstl/glue_execution_defs.h:50,
                 from /usr/include/c++/11.1.0/execution:32,
                 from /home/jacques/blender-git/blender/source/blender/nodes/geometry/nodes/node_geo_attribute_statistic.cc:18:
/home/jacques/blender-git/lib/linux_centos7_x86_64/tbb/include/tbb/task.h:21:139: note: ‘#pragma message: TBB Warning: tbb/task.h is deprecated. For details, please see Deprecated Features appendix in the TBB reference manual.’
   21 | #pragma message("TBB Warning: tbb/task.h is deprecated. For details, please see Deprecated Features appendix in the TBB reference manual.")
      |

Removing #include <execution> fixes it. Also the #include <numeric> header is necessay for e.g. std::accumulate.
Note that removing <execution> also requires removing the first argument in a few function calls below. Think that is fine.

Feels like the Size output shouldn't be in this node, it has nothing to do with the attribute itself.

This revision now requires changes to proceed.Sep 20 2021, 6:45 PM

Merge branch 'master' into arcpatch-D10202
Remove Size Output

Harbormaster completed remote builds in B17162: Diff 42109.Sep 20 2021, 7:38 PM

Added a couple smaller notes that should still be fixed. But besides that, looks good to me.

release/scripts/startup/nodeitems_builtins.py
487	Add the poll function for fields.
source/blender/blenkernel/BKE_node.h
1405	This needs to be set properly before the commit.
source/blender/nodes/geometry/nodes/node_geo_attribute_statistic.cc
36	Outputs don't need default values. Same below.

This revision is now accepted and ready to land.Sep 20 2021, 7:50 PM

Hans Goudey (HooglyBoogly) added inline comments.Sep 20 2021, 7:58 PM

source/blender/nodes/geometry/nodes/node_geo_attribute_statistic.cc
36	I did this because it zeroed the outputs when they weren't calculated but you used socket inspection to hover over them. But in retrospect it's clear that it would be much better to fix socket inspection if that's still a problem.

Remove defaults on output, other small cleanup

Harbormaster completed remote builds in B17163: Diff 42110.Sep 20 2021, 8:21 PM

Closed by commit rB05f3f11d553e: Geometry Nodes: Attribute Statistic Node (authored by Victor-Louis De Gusseme (victorlouis), committed by Hans Goudey (HooglyBoogly)). · Explain WhySep 21 2021, 1:46 AM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rB05f3f11d553e: Geometry Nodes: Attribute Statistic Node.

Geometry Nodes: Attribute Statistic node
ClosedPublic
Actions

Details

Attribute Statistic node

Description

Design

Benchmarks

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 36198

release/scripts/startup/nodeitems_builtins.py

source/blender/blenkernel/BKE_node.h

source/blender/blenkernel/intern/node.cc

source/blender/makesrna/intern/rna_nodetree.c

source/blender/nodes/CMakeLists.txt

source/blender/nodes/NOD_geometry.h

source/blender/nodes/NOD_static_types.h

source/blender/nodes/geometry/nodes/node_geo_attribute_statistic.cc

Geometry Nodes: Attribute Statistic nodeClosedPublicActions

Details

Attribute Statistic node

Description

Design

Benchmarks

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 36198

release/scripts/startup/nodeitems_builtins.py

source/blender/blenkernel/BKE_node.h

source/blender/blenkernel/intern/node.cc

source/blender/makesrna/intern/rna_nodetree.c

source/blender/nodes/CMakeLists.txt

source/blender/nodes/NOD_geometry.h

source/blender/nodes/NOD_static_types.h

source/blender/nodes/geometry/nodes/node_geo_attribute_statistic.cc

Geometry Nodes: Attribute Statistic node
ClosedPublic
Actions

Revision Contents
Changeset List