Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve "Add Node" workflows in the graph editor #1597

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

GregoryD2017
Copy link

Context Aware "Add Node" Workflow

Previously, the tab menu was the only way to add new nodes to the graph editor. Now users can select any pin in the graph, and drag a link off of that pin. If this link does not connect to another pin, on mouse release, the user will be prompted to add a new node. The list of nodes that users can add will be filtered to only include nodes that can be connected to the pin from which this link originates.

This context-aware node filtering in the "Add Node" popup can be toggled on/off with the "Smart Filter" check box.

Demo 🥳

context-aware-add
Notice how as you toggle the Smart Filter on and off, the list of allowable nodes changes as well. When Smart Filter is off, every defined node is allowed to be added.

One future improvement:

Right now, when dragging a link from a pin, nodes that have valid connections are shown in the "Add Node" popup. We could make this popup even more granular -- show both the node name and the specific pin of that node that would be connected by this link. Certain nodes have multiple input or output pins of the same type. By having separate entries for each of the specific pins as well, users can select which pin they want to be connected in the newly added node, and when that node is added, we can automatically draw that link for them.

Previously, the tab menu was the only way to add new nodes to the graph editor.
Now users can select any pin in the graph, and drag a link off of that pin. If
this link does not connect to another pin, on mouse release, the user will be
prompted to add a new node. The list of nodes that users can add will be filtered
to only include nodes that can be connected to the pin from which this link originates.

This context-aware node filtering in the "Add Node" popup can be toggled on/off
with the "Smart Filter" check box.
Copy link

linux-foundation-easycla bot commented Nov 7, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@jstone-lucasfilm
Copy link
Member

This looks like a great start, thanks @GregoryD2017! If you run into trouble with the EasyCLA process, feel free to write up a support ticket using the link above, and the Linux Foundation is usually very responsive in resolving these issues.

@jstone-lucasfilm
Copy link
Member

@GregoryD2017 The build errors on Linux/MacOS should be straightforward to fix, and it looks like they're caused by initializing member variables in a different order than they are declared in the class:

/home/runner/work/MaterialX/MaterialX/source/MaterialXGraphEditor/Graph.cpp:128:5: error: field '_smartFilter' will be initialized after field '_autoLayout' [-Werror,-Wreorder-ctor]

@GregoryD2017
Copy link
Author

@jstone-lucasfilm Ach so sorry about that -- I wasn't getting email notifications for github since I haven't signed into this email account since college 😅 . I'll get the license and build errors fixed ASAP. Apologies!

GregoryD2017 and others added 2 commits December 3, 2023 03:29
Previously, the tab menu was the only way to add new nodes to the graph editor.
Now users can select any pin in the graph, and drag a link off of that pin. If
this link does not connect to another pin, on mouse release, the user will be
prompted to add a new node. The list of nodes that users can add will be filtered
to only include nodes that can be connected to the pin from which this link originates.

This context-aware node filtering in the "Add Node" popup can be toggled on/off
with the "Smart Filter" check box.
@GregoryD2017
Copy link
Author

License + build errors fixed @jstone-lucasfilm 😃

@jstone-lucasfilm
Copy link
Member

This looks very promising, thanks @GregoryD2017, and I'm CC'ing @lfl-eholthouser for her thoughts on this proposal.

@jstone-lucasfilm
Copy link
Member

@GregoryD2017 In order to keep this proposal moving forward, make sure to resolve the merge conflicts mentioned above! This change still seems promising, but we'll need to keep it up-to-date with the main branch in order to provide meaningful review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants