Page MenuHome

Nodes: Use dynamic declarations for group nodes
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Dec 22 2022, 9:30 PM.
Tokens
"Love" token, awarded by billreynish."Love" token, awarded by baoyu."Evil Spooky Haunted Tree" token, awarded by Moder.

Details

Summary

Since a year and a half ago, we've been switching to a new way
to represent what sockets a node should have called "declarations"
that's easier to use, clearer, and more flexible for upcoming features
like dynamic socket counts or generic type sockets.

All builtin nodes with a static set of sockets have switched, but one
missing area has been group nodes and group input/output nodes.
These nodes have dynamic declarations which change based on
their properties or the group they're inside of. This patch addresses that,
in preparation for using the same dynamic declaration feature for
simulation nodes.

Generally there shouldn't be user-visible differences, but one benefit
is that user-created socket descriptions are now visible directly in the
node editor for group nodes and group input/output nodes.

The patch contains a few changes:

  • Add a node type callback for building dynamic declarations with different arguments
  • Add an Extend socket declaration for the "virtual" sockets used for connecting new links
  • A similar Custom socket declaration is used for addon-defined socket
  • Simplify the node update loop to use the declaration to build update sockets
  • Replace the "group update" functions with the declaration building
  • Move the node group input/output link creation to link drag operator
  • Make the field status part of group node declarations (not for group input/output nodes though)
  • Some fixes for socket declarations to make them update and build properly

I did a bit of refactoring in master to make this possible (and may do more if other problems come up):

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Dec 22 2022, 9:30 PM
Hans Goudey (HooglyBoogly) created this revision.

What about

  • Socket value subtype
  • Visibility flags for custom tree node sockets

Thanks for taking a look!

Socket value subtype

Good catch. That should work now.

Visibility flags for custom tree node sockets

I added SOCK_COLLAPSED, but it looks like that's not set in the UI when creating a group right now. Were you thinking of any others?

  • Copy float/vector/int subtypes
  • Copy "collapsed" flag, though that didn't seem to do anything when grouping the principled BSDF node (for its collapsed SSS radius socket)

Were you thinking of any others?

I'm not sure about this, it seemed that now users custom nodes have access to all socket flags to hide / make inactive, ... . Not sure if this also needs to be passed from group sockets to group node sockets via a declaration

Didn't read everything in detail yet since this is still WIP, but generally makes sense.

source/blender/blenkernel/BKE_node.h
349

typo (other)

source/blender/nodes/intern/node_common.cc
129

typo

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Hans Goudey (HooglyBoogly) retitled this revision from Nodes: Use dynamic declarations for group nodes (WIP) to Nodes: Use dynamic declarations for group nodes.Dec 30 2022, 5:21 AM
  • Looks like socket tooltips are actually available to users customise now? (Seems like it could be added to the description)
  • Also, this patch seems to change the vector input socket display for groups

Jacques Lucke (JacquesLucke) requested changes to this revision.Dec 30 2022, 2:36 PM
  • I noticed that a few tests are failing now because there are missing update_or_build methods for e.g. decl::Bool (maybe others). Currently, it always resets those sockets to the default if there is some mismatch.
  • Creating a node group crashes for me currently. Just add a Set Position node between the group input and output node and hit ctrl+g. This seems to be caused by invalid links. I added asserts in master to make it easier to see the issue.
This revision now requires changes to proceed.Dec 30 2022, 2:36 PM
  • Fix various issues in socket declaration updating/building/comparison
  • Remove redundant node declaration allocation
  • Correct flag used for compact sockets
  • Fix crash when creating a group (only update the field interface, don't use the whole tree update as an intermediate step)
  • Merge master

Found another bug:

source/blender/editors/space_node/node_group.cc
996

Probably ok for now, but does feel a little bit fishy. It might happen that the field inferencing depends on nodes being updated or whatever else happens during node tree update before inferencing.
I think the more correct long term fix would be to use ED_node_tree_propagate_change, but to make sure that there are no left-over links that point to nodes that are in different node trees when the update happens.

1013

Can also pass in null as the "root" tree. Might be better here since we are changing more than one. Passing in a root tree can really only help when only a single tree changed.

  • Fix reordering bug (existing missing update tag in node declaration socket list refreshing)

I still have to address the two other comments.

  • Use null for root tree for node tree change propagation
source/blender/editors/space_node/node_group.cc
996

There's a dependency cycle that's difficult to handle here. Originally I had the same thought and called ED_node_tree_propagate_change here, giving it the group as the root tree since I only wanted to update that one. However, doing that caused the parent group ntree to update too, which can't be done yet because the group node doesn't have it's declaration, because the field inferencing isn't updated. So that's how I ended up with this. If I could call the node tree update utility without propagating the update to other trees that would do the trick. But I'm not sure that's possible? I'm curious if you have a thought about this.

It probably makes sense to put this in a separate patch, but does it make sense to make adding a new socket the functionality of updating the __extend__ socket?
Thinking about it, it seems that being able to make such sockets would also be useful.
Although so far this patch is independent in this regard.

Jacques Lucke (JacquesLucke) added inline comments.
source/blender/blenkernel/intern/node_tree_update.cc
567–568

Shouldn't call ensure_topoogy_cache if it's not used immediately afterwards.

This revision is now accepted and ready to land.Mon, Jan 16, 12:32 PM
This revision was automatically updated to reflect the committed changes.
Hans Goudey (HooglyBoogly) marked an inline comment as done.