Nodes: revert the inline (pass-through) socket feature #112560

Lukas Tönne · 2023-09-19T10:30:59+02:00

Lukas Tönne commented

2023-09-19 10:30:59 +02:00

Inlined sockets in the same vertical space are no longer supported.
This removes input_output socket declarations, the inlining feature in
node drawing, and the Both option for node group interface sockets.

Versioning code splits existing node group sockets into individual
sockets again. Unfortunately some links may get lost in versioning files
using the feature, because of an unnoticed bug: Socket identifiers have
to be unique in the node group items list but inlined input/output
sockets have the same identifier. This still works for most situations
because uniqueness is only required within input/output lists. Creating
proper unique identifiers will discard any link from the previous output
socket. This cannot easily be fixed without after_linking versioning
code, which should be avoided.

Inlined sockets in the same vertical space are no longer supported. This removes `input_output` socket declarations, the inlining feature in node drawing, and the `Both` option for node group interface sockets. Versioning code splits existing node group sockets into individual sockets again. Unfortunately some links may get lost in versioning files using the feature, because of an unnoticed bug: Socket identifiers have to be unique in the node group items list but inlined input/output sockets have the same identifier. This still works for most situations because uniqueness is only required within input/output lists. Creating proper unique identifiers will discard any link from the previous output socket. This cannot easily be fixed without `after_linking` versioning code, which should be avoided.

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

d8babe7057 Revert the socket inlining feature.

917681a5a7 Add outputs before inputs.

Lukas Tönne requested review from Jacques Lucke 2023-09-19 10:31:26 +02:00

Lukas Tönne added this to the Nodes & Physics project 2023-09-19 10:31:30 +02:00

Lukas Tönne commented

2023-09-19 10:32:57 +02:00

@dfelinto Do we also want to remove the Both socket option (only allow single input/output sockets)? If we want to enforce strict outputs..inputs sorting these pass-through sockets become weird, the two sockets can be far apart.

@dfelinto Do we also want to remove the `Both` socket option (only allow single input/output sockets)? If we want to enforce strict outputs..inputs sorting these pass-through sockets become weird, the two sockets can be far apart.

Charlie Jolly commented

2023-09-19 10:50:03 +02:00

I haven't tested this but would this feature allow the following, this would be a shame to lose as it would allow users to reduce the amount of nodes they need to add.

I haven't tested this but would this feature allow the following, this would be a shame to lose as it would allow users to reduce the amount of nodes they need to add. ![image](/attachments/60c3a37d-953f-432a-9869-7396dfd43e36)

image.png

43 KiB

❤️ 1

Lukas Tönne commented

2023-09-19 10:53:59 +02:00

@CharlieJolly No it doesn't work like that. The inlining is purely a UI feature, while "pass-through" is a description of intended functionality (but nothing enforcing how nodes use these).

@CharlieJolly No it doesn't work like that. The inlining is purely a UI feature, while "pass-through" is a description of _intended_ functionality (but nothing enforcing how nodes use these).

👍 1

Dalai Felinto commented

2023-09-19 10:54:03 +02:00

@dfelinto Do we also want to remove the Both socket option (only allow single input/output sockets)? If we want to enforce strict outputs..inputs sorting these pass-through sockets become weird, the two sockets can be far apart.

Yes, it makes no sense to have the "Both" in this scenario. But I'm not sure I understand the point you are making (are you "pro" or against" removing the "Both"?)

> @dfelinto Do we also want to remove the `Both` socket option (only allow single input/output sockets)? If we want to enforce strict outputs..inputs sorting these pass-through sockets become weird, the two sockets can be far apart. Yes, it makes no sense to have the "Both" in this scenario. But I'm not sure I understand the point you are making (are you "pro" or against" removing the "Both"?)

Dalai Felinto commented

2023-09-19 10:57:54 +02:00

Just for context, since this may be the first place people find about this.

The idea behind the inlined sockets was to provice an elegant way to get the nodes to be more compact. This topic is still on the agenda, but this particular solution has some shortcomes and wasn't widely well received.

The current (3.6) solution is based on the principles of "form-follow-function", where the function of the inputs/outputs is to be a list of parameters for the node. That was the original thought behind the compositor nodes design back in Elephants Dream day's.

Just for context, since this may be the first place people find about this. The idea behind the inlined sockets was to provice an elegant way to get the nodes to be more compact. This topic is still on the agenda, but this particular solution has some shortcomes and wasn't widely well received. The current (3.6) solution is based on the principles of "form-follow-function", where the function of the inputs/outputs is to be a list of parameters for the node. That was the original thought behind the compositor nodes design back in Elephants Dream day's.

Lukas Tönne commented

2023-09-19 11:01:59 +02:00

Without the inlining i think it makes more sense to remove Both option as well. It generates alternating input/output atm and if we sort these the two sockets move even further apart, making it difficult to figure out which group socket declaration belongs to which pair of sockets.

Without the inlining i think it makes more sense to remove `Both` option as well. It generates alternating input/output atm and if we sort these the two sockets move even further apart, making it difficult to figure out which group socket declaration belongs to which pair of sockets. ![image](/attachments/7f9f6676-d63e-4037-8e63-d60cee2621b9)

image.png

29 KiB

Lukas Tönne added 1 commit 2023-09-19 12:07:35 +02:00

6563e3e6b8 Remove the 'Both' socket in/out enum value.

This feature does not make much sense without inlined sockets. All
sockets are back to being either inputs or outputs.

Nika Kutsniashvili commented

2023-09-19 12:19:54 +02:00

Will separate panels for Input and Output sockets come back as well? Without Inlined sockets there is no necessity for them, and separate panels helped with organization better I thought.

Dalai Felinto commented

2023-09-19 12:36:20 +02:00

@Nika-Kutsniashvili having separate panels for input and outputs make it harder to drag the sockets around from/to different panels. Having them all on the same list facilitates that.

👍 1

Jacques Lucke commented

2023-09-20 13:57:38 +02:00

It seems like this patch contains many unrelated formatting changes.

Lukas Tönne added 1 commit 2023-09-21 10:56:17 +02:00

a0b016c64e Merge branch 'main' into nodes-revert-inline-sockets

Lukas Tönne commented

2023-09-21 11:00:24 +02:00

It seems like this patch contains many unrelated formatting changes.

Sorry, recent system update broke all the clang-format stuff on my machine again ... and then gitea hides the file will all the bogus changes from the diff by default :/

> It seems like this patch contains many unrelated formatting changes. Sorry, recent system update broke all the clang-format stuff on my machine again ... and then gitea hides the file will all the bogus changes from the diff by default :/

Lukas Tönne added 1 commit 2023-09-21 11:06:49 +02:00

7bbf761906 Cleanup: Revert unwanted formatting changes.

Lukas Tönne added 1 commit 2023-09-21 11:09:57 +02:00

ff85c97a1c Removed input_output API functions from node declarations.

These would now simply add an input and an output separately, but this
is not very useful.

Lukas Tönne added 1 commit 2023-09-22 11:34:15 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

a8f2f50df3 Merge branch 'main' into nodes-revert-inline-sockets

Lukas Tönne commented

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

@blender-bot build

Jacques Lucke reviewed 2023-09-22 12:04:25 +02:00

source/blender/blenloader/intern/versioning_400.cc Outdated

						
				@ -1234,1 +1234,4 @@

				    }

				    /* Convert sockets with both input and output flag into two separate sockets. */

				    FOREACH_NODETREE_BEGIN (bmain, ntree, id) {

Jacques Lucke commented

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

This versioning code does break files that use the feature. Might be okay, but that should be an explicit decision that is also mentioned in the PR description.

Files break when the inlined interface sockets where linked to something else.

This versioning code does break files that use the feature. Might be okay, but that should be an explicit decision that is also mentioned in the PR description. Files break when the inlined interface sockets where linked to something else.

Lukas Tönne commented

2023-09-22 12:26:42 +02:00

By "break files" i guess you mean it removes links? Because it may now generate a different identifier i guess. Could try and replicate what the socket identifiers would be to make that work.

Lukas Tönne commented

2023-09-22 16:40:43 +02:00

I've added an explanation as to why node links may get discarded and why we don't want to version that.

LukasTonne marked this conversation as resolved

source/blender/blenloader/intern/versioning_400.cc Outdated

						
				@ -1235,0 +1236,4 @@

				    /* Convert sockets with both input and output flag into two separate sockets. */

				    FOREACH_NODETREE_BEGIN (bmain, ntree, id) {

				      blender::Vector<bNodeTreeInterfaceSocket *> sockets_to_split;

				      ntree->tree_interface.foreach_item([&](bNodeTreeInterfaceItem &item) {

Jacques Lucke commented

2023-09-22 12:02:09 +02:00

Feels like it might be nice not to rely on code that may change too much in versioning, but to stay closer to the actual data. Should be fine for now, but this may have to be rewritten at some point if the called code changes.

Lukas Tönne commented

2023-09-22 16:31:50 +02:00

I added copying code for sockets without blenkernel calls here now. The fact that we don't need ID user count managing makes this a whole lot simpler than in blenkernel, all socket default_value data can be MEM_dupalloc'd.

I added copying code for sockets without blenkernel calls here now. The fact that we don't need ID user count managing makes this a whole lot simpler than in blenkernel, all socket `default_value` data can be MEM_dupalloc'd.

LukasTonne marked this conversation as resolved

source/blender/blenloader/intern/versioning_400.cc Outdated

						
				@ -1235,0 +1252,4 @@

				        bNodeTreeInterfacePanel *parent = ntree->tree_interface.find_item_parent(socket->item);

				        bNodeTreeInterfaceSocket *socket_copy = reinterpret_cast<bNodeTreeInterfaceSocket *>(

				            ntree->tree_interface.insert_item_copy(socket->item, parent, position + 1));

				        /* Original socket becomes output */

Jacques Lucke commented

2023-09-22 12:00:28 +02:00

And comments with dot.

LukasTonne marked this conversation as resolved

source/blender/makesdna/DNA_node_tree_interface_types.h Outdated

						
				@ -255,3 +255,3 @@

				    if (parent == nullptr || parent == &root_panel) {

				      /* Panel is the root panel. */

				      return 0;

				      return root_panel.item_position(item);