Nodes: automatically move viewer node to current node #121951

Jacques Lucke · 2024-05-18T18:28:11+02:00

Jacques Lucke commented

2024-05-18 18:28:11 +02:00

This improves working with viewer nodes in geometry and compositor nodes. Previously, the viewer node would typically stay at the position where it was first inserted which leads to very long links in many cases. Now the viewer node automatically moves to the place where the user ctrl+shift+clicked to view data.

The viewer is placed slightly to the right and top of the current node. It is moved up a bit, so that it does not get in the way as quickly when the user wants to add another new node. Furthermore, the viewer node position is chosen so that it does not intersect with other nodes.

In the future we could implement animating the node position so that it slowly transitions to it's new places.

This improves working with viewer nodes in geometry and compositor nodes. Previously, the viewer node would typically stay at the position where it was first inserted which leads to very long links in many cases. Now the viewer node automatically moves to the place where the user ctrl+shift+clicked to view data. The viewer is placed slightly to the right and top of the current node. It is moved up a bit, so that it does not get in the way as quickly when the user wants to add another new node. Furthermore, the viewer node position is chosen so that it does not intersect with other nodes. In the future we could implement animating the node position so that it slowly transitions to it's new places. ![image](/attachments/2a91c01c-2bd1-427b-ab5a-e7be72503e01)

image.png

24 KiB

❤️ 10 🚀 2

Jacques Lucke added 3 commits 2024-05-18 18:28:23 +02:00

initial viewer node positioning 9e1f902059

better viewer positioning b02bd0603c

add comment 34f8be0341

Jacques Lucke added 5 commits 2024-05-18 19:02:37 +02:00

use ui scale c07614bf18

reposition node even if the socket is linked to the viewer already 1b6a16cf4b

Merge branch 'main' into viewer-node-position 84fe45764b

fix 29e17d8a58

improve handling of frames 4c99edbe69

Jacques Lucke requested review from Simon Thommes 2024-05-18 19:03:04 +02:00

Jacques Lucke requested review from Hans Goudey 2024-05-18 19:03:04 +02:00

Hans Goudey reviewed 2024-05-20 01:36:08 +02:00

Hans Goudey left a comment

I didn't think too hard about the details of the algorithm. It didn't seem worth it since it works really nicely in my testing, like night and day with the crazy long links we had before.

The only think I can think of that would make this even cooler is if the viewer node animated to the new position. That might make things a little less disorienting? Actually seems pretty simple to do that by generalizing NodeStackAnimationData or so, but it's also not necessary.

I didn't think too hard about the details of the algorithm. It didn't seem worth it since it works really nicely in my testing, like night and day with the crazy long links we had before. The only think I can think of that would make this even cooler is if the viewer node animated to the new position. That might make things a little less disorienting? Actually seems pretty simple to do that by generalizing `NodeStackAnimationData` or so, but it's also not necessary.

👍 1

source/blender/editors/space_node/node_relationships.cc Outdated

						
				@ -590,0 +599,4 @@

				 * If possible, the viewer is aligned to another node that is close by to result in a better

				 * looking node position.

				 */

				static void position_viewer_node(bNodeTree &tree,

Hans Goudey commented

2024-05-20 01:29:34 +02:00

Looks like all the arguments could be const besides the viewer node

JacquesLucke marked this conversation as resolved

source/blender/editors/space_node/node_relationships.cc Outdated

						
				@ -590,0 +625,4 @@

				    BLI_rctf_pad(&main_candidate_rect, default_padding, default_padding);

				    const bNode *collided_node = nullptr;

				    LISTBASE_FOREACH (const bNode *, node, &tree.nodes) {

Hans Goudey commented

2024-05-20 01:29:18 +02:00

for (const bNode *node : tree.all_nodes())

`for (const bNode *node : tree.all_nodes())`

JacquesLucke marked this conversation as resolved

source/blender/editors/space_node/node_relationships.cc Outdated

						
				@ -590,0 +661,4 @@

				    bool found_collision_for_aligned_rect = false;

				    LISTBASE_FOREACH (const bNode *, node, &tree.nodes) {

				      if (node->type == NODE_FRAME) {

Hans Goudey commented

2024-05-20 01:33:12 +02:00

node->is_frame()?

`node->is_frame()`?

JacquesLucke marked this conversation as resolved

Jacques Lucke added 4 commits 2024-05-20 19:05:15 +02:00

Merge branch 'main' into viewer-node-position 3333c25320

use const args b05ddafa66

use tree.all_nodes() e1b20c29c9

use node.is_frame() 6817aa3ea3

Jacques Lucke commented

2024-05-20 19:06:08 +02:00

Animating the viewer movement seems nice, but won't do it as part of this patch, especially because it also requires this other refactor to be done first.

Hans Goudey approved these changes 2024-05-20 19:07:59 +02:00

Hans Goudey commented

2024-05-20 19:08:29 +02:00

Sounds good. Maybe animation could be mentioned as future work in the description?

Simon Thommes requested changes 2024-05-22 18:07:23 +02:00

Dismissed

Simon Thommes left a comment

This feels quite good for a start 👍

I did run into some weird behavior here and there, particularly when there is no free space towards the top, where the viewer gets stacked quite far away from the target node.

Some ideas:

Right now it seems quite clear that the node checks incrementally upwards for free space. It would make sense to me to alternate between incrementing up and down to also catch free space just below, if there are a lot of nodes above. (Alternatively even searching in an arch pattern to include free spaces further to the right.)
I think a simple heuristic that would help would be to check if the new chosen location is further from the target node than the previous one and just leave it in place if so (probably only when the viewer is to the right of the target)

This feels quite good for a start 👍 I did run into some weird behavior here and there, particularly when there is no free space towards the top, where the viewer gets stacked quite far away from the target node. Some ideas: - Right now it seems quite clear that the node checks incrementally upwards for free space. It would make sense to me to alternate between incrementing up and down to also catch free space just below, if there are a lot of nodes above. (Alternatively even searching in an arch pattern to include free spaces further to the right.) - I think a simple heuristic that would help would be to check if the new chosen location is further from the target node than the previous one and just leave it in place if so (probably only when the viewer is to the right of the target)

Simon Thommes commented

2024-05-22 18:10:19 +02:00

Another thought:

Could it make sense to make the viewer node active when using the CTRL+SHIFT+LMB functionality rather than the node that is clicked on? That would help highlighting it and make it easy to move manually right after.
I'm not sure what purpose the current functionality is serving regarding that, but it would feel pretty simple to me to just make the node active again that I just clicked on for the viewer and still have under my cursor.
But maybe I'm missing something.

Another thought: Could it make sense to make the viewer node active when using the CTRL+SHIFT+LMB functionality rather than the node that is clicked on? That would help highlighting it and make it easy to move manually right after. I'm not sure what purpose the current functionality is serving regarding that, but it would feel pretty simple to me to just make the node active again that I just clicked on for the viewer and still have under my cursor. But maybe I'm missing something.

👍 2

Simon Thommes commented

2024-05-22 18:28:07 +02:00

I forgot one more small note:
When there is no near node found for horizontal alignment the distance should be more than it is now/potentially just the same as the margin for auto offset, where we just set the default to 40. Looks like right now it's 20.

I forgot one more small note: When there is no near node found for horizontal alignment the distance should be more than it is now/potentially just the same as the margin for auto offset, where we just set the default to 40. Looks like right now it's 20.

Jacques Lucke added 5 commits 2024-05-29 13:02:30 +02:00

Merge branch 'main' into viewer-node-position d6be462c88

Merge branch 'main' into viewer-node-position 2008134029

extract function to find overlapping node 1f2fa4a9d7

cleanup ff0d500a59

try different search pattern 3b529bac45

Simon Thommes commented

2024-05-29 14:16:53 +02:00

The new search pattern works a lot better for me!

It's still trying quite hard not to put it below the target node. But that's probably fine to avoid putting it in the way of new nodes.

For my testing so far I only used it for inspecting existing node-trees rather than in an actual workflow. For the actual fine-tuning I have the feeling it might be easier to do when the initial version is in main.

2 points that I have left:

could it make sense to use the currently visible area of the node-editor and interpret points that are out of view as a collision? To avoid placing the viewer out of view
I still do like the idea of leaving the viewer where it is in certain cases. I do think in cases where the user moved the viewer manually into a more convenient place and then keeps inspecting the same general area of nodes, it might be more annoying than useful to have it jump around. One case especially that I'd like to avoid for example is moving it back when cycling through outputs of the same node it is already connected to, after the user moved it manually.
So I would propose to try two things:
- if the viewer is already connected to the targeted node: don't move it, just cycle through the outputs
- if the viewer is already to the right of the target node, it is currently not colliding (+in view) and the new found position is further from the target than the current one: don't move it, just connect

I think that logic could work well. Not sure if you would try these things in a separate PR.

The new search pattern works a lot better for me! It's still trying quite hard not to put it below the target node. But that's probably fine to avoid putting it in the way of new nodes. For my testing so far I only used it for inspecting existing node-trees rather than in an actual workflow. For the actual fine-tuning I have the feeling it might be easier to do when the initial version is in main. 2 points that I have left: - could it make sense to use the currently visible area of the node-editor and interpret points that are out of view as a collision? To avoid placing the viewer out of view - I still do like the idea of leaving the viewer where it is in certain cases. I do think in cases where the user moved the viewer manually into a more convenient place and then keeps inspecting the same general area of nodes, it might be more annoying than useful to have it jump around. One case especially that I'd like to avoid for example is moving it back when cycling through outputs of the same node it is already connected to, after the user moved it manually. So I would propose to try two things: - if the viewer is already connected to the targeted node: don't move it, just cycle through the outputs - if the viewer is already to the right of the target node, it is currently not colliding (+in view) and the new found position is further from the target than the current one: don't move it, just connect I think that logic could work well. Not sure if you would try these things in a separate PR.

👍 1

Jacques Lucke added 5 commits 2024-05-29 15:34:41 +02:00

cleanup d7b2d9a132

take region bounds into account 7cc14ce7ee

Merge branch 'main' into viewer-node-position 12a296895b

remove reliance on socket position a311760117

avoid moving node if the new location is further away 7773b4ed04

Jacques Lucke added 1 commit 2024-05-29 15:36:53 +02:00

fix description 072f499734

Jacques Lucke commented

2024-05-29 15:39:50 +02:00

I implemented some of the suggested improvements. Please have another look.

I don't have a good example where I moved the viewer manually where I would want it to stay at a specific location when viewer another socket of the same node again. So I did not implement that part yet. The viewer generally shouldn't end up further than it was anymore though.

When not moving the viewer manually, cycling through the sockets of a single node should not move the viewer.

I implemented some of the suggested improvements. Please have another look. I don't have a good example where I moved the viewer manually where I would want it to stay at a specific location when viewer another socket of the same node again. So I did not implement that part yet. The viewer generally shouldn't end up further than it was anymore though. When not moving the viewer manually, cycling through the sockets of a single node should not move the viewer.

Simon Thommes approved these changes 2024-05-29 16:11:18 +02:00

Simon Thommes left a comment

This is pretty good to me now!
If we identify certain cases where it doesn't perform we can still tweak that later, but this is good to go into main imo.

Looking into animating the position as a transition could still be interesting, but it's not required

This is pretty good to me now! If we identify certain cases where it doesn't perform we can still tweak that later, but this is good to go into main imo. Looking into animating the position as a transition could still be interesting, but it's not required

Jacques Lucke merged commit 7be4d4f443 into main

2024-05-29 16:49:47 +02:00

Jacques Lucke referenced this issue from a commit

2024-05-29 16:49:47 +02:00

Nodes: automatically move viewer node to current node

Jacques Lucke deleted branch viewer-node-position

2024-05-29 16:49:50 +02:00

John Kiril Swenson referenced this issue from a commit

2024-06-05 05:27:05 +02:00

Nodes: automatically move viewer node to current node

Iliya Katushenock referenced this pull request

2024-09-22 23:30:40 +02:00