Page MenuHome

Geometry Nodes: Make Random ID a builtin attribute
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Oct 18 2021, 7:03 AM.

Details

Summary

In order to address feedback that the "Stable ID" was not easy enough
to use, this commit removes the "Stable ID" output from the distribution node,
and the input from the instance on points node. Instead, the nodes write
or read a builtin named attribute, id.

The downside is that more behavior is invisible, which is less expected
now that most attributes are passed around with node links. This behavior
will have to be explained in the manual.

The random value node's "ID" input that had an implicit index input
is converted to a special implicit input that uses the stable ID if possible,
but otherwise defaults to the index. There is no way to tell in the UI which
it uses, except by knowing that rule and checking in the spreadsheet for
the id attribute.

Because it isn't always possible to create stable randomness, this attribute
does not always exist. And it will be possible to remove it when we have
the attribute remove node back, to improve performance.

Addresses T92005


If it isn't possible to tell based on the patch description, I'm conflicted
about whether this change is actually better. But we did decide on it earlier,
and it means new users will have less to think about anyway.

Diff Detail

Repository
rB Blender
Branch
geometry-nodes-random-id-attribute (branched from master)
Build Status
Buildable 18042
Build 18042: arc lint + arc unit

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Oct 18 2021, 7:03 AM
Hans Goudey (HooglyBoogly) created this revision.

I know you just wanted this up, but for the records:

At the moment this crash with the Trees and Leaves. And for the Pebbles Scattering it is not stable at all.

  • Update to avoid code duplication
  • Fix is_equal_to function

Yeah, I wouldn't have expected it to work at all. Just wanted to share the approach I was looking into with Jacques.

Hans Goudey (HooglyBoogly) planned changes to this revision.Oct 18 2021, 8:26 PM
Hans Goudey (HooglyBoogly) retitled this revision from Geometry Nodes: Make Random ID a builtin attribute (WIP) to Geometry Nodes: Make Random ID a builtin attribute.
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
  • Fix: use the new names
  • Add versioning for renamed socket

I get a crash when opening pebbles_scattering.blend and setting the factor of any pebble (e.g., the large pebbles) to 0.0.

Jacques Lucke (JacquesLucke) requested changes to this revision.Oct 19 2021, 1:27 PM

Let's not add random_edge_id and random_face_id for now, only random_point_id. They can still be added later when we have a real use case.

source/blender/blenkernel/intern/attribute_access.cc
349

Add a stored_as_named_attribute_ = data_type_ == stored_type_ data member to the class instead of doing this comparison in many places.

source/blender/blenkernel/intern/geometry_component_mesh.cc
1308

edge_access

1319

face_access

source/blender/blenloader/intern/versioning_300.c
1799 ↗(On Diff #43515)

Has this rename been agreed upon?

source/blender/modifiers/intern/MOD_nodes_evaluator.cc
352

GEO_NODE_INSTANCE_ON_POINTS does not make use of that.

source/blender/nodes/geometry/nodes/node_geo_distribute_points_on_faces.cc
340–343

This returns null for some reason when the point cloud is empty, which is causing the crash Dalai mentions.

This revision now requires changes to proceed.Oct 19 2021, 1:27 PM
Hans Goudey (HooglyBoogly) marked 4 inline comments as done.
  • Add helper boolean
  • Remove edge and face random IDs
  • Fix ELEM in evaluator
  • Fix already exists check
  • Add return early when positions is empty (don't create empty component or point cloud in distribute node)
Hans Goudey (HooglyBoogly) marked an inline comment as done.Oct 19 2021, 5:31 PM
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/blenkernel/intern/attribute_access.cc
349

Much better, good idea.

source/blender/blenloader/intern/versioning_300.c
1799 ↗(On Diff #43515)

I thought I remembered it as part of our discussions, but maybe I'm not remembering correctly.

source/blender/nodes/geometry/nodes/node_geo_distribute_points_on_faces.cc
340–343

I think it might be valid to return null when the point cloud has no points, even if it didn't happen before for some reason (looked into it for a bit and didn't figure out why that changed).
I added an empty check earlier in the node so this function isn't reached, which also has the benefit that the node doesn't create an empty component/point cloud.

source/blender/blenloader/intern/versioning_300.c
1799 ↗(On Diff #43515)

Hm ok, now I think it shouldn't be renamed. Not sure why Random ID should be better than ID here.

Hans Goudey (HooglyBoogly) marked an inline comment as done.Oct 19 2021, 6:09 PM
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/blenloader/intern/versioning_300.c
1799 ↗(On Diff #43515)

Yeah, me neither. Honestly I'm pretty ambivalent about this entire patch, I'll do whatever someone says.

  • Revert name change to random value node

+1 for calling the input socket "ID" in the Random Value node. Random ID is simply the fallback.

Hans, the patch still doesn't seem to be working for the Instance on Points. The Instance Index also needs to have a stable id (Random ID) as a fallback:

In the last update I forgot about that socket and removed the implicit input for the node, I'll add it back.

  • Add back implicit input to instance on points node
  • Mention index in socket inspection
  • Use "id" name for the attribute on the point domain

The commit message should mention that we may add edge_id etc. in the future.

source/blender/blenkernel/BKE_geometry_set.hh
763

IDAttributeFieldInput

source/blender/blenkernel/intern/attribute_access.cc
1431

Random ID -> ID

This revision is now accepted and ready to land.Oct 20 2021, 5:42 PM