source/blender/modifiers/intern/MOD_nodes.cc
307–311	@Jacques Lucke (JacquesLucke) You had concerns about this change before but for other sockets then geometry it seems nice to get the default value from the input field if no socket is connected.

Fabian Schempp (fabian_schempp) mentioned this in T86457: Add "Attribute Remove" node.Mar 12 2021, 5:48 AM

Fabian Schempp (fabian_schempp) added a task: T86457: Add "Attribute Remove" node.Mar 12 2021, 6:17 PM

In the context of this change we should also look at the decision not to output attribute names from most nodes, because that could be confusing.
Having a multi-input for attribute names encourages passing attribute names around, which is not a bad thing, but needs to be an explicit decision.
Also note that the String Inputs node don't show the attribute search currently.

source/blender/modifiers/intern/MOD_nodes.cc
307–311	It's more fine for input sockets that have a value. Is there a way to say that such a multi-input has no inputs (i.e. the vector is empty)? It's not really needed for this specific socket, but might be useful for others maybe?
source/blender/nodes/geometry/nodes/node_geo_attribute_remove.cc
46	This check is not necessary.
52	This should probably be `continue`.

Fabian Schempp (fabian_schempp) marked 2 inline comments as done.Mar 13 2021, 5:39 PM

Changes based on review by Jacques Lucke.

Harbormaster completed remote builds in B13480: Diff 35137.Mar 13 2021, 5:40 PM

Didn't test yet, but I really like this idea!

source/blender/nodes/geometry/nodes/node_geo_attribute_remove.cc
45–48	You wouldn't need to add a new `get_multi_input` function if you used `extract_input` in `geo_node_attribute_remove_exec` and passed a span into this function. I'd like to move in that direction anyway, since getting inputs twice will only add more overhead as we add more geometry component types

This revision now requires changes to proceed.Mar 13 2021, 5:58 PM

Erindale (Erindale) awarded a token.Mar 13 2021, 6:45 PM

Fabian Schempp (fabian_schempp) mentioned this in D10697: Nodes: Add Attribute Remove Node.Mar 13 2021, 7:40 PM

Changes based on review by Hans Goudey.

Harbormaster completed remote builds in B13481: Diff 35141.Mar 13 2021, 8:50 PM

Fabian Schempp (fabian_schempp) marked 2 inline comments as done.Mar 13 2021, 8:52 PM

This looks good to me, and it works well. Maybe Jacques should check the change in NOD_geometry_exec.hh though (which we worked out in chat).

Don't forget clang format though.

This revision is now accepted and ready to land.Mar 13 2021, 9:09 PM

Jacques Lucke (JacquesLucke) added inline comments.Mar 13 2021, 10:42 PM

source/blender/nodes/NOD_geometry_exec.hh
132–133	Moving from a const value doesn't really make sense usually. Why is this change necessary?

Hans Goudey (HooglyBoogly) added inline comments.Mar 13 2021, 10:53 PM

source/blender/nodes/NOD_geometry_exec.hh
132–133	Agh, turns out the first part wasn't necessary, it's just adding the second template parameter that does it. values.append(input_values_.extract<T, StringRef>(sub_identifier));

Change based on Review by Jacques Lucke.

Harbormaster completed remote builds in B13500: Diff 35171.Mar 14 2021, 10:54 PM

Fabian Schempp (fabian_schempp) marked 2 inline comments as done.Mar 14 2021, 10:55 PM

Closed by commit rB070010e203f6: Nodes: multi-input support for Attribute Remove node (authored by Fabian Schempp (fabian_schempp)). · Explain WhyMar 14 2021, 11:12 PM

This revision was automatically updated to reflect the committed changes.

Fabian Schempp (fabian_schempp) added a commit: rB070010e203f6: Nodes: multi-input support for Attribute Remove node.

Nodes: multi-input support for Attribute Remove Node
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 35172

source/blender/modifiers/intern/MOD_nodes.cc

source/blender/nodes/NOD_geometry_exec.hh

source/blender/nodes/geometry/nodes/node_geo_attribute_remove.cc

Nodes: multi-input support for Attribute Remove NodeClosedPublicActions