Nodes: Support updating socket availability and labels in declaration.
Needs RevisionPublic
Actions

Authored by Jacques Lucke (JacquesLucke) on Sep 30 2021, 12:04 PM.

Details

Reviewers

Hans Goudey (HooglyBoogly)

Summary

In general, I'm trying to get us closer to reaching the following two goals:

Fully declarative node interface (the *_update function was imperative in this context, because it would change the sockets directly). This makes it easier to reason about how nodes behave, because a node can't do arbitrary things in its update function anymore. Being able to reason about nodes allows for more complex features like type inferencing.
Have all inputs to a node as sockets instead of in node->storage. This gives users the most flexibility, because everything inside a group can be exposed to the outside.

This patch is clearly focused on the first part, but the second goal impacts the API. More specifically, without the second goal I'd opt for an API that uses if-else statements to handle socket availability.

Just a few nodes are updated in this patch to test the API. In some cases like the Switch node, this new API results in quite a bit more code. However, hopefully we'll find a better solution in the future for nodes with changing data types making this a non-issue.

Diff Detail

Repository

rB Blender

Branch

temp-socket-update-in-declaration (branched from master)

Build Status

Buildable 17461
Build 17461: arc lint + arc unit

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.Sep 30 2021, 12:04 PM

Jacques Lucke (JacquesLucke) created this revision.

Harbormaster completed remote builds in B17461: Diff 42708.Sep 30 2021, 12:04 PM

Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)Sep 30 2021, 12:14 PM

Jacques Lucke (JacquesLucke) added a subscriber: Hans Goudey (HooglyBoogly).

Aaron Carlisle (Blendify) added a subscriber: Aaron Carlisle (Blendify).Sep 30 2021, 3:49 PM

Update more nodes.

Harbormaster completed remote builds in B17914: Diff 43441.Oct 17 2021, 3:45 PM

Jacques Lucke (JacquesLucke) retitled this revision from Nodes: Support updating socket availability and labels in declaration (WIP). to Nodes: Support updating socket availability and labels in declaration..Oct 17 2021, 3:46 PM

Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)

Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)Oct 17 2021, 3:49 PM

Jacques Lucke (JacquesLucke) added a reviewer: Hans Goudey (HooglyBoogly).

Paul Larson (GeorgiaPacific) awarded a token.Oct 19 2021, 3:42 AM

Paul Larson (GeorgiaPacific) added a subscriber: Paul Larson (GeorgiaPacific).

Generally this is a nice improvement, I just have one concern about the available function, noted inline.

Have you thought about how this might work when things that are currently enums in node.storage are sockets? I guess the available and label functions would get the ability to check for the value and single-ness of other inputs?

Also, about the second goal, I wonder if it's really feasible to keep the dependencies between sockets when they are exposed to other nodes. That, combined with the fact that the dependencies will be sockets instead of storage makes it feel like a pretty limited idea.
Generally, since this patch is heading into "design" territory, it's nice if your current higher-level thinking like that is written somewhere in more detail.

source/blender/nodes/geometry/nodes/node_geo_curve_resample.cc
45–46 ↗	(On Diff #43441)	I see why you did this the way you did, but honestly I'm not a big fan of the clever use of the node storage here. Especially when we want to move away from using the node storage, it seems odd to make it part of the API like this. Another option is to do something like this: static const NodeGeometryCurveResample &get_storage(const bNode &node) { return static_cast<const NodeGeometryCurveResample >(node.storage); } .available([](const bNode &node) { return get_storage(node).mode == GEO_NODE_CURVE_RESAMPLE_COUNT; } A const and non-const version of `get_storage` could even be generated by a macro like `STORAGE_FUNCTIONS<NodeGeometryCurveResample>` at the top of each node file where it's necessary. This would be helpful elsewhere in the file too, not just the declaration.