Page MenuHome

Geometry Nodes: Fields transfer attribute node
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Oct 8 2021, 1:10 AM.

Details

Summary

This commit adds an updated version of the old attribute transfer node.
It works like a function node, so it works in the context of a
geometry, with a simple data output.

The node has a new "Index" mode, which can pick data from specific
indices on the target geometry. The implicit default is to do a simple
copy from the target geometry, but any indices could be used.

Copy the position of one geometry to another, the data types don't matter

Output color data from a noise texture transferred from another geometry

Diff Detail

Repository
rB Blender
Branch
geometry-nodes-attribute-transfer-fields (branched from master)
Build Status
Buildable 17812
Build 17812: arc lint + arc unit

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Oct 8 2021, 1:10 AM
Hans Goudey (HooglyBoogly) created this revision.

I was wondering if it really makes sense to have "Attribute" in the socket names. Maybe "Value" is better.

Hans Goudey (HooglyBoogly) retitled this revision from Geometry Nodes: Fields version of the attribute transfer node to Geometry Nodes: Fields transfer attribute node.Oct 8 2021, 1:18 AM
  • Use "Transfer Attribute" name, fix incomplete rename
Jacques Lucke (JacquesLucke) requested changes to this revision.Oct 13 2021, 5:15 PM

The node needs a small fix in the field inferencing code. The output socket should be a diamond with dot here, but it's just a diamond because the Index input is a field implicitly.

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

How safe is it to assume that this socket declaration exist? Not sure if we can really guarantee it here yet (also think about undefined nodes).

source/blender/nodes/geometry/nodes/node_geo_attribute_transfer.cc
302 ↗(On Diff #43031)

Return early when src.is_empty(), otherwise we rely on std::clamp doing the right thing below.

642 ↗(On Diff #43031)

The std::move feels wrong. Same below with src_field_.

This revision now requires changes to proceed.Oct 13 2021, 5:15 PM
Hans Goudey (HooglyBoogly) marked 3 inline comments as done.Oct 13 2021, 8:16 PM
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/modifiers/intern/MOD_nodes_evaluator.cc
333

Good point, it's not safe is it. I refactored this to a separate function.

source/blender/nodes/geometry/nodes/node_geo_attribute_transfer.cc
642 ↗(On Diff #43031)

It works, but it's probably not worth leaving the member variable in a moved-from state. I'll remove it.

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.
  • Merge branch 'master' into geometry-nodes-attribute-transfer-fields
  • Deeper rename in the code to "Transfer Attribute"
  • Fix implicit inputs
  • Return early with empty source attribute data
  • Remove benign-but-suspicious uses of std::move
Jacques Lucke (JacquesLucke) requested changes to this revision.Oct 14 2021, 12:20 PM
Jacques Lucke (JacquesLucke) added inline comments.
source/blender/functions/FN_generic_array.hh
65

The default values should be set where the variable is declared above.

130

Nothing is destructed here.

source/blender/functions/FN_generic_virtual_array.hh
402

Since a GArray is just a span, it should be possible to derive this class from GVArray_For_GSpan. Then you only have to implement the constructor, all the other methods are already implemented by GVArray_For_GSpan.

source/blender/functions/tests/FN_generic_array_test.cc
32

Needs a NOLINT. bugprone-use-after-move.

source/blender/nodes/CMakeLists.txt
192

whitespace

source/blender/nodes/geometry/nodes/node_geo_transfer_attribute.cc
54

Nice that the dependent_field stuff works xD

774

This warning should only display when there are no faces but there are vertices. Otherwise this may result in a warning on empty meshes, no?

Also, this crashes when the mesh is null.

This revision now requires changes to proceed.Oct 14 2021, 12:20 PM
source/blender/nodes/geometry/nodes/node_geo_transfer_attribute.cc
365

You might have to call ensure_owns_direct_data on this geometry. I think I got a crash because of that, but I forgot to check in detail. This was also necessary in the instance on points node.

Hans Goudey (HooglyBoogly) marked 8 inline comments as done.
  • Merge after committing GArray to master separately
  • Cleanup: Remove unnecessary code
  • Fix white-space mistake
  • Fix error messages and null mesh crash, add ensure_owns_direct_data
source/blender/nodes/geometry/nodes/node_geo_transfer_attribute.cc
54

Yeah, I was happy about that too!

Works great.

My test file:


In the future, we probably want to support multiple transferred attributes. Also we probably want to add a position output etc to avoid computing the bvh lookup multiple times. Alternatively, we could think about ways to deduplicate the bvh tree lookups by "merging" them during field evaluation somehow. That's not entirely straight forward, but should be possible to some degree with some refactoring.
Supporting transferring multiple attributes in a single node would be good anyway (especially once we support group nodes to have dynamic numbers if inputs/outputs).

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

Kinda not true given that the Set Curve Handles node has special handling below.

This revision is now accepted and ready to land.Oct 15 2021, 1:29 PM