Fix #116435: Rotate Vector node on a link connects wrong sockets #116550

Merged

Jacques Lucke merged 5 commits from PratikPB2123/blender:116435-vector-default into blender-v4.1-release

2024-03-03 00:38:50 +01:00

Pratik Borhade commented

2023-12-26 10:36:47 +01:00

Member

sock_rotation has higher priority (6) than the vector socket (4).
Hence inserting node between vector-vector link joins thorugh "rotation"
socket (get_main_socket() / get_main_socket_priority()).
This can be fixed by making "vector" input socket as default for link.

But this will break the link creation when node is inserted between
rotation-rotation link. It seems rotation is converted to vectors so
perhaps make sense to make "vector" as default socket for link.

`sock_rotation` has higher priority (6) than the vector socket (4). Hence inserting node between vector-vector link joins thorugh "rotation" socket (`get_main_socket()` / `get_main_socket_priority()`). This can be fixed by making "vector" input socket as default for link. But this will break the link creation when node is inserted between rotation-rotation link. It seems rotation is converted to vectors so perhaps make sense to make "vector" as default socket for link.

Pratik Borhade requested review from Hans Goudey 2023-12-26 10:37:11 +01:00

Pratik Borhade added the

Nodes & Physics

label 2023-12-26 10:37:27 +01:00

Iliya Katushenock added this to the Nodes & Physics project 2023-12-26 12:57:15 +01:00

Iliya Katushenock added

and removed

Nodes & Physics

labels 2023-12-26 12:57:20 +01:00

Hans Goudey commented

2024-01-10 18:30:17 +01:00

Member

I'm unsure about this. It feels like the link chosen should depend on the type of the sockets from the other nodes. For example, a vector link should try to connect to the vector socket, and likewise for rotations, probably regardless of the "default link status" of each declaration. I'm not sure if we have the tools to do that with the current auto-link connection. What do you think?

I'm unsure about this. It feels like the link chosen should depend on the type of the sockets from the other nodes. For example, a vector link should try to connect to the vector socket, and likewise for rotations, probably regardless of the "default link status" of each declaration. I'm not sure if we have the tools to do that with the current auto-link connection. What do you think?

Iliya Katushenock commented

2024-01-10 18:38:07 +01:00

Member

Might issue related with the fact that link is connected to rotation, instead of vector. So auto-linking should be fixed instead.

Might issue related with the fact that link is connected to rotation, instead of vector. So auto-linking should be fixed instead.

Pratik Borhade commented

2024-01-11 11:23:46 +01:00

Author

Member

For example, a vector link should try to connect to the vector socket, and likewise for rotations

This make sense. Right now, get_main_socket is returning best_input/best_output socket based on is_default_link_socket. If default_link is nullptr, it chooses "socket with highest priority" for connection. This looks odd. I can make changes in get_main_socket so it'll use the socket types of old-link for the new connection

> For example, a vector link should try to connect to the vector socket, and likewise for rotations This make sense. Right now, `get_main_socket` is returning best_input/best_output socket based on `is_default_link_socket`. If default_link is nullptr, it chooses "socket with highest priority" for connection. This looks odd. I can make changes in `get_main_socket` so it'll use the socket types of old-link for the new connection

Pratik Borhade commented

2024-01-11 12:05:07 +01:00

Author

Member

Like this? I'll wait to update the PR

Code Changes

diff --git a/source/blender/editors/space_node/node_intern.hh b/source/blender/editors/space_node/node_intern.hh
index 5c324ffbb61..83939dcc14b 100644
--- a/source/blender/editors/space_node/node_intern.hh
+++ b/source/blender/editors/space_node/node_intern.hh
@@ -312,7 +312,10 @@ void NODE_OT_group_edit(wmOperatorType *ot);

 void update_multi_input_indices_for_removed_links(bNode &node);
 bool all_links_muted(const bNodeSocket &socket);
-bNodeSocket *get_main_socket(bNodeTree &ntree, bNode &node, eNodeSocketInOut in_out);
+bNodeSocket *get_main_socket(bNodeTree &ntree,
+                             bNode &node,
+                             eNodeSocketInOut in_out,
+                             bNodeLink *old_link);

 void NODE_OT_link(wmOperatorType *ot);
 void NODE_OT_link_make(wmOperatorType *ot);
diff --git a/source/blender/editors/space_node/node_relationships.cc b/source/blender/editors/space_node/node_relationships.cc
index 262941646de..bc3007749d6 100644
--- a/source/blender/editors/space_node/node_relationships.cc
+++ b/source/blender/editors/space_node/node_relationships.cc
@@ -2156,8 +2156,8 @@ void node_insert_on_link_flags(Main &bmain, SpaceNode &snode)

   old_link->flag &= ~NODE_LINKFLAG_HILITE;

-  bNodeSocket *best_input = get_main_socket(ntree, *node_to_insert, SOCK_IN);
-  bNodeSocket *best_output = get_main_socket(ntree, *node_to_insert, SOCK_OUT);
+  bNodeSocket *best_input = get_main_socket(ntree, *node_to_insert, SOCK_IN, old_link);
+  bNodeSocket *best_output = get_main_socket(ntree, *node_to_insert, SOCK_OUT, old_link);

   if (node_to_insert->type != NODE_REROUTE) {
     /* Ignore main sockets when the types don't match. */
@@ -2245,7 +2245,10 @@ static int get_main_socket_priority(const bNodeSocket *socket)
 }

 /** Get the "main" socket based on the node declaration or an heuristic. */
-bNodeSocket *get_main_socket(bNodeTree &ntree, bNode &node, eNodeSocketInOut in_out)
+bNodeSocket *get_main_socket(bNodeTree &ntree,
+                             bNode &node,
+                             eNodeSocketInOut in_out,
+                             bNodeLink *old_link)
 {
   ListBase *sockets = (in_out == SOCK_IN) ? &node.inputs : &node.outputs;

@@ -2261,6 +2264,13 @@ bNodeSocket *get_main_socket(bNodeTree &ntree, bNode &node, eNodeSocketInOut in_
       if (!socket->is_visible()) {
         continue;
       }
+      if (old_link != nullptr) {
+        eNodeSocketDatatype sock_type = static_cast<eNodeSocketDatatype>(
+            (in_out == SOCK_IN) ? old_link->fromsock->type : old_link->tosock->type);
+        if (sock_type == socket->type) {
+          return socket;
+        }
+      }
       if (socket_decl.is_default_link_socket) {
         return socket;
       }
diff --git a/source/blender/editors/space_node/node_shader_preview.cc b/source/blender/editors/space_node/node_shader_preview.cc
index 524aba197a2..81b639e32d9 100644
--- a/source/blender/editors/space_node/node_shader_preview.cc
+++ b/source/blender/editors/space_node/node_shader_preview.cc
@@ -258,9 +258,9 @@ static Scene *preview_prepare_scene(const Main *bmain,
  */
 static bNodeSocket *node_find_preview_socket(bNodeTree &ntree, bNode &node)
 {
-  bNodeSocket *socket = get_main_socket(ntree, node, SOCK_OUT);
+  bNodeSocket *socket = get_main_socket(ntree, node, SOCK_OUT, nullptr);
   if (socket == nullptr) {
-    socket = get_main_socket(ntree, node, SOCK_IN);
+    socket = get_main_socket(ntree, node, SOCK_IN, nullptr);
     if (socket != nullptr && socket->link == nullptr) {
       if (!ELEM(socket->type, SOCK_FLOAT, SOCK_VECTOR, SOCK_RGBA)) {
         /* We can not preview a socket with no link and no manual value. */

Like this? I'll wait to update the PR <details> <summary> Code Changes </summary> ```Diff diff --git a/source/blender/editors/space_node/node_intern.hh b/source/blender/editors/space_node/node_intern.hh index 5c324ffbb61..83939dcc14b 100644 --- a/source/blender/editors/space_node/node_intern.hh +++ b/source/blender/editors/space_node/node_intern.hh @@ -312,7 +312,10 @@ void NODE_OT_group_edit(wmOperatorType *ot); void update_multi_input_indices_for_removed_links(bNode &node); bool all_links_muted(const bNodeSocket &socket); -bNodeSocket *get_main_socket(bNodeTree &ntree, bNode &node, eNodeSocketInOut in_out); +bNodeSocket *get_main_socket(bNodeTree &ntree, + bNode &node, + eNodeSocketInOut in_out, + bNodeLink *old_link); void NODE_OT_link(wmOperatorType *ot); void NODE_OT_link_make(wmOperatorType *ot); diff --git a/source/blender/editors/space_node/node_relationships.cc b/source/blender/editors/space_node/node_relationships.cc index 262941646de..bc3007749d6 100644 --- a/source/blender/editors/space_node/node_relationships.cc +++ b/source/blender/editors/space_node/node_relationships.cc @@ -2156,8 +2156,8 @@ void node_insert_on_link_flags(Main &bmain, SpaceNode &snode) old_link->flag &= ~NODE_LINKFLAG_HILITE; - bNodeSocket *best_input = get_main_socket(ntree, *node_to_insert, SOCK_IN); - bNodeSocket *best_output = get_main_socket(ntree, *node_to_insert, SOCK_OUT); + bNodeSocket *best_input = get_main_socket(ntree, *node_to_insert, SOCK_IN, old_link); + bNodeSocket *best_output = get_main_socket(ntree, *node_to_insert, SOCK_OUT, old_link); if (node_to_insert->type != NODE_REROUTE) { /* Ignore main sockets when the types don't match. */ @@ -2245,7 +2245,10 @@ static int get_main_socket_priority(const bNodeSocket *socket) } /** Get the "main" socket based on the node declaration or an heuristic. */ -bNodeSocket *get_main_socket(bNodeTree &ntree, bNode &node, eNodeSocketInOut in_out) +bNodeSocket *get_main_socket(bNodeTree &ntree, + bNode &node, + eNodeSocketInOut in_out, + bNodeLink *old_link) { ListBase *sockets = (in_out == SOCK_IN) ? &node.inputs : &node.outputs; @@ -2261,6 +2264,13 @@ bNodeSocket *get_main_socket(bNodeTree &ntree, bNode &node, eNodeSocketInOut in_ if (!socket->is_visible()) { continue; } + if (old_link != nullptr) { + eNodeSocketDatatype sock_type = static_cast<eNodeSocketDatatype>( + (in_out == SOCK_IN) ? old_link->fromsock->type : old_link->tosock->type); + if (sock_type == socket->type) { + return socket; + } + } if (socket_decl.is_default_link_socket) { return socket; } diff --git a/source/blender/editors/space_node/node_shader_preview.cc b/source/blender/editors/space_node/node_shader_preview.cc index 524aba197a2..81b639e32d9 100644 --- a/source/blender/editors/space_node/node_shader_preview.cc +++ b/source/blender/editors/space_node/node_shader_preview.cc @@ -258,9 +258,9 @@ static Scene *preview_prepare_scene(const Main *bmain, */ static bNodeSocket *node_find_preview_socket(bNodeTree &ntree, bNode &node) { - bNodeSocket *socket = get_main_socket(ntree, node, SOCK_OUT); + bNodeSocket *socket = get_main_socket(ntree, node, SOCK_OUT, nullptr); if (socket == nullptr) { - socket = get_main_socket(ntree, node, SOCK_IN); + socket = get_main_socket(ntree, node, SOCK_IN, nullptr); if (socket != nullptr && socket->link == nullptr) { if (!ELEM(socket->type, SOCK_FLOAT, SOCK_VECTOR, SOCK_RGBA)) { /* We can not preview a socket with no link and no manual value. */ ``` </details>

Hans Goudey commented

2024-01-30 04:44:54 +01:00

Member

You might as well update the PR, it will be easier to review that code here. The default socket fix doesn't seem right to me, since neither the vector nor rotation input of this node feels "more important" than the other to me.

You might as well update the PR, it will be easier to review that code here. The default socket fix doesn't seem right to me, since neither the vector nor rotation input of this node feels "more important" than the other to me.

Pratik Borhade commented

2024-02-02 10:15:24 +01:00

Author

Member

@HooglyBoogly hi, updated the PR BTW.

Gitea had some issues that day (wasn't sending emails, nor updating PR for some time). So a friendly poke in case you haven't received the mail either 🙂

No rush for the review, take your time.

@HooglyBoogly hi, updated the PR BTW. Gitea had some issues that day (wasn't sending emails, nor updating PR for some time). So a friendly poke in case you haven't received the mail either 🙂 No rush for the review, take your time.

Jacques Lucke commented

2024-02-27 16:52:14 +01:00

Member

I actually kinda think that the original fix was correct. This is about dropping the node on an existing link. It doesn't really make sense to insert that node on a link that goes to a rotation socket.

I actually kinda think that the original fix was correct. This is about dropping the node on an existing link. It doesn't really make sense to insert that node on a link that goes to a rotation socket.

Hans Goudey commented

2024-02-27 19:56:21 +01:00

Member

Using the original fix is fine with me if it seems better to Jacques.

Using the original fix is fine with me if it seems better to Jacques.

Pratik Borhade commented

2024-02-28 04:00:39 +01:00

Author

Member

I'll update the PR tommorw then :)

I'll update the PR tommorw then :)

Pratik Borhade force-pushed 116435-vector-default from bce57da558 to be400677f5

2024-03-01 11:43:28 +01:00

Compare

PratikPB2123 changed target branch from main to blender-v4.1-release

2024-03-01 11:43:46 +01:00

Pratik Borhade commented

2024-03-01 11:44:34 +01:00

Author

Member

ok, reverted the change and updated the description as well :)

ok, reverted the change and updated the description as well :)

Jacques Lucke approved these changes 2024-03-01 11:45:13 +01:00

Hans Goudey approved these changes 2024-03-01 15:44:44 +01:00

Jacques Lucke referenced this issue from a commit

2024-03-03 00:38:43 +01:00

Fix #116435: Rotate Vector node on a link connects wrong sockets

Jacques Lucke merged commit 5f70bd0e46 into blender-v4.1-release

2024-03-03 00:38:43 +01:00

Pratik Borhade deleted branch 116435-vector-default

2024-03-03 12:32:09 +01:00

Pratik Borhade referenced this pull request

2024-03-28 10:12:20 +01:00

Geometry Nodes: automatic connection doesn't work for Geometry if first input is Menu #119923

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

4 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#116550

No description provided.