Fix #104219: Node links are sometimes created from the wrong socket #104420

Hans Goudey · 2023-02-07T19:15:07+01:00

Hans Goudey commented

2023-02-07 19:15:07 +01:00

For reasons described in more depth in the new code comment,
reordering nodes can sometimes invalidate the socket locations
in a way that makes random connections after a node is selected.

A better change would probably be to stop reordering nodes and
store their "UI order" separately instead, but that's more involved.
On its own, storing socket locations in a SoA format probably isn't
worth this complexity, but I think it's useful to have something like
this working so more data can be stored this way in the future.

I think the reproduce-ability of this bug depends on Blender's
performance, since I had a much easier time recreating it in a
debug build with a bunch of extra nodes added to the file.

For reasons described in more depth in the new code comment, reordering nodes can sometimes invalidate the socket locations in a way that makes random connections after a node is selected. A better change would probably be to stop reordering nodes and store their "UI order" separately instead, but that's more involved. On its own, storing socket locations in a SoA format probably isn't worth this complexity, but I think it's useful to have something like this working so more data can be stored this way in the future. I think the reproduce-ability of this bug depends on Blender's performance, since I had a much easier time recreating it in a debug build with a bunch of extra nodes added to the file.

Hans Goudey added 1 commit 2023-02-07 19:15:09 +01:00

ae4c5a493c Fix #104219 : Node links are sometimes created from the wrong socket

For reasons described in more depth in the new code comment,
reordering nodes can sometimes invalidate the socket locations
in a way that makes random connections after a node is selected.

A better change would probably be to stop reordering nodes and
store their "UI order" separately instead, but that's more involved.
On its own, storing socket locations in a SoA format probably isn't
worth this complexity, but I think it's useful to have something like
this working so more data can be stored this way in the future.

I think the reproduce-ability of this bug depends on Blender's
performance, since I had a much easier time recreating it in a
debug build with a bunch of extra nodes added to the file.

Hans Goudey requested review from Jacques Lucke 2023-02-10 20:07:29 +01:00

Jacques Lucke requested changes 2023-02-10 20:17:20 +01:00

source/blender/editors/space_node/node_draw.cc

					
				@ -252,0 +260,4 @@

				  if (copy_socket_locations) {

				    ntree.ensure_topology_cache();

				    old_socket_indices.reserve(ntree.all_sockets().size());

				    for (const bNodeSocket *socket : ntree.all_sockets()) {

Jacques Lucke commented

2023-02-10 20:15:49 +01:00

I think this could be implemented more efficiently (e.g. by storing something on bNodeSocketRuntime, or by only keeping track of the old node index which might work because the sockets of a node are consecutive in all_sockets()).
It's unlikely that this significantly impacts performance in practice though.

I think this could be implemented more efficiently (e.g. by storing something on `bNodeSocketRuntime`, or by only keeping track of the old node index which might work because the sockets of a node are consecutive in `all_sockets()`). It's unlikely that this significantly impacts performance in practice though.

HooglyBoogly marked this conversation as resolved

source/blender/editors/space_node/node_intern.hh

					
				@ -174,3 +174,3 @@

				 * then the active node at the very end. Relative order is kept intact.

				 */

				void node_sort(bNodeTree &ntree);

				void node_sort(SpaceNode &snode, bNodeTree &ntree);

Jacques Lucke commented

2023-02-10 20:16:53 +01:00

This is problematic, because the tree might be visible in multiple node editors, all of which would have to be updated.

HooglyBoogly marked this conversation as resolved

Brecht Van Lommel added this to the Nodes & Physics project 2023-02-13 09:19:22 +01:00

Hans Goudey added 2 commits 2023-02-28 00:14:30 +01:00

d629dd98ec Revert "Fix #104219 : Node links are sometimes created from the wrong socket"

This reverts commit ae4c5a493c.

4bcda9cc2d Fix #104219 : Node links created from wrong socket after selection

Nodes are sorted based on the selection. In some cases (even depending
on processor speed, nodes can be selected and reordered, and another
operation can run before the next redraw). That gives a window where
operators mapped to the same input as selection can run with invalid
socket locations (which aren't updated after the nodes are reordered,
since they are stored in a separate array).

To fix this, move the socket locations from the node editor runtime
data to the node tree, tag them as invalid when the nodes are
reordere, and check for that status in a few more places.

A better longer term solution is not reordering nodes based on
UI status and instead storing the UI drawing order separately.

Hans Goudey requested review from Jacques Lucke 2023-02-28 00:14:34 +01:00

Jacques Lucke approved these changes 2023-02-28 12:17:55 +01:00

source/blender/blenkernel/BKE_node_runtime.hh

					
				@ -153,0 +154,4 @@

				  /**

				   * The location of all sockets in the tree, calculated while drawing the nodes.

				   * Indexed with #bNodeSocket::index_in_tree().