Nodes: Port "Connect to Output" operator from Node Wrangler #122016

Merged
Jacques Lucke merged 11 commits from nickberckley/blender:connect-to-output into main 2024-05-28 16:43:08 +02:00
Contributor

As Node Wrangler add-on is moving to extensions platform and isn't shipped with Blender from 4.2 onwards, some of it's more important functionalities which are crucial for Blender UX can be ported inside core. Design task here.

This PR ports "Preview Node" operator from add-on inside scripts/startup/bl_operators.

When Shift-Ctrl (or Shift-Alt) clicked on the node operator connects output socket of the node to active Output node in the tree (group, material, light, or world output). For Geometry Nodes this is just handy operator, but in Shader Nodes, because Viewer Node isn't implemented, this has always been the only way to quickly preview nodes by connecting it to Material Output.

Changes made from Node Wrangler version:

  • Renamed operator from "Preview Node" to "Connect to Output", because node previews already mean a different thing in context of compositor and shader editor, and viewer node is used for previewing in geometry nodes. Connect to Output is correct name because in every context it's called that's what it does.

  • Assigned shortcut Shift-Alt-Click in shader editor as well. Even though Shift-Ctrl click already works, it's good to have consistency with geometry nodes, so that users can use same shortcut in both contexts.

Operator doesn't work in compositor. It's not clear if that's wanted (to connect to Composite node), and if it is it'll be added in separate PR.


Some functions are implemented in separate python file, because they're reused by other important features from Node Wrangler that can be ported as well, so this way it's easier to reuse them.

After PR is merged this feature will be removed from Node Wrangler extension (which is hosted here), so that it doesn't implement what is already part of Blender.


Original authors of the add-on: Bartek Skorupa, Greg Zaal, Sebastian Koenig, Christian Brinkmann, Florian Meyer

As Node Wrangler add-on is moving to extensions platform and isn't shipped with Blender from 4.2 onwards, some of it's more important functionalities which are crucial for Blender UX can be ported inside core. [Design task here.](https://projects.blender.org/blender/blender/issues/121749) This PR ports "Preview Node" operator from add-on inside scripts/startup/bl_operators. When Shift-Ctrl (or Shift-Alt) clicked on the node operator connects output socket of the node to active Output node in the tree (group, material, light, or world output). For Geometry Nodes this is just handy operator, but in Shader Nodes, because Viewer Node isn't implemented, this has always been the only way to quickly preview nodes by connecting it to Material Output. Changes made from Node Wrangler version: - Renamed operator from "Preview Node" to "**Connect to Output**", because node previews already mean a different thing in context of compositor and shader editor, and viewer node is used for previewing in geometry nodes. Connect to Output is correct name because in every context it's called that's what it does. - Assigned shortcut Shift-Alt-Click in shader editor as well. Even though Shift-Ctrl click already works, it's good to have consistency with geometry nodes, so that users can use same shortcut in both contexts. Operator doesn't work in compositor. It's not clear if that's wanted (to connect to Composite node), and if it is it'll be added in separate PR. --- Some functions are implemented in separate python file, because they're reused by other important features from Node Wrangler that can be ported as well, so this way it's easier to reuse them. After PR is merged this feature will be removed from Node Wrangler extension (which is hosted here), so that it doesn't implement what is already part of Blender. --- Original authors of the add-on: Bartek Skorupa, Greg Zaal, Sebastian Koenig, Christian Brinkmann, Florian Meyer
Nika Kutsniashvili added 1 commit 2024-05-20 18:02:25 +02:00
Nika Kutsniashvili changed title from Nodes: Port onnect to output operator to WIP: Nodes: Port "Connect to Output" operator from Node Wrangler 2024-05-20 18:02:48 +02:00
Iliya Katushenock added this to the Nodes & Physics project 2024-05-20 18:02:53 +02:00
Iliya Katushenock added the
Interest
Python API
label 2024-05-20 18:02:58 +02:00
Nika Kutsniashvili changed title from WIP: Nodes: Port "Connect to Output" operator from Node Wrangler to Nodes: Port "Connect to Output" operator from Node Wrangler 2024-05-20 18:11:51 +02:00
Nika Kutsniashvili requested review from Jacques Lucke 2024-05-20 18:12:02 +02:00
Nika Kutsniashvili reviewed 2024-05-20 18:18:24 +02:00
@ -61,0 +65,4 @@
default=False,
description="An internal property used to determine if a socket is generated for preview purposes"
)
Author
Contributor

This works as you would expect. But there are no other properties registered by files in bl_operators. I wonder if there is more correct place for this?

This works as you would expect. But there are no other properties registered by files in bl_operators. I wonder if there is more correct place for this?
JacquesLucke marked this conversation as resolved
Nika Kutsniashvili added 1 commit 2024-05-20 18:43:38 +02:00
Jacques Lucke requested changes 2024-05-20 20:04:26 +02:00
Jacques Lucke left a comment
Member

Thanks. For inclusion into core Blender, a few more things have to be done. I added some inline comments. Hope it's clear enough.

Thanks. For inclusion into core Blender, a few more things have to be done. I added some inline comments. Hope it's clear enough.
@ -2176,6 +2176,12 @@ def km_node_editor(params):
{"type": 'LEFTMOUSE' if params.legacy else 'RIGHTMOUSE', "value": 'CLICK_DRAG', "ctrl": True}, None),
("node.links_mute", {"type": 'RIGHTMOUSE', "value": 'CLICK_DRAG', "ctrl": True, "alt": True}, None),
("node.select_link_viewer", {"type": 'LEFTMOUSE', "value": 'PRESS', "shift": True, "ctrl": True}, None),
("node.connect_to_output", {"type": 'LEFTMOUSE', "value": 'PRESS', "shift": True, "ctrl": True},
Member

Some comment here for why three keymap entries are necessary would be good.

Some comment here for why three keymap entries are necessary would be good.
nickberckley marked this conversation as resolved
@ -59,2 +60,4 @@
register_class(cls)
# properties
bpy.types.NodeTreeInterfaceSocket.ViewerSocket = bpy.props.BoolProperty(
Member

The more correct way to do this is to actually make this a built-in property stored in bNodeTreeInterfaceSocket (probably in its flag). The property should also be called is_main_output or so. There should be a comment mentioning that this is used for this operator.

The more correct way to do this is to actually make this a built-in property stored in `bNodeTreeInterfaceSocket` (probably in its `flag`). The property should also be called `is_main_output` or so. There should be a comment mentioning that this is used for this operator.
Author
Contributor

I resolved rest, but this is above me I'm afraid, even though it's a simple request. I only know python. Perhaps it's fine as custom property for now, as it's workaround? Until Viewer node is added to Shader editor

I resolved rest, but this is above me I'm afraid, even though it's a simple request. I only know python. Perhaps it's fine as custom property for now, as it's workaround? Until Viewer node is added to Shader editor
Member

Think it would still be better to add it as a built-in flag. I can guide you through it if you need help. It should be fairly straight forward. Would also be a good learning experience for you I think.

  1. Add new flag to NodeTreeInterfaceSocketFlag (in DNA_node_tree_interface_types.h): NODE_INTERFACE_SOCKET_VIEWER. Also add a comment above it mentioning that it exists until shader nodes have a proper viewer.
  2. Add the RNA definition for this new flag in rna_node_tree_interface.cc. This makes the property available in Python. This is very similar to NODE_INTERFACE_SOCKET_LAYER_SELECTION, so you can copy the corresponding (5 or 6) lines and adapt them to the new property.
  3. Then you can just use the property like you already did (just rename it from ViewerSocket).
Think it would still be better to add it as a built-in flag. I can guide you through it if you need help. It should be fairly straight forward. Would also be a good learning experience for you I think. 1. Add new flag to `NodeTreeInterfaceSocketFlag` (in `DNA_node_tree_interface_types.h`): `NODE_INTERFACE_SOCKET_VIEWER`. Also add a comment above it mentioning that it exists until shader nodes have a proper viewer. 2. Add the RNA definition for this new flag in `rna_node_tree_interface.cc`. This makes the property available in Python. This is very similar to `NODE_INTERFACE_SOCKET_LAYER_SELECTION`, so you can copy the corresponding (5 or 6) lines and adapt them to the new property. 3. Then you can just use the property like you already did (just rename it from `ViewerSocket`).
nickberckley marked this conversation as resolved
@ -0,0 +56,4 @@
"""Get correct output node in shader editor"""
if shader_type == 'OBJECT':
if space.id in bpy.data.lights.values():
self.shader_output_type = 'OUTPUT_LIGHT'
Member

I think we shouldn't need to type here, just using the idname below should be enough.

I think we shouldn't need to type here, just using the `idname` below should be enough.
nickberckley marked this conversation as resolved
@ -0,0 +57,4 @@
if shader_type == 'OBJECT':
if space.id in bpy.data.lights.values():
self.shader_output_type = 'OUTPUT_LIGHT'
self.shader_output_ident = 'ShaderNodeOutputLight'
Member

rename to shader_output_idname

rename to `shader_output_idname`
nickberckley marked this conversation as resolved
@ -0,0 +65,4 @@
self.shader_output_type = 'OUTPUT_WORLD'
self.shader_output_ident = 'ShaderNodeOutputWorld'
def ensure_viewer_socket(self, node_tree, socket_type, connect_socket=None):
Member

I think the term "viewer" should be removed from this code as it's actually about connecting to an output, not to a viewer.

I think the term "viewer" should be removed from this code as it's actually about connecting to an output, not to a viewer.
Author
Contributor

This refers to temporary socket it creates in shader editor node groups, so that link exits from that to connect to output in root tree. In this context I think it makes sense, as it substitutes for viewer node. But maybe we can refer to it as preview socket? or inspect socket

This refers to temporary socket it creates in shader editor node groups, so that link exits from that to connect to output in root tree. In this context I think it makes sense, as it substitutes for viewer node. But maybe we can refer to it as preview socket? or inspect socket
Member

Hmm right, don't have a strong opinion right now. Maybe @HooglyBoogly has an opinion.

Hmm right, don't have a strong opinion right now. Maybe @HooglyBoogly has an opinion.
Member

No strong opinion, if this is just emulating a viewer because the lack of a viewer node, seems fine to reuse that word.

No strong opinion, if this is just emulating a viewer because the lack of a viewer node, seems fine to reuse that word.
JacquesLucke marked this conversation as resolved
@ -0,0 +72,4 @@
if len(output_sockets):
for i, socket in enumerate(output_sockets):
if is_viewer_socket(socket) and socket.socket_type == socket_type:
# If viewer output is already used but leads to the same socket we can still use it
Member

Use Blenders comment style, which means that it should end with a dot. Same for most other comments.

Use Blenders comment style, which means that it should end with a dot. Same for most other comments.
nickberckley marked this conversation as resolved
@ -0,0 +107,4 @@
return groupout
@classmethod
def search_sockets(cls, node, sockets, index=None):
Member

sockets -> r_sockets

`sockets` -> `r_sockets`
Author
Contributor

Done. But can you explain style behind this?
Also, should I rename all sockets args to r_sockets or just this one?

Done. But can you explain style behind this? Also, should I rename all sockets args to r_sockets or just this one?
Member

We use the r_ prefix sometimes for "return parameters", i.e. parameters that are filled in by the function. An alternative could be e.g. "found_sockets". I found just "sockets" to be to ambiguous.

We use the `r_` prefix sometimes for "return parameters", i.e. parameters that are filled in by the function. An alternative could be e.g. "found_sockets". I found just "sockets" to be to ambiguous.
nickberckley marked this conversation as resolved
@ -0,0 +166,4 @@
return socket in self.used_viewer_sockets_active_mat
def has_socket_other_users(self, socket):
"""List the other users for this socket (other materials or GN groups)"""
Member

We don't really shorten geometry nodes to GN in the code base.

We don't really shorten geometry nodes to GN in the code base.
nickberckley marked this conversation as resolved
@ -0,0 +295,4 @@
break
if output_node_socket_index is None:
# Create geometry socket
geometry_out_socket = base_node_tree.interface.new_socket(
Member

Seems like this should call ensure_viewer_socket instead of adding the interface socket here?

Seems like this should call `ensure_viewer_socket` instead of adding the interface socket here?
nickberckley marked this conversation as resolved
Jacques Lucke requested review from Hans Goudey 2024-05-20 20:06:57 +02:00
Member

You might be interested in blender/blender-addons!105136, since it's not going to make it into Node Wrangler at this point.
I tried getting feedback for it at the time on forums but didn't get much luck. However the original author of the feature, Greg Zaal, seemed to think it was ok.
I could port it at a later time if this PR is accepted.

You might be interested in blender/blender-addons!105136, since it's not going to make it into Node Wrangler at this point. I tried getting feedback for it at the time on forums but didn't get much luck. However the original author of the feature, Greg Zaal, seemed to think it was ok. I could port it at a later time if this PR is accepted.
Nika Kutsniashvili added 3 commits 2024-05-20 21:28:39 +02:00
Nika Kutsniashvili added 1 commit 2024-05-20 21:38:56 +02:00
Jacques Lucke requested changes 2024-05-21 19:34:35 +02:00
@ -2179,0 +2182,4 @@
{"properties": [("run_in_geometry_nodes", False)]}),
("node.connect_to_output", {"type": 'LEFTMOUSE', "value": 'PRESS', "shift": True, "alt": True},
{"properties": [("run_in_geometry_nodes", False)]}),
# Second shortcut is added in shader editor as this operator is stand-in for absent viewer node functionality.
Member

Comments should come before the code they refer to. Also double check the indentation.

Comments should come before the code they refer to. Also double check the indentation.
nickberckley marked this conversation as resolved
Nika Kutsniashvili added 3 commits 2024-05-22 20:59:22 +02:00
Author
Contributor

@JacquesLucke think I added the property correctly, it works when I test it at least. Thank you for guidance.

I named it "is_inspect_output", because:

  • I didn't want to use word "viewer" as it already means something
  • I saw that geometry nodes also uses this. When you're in nested node group and connect one of the nodes to output it creates that socket in there too, so that link exits and connects to Group Output in root tree. So it is going to be needed even if Viewer node is added in shader editor.

Inspect I though made sense, but maybe you have better names to suggest.

@JacquesLucke think I added the property correctly, it works when I test it at least. Thank you for guidance. I named it "is_inspect_output", because: - I didn't want to use word "viewer" as it already means something - I saw that geometry nodes also uses this. When you're in nested node group and connect one of the nodes to output it creates that socket in there too, so that link exits and connects to Group Output in root tree. So it is going to be needed even if Viewer node is added in shader editor. Inspect I though made sense, but maybe you have better names to suggest.
Jacques Lucke approved these changes 2024-05-23 13:57:01 +02:00
Jacques Lucke left a comment
Member

Overall looks good now. Only needs some remaining minor changes.

I attached a diff with the format fixes I get when running make format.

Overall looks good now. Only needs some remaining minor changes. I attached a diff with the format fixes I get when running `make format`.
@ -2178,1 +2178,4 @@
("node.select_link_viewer", {"type": 'LEFTMOUSE', "value": 'PRESS', "shift": True, "ctrl": True}, None),
("node.connect_to_output", {"type": 'LEFTMOUSE', "value": 'PRESS', "shift": True, "alt": True},
{"properties": [("run_in_geometry_nodes", True)]}),
# Shortcut is added twice in shader editor as this operator is stand-in for absent viewer node functionality.
Member

Looks like now this comment is in front of the shortcut that is only added once (shift+ctrl+click) instead of at the one that is added twice (shift+alt+click). Better write one comment above all three new entries explaining why there are three entries (it's ok to say that it is for historic reasons).

Looks like now this comment is in front of the shortcut that is only added once (shift+ctrl+click) instead of at the one that is added twice (shift+alt+click). Better write one comment above all three new entries explaining why there are three entries (it's ok to say that it is for historic reasons).
JacquesLucke marked this conversation as resolved
Nika Kutsniashvili added 1 commit 2024-05-23 14:32:56 +02:00
Formatting
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
52ba2a48ab
Member

@blender-bot build

@blender-bot build
Nika Kutsniashvili added 1 commit 2024-05-24 14:40:59 +02:00
More formatting
All checks were successful
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
ca4f44e93b
Member

@blender-bot build

@blender-bot build
Jacques Lucke merged commit 61178b22a2 into main 2024-05-28 16:43:08 +02:00
Nika Kutsniashvili deleted branch connect-to-output 2024-05-28 16:44:42 +02:00
Contributor

Compositor preview I think important. Consistency helps to get a good UX.

Compositor preview I think important. Consistency helps to get a good UX.
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
5 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#122016
No description provided.