Details

Reviewers

Jacques Lucke (JacquesLucke)
Dalai Felinto (dfelinto)

Group Reviewers

Geometry Nodes

Maniphest Tasks

T83108: Stable point random attribute for pebbles use case

Commits

rBc9f8f7915fd8: Geometry Nodes: Make random attribute node stable

Summary

Currently, the random attribute node doesn't work well for most
workflows because for any change in the input data it outputs
completely different results.

This patch adds an implicit seed attribute input to the node, referred to
by "random_id". The attribute is hashed for each element using
the CPPType system's hash method, meaning the attribute can have any
data type. Supporting any data type is also important so any attribute
can be copied into the "random_id" attribute and used as a seed.

The "random_id" attribute is an example of a "reserved name" attribute,
meaning attributes with this name can be used implicitly by nodes like
the random attribute node. Although it makes it a bit more difficult to
dig deeper, using the name implicitly rather than exposing it as an input
should make the system more accessible and predictable.

Finding a better name than "random_id" would be nice, but a bit tricky too.

Diff Detail

Repository

rB Blender

Branch

geometry-nodes-attribute-random-stable-v2 (branched from master)

Build Status

Buildable 11697
Build 11697: arc lint + arc unit

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Dec 11 2020, 9:21 PM

Hans Goudey (HooglyBoogly) created this revision.

Harbormaster completed remote builds in B11696: Diff 31893.Dec 11 2020, 9:22 PM

Hans Goudey (HooglyBoogly) added a reviewer: Geometry Nodes.Dec 11 2020, 9:22 PM

Hans Goudey (HooglyBoogly) added a project: Geometry Nodes.

Rename variables, update comments

Harbormaster completed remote builds in B11697: Diff 31894.Dec 11 2020, 9:29 PM

Ray Molenkamp (LazyDodo) awarded a token.Dec 11 2020, 9:34 PM

Hans Goudey (HooglyBoogly) added a task: T83108: Stable point random attribute for pebbles use case.Dec 11 2020, 9:38 PM

Kenzie (kenziemac130) awarded a token.Dec 12 2020, 5:24 AM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Dec 13 2020, 5:56 PM

Good work :)
I wonder if we can use a noise function that does not depend on expensive functions like sin in the context of this node.

Also, it might be good to commit this after the poisson distribution stuff, because that might make merge conflicts easier to resolve.

source/blender/nodes/geometry/nodes/node_geo_random_attribute.cc
126	This should probably make sure that the `hash_attribute` in in the correct domain?

properties_data_pointcloud.py should also be updated to have the same standard attribute name, where it's called Particle ID now. Actually that file and properties_data_hair.py were not updated for new attribute names at all yet.

source/blender/nodes/geometry/nodes/node_geo_random_attribute.cc
62–70	`seed` should be an integer, not a float. `mutator` should also be an integer, and `BLI_hash_int_2d` can be called again, no need for another hashing function. Use `BLI_hash_int_01` to convert to float. For efficiency, you could add a `BLI_hash_int_3d` copied from `intern/cycles/util/util_hash.h`.
125	Just `id` seems like a good name to me, and is also used in another 3D app as a convention, so would help with interop.

Léo Depoix (PiloeGAO) awarded a token.Dec 14 2020, 2:02 PM

Hans Goudey (HooglyBoogly) marked 3 inline comments as done.Dec 14 2020, 6:26 PM

Add BLI_hash_int_3d
Add a version of attribute_try_get_for_read that doesn't adapt data type
Improve noise function, add per point variation without attribute, cleanup
Rename "random_id" to "id"

Harbormaster completed remote builds in B11745: Diff 31977.Dec 14 2020, 6:30 PM

Looks good, will leave the rest of the review to others.

source/blender/blenlib/BLI_hash.h
75–88 ↗	(On Diff #31977)	This can now be replaced with `final(a, b, c)` to avoid code duplication.
source/blender/nodes/geometry/nodes/node_geo_random_attribute.cc
68	`fractf` should be unnecessary now.

Hans Goudey (HooglyBoogly) added inline comments.Dec 14 2020, 7:39 PM

source/blender/blenlib/BLI_hash.h
75–88 ↗	(On Diff #31977)	Oops, I meant to do that but forgot, thanks.
source/blender/nodes/geometry/nodes/node_geo_random_attribute.cc
68	Oh, great, I should have tested that.

Hans Goudey (HooglyBoogly) mentioned this in T83224: Nodes required to make the "random attributes" to produce stable outputs for the scattering.Dec 15 2020, 12:05 AM

Deduplicate code
Remove unecessary fractf call

Harbormaster completed remote builds in B11750: Diff 31993.Dec 15 2020, 4:48 AM

Tested it briefly, and it seemed to work well :)

source/blender/nodes/geometry/nodes/node_geo_random_attribute.cc
135	`as_span` isn't necessary here. Arrays are converted to spans implicitly. If you are curious, this conversion is defined in functions called `operator Span<T>() const`.