Geometry Nodes: Support custom instance attributes
ClosedPublic
Actions

Authored by Erik Abrahamsson (erik85) on Nov 8 2021, 10:26 PM.

Details

Reviewers

Jacques Lucke (JacquesLucke)
Hans Goudey (HooglyBoogly)

Maniphest Tasks

T92926: Support dynamic attributes on instances

Commits

rB97533eede444: Geometry Nodes: Support custom instance attributes

Summary

Initial support for custom instance attributes.

Adds a new ComponentAttributeProvider for custom attributes.
Adds a new domain ATTR_DOMAIN_INSTANCE.
Adds the domain to the RNA Enums to be visible in nodes that use attributes. It works already in the Attribute Statistics node.

T92926

Diff Detail

Repository

rB Blender

Branch

instances-attributes (branched from master)

Build Status

Buildable 18774
Build 18774: arc lint + arc unit

Event Timeline

Erik Abrahamsson (erik85) requested review of this revision.Nov 8 2021, 10:26 PM

Erik Abrahamsson (erik85) created this revision.

Harbormaster completed remote builds in B18548: Diff 44506.Nov 8 2021, 10:26 PM

Erik Abrahamsson (erik85) edited the summary of this revision. (Show Details)Nov 8 2021, 10:31 PM

Erik Abrahamsson (erik85) added a reviewer: Jacques Lucke (JacquesLucke).

Erik Abrahamsson (erik85) added a project: Geometry Nodes.

Jacques Lucke (JacquesLucke) added a reviewer: Hans Goudey (HooglyBoogly).Nov 11 2021, 2:17 PM

Generally I think this makes sense as a first step. At least the capture attribute node can work on instances then, which has come up a few times.

With this patch, the set position node doesn't seem to work on instances, like it did before.
That raises the question of what it should do. The other builtin attributes nodes work on instance data rather than only instances.
Maybe the position and ID nodes need a Point/Instances enum choice to choose whether to affect instances or the point domain.
@Jacques Lucke (JacquesLucke) What do you think about that? Two other options are:

Hardcoding the domain to instances for instances component
Using whatever domain the position attribute is on.

It does seem a bit inconsistent to me that the node doesn't work on instance data though.

DatasetRegionDrawer::draw_component_row requires a fix since the instances row now has a domain.
I think rna_SpaceSpreadsheet_attribute_domain_itemf needs a special case for instances as well.

source/blender/blenkernel/BKE_geometry_set.hh
656	Maybe making this private and adding const and mutable accessors for it like the other data is

This revision now requires changes to proceed.Nov 11 2021, 6:14 PM

I don't understand the "whatever domain the position attribute is on" part, because it exists on points and on instances. I think for the time being the Set Position node and Set ID node should change instance attributes. I'm not sure if that ever has to change. I wouldn't bother with adding an enum unless a real use case comes up. Once we have loops this will become less of a problem anyway.

In D13149#348112, @Jacques Lucke (JacquesLucke) wrote:

I don't understand the "whatever domain the position attribute is on" part, because it exists on points and on instances.

Right, I guess this wouldn't work, because we don't want to modify both the point attributes and the instance locations.

I think for the time being the Set Position node and Set ID node should change instance attributes. I'm not sure if that ever has to change. I wouldn't bother with adding an enum unless a real use case comes up. Once we have loops this will become less of a problem anyway.

One issue with not adding the enum is that we can't preserve the existing behavior of those two nodes, as far as I can tell.
I also think the enum would help make the choice explicit: "This attribute can exist for instances and points, which should be changed here?" seems like a fairly straightforward question and something that would come up a fair amount.

Granted, the enum isn't a beautiful solution, making the set position node more complicated when it's used so much is a little unfortunate. But we're making it more complicated either way, it's just a question of how we expose the complexity.

Curious what you think about that.

For the records, we talked about this in chat, the conclusion was basically this:

"Set Position" should have the "for every unique instance" behavior and only deal with the point domain.
The "Translate Instances" node could become a "Set Instance Position" node-- translation becomes "Offset", and it only deals with the instance domain for top-level instances
These changes can come after this patch. For this patch, we just need to use the instance domain for the instances component.

That still leaves the "Set ID" affecting both domains and not having the "per-unique-instance-geometry" behavior.
IMO set ID should have the point/instance drop-down, since it doesn't have more options, and it's helpful to be explicit. That hasn't been agreed on at the moment though.