Fix #94413: Hide added input sockets on group inputs with hidden sockets #120067

Leon Schittek · 2024-03-29T16:19:44+01:00

Leon Schittek commented

2024-03-29 16:19:44 +01:00

Improve working with multiple group input nodes, by automatically
hiding new node group input sockets on any group input node with
hidden extension socket.

Whenever the extension socket is hidden, the user can now expect
the socket visibility of the group input node to stay fixed.
When the extension socket is shown on the other hand, the group input
node will update when sockets are added to the group interface.

This PR started as an updated version of https://archive.blender.org/developer/D14531

Note: During review of that patch Jacques mentioned:

Hm, I'm unsure I like the behavior. Looking at the second video it really feels like the new input should be added to the selected group input node.

I can't really think of a use case where you'd want to add the new input in multiple places like that. Maybe we could add a visibility toggle in the Inputs list that indicates whether this input is shown in the currently selected Group Input. I don't find the current behavior super intuitive.

I went for the current approach anyway, because it's basically what users are used to - just more consistent.

When tying the addition of new sockets to the selection during the editing of the node group interface, it's easy for the interface and the group input nodes to unintentionally get out of sync, possibly leaving users confused, why some of their inputs are missing.

Improve working with multiple group input nodes, by automatically hiding new node group input sockets on any group input node with hidden extension socket. Whenever the extension socket is hidden, the user can now expect the socket visibility of the group input node to stay fixed. When the extension socket is shown on the other hand, the group input node will update when sockets are added to the group interface. --- This PR started as an updated version of https://archive.blender.org/developer/D14531 Note: During review of that patch Jacques mentioned: > Hm, I'm unsure I like the behavior. Looking at the second video it really feels like the new input should be added to the selected group input node. >I can't really think of a use case where you'd want to add the new input in multiple places like that. Maybe we could add a visibility toggle in the Inputs list that indicates whether this input is shown in the currently selected Group Input. I don't find the current behavior super intuitive. I went for the current approach anyway, because it's basically what users are used to - just more consistent. When tying the addition of new sockets to the selection during the editing of the node group interface, it's easy for the interface and the group input nodes to unintentionally get out of sync, possibly leaving users confused, why some of their inputs are missing.

👍 2 ❤️ 2 🎉 1

Leon Schittek added 4 commits 2024-03-29 16:19:57 +01:00

1ab7885c50 Fix #94413 : Keep new group input sockets hidden

Update old patch: https://archive.blender.org/developer/D14531

5d32254113 Don't always add new group input sockets to selected input nodes.

326d008ef5 Always update group input nodes that are not set to hide unused sockets.

96b5c1454c Add versioning.

Leon Schittek added this to the Nodes & Physics project 2024-03-29 16:20:03 +01:00

Hans Goudey reviewed 2024-03-29 17:00:07 +01:00

Hans Goudey left a comment

I like the idea of making this a state, and the code looks reasonable. I'd still like to test this though, didn't get to that yet.

source/blender/makesdna/DNA_node_types.h Outdated

						
				@ -481,3 +481,3 @@

				  NODE_HIDDEN = 1 << 3,

				  NODE_ACTIVE = 1 << 4,

				  // NODE_ACTIVE_ID = 1 << 5, /* Deprecated. */

				  NODE_HIDE_UNUSED_SOCKETS = 1 << 5,

Hans Goudey commented

2024-03-29 16:23:37 +01:00

Let's add a comment here. Suggestion:

For group input nodes: new group inputs aren't displayed in this node.

Let's add a comment here. Suggestion: `For group input nodes: new group inputs aren't displayed in this node.`

👍 1

lone_noel marked this conversation as resolved

source/blender/nodes/intern/node_socket.cc Outdated

						
				@ -187,2 +187,4 @@

				                                VectorSet<bNodeSocket *> &new_sockets)

				{

				  /* Only group inputs have the option to hide new sockets. */

				  const bool hide_new_group_input_sockets = (node.is_group_input()) &&

Hans Goudey commented

2024-03-29 16:58:51 +01:00

Unnecessary parentheses around node.is_group_input()

Unnecessary parentheses around `node.is_group_input()`

lone_noel marked this conversation as resolved

Leon Schittek added 1 commit 2024-03-29 17:24:35 +01:00

63b4a13d43 Add comment and remove parentheses.

Hans Goudey requested review from Jacques Lucke 2024-04-11 17:54:15 +02:00

Hans Goudey requested review from Hans Goudey 2024-04-11 17:54:15 +02:00

Jacques Lucke reviewed 2024-04-11 18:17:06 +02:00

Jacques Lucke left a comment

I'm wondering whether this needs a new flag at all. What is the benefit of a new flag compared to just checking whether there are hidden sockets already on-demand?

👍 3 👀 1

source/blender/makesdna/DNA_node_types.h Outdated

						
				@ -482,2 +482,3 @@

				  NODE_ACTIVE = 1 << 4,

				  // NODE_ACTIVE_ID = 1 << 5, /* Deprecated. */

				  /** For group input nodes: new group inputs aren't displayed in this node. */

				  NODE_HIDE_UNUSED_SOCKETS = 1 << 5,

Jacques Lucke commented

2024-04-11 18:14:17 +02:00

The naming of this seems quite misleading, because it doesn't really check whether sockets are unused or not.

lone_noel marked this conversation as resolved

Leon Schittek added 5 commits 2024-04-14 16:14:09 +02:00

7599e98857 Merge branch 'main' into node-group-hidden-sockets

d0d9d00a1e Don't use flag to keep track of socket visibility update state.

cfa488d2f2 Fix sockets being unhidden, when changing their type.

5535233f74 Fix typo.

5985473dd5 Cleanup: Use macro to set flag.

Leon Schittek commented

2024-04-14 16:27:25 +02:00

I'm wondering whether this needs a new flag at all. What is the benefit of a new flag compared to just checking whether there are hidden sockets already on-demand?

I was hoping that the flag was more explicit and originally I thought it might be nice to expose this in the UI. But since that's off the table, just checking directly seems fine (and probably more clear, when the flag's name was bad anyway).

> I'm wondering whether this needs a new flag at all. What is the benefit of a new flag compared to just checking whether there are hidden sockets already on-demand? I was hoping that the flag was more explicit and originally I thought it might be nice to expose this in the UI. But since that's off the table, just checking directly seems fine (and probably more clear, when the flag's name was bad anyway).

Jacques Lucke requested changes 2024-04-14 18:38:40 +02:00

Jacques Lucke left a comment

I kind of think that the versioning should be removed here for now. It can be added separately if people think that this behavior is desirable. For now it seems somewhat unrelated.

source/blender/nodes/intern/node_socket.cc Outdated

						
				@ -186,2 +186,3 @@

				                                Vector<bNodeSocket *> &old_sockets,

				                                VectorSet<bNodeSocket *> &new_sockets)

				                                VectorSet<bNodeSocket *> &new_sockets,

				                                const bool hide_new_socket)

Jacques Lucke commented

2024-04-14 18:33:56 +02:00

hide_new_sockets

`hide_new_sockets`

lone_noel marked this conversation as resolved

source/blender/nodes/intern/node_socket.cc Outdated

						
				@ -416,9 +420,12 @@ static void refresh_node_sockets_and_panels(bNodeTree &ntree,

				  }

				  Vector<bNodeSocket *> old_outputs;

				  bool has_hidden_socket = false;

Jacques Lucke commented

2024-04-14 18:31:54 +02:00

has_hidden_output_socket

`has_hidden_output_socket`

lone_noel marked this conversation as resolved

Leon Schittek added 2 commits 2024-04-14 21:03:43 +02:00

5c2049a601 Rename variables according to review.

2ddf30b29d Remove versioning.

Leon Schittek commented

2024-04-15 07:49:34 +02:00

I kind of think that the versioning should be removed here for now. It can be added separately if people think that this behavior is desirable.

I thought about this again. Currently, adding a group input via the link search hides the new sockets on other group input nodes.
So without the versioning, we can end up in situations where, when adding an input via the sidebar, the socket isn't added to any of the existing group input nodes because all of them have some sockets hidden.
But I don't have an issue with adding the versioning later.

Another option:
What do you think about checking for the visibility of just the extension socket to determine whether a socket should be visible when creating an input via the sidebar?
That would completely remove the need for versioning while also allowing us to keep the current behavior described above, where adding group inputs via link-drag search hides the input on all other group input nodes.

> I kind of think that the versioning should be removed here for now. It can be added separately if people think that this behavior is desirable. I thought about this again. Currently, adding a group input via the link search hides the new sockets on other group input nodes. So without the versioning, we can end up in situations where, when adding an input via the sidebar, the socket isn't added to *any* of the existing group input nodes because all of them have some sockets hidden. But I don't have an issue with adding the versioning later. Another option: What do you think about checking for the visibility of just the extension socket to determine whether a socket should be visible when creating an input via the sidebar? That would completely remove the need for versioning while also allowing us to keep the current behavior described above, where adding group inputs via link-drag search hides the input on all other group input nodes.

Jacques Lucke commented

2024-04-15 12:47:51 +02:00

What do you think about checking for the visibility of just the extension socket to determine whether a socket should be visible when creating an input via the sidebar?

That sounds good to me too.

> What do you think about checking for the visibility of just the extension socket to determine whether a socket should be visible when creating an input via the sidebar? That sounds good to me too.

Leon Schittek added 2 commits 2024-04-16 23:33:48 +02:00

bcc3ac26df Use the visibility of the extension socket as indicator for when to show

new inputs on group input nodes.

aa124beb7a Merge branch 'main' into node-group-hidden-sockets

Hans Goudey approved these changes 2024-04-17 14:18:59 +02:00

Hans Goudey left a comment

This is so simple now! And it works well in my testing.

Hans Goudey requested review from Jacques Lucke 2024-04-17 14:19:09 +02:00

Jacques Lucke approved these changes 2024-04-17 18:44:01 +02:00

Jacques Lucke left a comment

I have one more inline comment, but LGTM otherwise.

source/blender/nodes/intern/node_socket.cc

						
				@ -395,0 +400,4 @@

				 * When the extension socket on group input nodes is hidden, we consider the socket visibility

				 * fixed and don't want to add newly created group inputs.

				 */

				static bool hide_new_group_input_sockets(const bNode &node)