Node Wrangler: LTS: backport many bugfixes from main #105169

Damien Picard · 2024-02-08T18:45:20+01:00

Damien Picard commented

2024-02-08 18:45:20 +01:00

This PR includes many bug fixes to Node Wrangler done over the past few months. Some of these fixes relied on previous refactors, so they are included as well.

Main areas affected are the Preview and Add Reroute operators.

This was discussed with @PratikPB2123 in #105095, and a fix for that issue is included as well.

This PR includes many bug fixes to Node Wrangler done over the past few months. Some of these fixes relied on previous refactors, so they are included as well. Main areas affected are the Preview and Add Reroute operators. ----- This was discussed with @PratikPB2123 in #105095, and a fix for that issue is included as well.

Damien Picard added 18 commits 2024-02-08 18:45:22 +01:00

Fix: Node Wrangler: wrong API in CopySettings operator 7274a3eabe

Previous commit dc6c334c4d changed the API used to create links
between nodes, but the call for the CopySettings operator was not
updated properly.

Refactor: Node Wrangler: PreviewNode operator 0e0844499f

This commit refactors the PreviewNode operator so that code is
deduplicated between the Geometry Nodes and the shader branches.

- The link collected in `make_links` is redundant as it would have
  been created later in the function.
- Remove intermediate variable `node_socket`.
- Extract `get_output_index()` and `create_links()` to methods to
  avoid duplication in `invoke()`
- Return `{'CANCELLED'}` instead of `{'FINISHED'}` when there was no
  change.
- Exit early when possible.
- Use `if ... is None:` instead of `if ...:` when applicable.
- Remove bits of unused code.
- Tweak comments and convert them to docstrings when extracted to a
  function.
- Convert methods to `staticmethod`s or `classmethod`s when possible.

Removes a warning when previewing inside a group:
"Sockets do not belong to the same node tree"

Refactor: Node Wrangler: poll methods bac0dea7ae

Chain conditions with boolean operators instead of setting a default
False variable and nested `if`s.

Node Wrangler: make tree type check more granular in operator polls 7de11727b4

Currently, all Node Wrangler operators call `nw_check()` as part of
their poll method. Among other things, this function ensures that
operators can only be run on built-in node types. But many useful NW
operators work as well on custom Python nodes.

This commit replaces this type check with a more specific
`nw_check_space_type()` function, with appropriate checks where needed
in individual polls. This function also sets the poll message to
report why an operator cannot be called.

Fix #100973 : Node Wrangler: Previewing node if hierarchy not active f4d395decf

Node Wrangler's preview_node operator can link the active node to the
output node. If the current node tree is a node group inside the base
tree, it can go up the hierarchy to connect until reaching the output
node.

Until now it used a recursive function to go through each active node
from the base tree. If only a single node editor is open, the user can
only open a node group by activating it. But otherwise, they can
select another node at a lower level in the node group hierachy and
the recursion breaks.

This commit changes the operator to use instead the
`SpaceNodeEditor.path`, which gets each node tree in the hierarchy.
This way, we no longer depend on any node in the group hierarchy being
active or not.

Fix #100895 : Node Wrangler not working properly inside Groups 31a4b1a1d5

The logic getting the next available socket for the active node when
trying to preview several sockets in turn, was only ever implemented
for materials and not for geometry nodes.

Fix: Node Wrangler: error previewing input node with virtual socket 99278d6b05

When searching for the socket to preview, we need to consider the case
where there is one socket available, but we cannot preview it, such as
with group input nodes' virtual socket.

The check was already there for Geometry Nodes, and this commit adds
it for shading nodes.

Node Wrangler: make users of GN node groups independent for viewer b5b7dd9e34

A GN node group may be used by multiple modifiers. If multiple nodes
inside a nested node group are viewed by different modifiers, the
other users would get disconnected because the group output was
recreated.

Instead, check for other users of this group and create a new socket
if needed. This was already the behavior for Shaders.

Refactor: Node Wrangler: NWAddReroutes operator b654e24050

- Remove unused variables.
- Exit early.

Node Wrangler: do not add reroutes to unavailable outputs e281baeb1a

This avoids unconnected reroutes in some nodes such as the Shader
Editor's Mix node. This node has multiple modes, and one output for
each mode, but only one is shown at a time. The others are unavailable
and should be ignored.

Node Wrangler: select only newly-created nodes when adding reroutes e7138d6699

Selection was not always coherent after adding reroutes, such as with
hidden nodes.

This also allows restoring selection and cancelling the operator if no
change was made.

Fix: Node Wrangler: new reroute locations on hidden nodes 9f68635acf

When a node was hidden, the NWAddReroutes operator used a hack to
calculate its width. This was broken since at least 2.80, and not
really needed AFAICT.

Replace this hack with the width directly read from the node.

Fix: Node Wrangler: new reroutes offset when output hidden 280867f827

Depending on option, some outputs would be counted in the offset of
the new reroutes, even though they were hidden and no reroute was
created. This resulted in a misalignment.

Reroutes are now offset according to the actual spacing of displayed
outputs.

Node Wrangler: add more specific poll methods e6070989ed

Since it is useful to set a poll message to tell the user why an
operator cannot be called, decompose poll conditions into basic
functions and make every operator use them.

- Make the basic nw_check report issues.
- Add functions to check that:
  - the node tree is not empty,
  - a node is active,
  - a specified number of nodes are selected,
  - the active node is of a specific type,
  - it has visible outputs,
  - there is a viewer image.
- These functions report the issue to the poll message otherwise.
- Go through operators and add or update poll methods using those
  various functions.
- In a few operators, remove obsolete error reports that are now
  caught at the poll stage.

Fixes a few issues:
- PreviewNode would not work immediately when the active node was a
  material or world output.
- ReloadImages needed an active node.

Fix #104991 : Node Wrangler Merge Nodes shadows other keymaps (Ctrl+,) 0c44af5635

Node Wrangler's Merge Nodes operator uses many shortcuts to merge
inputs in different ways. One of those shortcuts is Ctrl + , (comma)
which was not used by default until Blender 4.0. In this version it
was affected the Open Preferences Operator, resulting in a conflict.

In many cases, the operator will have no action, such as when no node
tree is active, or when no node is selected. In those cases, the
operator can simply return {'PASS_THROUGH'} and let the shortcut be
handled by another operator.

A cleaner solution to this issue would be to refactor the operator so
that the list of nodes to act on is gathered in the poll() method, and
not call the operator at all if there is nothing to do, but this
refactor would require deeper changes.

Fix #104941 : Node Wrangler cannot use both bump and normal abc910a7fb

Currently, if both bump and normal maps are selected in Principled
Setup, only one of them will be used. But they can both be used in a
PBR setup by plugging the normal output to the bump input.

See for instance Brecht's answer at
https://blender.stackexchange.com/a/16447.

This commit separates setup of bump and normal so they can both be
used at the same time.

Node Wrangler: skip internal attributes while populating menu 6636ee5856

The Attributes menu generated by node wrangler for the shader editor included internal attributes not meant for use in shaders.

Pull Request: #105084

Node Wrangler: bump version 07ed2593ad

This version is specific to Blender 3.6 LTS. Since NW only uses major
and minor version numbers, adding a bugfix version will not collide
with main (4.0+).

Pratik Borhade commented

2024-02-09 12:30:42 +01:00

@lichtwerk ^

Philipp Oeser commented

2024-02-09 12:34:03 +01:00

Please include this in #109399 so it is not forgotten

👍 1

Damien Picard referenced this pull request from blender/blender

2024-02-09 14:26:20 +01:00

Blender LTS: Maintenance Task 3.6 #109399

Philipp Oeser commented

2024-02-15 10:50:27 +01:00

I tried to check with others if such larger refactors are fine for LTS, but havent recieved feedback yet...

Damien Picard commented

2024-02-15 11:19:12 +01:00

I tried to check with others if such larger refactors are fine for LTS, but havent recieved feedback yet...

I’d understand if they weren’t honestly, especially since I don’t know how to write tests for interactive tools. If the refactors are unacceptable, I could maybe still extract a few easy fixes out of this, but I won’t rewrite each commit that relies on the refactors.
Thanks for considering it anyway :)

> I tried to check with others if such larger refactors are fine for LTS, but havent recieved feedback yet... I’d understand if they weren’t honestly, especially since I don’t know how to write tests for interactive tools. If the refactors are unacceptable, I could maybe still extract a few easy fixes out of this, but I won’t rewrite each commit that relies on the refactors. Thanks for considering it anyway :)

Brecht Van Lommel commented

2024-02-16 15:08:59 +01:00

I think this is beyond the types of bugfixes we should backport. It's too hard to review this and conclude with some certainty that it won't introduce new bugs.

Damien Picard force-pushed dp_nw_3.6 from 07ed2593ad to f42344c798

2024-02-16 18:43:33 +01:00

Compare

Damien Picard commented

2024-02-16 18:46:13 +01:00

That’s understandable. I removed the more involved commits, and will keep them available in my personal fork.

That’s understandable. I removed the more involved commits, and will keep them available in [my personal fork](https://projects.blender.org/pioverfour/blender-addons/src/branch/blender-v3.6-release).

Brecht Van Lommel approved these changes 2024-02-19 22:41:09 +01:00

Damien Picard commented

2024-02-20 20:30:36 +01:00

Just to be clear, am I expected to merge this myself or must this be handled by @lichtwerk as part of LTS maintenance?