Page MenuHome
Differential D12785

Geometry Nodes: Fields transfer attribute node
ClosedPublic
Actions

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

Details

Reviewers
Jacques Lucke (JacquesLucke)
Maniphest Tasks
T91651: Updated Attribute Transfer Node
Commits
rB76f386a37a9c: Geometry Nodes: Fields transfer attribute node
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 17832
Build 17832: 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.
Harbormaster completed remote builds in B17643: Diff 43030.Oct 8 2021, 1:10 AM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Oct 8 2021, 1:14 AM
Hans Goudey (HooglyBoogly) added a reviewer: Jacques Lucke (JacquesLucke).
Hans Goudey (HooglyBoogly) added a project: Geometry Nodes.
Hans Goudey (HooglyBoogly) added a task: T91651: Updated Attribute Transfer Node.
Hans Goudey (HooglyBoogly) added a comment.Oct 8 2021, 1:17 AM

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
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Oct 8 2021, 1:22 AM
Hans Goudey (HooglyBoogly) updated this revision to Diff 43031.Oct 8 2021, 1:22 AM
  • Use "Transfer Attribute" name, fix incomplete rename
Harbormaster completed remote builds in B17644: Diff 43031.Oct 8 2021, 1:22 AM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Oct 8 2021, 1:22 AM
kursad k (kursadk) awarded a token.Oct 8 2021, 6:37 AM
Christian Holl (MoonMoon) added a subscriber: Christian Holl (MoonMoon).Oct 8 2021, 8:24 AM
SecuoyaEx (SecuoyaEx) awarded a token.Oct 8 2021, 10:47 AM
Hans Goudey (HooglyBoogly) mentioned this in T91650: Support sampling custom attributes in Sample Curve node.Oct 8 2021, 10:17 PM
Wannes Malfait (Wannes) mentioned this in D11835: Topology mapping mode for Attribute Transfer node.Oct 11 2021, 5:32 PM
NoobiePoopsie (helloidonthaveanyideaformyusername) added a subscriber: NoobiePoopsie (helloidonthaveanyideaformyusername).Oct 12 2021, 10:22 PM
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
331

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) added a parent revision: D11103: Generic array data structure.Oct 13 2021, 6:45 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
331

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) updated this revision to Diff 43276.Oct 13 2021, 8:17 PM
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
Harbormaster completed remote builds in B17812: Diff 43276.Oct 13 2021, 8:17 PM
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 ↗(On Diff #43276)

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

130 ↗(On Diff #43276)

Nothing is destructed here.

source/blender/functions/FN_generic_virtual_array.hh
402 ↗(On Diff #43276)

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 ↗(On Diff #43276)

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

source/blender/nodes/CMakeLists.txt
191–192

whitespace

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

Nice that the dependent_field stuff works xD

775

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
Jacques Lucke (JacquesLucke) added inline comments.Oct 14 2021, 1:47 PM
source/blender/nodes/geometry/nodes/node_geo_transfer_attribute.cc
366

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) updated this revision to Diff 43311.Oct 14 2021, 6:12 PM
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
Harbormaster completed remote builds in B17832: Diff 43311.Oct 14 2021, 6:12 PM
Hans Goudey (HooglyBoogly) added inline comments.Oct 14 2021, 8:38 PM
source/blender/nodes/geometry/nodes/node_geo_transfer_attribute.cc
55

Yeah, I was happy about that too!

Jacques Lucke (JacquesLucke) accepted this revision.Oct 15 2021, 1:29 PM

Works great.

My test file:

transfer_attribute_test.blend918 KBDownload

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
337

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
Closed by commit rB76f386a37a9c: Geometry Nodes: Fields transfer attribute node (authored by Hans Goudey (HooglyBoogly)). · Explain WhyOct 15 2021, 9:10 PM
This revision was automatically updated to reflect the committed changes.
Hans Goudey (HooglyBoogly) added a commit: rB76f386a37a9c: Geometry Nodes: Fields transfer attribute node.

Revision Contents
Changeset List

PathSize
release/
scripts/
startup/
1 line
source/
blender/
blenkernel/
9 lines
1 line
intern/
56 lines
6 lines
makesdna/
16 lines
makesrna/
intern/
60 lines
modifiers/
intern/
53 lines
nodes/
2 lines
3 lines
1 line
geometry/
nodes/
legacy/
2 lines
18 lines
18 lines
837 lines

Diff 43311

View Options

release/scripts/startup/nodeitems_builtins.py

Loading...
View Options

source/blender/blenkernel/BKE_mesh_sample.hh

Loading...
View Options

source/blender/blenkernel/BKE_node.h

Loading...
View Options

source/blender/blenkernel/intern/mesh_sample.cc

Loading...
View Options

source/blender/blenkernel/intern/node.cc

Loading...
View Options

source/blender/makesdna/DNA_node_types.h

Loading...
View Options

source/blender/makesrna/intern/rna_nodetree.c

Loading...
View Options

source/blender/modifiers/intern/MOD_nodes_evaluator.cc

Loading...
View Options

source/blender/nodes/CMakeLists.txt

Loading...
View Options

source/blender/nodes/NOD_geometry.h

Loading...
View Options

source/blender/nodes/NOD_static_types.h

Loading...
View Options

source/blender/nodes/geometry/nodes/legacy/node_geo_attribute_transfer.cc

Loading...
View Options

source/blender/nodes/geometry/nodes/legacy/node_geo_point_distribute.cc

Loading...
View Options

source/blender/nodes/geometry/nodes/node_geo_distribute_points_on_faces.cc

Loading...
View Options

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

Loading...