Fix #112233: Panel collapsed state should not hide socket links #112326

Lukas Tönne · 2023-09-13T14:11:16+02:00

Lukas Tönne commented

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

#112019 included open/closed state of the parent panel in socket
visibility calculation. This prevents dragging links, but also disables
other features that should still work, such as drawing links.

A narrower condition is needed for icon visibility vs. general socket
visibility. The cases which use the new is_icon_visible condition:

Drawing socket selection outlines (same as unselected sockets)
Drawing multi-input sockets (same as unselected sockets)
node_find_indicated_socket, used by a wide range of mouse click
operators, including the link-drag operator that was cause for
#112019.

Cases using the original is_visible (true even if parent panel is
collapsed):

nodeLinkIsHidden draws links only when at least one socket is
visible.
node_update_basis, sockets still added to layout even if icon isn't
rendered.
node_update_hidden, panels are ignored for "hidden" nodes, all
sockets are rendered.
NODE_OT_link_make operator for finding "best" sockets to connect.
node_link_viewer finding sockets to connect to a viewer node.
get_main_socket used for insert-on-links (find sockets to splice
into) and some shader previews
node_gather_link_searches, suggestions for adding a new node at the
end of a link.

#112019 included open/closed state of the parent panel in socket visibility calculation. This prevents dragging links, but also disables other features that should still work, such as drawing links. A narrower condition is needed for icon visibility vs. general socket visibility. The cases which use the new `is_icon_visible` condition: - Drawing socket selection outlines (same as unselected sockets) - Drawing multi-input sockets (same as unselected sockets) - `node_find_indicated_socket`, used by a wide range of mouse click operators, including the link-drag operator that was cause for #112019. Cases using the original `is_visible` (true even if parent panel is collapsed): - `nodeLinkIsHidden` draws links only when at least one socket is visible. - `node_update_basis`, sockets still added to layout even if icon isn't rendered. - `node_update_hidden`, panels are ignored for "hidden" nodes, all sockets are rendered. - `NODE_OT_link_make` operator for finding "best" sockets to connect. - `node_link_viewer` finding sockets to connect to a viewer node. - `get_main_socket` used for insert-on-links (find sockets to splice into) and some shader previews - `node_gather_link_searches`, suggestions for adding a new node at the end of a link.

Lukas Tönne added 1 commit 2023-09-13 14:11:27 +02:00

23443d1376 Fix #112233 : Panel collapsed state should not hide socket links.

visibility calculation. This prevents dragging links, but also disables
other features that should still work, such as drawing links.

A narrower condition is needed for icon visibility vs. general socket
visibility. The cases which use the new condition are:
- Drawing socket selection outlines (same as unselected sockets)
- Drawing multi-input sockets (same as unselected sockets)
- `node_find_indicated_socket`, used by a wide range of mouse click
  operators, including the link-drag operator that was cause for
  #112019.

Cases using the original `is_visible` (true even if parent panel is
collapsed):
- `nodeLinkIsHidden` draws links only when at least one socket is
  visible.
- `node_update_basis`, sockets still added to layout even if icon isn't
  rendered.
- `node_update_hidden`, panels are ignored for "hidden" nodes, all
  sockets are rendered.
- `NODE_OT_link_make` operator for finding "best" sockets to connect.
- `node_link_viewer` finding sockets to connect to a viewer node.
- `get_main_socket` used for insert-on-links (find sockets to splice
  into) and some shader previews
- `node_gather_link_searches`, suggestions for adding a new node at the
  end of a link.

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

Lukas Tönne added this to the Nodes & Physics project 2023-09-13 14:11:37 +02:00

Hans Goudey commented

2023-09-13 14:31:17 +02:00

is_icon_visible() makes sense to me now, but is_visible returning true when the socket is inside a collapsed panel doesn't seem right. Maybe there's a better name for that?

node_update_basis, sockets still added to layout even if icon isn't rendered.

Why are the sockets added to the layout when they're inside a collapsed panel?

`is_icon_visible()` makes sense to me now, but `is_visible` returning true when the socket is inside a collapsed panel doesn't seem right. Maybe there's a better name for that? > `node_update_basis`, sockets still added to layout even if icon isn't rendered. Why are the sockets added to the layout when they're inside a collapsed panel?

Lukas Tönne commented

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

Why are the sockets added to the layout when they're inside a collapsed panel?

They still have a location in the 2D view, the point where links go to. All sockets get put in the same place and nothing is rendered, but they are not technically "invisible".

The is_visible flag would be more accurately described as "exists in 2d view", as opposed to unavailable or hidden sockets which have no meaningful location at all. A "visible" socket does not necessarily have a rendered icon. Can't think of a name that is both accurate enough and intuitive enough ...

> Why are the sockets added to the layout when they're inside a collapsed panel? They still have a location in the 2D view, the point where links go to. All sockets get put in the same place and nothing is rendered, but they are not technically "invisible". The `is_visible` flag would be more accurately described as "exists in 2d view", as opposed to `unavailable` or `hidden` sockets which have no meaningful location at all. A "visible" socket does not necessarily have a rendered icon. Can't think of a name that is both accurate enough and intuitive enough ...

Alaska referenced this pull request

2023-09-14 01:30:26 +02:00

Add panels to Principled BSDF node #112314

Lukas Tönne commented

2023-09-14 10:55:39 +02:00

If we want to rename it, these are the options that make sense to me:

is_icon_visible to is_drawn or is_rendered ("The socket has a visible icon and can be interacted with")

is_visible could remain as-is, or maybe renamed to is_available or exists ("The socket is has a valid location and may be connected").

If we want to rename it, these are the options that make sense to me: `is_icon_visible` to `is_drawn` or `is_rendered` ("The socket has a visible icon and can be interacted with") `is_visible` could remain as-is, or maybe renamed to `is_available` or `exists` ("The socket is has a valid location and may be connected").

Hans Goudey commented

2023-09-14 14:08:00 +02:00

I just find is_visible pretty confusing when the socket is in a collapsed panel.

is_available is already taken, but that would be a nice alternative to what's currently is_visible. Maybe that could be renamed to something else.

I just find `is_visible` pretty confusing when the socket is in a collapsed panel. `is_available` is already taken, but that would be a nice alternative to what's currently `is_visible`. Maybe that could be renamed to something else.

Lukas Tönne commented

2023-09-14 15:29:15 +02:00

@JacquesLucke any opinion on naming here?

Jacques Lucke approved these changes 2023-09-14 16:51:42 +02:00

source/blender/makesdna/DNA_node_types.h Outdated

						
				@ -190,6 +190,7 @@ typedef struct bNodeSocket {

				  bool is_available() const;

				  bool is_panel_collapsed() const;

				  bool is_visible() const;

				  bool is_icon_visible() const;

Jacques Lucke commented

2023-09-14 16:51:36 +02:00

I think this needs some comment. Took me a while to understand what icon refers to here.

I think this needs some comment. Took me a while to understand what `icon` refers to here.

Lukas Tönne commented

2023-09-14 16:58:21 +02:00

Yeah that's what our discussion was about. Hans wants to rename the is_visible but it's not clear what a better name would be (meaning "The socket is has a valid location and may be connected" but there may not be any visible parts). Likewise is_icon_visible means "The socket has a visible icon and can be interacted with".

I'm happy to rename things but i don't know what to rename it to.

Yeah that's what our discussion was about. Hans wants to rename the `is_visible` but it's not clear what a better name would be (meaning "The socket is has a valid location and may be connected" but there may not be any visible parts). Likewise `is_icon_visible` means "The socket has a visible icon and can be interacted with". I'm happy to rename things but i don't know what to rename it to.

Jacques Lucke commented

2023-09-14 17:11:27 +02:00

Maybe something like has_interactive_icon. Don't have a better idea right now.

Maybe something like `has_interactive_icon`. Don't have a better idea right now.

Lukas Tönne commented

2023-09-14 17:19:56 +02:00

ok can we agree on keeping is_visible and renaming is_icon_visible to has_interactive_icon? I want to wrap this up.

ok can we agree on keeping `is_visible` and renaming `is_icon_visible` to `has_interactive_icon`? I want to wrap this up.

👍 1

Hans Goudey commented

2023-09-14 17:27:21 +02:00

My point is that the is_visible doesn't make sense anymore when the socket can be in a closed panel, and it makes more sense to rename it to prevent this sort of bug from happening again. What about is_visible and is_visible_panel_closed?

My point is that the `is_visible` doesn't make sense anymore when the socket can be in a closed panel, and it makes more sense to rename it to prevent this sort of bug from happening again. What about `is_visible` and `is_visible_panel_closed`?

Lukas Tönne commented

2023-09-15 11:39:34 +02:00

Well, is_visible_panel_closed is also not quite accurate: The function just does not care if the panel is closed or not, while the is_visible would strictly require !this->is_panel_collapsed().

is_visible_or_panel_closed could work. Not great but i'm willing to accept it in exchange for a green tick mark.

EDIT: Oh and i'm going to name it is_visible_or_panel_collapsed for consistency with the other methods.

Well, `is_visible_panel_closed` is also not quite accurate: The function just does not care if the panel is closed or not, while the `is_visible` would strictly require `!this->is_panel_collapsed()`. `is_visible_or_panel_closed` could work. Not great but i'm willing to accept it in exchange for a green tick mark. EDIT: Oh and i'm going to name it `is_visible_or_panel_collapsed` for consistency with the other methods.

Lukas Tönne added 2 commits 2023-09-15 11:58:56 +02:00

f96841e9b5 Merge branch 'main' into fix-collapsed-panel-link-drawing

5d5293aaef Renamed node socket visibility functions.

The `is_visible` method does not consider panel state, a socket is
"visible" even when its parent panel is collapsed.
`is_visible` -> `is_visible_or_panel_collapsed`

The `is_icon_visible` method represents true visibility, so this becomes
the new `is_visible` method.
`is_icon_visible` -> `is_visible`

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

Lukas Tönne merged commit 5d77d8d832 into main

2023-09-15 12:58:03 +02:00

Lukas Tönne deleted branch fix-collapsed-panel-link-drawing

2023-09-15 12:58:04 +02:00

Lukas Tönne referenced this issue from a commit

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

Fix #112233: Panel collapsed state should not hide socket links

Lukas Tönne referenced this pull request

2023-09-18 12:04:59 +02:00

UI: Node Panels: Issues when the node is in a hidden state and node panels are collapsed #112490

Lukas Tönne referenced this pull request