Node Wrangler: Improved accuracy on Align Nodes operator #104551

quackarooni · 2023-04-17T17:56:35+02:00

quackarooni commented

2023-04-17 17:56:35 +02:00

Summary of the issue:

The Align Nodes operator for Node Wrangler – NWAlignNodes, was intended to evenly space nodes such that their midpoints lie in the same horizontal/vertical line. However, due to edge cases not considered by the code, it is often very easy to run into situations where the nodes are not properly aligned. Nodes inside frames, hidden nodes, reroutes, or any combination of those three would usually lead to inaccurate alignments, as they were not being considered by the original code.

Implemented solution:

This PR addresses that by having different midpoint calculations to handle hidden nodes and reroutes. As for framed nodes, a context manager was implemented to temporarily unframe them, do the alignment, and put them back again to their parent frames.

User-facing changes:

Addtionally, the process of improving the calculation logic also led to tweaks in the UI. One of these was adding a selection length check into the operator's poll function which grays out the operator buttons when not enough nodes are selected. This also had the effect of preventing an arguably very unintuitive behavior where an empty selection gets treated as if all nodes are selected.

(It isn't a bug, but rather deliberately coded in the operator. Arguably it saves a keystroke from having to select all nodes, but just leads to confusion especially in the previously mentioned edge cases where the alignment calculations were also broken - see #99904)

Other UI changes include exposing alignment axis options in the context menu and N-panel, and having the margin properties that are configurable in the preference menu.

## Summary of the issue: The Align Nodes operator for Node Wrangler – `NWAlignNodes`, was intended to evenly space nodes such that their midpoints lie in the same horizontal/vertical line. However, due to edge cases not considered by the code, it is often very easy to run into situations where the nodes are not properly aligned. Nodes inside frames, hidden nodes, reroutes, or any combination of those three would usually lead to inaccurate alignments, as they were not being considered by the original code. ## Implemented solution: This PR addresses that by having different midpoint calculations to handle hidden nodes and reroutes. As for framed nodes, a context manager was implemented to temporarily unframe them, do the alignment, and put them back again to their parent frames. ## User-facing changes: Addtionally, the process of improving the calculation logic also led to tweaks in the UI. One of these was adding a selection length check into the operator's `poll` function which grays out the operator buttons when not enough nodes are selected. This also had the effect of preventing an arguably very unintuitive behavior where an empty selection gets treated as if all nodes are selected. *(It isn't a bug, but rather deliberately coded in the operator. Arguably it saves a keystroke from having to select all nodes, but just leads to confusion especially in the previously mentioned edge cases where the alignment calculations were also broken - see #99904)* Other UI changes include exposing alignment axis options in the context menu and N-panel, and having the margin properties that are configurable in the preference menu.

quackarooni added 9 commits 2023-04-17 17:56:37 +02:00

Fix #99904 : Fixed Unintuitive Selection Behavior during Align Nodes 39aba28381

This commit adds a poll check for the Align Nodes
operator that only allows execution if at least two non-
frame nodes are selected. This is intended to address
two behaviors in the operator that work exactly as
intended but are neither intuitive for the end user.

First, frames are ignored by this operator, as they're not
relevant to the calculations being done. However, this is
not made apparent to the user in any way. The poll
function check hopes to address this by not allowing the user
to call the operator in cases where only frames are selected.

Second, is that when the operator is given an empty selection,
it is coded to act as if all nodes were selected. This doesn't
seem to offer much in the way of practical utility, since a user
can easily just call Select All before calling the operator. If
anything, it seems to be more a cause for confusion,
as can be seen in the relevant issue.

Since the poll function does not allow execution for empty
selections, the aforementioned behavior has been rendered
obsolete and was removed. Overall, the new behavior of being
unable to call Align Nodes when no nodes are selected
would seem to be more in line with how a user might
reasonably expect the operator to behave.

Fix: Node Wrangler's Align Nodes inaccurately aligning nodes in frames cbe34040be

The calculations for the Align Nodes operator currently uses
node.location, which is relative to the frame a node is parented to.
This works fine when the selected nodes are not in a frame, or when
they're all parented under the same frame, but breaks in cases where
the nodes are inside separate frames.

The commit fixes this by implementing a context manager that
temporarily unframes the selected nodes, allowing the operator to
do its calculations using the nodes' absolute locations, before
returning the nodes to their parent frames after execution. This
allows the operator to work in whatever configuration the selected
nodes are framed in.

Node Wrangler: Refactored Align Nodes operator 8e834b4433

Refactored NWAlignNodes to make it easier to implement future changes.
This is done as a cleanup and precursor to later commits
planned on this branch.

Notable changes in this commit are as follows:
1.) Used .sort() instead of sorted().
This avoids making an extra copy of the list, and is more succinct and
reflective on the original code's intention of an in-place sort.

2.)  Moved is_horizontal checks to occur out of loops.
The variable doesn't change per node, so it makes more sense to
not repeatedly check it while iterating nodes. Putting it outside
of loops also separates horizontal and vertical alignment behavior,
which would be needed for later bug fixes down the line.

3.) Changed all instances of "x / 2" to "0.5 * x".
Both cases of dividing by 2 and multiplying by 0.5 occur in the code,
this commit changed all of them to be consistent. It could be debated
which one is more readable, but multiplying by a factor would probably
be more workable if ever options for left or right-aligning and
top or bottom-aligning would be implemented in the future.

Fix: Inaccurate alignment for hidden nodes in Node Wrangler's Align Nodes 4d9b5451fc

For collapsed/hidden nodes, their origin is located on their middle-left point,
instead of the top-left corner like in uncollapsed nodes. This wasn't
fully taken into consideration by the original code, leading to inaccuracies
when aligning hidden nodes, which this commit aims to address.

The changes implemented by this commit are as follows:

1.) Whether a node is hidden or not is no longer considered during
horizontal alignment, which avoids uneven spacing. As hiding a node
only affects its vertical dimensions, not its horizontal one.

2.) Added corrective offsets for when vertically aligning hidden nodes.
Due to how their origins are positioned, the original code would align
hidden nodes by their bottoms, instead of their middle like in
non-hidden nodes.  So an extra offset was needed to align hidden nodes
by their center.

3.) Improved consistency when aligning non-hidden and hidden nodes
together. The original code's inaccuracies become most apparent when
mixing both types of nodes together, where uneven margins and
overlapping frequently occurs.

Fix: Nodes drifting downward when Align Nodes are repeatedly applied f8816bd075

The commit preceding this one introduced code that applied corrective
offsets to hidden nodes so that they would be visually aligned with non-
hidden nodes.  A separate calculation for calculating midpoints did not
factor in those offsets, leading to a bug wherein nodes slowly inch
downward when a horizontal align was applied repeatedly.

This behavior occurs specifically during horizontal alignment where the
active node is not selected. The changes in this commit aim to fix that
behavior by accounting for said corrective offsets during midpoint
calculation. Some stylistic code changes are also applied to
relevant code for better readability.

Node Wrangler: Added explicit axis options for Align Nodes operator e42d6620c5

The Align Nodes operator has functionality for both horizontal and
vertical alignment, but which axis was used is something that was
automatically decided by the operator. While this is sufficient in most
of cases, there is a good number of instances in which the automatic
alignment is not ideal. Such as when a user may want to turn a row of
nodes into a column, and vice versa, for example.

This commit aims to make the operator more flexible by giving the user
the option to explicitly choose which axis they would want the alignment
to occur in.  Additional buttons in the UI panel and context menu were
added for these modes, currently named "Align X" and "Align Y". The
"Align Nodes" button has been also renamed to "Auto-Align Nodes" for
better clarity.

In conjunction to that, a property is added in the addon's preferences
menu that allows the user to specify different spacings for X and Y
alignment. This gets rid of the hard coded (0.3 * margin) expression for
vertical alignment, and allows the user to customize the operator
whatever margins they find ideal. Layout changes in the user preferences
UI have been made to accomodate this property, how this was
approached is tentative and could be change given
subsequent review/feedback.

Fix: Uneven spacing on reroutes in Node Wrangler's Align Nodes 58a0f9ee31

Node dimensions are taken into account in order to properly align nodes, but the dimensions of reroutes lead to them getting placed off-center during alignment. According to the API, reroutes have a dimension of (16, 16) units, but visually, dimensions of 10 units instead of 16 seem to be closer to what the user sees. The changes in this commit make it so that the calculations use those dimensions instead, which results in visually more even spacing between nodes and reroutes.

Additionally, the fact that a hidden state (where node.hide == True)
would not affect placement calculation for reroutes, as opposed to
hidden nodes where it does, having separated placement logic for
reroutes and nodes seems justified enough to implement.

Correcting for the position of reroutes however, does seem to introduce
drift perpendicular to whatever axis the alignment takes place in. At the
cost of doing more calculations, addressing this actually compresses the
code as the nodes now always have to be repositioned in both axes to
avoid drift.

Node Wrangler: Cleaned up NWAlignNodes operator 0c96a3fded

Final cleanup of the operator before opening it as a pull request.
Moved the midpoint calculation to its own staticmethod since it
is called multiple times, and being named "get_midpoint" should
make it what it is doing clear for anybody reading the code.

Additionally, trailing whitespace has been cleaned out, and some
parentheses are added to make certain expressions clearer.

Node Wrangler: Refactored NWAlignNodes poll function 8e80eeef06

Forgot to add this one in the cleanup commit, but this refactors
the poll function to use a generator expression instead of a list.
This avoids having to make an entire list of selected nodes just to
find out whether at least two nodes are selected.

quackarooni added 1 commit 2023-04-17 17:57:00 +02:00

Merge branch 'main' into nw_rework_align_nodes edfb41c14c

quackarooni commented

2023-04-17 18:00:50 +02:00

Documentation:

Comparison of old and new operator behavior

# Documentation: ## Comparison of old and new operator behavior

nw.align_nodes.fix_reroutes_being_off_center.mp4

395 KiB

nw.align_nodes.fix_hidden_nodes_misaligned.mp4

582 KiB

nw.align_nodes.unintuitive_selection_behavior.mp4

471 KiB

nw.align_nodes.fix_frames.mp4

279 KiB

quackarooni commented

2023-04-17 18:11:30 +02:00

Documentation:

UI Changes from this patch

# Documentation: ## UI Changes from this patch

ui_nw.align_nodes.context_menu_changes.png

131 KiB

ui_nw.align_nodes.n_panel_changes.png

90 KiB

ui_nw.align_nodes.preference_menu_changes.png

115 KiB

ui_nw.align_nodes.context_menu_changes.png

ui_nw.align_nodes.preference_menu_changes.png

Damien Picard commented

2023-04-17 18:20:34 +02:00

Not really a review, but:

Align X and Y should behave like the UVs I think: Align X should give the same X location to each element. It seems to me your patch does the opposite: Align X puts all elements on the same horizontal axis. It’s arbitrary but consistency is important.
I believe it would be better to use a single column for the preferences.

Not really a review, but: - Align X and Y should behave like the UVs I think: Align X should give the same X location to each element. It seems to me your patch does the opposite: Align X puts all elements on the same horizontal axis. It’s arbitrary but consistency is important. - I believe it would be better to use a single column for the preferences.

quackarooni commented

2023-04-17 18:31:53 +02:00

Testing:

The blend file below contains all the test scenes shown above, along with a script that spawns a panel which allows for easy cross-comparisons between the old and new versions of the operator.

# Testing: The blend file below contains all the test scenes shown above, along with a script that spawns a panel which allows for easy cross-comparisons between the old and new versions of the operator. ---

NWAlignNodes Test File.blend

212 KiB

quackarooni commented

2023-04-17 18:53:31 +02:00

@pioverfour thanks, I appreciate the feedback!

Align X and Y should behave like the UVs I think: Align X should give the same X location to each element. It seems to me your patch does the opposite: Align X puts all elements on the same horizontal axis. It’s arbitrary but consistency is important.

I actually agree, personally that's what I also expect with the name 'Align Nodes' tbh.
The behavior is from the original code, my patch just takes care of the edge cases where it breaks.

Personally I think, "Distribute Nodes" is more in line to what the operator is actually doing, if the name is to be taken from 'Align and Distribute' operations you often see in softwares like Photoshop or Word.

I believe it would be better to use a single column for the preferences.

Duly noted, was going back and forth myself on whether to go with a single or double column layout in the preferences. I guess double-column has the downside of obscuring some of the text if the prefs menu was on a small window.

@pioverfour thanks, I appreciate the feedback! > - Align X and Y should behave like the UVs I think: Align X should give the same X location to each element. It seems to me your patch does the opposite: Align X puts all elements on the same horizontal axis. It’s arbitrary but consistency is important. I actually agree, personally that's what I also expect with the name 'Align Nodes' tbh. The behavior is from the original code, my patch just takes care of the edge cases where it breaks. Personally I think, "Distribute Nodes" is more in line to what the operator is actually doing, if the name is to be taken from 'Align and Distribute' operations you often see in softwares like Photoshop or Word. > - I believe it would be better to use a single column for the preferences. Duly noted, was going back and forth myself on whether to go with a single or double column layout in the preferences. I guess double-column has the downside of obscuring some of the text if the prefs menu was on a small window.

quackarooni commented

2023-04-17 19:08:26 +02:00

Adjusting the preferences is something that I should be able to quickly address in this patch.

But renaming the operator from "Align Nodes" to "Distribute Nodes" would be something that'll entail changing a good amount of internal names, variables, labels, tooltips, etc., I wonder if that's out of scope for a PR like this and should be left to a later PR that'll focus mostly on that?

Adjusting the preferences is something that I should be able to quickly address in this patch. But renaming the operator from "Align Nodes" to "Distribute Nodes" would be something that'll entail changing a good amount of internal names, variables, labels, tooltips, etc., I wonder if that's out of scope for a PR like this and should be left to a later PR that'll focus mostly on that?

quackarooni added 2 commits 2023-04-17 19:40:39 +02:00

Node Wrangler: Changed Preferences layout from double to single-column 06271f6d4e

Changed the positioning of the Margin property to be single-column.
This avoids concern with certain labels being obscure when the
preference window is not fullscreen or is rather small.

Merge branch 'nw_rework_align_nodes' of projects.blender.org:quackarooni/blender-addons into nw_rework_align_nodes a16c51c882

Damien Picard commented

2023-04-17 20:08:41 +02:00

I guess double-column has the downside of obscuring some of the text if the prefs menu was on a small window.

Yes, and also it feels like the options for the margin and merge node are related if they are next to each other. And you kind of lose the benefit of quickly scanning vertically. (But then again I know little about UI design so take this with a grain of salt!)

But renaming the operator from "Align Nodes" to "Distribute Nodes" would be something that'll entail changing a good amount of internal names, variables, labels, tooltips, etc., I wonder if that's out of scope for a PR like this and should be left to a later PR that'll focus mostly on that?

Oh, I thought it was new code since I couldn’t find these operators in the current UI. But if it’s from the original code, then it’s definitely out of scope :)

> I guess double-column has the downside of obscuring some of the text if the prefs menu was on a small window. Yes, and also it feels like the options for the margin and merge node are related if they are next to each other. And you kind of lose the benefit of quickly scanning vertically. (But then again I know little about UI design so take this with a grain of salt!) > But renaming the operator from "Align Nodes" to "Distribute Nodes" would be something that'll entail changing a good amount of internal names, variables, labels, tooltips, etc., I wonder if that's out of scope for a PR like this and should be left to a later PR that'll focus mostly on that? Oh, I thought it was new code since I couldn’t find these operators in the current UI. But if it’s from the original code, then it’s definitely out of scope :)

👍 1

quackarooni added 1 commit 2023-04-21 12:40:30 +02:00

Merge branch 'main' into nw_rework_align_nodes 6e5c7b7355

quackarooni added 1 commit 2023-04-25 09:20:37 +02:00

Merge branch 'main' into nw_rework_align_nodes cf69c82250

quackarooni added 1 commit 2023-05-03 13:47:36 +02:00

Merge branch 'main' into nw_rework_align_nodes fb47ea49a5

quackarooni added 1 commit 2023-05-08 09:22:26 +02:00

Merge branch 'main' into nw_rework_align_nodes 1767393035

quackarooni added 1 commit 2023-05-16 05:27:57 +02:00

Merge branch 'main' into nw_rework_align_nodes d870440173

Jannai commented

2023-07-11 20:50:26 +02:00

First-time contributor

I've been working on a patch addressing the same issues and saw this PR only a few days ago. Now while re-evaluating if my patch is worth developing further I have two small notes for your PR:

To address the ambiguity with Align X/Y, the buttons and menu entries could be renamed to "Horizontal" and "Vertical" respectively. In addition to that the buttons/entries could be assigned the NODE_TOP and the NODE_SIDE icons to further indicate the button's purpose. Maybe the names of x/y margin in User-Prefs could also be renamed to horizontal/vertical margin just for consistency.
A small Note could be added to the Tooltips string stating that two or more nodes need to be selected for the NWAlign operator to become available. This might seem a bit unnecessary at first but I was a bit confused why the buttons were initially grayed out.

I've been working on a patch addressing the same issues and saw this PR only a few days ago. Now while re-evaluating if my patch is worth developing further I have two small notes for your PR: - To address the ambiguity with Align X/Y, the buttons and menu entries could be renamed to "Horizontal" and "Vertical" respectively. In addition to that the buttons/entries could be assigned the NODE_TOP and the NODE_SIDE icons to further indicate the button's purpose. Maybe the names of x/y margin in User-Prefs could also be renamed to horizontal/vertical margin just for consistency. - A small Note could be added to the Tooltips string stating that two or more nodes need to be selected for the NWAlign operator to become available. This might seem a bit unnecessary at first but I was a bit confused why the buttons were initially grayed out.

Damien Picard added 1 commit 2023-12-20 19:06:48 +01:00

Merge branch 'main' into nw_rework_align_nodes 7b462566fe

This pull request has changes conflicting with the target branch.