Nodes: Support for input/output sockets in same vertical space #112250

Lukas Tönne · 2023-09-11T19:05:02+02:00

Lukas Tönne commented

2023-09-11 19:05:02 +02:00

Declarations can use the add_input_output method to create a combined input/output socket. The drawing code supports moving sockets up one vertical slot to align them with the predecessor.

Closes #112235

Declarations can use the `add_input_output` method to create a combined input/output socket. The drawing code supports moving sockets up one vertical slot to align them with the predecessor. Closes #112235

❤️ 2 🎉 2

Lukas Tönne added 5 commits 2023-09-11 19:05:15 +02:00

b012955375 Support for 'inlined' sockets with input and output on the same line.

3f3f7afbec Merge branch 'main' into node-inline-sockets

0fb30a3e7b Unified list of socket decl builders.

This is to allow 1 builder to generate 2 sockets/decls.

20252852ae Store two socket declarations per builder to allow for dual sockets.

f9b4893192 API method to support inline sockets that are both input and output.

Lukas Tönne commented

2023-09-11 19:06:51 +02:00

Currently this will fail on the socket validation function (correctly!) because the inlined input socket appears before output sockets, which is considered invalid. Either have to relax this condition or add a special flag to exempt these sockets.

Lukas Tönne added this to the Nodes & Physics project 2023-09-11 19:07:01 +02:00

Hans Goudey added this to the 4.0 milestone 2023-09-11 19:13:38 +02:00

Hans Goudey requested changes 2023-09-11 20:06:39 +02:00

Hans Goudey left a comment

Do you think eventually node declarations could be refactored to be stored more like the interface tree items, so that code like this wouldn't be necessary? I'm fine with doing that later, but it seems like this leaves the code in an unnecessarily complicated state.

    if (decl_in_) {
      decl_in_->description = std::move(value);
    }
    if (decl_out_) {
      decl_out_->description = std::move(value);
    }

Do you think eventually node declarations could be refactored to be stored more like the interface tree items, so that code like this wouldn't be necessary? I'm fine with doing that later, but it seems like this leaves the code in an unnecessarily complicated state. ``` if (decl_in_) { decl_in_->description = std::move(value); } if (decl_out_) { decl_out_->description = std::move(value); } ```

source/blender/editors/space_node/node_draw.cc Outdated

						
				@ -474,1 +501,4 @@

				struct NodeInterfacePanelData {

				  const nodes::PanelDeclaration *decl_;

				  bNodePanelState *state_;

Hans Goudey commented

2023-09-11 20:05:11 +02:00

Since this is drawing code, it would be nice if these were const pointers. Same below.

Lukas Tönne commented

2023-09-12 11:55:18 +02:00

I've added some comments to this struct. state could potentially be made const, but currently draw code also updates the NODE_PANEL_PARENT_COLLAPSED flag in there (hiding the panel and its content). This could be done in a separate pass, but that would just duplicate the drawing code loop.

The runtime data stores transient draw info like the location, so it needs to be mutable.

I've added some comments to this struct. `state` could potentially be made const, but currently draw code also updates the `NODE_PANEL_PARENT_COLLAPSED` flag in there (hiding the panel and its content). This could be done in a separate pass, but that would just duplicate the drawing code loop. The `runtime` data stores transient draw info like the location, so it needs to be mutable.

Hans Goudey commented

2023-09-12 13:31:13 +02:00

Okay-- I'd like to move that runtime location storage to SpaceNode runtime, but while we still reorder nodes based on selection that's too tricky.

Okay-- I'd like to move that runtime location storage to `SpaceNode` runtime, but while we still reorder nodes based on selection that's too tricky.

LukasTonne marked this conversation as resolved

source/blender/editors/space_node/node_draw.cc Outdated

						
				@ -475,0 +511,4 @@

				};

				struct NodeInterfaceSocketData {

				  const nodes::SocketDeclaration *decl_;

Hans Goudey commented

2023-09-11 20:05:43 +02:00

These are public members, which shouldn't have the _ suffix. Same above.

These are public members, which shouldn't have the `_` suffix. Same above.

LukasTonne marked this conversation as resolved

source/blender/editors/space_node/node_draw.cc Outdated

						
				@ -475,0 +517,4 @@

				  operator bool() const

				  {

				    /* One socket side is enough to be valid. */

Hans Goudey commented

2023-09-11 20:05:46 +02:00

It's not clear what "valid" means here

LukasTonne marked this conversation as resolved

source/blender/editors/space_node/node_draw.cc Outdated

						
				@ -500,2 +646,2 @@

				   * the stack. Each panel expects a number of items to be added, after which the panel is removed

				   * from the stack again. */

				   * new #PanelUpdate is added to the stack. Items in the declaration are added to the top panel

				   * of the stack. Each panel expects a number of items to be added, after which the panel is

Hans Goudey commented

2023-09-11 20:00:02 +02:00

Unnecessary formatting change?

Lukas Tönne commented

2023-09-12 14:47:04 +02:00

Bah, clang-format keeps doing this, and it's usually in a multiline comment where it's not immediately obvious ...

Lukas Tönne commented

2023-09-12 14:50:16 +02:00

make format insists on this, guess it was wrong before 🤷

`make format` insists on this, guess it was wrong before 🤷

LukasTonne marked this conversation as resolved

source/blender/nodes/NOD_node_declaration.hh Outdated

						
				@ -247,3 +248,3 @@

				class BaseSocketDeclarationBuilder {

				 protected:

				  int index_ = -1;

				  int index_in_ = -1;

Hans Goudey commented

2023-09-11 20:02:18 +02:00

Seems important to add comments here

LukasTonne marked this conversation as resolved

Lukas Tönne commented

2023-09-12 08:35:01 +02:00

Do you think eventually node declarations could be refactored to be stored more like the interface tree items, so that code like this wouldn't be necessary?

That might be nice, but it could also just become more complicated.

The declarations code sits between two different systems:

The semantic system of declarations and node group interfaces, where there is a true hierarchy of items and a conceptual "pass-through" socket.
The actual node instances where sockets are simple flat lists on nodes. Panels, pass-throughs, and the like are just UI layout features.

I want to have a single builder instance returned by add_input_output to make declarations intuitive, but it has to generate 2 basic sockets. The node declarations themselves also have to match the sockets 1:1 because of how they are used in various places as supplementary info for sockets. So now 1 builder has to keep track of 2 declarations.

> Do you think eventually node declarations could be refactored to be stored more like the interface tree items, so that code like this wouldn't be necessary? That might be nice, but it could also just become more complicated. The declarations code sits between two different systems: 1. The semantic system of declarations and node group interfaces, where there is a true hierarchy of items and a conceptual "pass-through" socket. 2. The actual node instances where sockets are simple flat lists on nodes. Panels, pass-throughs, and the like are just UI layout features. I want to have a single builder instance returned by `add_input_output` to make declarations intuitive, but it has to generate 2 basic sockets. The node declarations themselves also have to match the sockets 1:1 because of how they are used in various places as supplementary info for sockets. So now 1 builder has to keep track of 2 declarations.

Dalai Felinto commented

2023-09-12 09:58:10 +02:00

Hi Lukas, could you please check what am I doing wrong? Because on my initial tests this is not working for me in two ways:

The inlined sockets are still drawn one above the other.
The label of the inlined output should be hidden.

diff --git a/source/blender/nodes/composite/nodes/node_composite_exposure.cc b/source/blender/nodes/composite/nodes/node_composite_exposure.cc
index 7abad178f2f..4a2786bac63 100644
--- a/source/blender/nodes/composite/nodes/node_composite_exposure.cc
+++ b/source/blender/nodes/composite/nodes/node_composite_exposure.cc
@@ -18,11 +18,10 @@ namespace blender::nodes::node_composite_exposure_cc {
 
 static void cmp_node_exposure_declare(NodeDeclarationBuilder &b)
 {
-  b.add_input<decl::Color>("Image")
+  b.add_input_output<decl::Color>("Image")
       .default_value({1.0f, 1.0f, 1.0f, 1.0f})
       .compositor_domain_priority(0);
   b.add_input<decl::Float>("Exposure").min(-10.0f).max(10.0f).compositor_domain_priority(1);
-  b.add_output<decl::Color>("Image");
 }

Hi Lukas, could you please check what am I doing wrong? Because on my initial tests this is not working for me in two ways: * The inlined sockets are still drawn one above the other. * The label of the inlined output should be hidden. ![image](/attachments/64b2de1f-aa09-4235-a5f8-fe85af0d9c8e) ``` diff --git a/source/blender/nodes/composite/nodes/node_composite_exposure.cc b/source/blender/nodes/composite/nodes/node_composite_exposure.cc index 7abad178f2f..4a2786bac63 100644 --- a/source/blender/nodes/composite/nodes/node_composite_exposure.cc +++ b/source/blender/nodes/composite/nodes/node_composite_exposure.cc @@ -18,11 +18,10 @@ namespace blender::nodes::node_composite_exposure_cc { static void cmp_node_exposure_declare(NodeDeclarationBuilder &b) { - b.add_input<decl::Color>("Image") + b.add_input_output<decl::Color>("Image") .default_value({1.0f, 1.0f, 1.0f, 1.0f}) .compositor_domain_priority(0); b.add_input<decl::Float>("Exposure").min(-10.0f).max(10.0f).compositor_domain_priority(1); - b.add_output<decl::Color>("Image"); } ```

image.png

26 KiB

Lukas Tönne commented

2023-09-12 10:28:14 +02:00

@dfelinto You are just missing a line to enable the custom layout features:

static void cmp_node_exposure_declare(NodeDeclarationBuilder &b)
{
  b.use_custom_socket_order();
  ...

@dfelinto You are just missing a line to enable the custom layout features: ``` static void cmp_node_exposure_declare(NodeDeclarationBuilder &b) { b.use_custom_socket_order(); ... ```

Lukas Tönne added 2 commits 2023-09-12 11:36:01 +02:00

8c9a99ddaf Merge branch 'main' into node-inline-sockets

ac264eb55e Comments for interface panel draw data.

Lukas Tönne added 1 commit 2023-09-12 12:01:33 +02:00

e6c8bec94d Cleanup: remove underscores from public variables.

Lukas Tönne added 2 commits 2023-09-12 12:12:16 +02:00

35c3ce6a95 Can't move value since its used twice.

c87dff1ea8 Set the `inline_with_next` flag for pass-through socket pairs.

Lukas Tönne force-pushed node-inline-sockets from 5dcbc47f0e to e2313abf3a

2023-09-12 13:37:19 +02:00

Compare

Lukas Tönne added 1 commit 2023-09-12 13:53:45 +02:00

9cefb4cb06 Mark inlined sockets when generating node group declarations.

Lukas Tönne added 2 commits 2023-09-12 14:43:58 +02:00

1e89de8188 Merge branch 'main' into node-inline-sockets

6ba0985343 Skip inline sockets when deciding on placement of node options.

Lukas Tönne added 2 commits 2023-09-12 14:58:42 +02:00

d7f4f8a5a2 Comments explaining what the bool operator for socket data means.

1df67c126d Comment explaining the two index values in BaseSocketDeclarationBuilder.

Lukas Tönne requested review from Hans Goudey 2023-09-12 14:59:13 +02:00

Hans Goudey approved these changes 2023-09-12 15:29:09 +02:00

source/blender/editors/space_node/node_draw.cc

						
				@ -457,0 +445,4 @@

				    /* Context pointers for current node and socket. */

				    PointerRNA nodeptr = RNA_pointer_create(&ntree.id, &RNA_Node, &node);

				    PointerRNA sockptr = RNA_pointer_create(&ntree.id, &RNA_NodeSocket, input_socket);

				    uiLayoutSetContextPointer(row, "node", &nodeptr);

Hans Goudey commented

2023-09-12 15:01:05 +02:00

Might as well set the node constext pointer only once above

LukasTonne marked this conversation as resolved

source/blender/editors/space_node/node_draw.cc Outdated

						
				@ -475,0 +529,4 @@

				};

				/* Utility to ensure valid state while iterating over interface items and sockets. */

				struct NodeInterfaceIterator {

Hans Goudey commented

2023-09-12 15:26:21 +02:00

Maybe it's worth mentioning the similarity to bNodeTreeInterface::foreach_item here? Or how it's different too maybe?

Maybe it's worth mentioning the similarity to `bNodeTreeInterface::foreach_item` here? Or how it's different too maybe?

LukasTonne marked this conversation as resolved

source/blender/editors/space_node/node_draw.cc Outdated

						
				@ -475,0 +530,4 @@

				/* Utility to ensure valid state while iterating over interface items and sockets. */

				struct NodeInterfaceIterator {

				 private:

Hans Goudey commented

2023-09-12 15:27:50 +02:00

Instead of struct ... { private:, this could just be class ...

Instead of `struct ... { private:`, this could just be `class ...`

LukasTonne marked this conversation as resolved

source/blender/editors/space_node/node_draw.cc Outdated

						
				@ -475,0 +543,4 @@

				  NodeInterfaceIterator(bNode &node)

				      : node_(node),

				        decl_index_(0),

				        input_(static_cast<bNodeSocket *>(node.inputs.first)),

Hans Goudey commented

2023-09-12 15:26:50 +02:00

Can use node.inputs() and node.outputs() spans here instead of using the next and prev pointers I think

Can use `node.inputs()` and `node.outputs()` spans here instead of using the next and prev pointers I think

LukasTonne marked this conversation as resolved

Lukas Tönne added 2 commits 2023-09-13 10:31:53 +02:00

6a79c1cc1c Merge branch 'main' into node-inline-sockets

efeccf92d5 Fix node declarations: misplaced bracket excluded all socket relations.

Lukas Tönne added 4 commits 2023-09-13 11:08:30 +02:00

1190daa93a Deduplicate context pointer code.

fc50353d9d Extended comment to explain the purpose of `NodeInterfaceSocketData`.

4331d1bc30 class instead of struct for iterator with private variables.

8c71238c29 Use span iterators instead of raw pointers for node socket iterator.

Jacques Lucke reviewed 2023-09-13 11:08:50 +02:00

source/blender/editors/space_node/node_draw.cc Outdated

						
				@ -475,0 +528,4 @@

				  }

				};

				/* Utility to ensure valid state while iterating over interface items and sockets. */

Jacques Lucke commented

2023-09-13 11:06:53 +02:00

In my experience it is often easier to just fill a vector with all the elements instead of building an iterator that has to keep track of the current iteration state explicitly. You could just build a Vector<std::variant<NodeInterfaceSocketData, NodeInterfacePanelData>> in one go (with some large inline buffer), and then iterate over it afterwards. That also often makes debugging easier, because it's much easier to check that the generated vector contains what you expect.

In my experience it is often easier to just fill a vector with all the elements instead of building an iterator that has to keep track of the current iteration state explicitly. You could just build a `Vector<std::variant<NodeInterfaceSocketData, NodeInterfacePanelData>>` in one go (with some large inline buffer), and then iterate over it afterwards. That also often makes debugging easier, because it's much easier to check that the generated vector contains what you expect.

👍 1

Lukas Tönne commented

2023-09-13 11:32:44 +02:00

Sounds reasonable, i'll give it a try.

Lukas Tönne commented

2023-09-13 12:53:13 +02:00

I've replaced the iterator as suggested, hopefully the code is a little bit easier to understand.

Jacques Lucke reviewed 2023-09-13 11:14:14 +02:00

source/blender/nodes/NOD_node_declaration.hh

						
				@ -273,2 +278,3 @@

				  static_assert(std::is_base_of_v<SocketDeclaration, SocketDecl>);

				  SocketDecl *decl_;

				  SocketDecl *decl_in_;

				  SocketDecl *decl_out_;

Jacques Lucke commented

2023-09-13 11:14:14 +02:00

What do you think about also storing a Vector<SocketDecl *, 2> here that either contains one or two decls? This could reduce some redundancy in the methods below because one could use a loop instead of repeating the code twice.

What do you think about also storing a `Vector<SocketDecl *, 2>` here that either contains one or two decls? This could reduce some redundancy in the methods below because one could use a loop instead of repeating the code twice.

Lukas Tönne commented

2023-09-13 11:22:13 +02:00

I've tried this and it doesn't meaningfully reduce the amount of code. There are also a bunch of places where an input declaration is treated differently from an output declaration and would have to check the type, for example (code from main):

  Self &field_on_all()
  {
    if (decl_->in_out == SOCK_IN) {
      this->supports_field();
    }
    else {
      this->field_source();
    }
    field_on_all_ = true;
    return *(Self *)this;
  }

We might eventually have cases where a single declaration builder can generate a whole bunch of sockets, like a multi-input, but i'd rather implement this when needed.

I've tried this and it doesn't meaningfully reduce the amount of code. There are also a bunch of places where an input declaration is treated differently from an output declaration and would have to check the type, for example (code from `main`): ```cpp Self &field_on_all() { if (decl_->in_out == SOCK_IN) { this->supports_field(); } else { this->field_source(); } field_on_all_ = true; return *(Self *)this; } ``` We might eventually have cases where a single declaration builder can generate a whole bunch of sockets, like a multi-input, but i'd rather implement this when needed.

👍 1

LukasTonne marked this conversation as resolved

Lukas Tönne added 1 commit 2023-09-13 12:52:51 +02:00