GraphEditor: Add pin/lock preview to node feature

[MatthieuBEAUD]

Issue

#3039

Description

Added a feature to be able to lock with node is used for the preview render:

Added:

• Key binding: R (for Render, and to match similar keybinding in Houdini Solaris), but it could be P for Preview or L for Lock
• Help text in menu: under Help/Graph/Viewing
• Color on the UI node header to highlight the currently used render preview node (when locked only)
• function to update the renderNode (check if locked, and if new value different than the current value) and recompile the material (factorized since it was used in 4 places)

Notes:

• I used the "lock" word instead of "pin" in the variable names and comment to avoid confusion with the node pins (inputs).
• I did not add any button, as I thought it would be out of scope. I also figured that it's something that should be added on top of Graph Editor : UX Updates Towards "Keyboardless" Workflow #3043, like adding a button next to, or in, the hamburger menu. Should another issue be added for this ?
• Currently, when diving in a subgraph, the lock/pin remains on the upper level, maybe some UX could indicate that a pin/lock is still active, or maybe we should disable the pin when diving in a subgraph ? Let me know if that should be something to fix/dig into.

Demo

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: MatthieuBEAUD / name: Matthieu Beaud (6a5a89a)

[MatthieuBEAUD]

The committers listed above are authorized under a signed CLA.

• ✅ login: MatthieuBEAUD / name: Matthieu Beaud (3a12842, d3d3c1c)

[kwokcb]

This looks like a nice clean way to add in this functionality.

I have a few workflow cases that would be good to check / resolve:

1. What happens if you lock a node and then delete that node. Does the lock node state get cleared ?
2. What happens if the node you lock cannot be rendered? It is possible to occur.
3. I think this will work for nodes inside a functional graph ( node graph used to represent a definition or nodedef).
   I'm not sure if what the behaviour should/would be there and if it's desired. It seems like a nice feature but would be
   good to clarify what is expected here.

For both compound (non definition graphs) and funcation graphs for when you dive into the graph, lock a graph node and then go out of it. I can suggest 2 options:
a. Instead of relying on the node being visible. Add the node name / path to the render panel instead.
b. Add the visual cue to the parent graph.

Both may be useful even when you scroll the graph so that locked node is no longer visible. a. can be useful longer term as well to allow filtered selection of node / node outputs to render.

Think b. is the simplest to add.

Thanks.

[jstone-lucasfilm]

Thanks for this proposal, @MatthieuBEAUD! See the instructions at #3079 (comment) on how to resolve the CLA authorization warnings, so that we can begin reviewing this PR.

[MatthieuBEAUD]

This looks like a nice clean way to add in this functionality.

I have a few workflow cases that would be good to check / resolve:

1. What happens if you lock a node and then delete that node. Does the lock node state get cleared ?
2. What happens if the node you lock cannot be rendered? It is possible to occur.
3. I think this will work for nodes inside a functional graph ( node graph used to represent a definition or nodedef).
   I'm not sure if what the behaviour should/would be there and if it's desired. It seems like a nice feature but would be
   good to clarify what is expected here.

For both compound (non definition graphs) and funcation graphs for when you dive into the graph, lock a graph node and then go out of it. I can suggest 2 options: a. Instead of relying on the node being visible. Add the node name / path to the render panel instead. b. Add the visual cue to the parent graph.

Both may be useful even when you scroll the graph so that locked node is no longer visible. a. can be useful longer term as well to allow filtered selection of node / node outputs to render.

Think b. is the simplest to add.

Thanks.

Thank you very much for this feedback !

1. I indeed had forgotten about what happens when you delete the locked preview render node. It now disables the lock.
2. When you enable the lock mode, it stores the currRenderNode, so you cannot lock a non-renderable node (this feature never uses the current node selection). And in that case, if we add a button next to a hamburger menu, this button would only need to be present on renderable nodes. But maybe you meant a renderable node that can't be rendered again ?
3. Good points, I'll think about those, and what the visual cue could be in specific cases:

• if we select a node in the root graph, and then dive in a child, we could have a cue in the top graph path, to show the preview is currently set in the parent
• if the locked preview node is outside of the frame, we could have an arrow cue of that color pointing to the direction on the edge of the frame
• if we lock a node in a subgraph and then go out of it, the cue could indeed simply transfer to the parent node

All that being said, maybe adding the name / path of the current preview node to the viewport would be the more robust way afterall.

I see when I can find time to work on point 3.

Thanks again 🙏

[MatthieuBEAUD]

I added the current render node path display above the viewport

Here are some examples:

Let me know what you think of this

[kwokcb]

Maybe let's keep it simple.

1. Show the locked node name in the render panel as you have done.
2. Hilight the child or parent nodegraph if locking a child node.

If someone scrolls the node out of view then 1) handles that case.

Disallow functional node graph child node locking for now for this PR. There should be a global state that tells you this -- the graph is marked as read-only.

Not all nodes chosen can have its upstream graph or itself rendered. For this PR let's just leave it as rendering nothing, and handle either prevention or fallback as a separate item.

So I think the only addition is to check for read-only graph state.

[MatthieuBEAUD]

Thanks for the feedback

I'll try and find a few hours to highlight the parent/child node of the current lock selection, and check of readonly state

Thank you !

[MatthieuBEAUD]

I added the check for read-only graph to not allow for the lock of the render preview node in that case.

And in a separate commit the highlight of the parent if the locked currRenderNode is wihthin its children (I also had to check for the nodegraphs in that case). But it could get a bit confusing as you don't know if that node is currently being viewed, or one of its children, that might not be the same as the preview of the node itself.
Also, I'm not sure what should be highlighted if you're in the children and the locked is somewhere in the parents (since there is no node that references the parent, or maybe use the graph hierarchy on top of the graph ?), I suppose the path over the viewport is the reference in that case, right ?

Let me know if the highlight of the parent should be kept, or if it's redundant with the path over the viewport and we should only keep that text cue for out of scope/sight locked preview node.

Here is a quick demo of the highlight of the parent of the locked render node:

[kwokcb]

Suggestion

Parent / child indicators exist and we can use title bar color for now.
So the only small suggestion is that if it's at the parent level to dim or mute the "lock" colour so there is some differentiator. (Actually if you pick the output node on a graph it is the graph that is the target but that's "splitting hairs").

So something like this: (apologies in advance but this is AI generated slop, but shows the basic rule set :))

Thought blurb:

1. I tried to look for existing shader graph workflows and the closest I could find for the usage of parent/child indicators was in Houdini and Katana. Nuke also has some parent/child state, but couldn't find anything for Maya, Blender.
2. If a Houdini expand/collapse graph interaction was added vs this editor's complete context switch with single graph display then I think that would be a match.

From what I've seen having both a title color cue and and icon are both useful with a icon addition post #3043 PR.

Does this sound okay?

[MatthieuBEAUD]

Yes perfectly clear, I muted the color for when the child is selected then 👌 and we'll wait post #3043 to add the icon

I also changed the color to match your example palette, as the dimmed yellow-ish felt confusing and too brown, and didn't feel like a muted highlight color.

here are the results:

dimmed (the child node is the current lock preview):

main (the node itself is the current lock preview):

[kwokcb]

This looks pretty close.
I think the key item to address is file->new and file->load clearing lock state.
Rest is minor.

[kwokcb]

Thanks for the final updates. Looks good to me.
You just need to resolve and test your changes.

[MatthieuBEAUD]

Thanks for the final updates. Looks good to me. You just need to resolve and test your changes.

Thank you very much !
I had already tested before pushing, I'll do a rebase to avoid the merge conflicts, re test, and then merge

thank you for the constant reviews 🙌

[MatthieuBEAUD]

Rebased, and re tested the feature, and all seems good ! (lock outside, lock inside nodegraph, reload, and load graph)

I suppose I'll need another approval before merging
@jstone-lucasfilm ?

[jstone-lucasfilm]

Thanks for all of your work on this, @MatthieuBEAUD, and to @kwokcb for the detailed review! Routing every assignment of _currRenderNode through a single updateRenderNode call is a good way to introduce this feature, and the new path label above the viewport seems like a useful editor improvement on its own, independent of the lock.

I have one request on the keyboard shortcut, along with a few refinements that I think would be important to address before we merge:

1. I'd suggest moving this shortcut from R to P. In the MaterialX Viewer, R reloads the current material from file, and the Graph Editor already binds Ctrl+R to the same action, so a bare R that locks the preview would give our two applications contradictory meanings for the same key. P is currently unused in both applications and reads naturally as either "pin" or "preview", matching your new label above the viewport.

   While making that change, I'd also suggest treating the two directions of the toggle separately. When the current selection has no downstream renderable element, setRenderMaterial leaves _currRenderNode as null, and pressing the key in that state engages the lock with nothing to lock, so the user sees no label and no highlight while clicks no longer change the preview. The handler also requires !readOnly() && _currUiNode != nullptr for both directions, so after pressing U or diving into a subgraph the key does nothing until a node is selected, and a lock engaged at the top level can't be released from inside a functional graph. Engaging the lock should require a render node and a writable graph, while releasing it should always be allowed:

   // Hotkey to lock/unlock the current render node
   else if (ImGui::IsKeyReleased(ImGuiKey_P))
   {
       if (_lockRenderPreviewNode)
       {
           _lockRenderPreviewNode = false;
           if (_currUiNode)
           {
               setRenderMaterial(_currUiNode);
           }
       }
       else if (_currRenderNode && !readOnly())
       {
           _lockRenderPreviewNode = true;
       }
   }
   

2. The header highlight directly compares UiNodePtr objects, and I believe this will misbehave when a subgraph is re-entered. Each dive calls buildUiNodeGraph, which creates fresh UiNode objects, so after locking a node inside a nodegraph, going up, and diving back in, _currRenderNode == node no longer holds for the node that's locked. Since isChildOfNodeGraph starts its walk at the node itself rather than its parent, that node then falls through to the muted "parent" color instead of the full highlight. Comparing the underlying elements would make this more robust, e.g. _currRenderNode->getNode() == node->getNode() for the direct case, and starting the ancestor walk from getParent() so that the helper answers only the strict "contains" question. With that change, a name such as isDescendantOf might read more naturally at the call site than isChildOfNodeGraph.

3. Document-scope output elements are accepted as render nodes by setRenderMaterial, but the new label, the highlight, and the ancestor helper all require getNode() to be non-null, so locking one of these outputs gives no visual feedback at all. updateMaterials already handles the getOutput() case alongside getNode(), and a small helper returning the render node's element as an mx::ElementPtr would let the label and the highlight share that logic.

A few smaller notes:

• The in-app help text reads "Pin the preview render to the currently selected node", but the code locks whatever the render node currently is, which is often a material downstream of the selection. The wording in GraphEditor.md ("Lock/unlock the current render node") describes the behavior accurately, and it would be worth aligning the help text with it when both are updated for the new key.
• The two highlight colors are declared as local ImColor values inside createNodes. Since they'll likely be revisited once the icon work from Graph Editor : UX Updates Towards "Keyboardless" Workflow #3043 is merged to main, hoisting them to named constants alongside the other header colors would make that easier.
• With the outer _lockRenderPreviewNode check in place, the nested if (_currRenderNode == node) and else if (isParentOfCurrRenderNode) branches could be flattened into a single if / else if, since the outer condition already guarantees that one of them is true.

With those refinements, this PR should be in good shape to merge. Thanks again for taking this on, and for working through the UX questions with Bernard along the way!

[MatthieuBEAUD]

Thank you for all the detailled feedback. Indeed there is a getElement() that allows to simplify this a lot

Also moved all the nodeHeaderColor to named const in the header. And ensured the highlight and text support an output rendernode.

here is the sum up test:

mtlx.mp4

[MatthieuBEAUD]

Hello, is there anything else I should do for this PR ? (since the 2 week period after the dev day is today I was just wondering 😇 )
Thank you !

[jstone-lucasfilm]

Thanks for this latest revision, @MatthieuBEAUD!

Looking through the changes, I see one remaining issue that I think is important to address:

The lock release in deleteNode still compares UiNodePtr objects, so there's still a version of the issue that the highlight had in the previous revision. If a node is locked inside a nodegraph, and the user goes up and dives back in before deleting it, the new UiNode no longer matches _currRenderNode, and the lock stays engaged on an element that has been removed. The same result occurs without re-entering the graph when the user goes up and deletes the enclosing nodegraph, since the comparison only considers the node itself. In both cases the label continues to read [Locked] Preview: ... for a node that no longer exists, and clicks don't change the preview until P is pressed. You could use your new helper here to address both cases:

// Release the render node lock if the locked node or one of its ancestors is deleted.
if (_lockRenderPreviewNode && _currRenderNode &&
    (_currRenderNode->getElement() == node->getElement() || isDescendantOf(_currRenderNode, node)))
{
    _lockRenderPreviewNode = false;
}

I also have a question on the new label. In graphButtons, the bounds for cursorInRenderView are computed from tempWindowPos and screenSize before the label is drawn, and the label then shifts the render image down by a line of text. Can you check whether mouse interaction in the render view still lines up with the image, particularly along its bottom edge and over the label itself? If there's an offset, drawing the label just below the image rather than above it might be the simplest fix.

And one minor note: a render node is always a node or a document-scope output, so only a nodegraph can be its ancestor, and the muted branch in the node and output cases of createNodes can never be reached. Both of those blocks can become a single if (_lockRenderPreviewNode && isCurrRenderNode), leaving the muted color to the nodegraph case alone.

Otherwise, I think this is looking really close to the mark, and it should make a great Dev Days contribution!

[MatthieuBEAUD]

Thank you ! 🙏 I'll try address those before the weekend, and hopefully still make the cut for this to be considered a DevDays contribution 😇
Thanks !