Skip to content

Support token editing within GraphEditor UI#2943

Open
mialana wants to merge 24 commits into
AcademySoftwareFoundation:mainfrom
mialana:support-token-editing
Open

Support token editing within GraphEditor UI#2943
mialana wants to merge 24 commits into
AcademySoftwareFoundation:mainfrom
mialana:support-token-editing

Conversation

@mialana

@mialana mialana commented May 20, 2026

Copy link
Copy Markdown
Contributor
Screenshot 2026-05-20 130151

Preview of new detailed Tokens Table feature.

Hello! This is a PR for Dev Days 2026. It features:

  1. Token editing directly within the graph editor UI
  2. Display of other token data within Tokens table to improve user experience. Namely:
  • A Source Element column, which contains the graph element where the token is declared / was discovered during upwards traversal.
  • A Affected Inputs column, which lists which of the node's inputs reference the token.
  1. Tooltip features for both the overall tokens table and each of the individual columns (see preview GIF below).
materialx_token_tooltips_preview

Preview of new supplementary tooltip features.

Feel free to point out any inefficiencies or inconsistencies I've made in implementation. Thank you!

@mialana mialana changed the title GraphEditor: Support token editing through UI Support token editing within GraphEditor UI May 20, 2026

@kwokcb kwokcb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mialana,
This looks really good! There are some minor comments on variants to check.

I see you find all parent tokens for the active child node.
You could simplify logic by just getting the tokens for the current (selected) node/nodegraph. The user interaction and display would be simplified as well as a user knows exactly what scope the token change is occurring at. I leave it up to you -- either way is fine.

I didn't run this but have you checked that a token change can indeed cause the image's referenced to update, or is this a second step after the core + edit logic here is added ?

Thanks.

Comment thread source/MaterialXGraphEditor/UiNode.cpp
Comment thread source/MaterialXGraphEditor/UiNode.cpp Outdated
Signed-off-by: mialana <aliu@amyliu.dev>
@mialana

mialana commented May 20, 2026

Copy link
Copy Markdown
Contributor Author

Hi @mialana, This looks really good! There are some minor comments on variants to check.

I see you find all parent tokens for the active child node. You could simplify logic by just getting the tokens for the current (selected) node/nodegraph. The user interaction and display would be simplified as well as a user knows exactly what scope the token change is occurring at. I leave it up to you -- either way is fine.

I didn't run this but have you checked that a token change can indeed cause the image's referenced to update, or is this a second step after the core + edit logic here is added ?

Thanks.

Great! Yes, here is a demo where I edit a token in the table, and the material preview is updated thereafter.

materialx_token_edit_demo

For the design decision you mention, my initial thought process was that displaying parent tokens is useful as then the table exposes all tokens that are available for use in filename-type inputs of the current node -- i.e. all the tokens that are currently in scope.

Does that make sense to you as well? Thank you for the code review!

@kwokcb kwokcb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to go.

Just leaving a final test check that the editing values works for the cases in
the token_graph and token_graph_material. A snapshot or short gif/video would be good to show the result. There is a nodedef with token instance so that will check the latest logic you added in to cover that case. Thanks!

Comment thread source/MaterialXGraphEditor/UiNode.cpp Outdated
Comment thread source/MaterialXGraphEditor/UiNode.cpp
…class

Signed-off-by: mialana <aliu@amyliu.dev>
@mialana mialana force-pushed the support-token-editing branch from e8d5b3e to 3644bd6 Compare May 23, 2026 18:14
@mialana

mialana commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

MaterialX Token Editing - 'token_graph_material.mtlx' Functionality Demo

MaterialX Token Editing - 'token_graph.mtlx' Functionality Demo

@kwokcb Sorry for the delay in getting these videos posted. I will say that the videos demonstrate that the editing process is not yet completely bug-free. Namely:

  • In token_graph_material.mtlx the third test scenario with token_nodedef_graph will invoke an error message: "Could not find a matching implementation for node 'token_image' matching target 'genglsl'". From debugging, this occurs from calling updateMaterials() after editing. It seems that during this re-render the graph node traversal is different / incomplete, causing the implementation NG_token of ND_token to not be visited at all.
  • In token_graph.mtlx, only the Parent_Token_Graph test case has a material preview in the viewport. Furthermore, the mini 2D preview of the filename input in the sibling test does not show up (and never did).

Given these videos, I am curious whether you'd like to fix these bugs before this PR is approved? I believe that the problems demonstrated should have already existed before my changes, and if possible could be addressed separately.

I can also provide some more details on anything if needed! Thank you, hope my update make sense and let me know preferred next steps.

@kwokcb

kwokcb commented May 28, 2026

Copy link
Copy Markdown
Contributor

Hi @mialana, This looks really good! There are some minor comments on variants to check.
I see you find all parent tokens for the active child node. You could simplify logic by just getting the tokens for the current (selected) node/nodegraph. The user interaction and display would be simplified as well as a user knows exactly what scope the token change is occurring at. I leave it up to you -- either way is fine.
I didn't run this but have you checked that a token change can indeed cause the image's referenced to update, or is this a second step after the core + edit logic here is added ?
Thanks.

Great! Yes, here is a demo where I edit a token in the table, and the material preview is updated thereafter.

materialx_token_edit_demo


For the design decision you mention, my initial thought process was that displaying parent tokens is useful as then the table exposes all tokens that are available for use in filename-type inputs of the current node -- i.e. all the tokens that are currently in scope.

Does that make sense to you as well? Thank you for the code review!

  • Thanks for the update preview. Looks great.
  • For exposing all the tokens. I think the only concern I have is that parent tokens can affect more than the currently selected node. e.g. a parent token could affect 2 or more child images. Seems good to leave it as is and gather
    user feedback on this.

@kwokcb

kwokcb commented May 28, 2026

Copy link
Copy Markdown
Contributor

MaterialX Token Editing - 'token_graph_material.mtlx' Functionality Demo

MaterialX Token Editing - 'token_graph.mtlx' Functionality Demo

@kwokcb Sorry for the delay in getting these videos posted. I will say that the videos demonstrate that the editing process is not yet completely bug-free. Namely:

  • In token_graph_material.mtlx the third test scenario with token_nodedef_graph will invoke an error message: "Could not find a matching implementation for node 'token_image' matching target 'genglsl'". From debugging, this occurs from calling updateMaterials() after editing. It seems that during this re-render the graph node traversal is different / incomplete, causing the implementation NG_token of ND_token to not be visited at all.
  • In token_graph.mtlx, only the Parent_Token_Graph test case has a material preview in the viewport. Furthermore, the mini 2D preview of the filename input in the sibling test does not show up (and never did).

Given these videos, I am curious whether you'd like to fix these bugs before this PR is approved? I believe that the problems demonstrated should have already existed before my changes, and if possible could be addressed separately.

I can also provide some more details on anything if needed! Thank you, hope my update make sense and let me know preferred next steps.

  • Hmm. The first seems like a bug in the editor update logic not picking up the local nodedef properly but is picked up on initial file load. . Can you leave a new issue for this. Thanks.
  • token_graph.mtlx is unrelated and is relate to discovery of what is considered "renderable". Should log another issue for this, but you don't need to do this :).

@kwokcb kwokcb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this Amy. This looks good to go!

If you can log the update issue you found at your convenience that would be much
appreciated. Thx.

@jstone-lucasfilm jstone-lucasfilm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mialana and @kwokcb! I'm looking forward to reading through this code in more depth, and on my first read I had one thought on the introduction of std::atomic to the Graph Editor.

Comment thread source/MaterialXGraphEditor/UiNode.h Outdated
@jstone-lucasfilm

Copy link
Copy Markdown
Member

@mialana Thanks for the atomic fix! When you have a chance, would you mind resolving the merge conflicts between your PR and recent Graph Editor work from other Dev Days contributors?

@mialana

mialana commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

@mialana Thanks for the atomic fix! When you have a chance, would you mind resolving the merge conflicts between your PR and recent Graph Editor work from other Dev Days contributors?

Hi @jstone-lucasfilm sorry for such the slow turnaround times! I've just resolved the conflict. Please let me know if anything else is needed!

@jstone-lucasfilm jstone-lucasfilm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks nearly ready to merge, thanks @mialana, and I had just two additional suggestions for improvements.

Comment thread source/MaterialXGraphEditor/UiNode.h Outdated
Comment thread source/MaterialXGraphEditor/UiNode.cpp Outdated
@mialana

mialana commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Hi @jstone-lucasfilm your last two suggestions have both been applied. Let me know how it looks now. Thank you so much for taking the time to give me pointers on my code quality!

@jstone-lucasfilm jstone-lucasfilm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one more suggested fix, and then I think this is good to go!

}

// Traverse through inputs and determine which tokens their value depends on
for (const auto& input : getNode()->getActiveInputs())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, as I should have caught this in my earlier review, but I believe we need a null check for this call to getNode, as it's very possible that the current element is not a node.

Since you're storing the return value of getNode in the currElem variable above, you might consider storing this value as node and then returning early if its value is null, as there is effectively no further work to do in this case.

For example, this might look like the following:

void UiNode::buildUiTokenMap()
{
    _uiTokenMap.clear();

    mx::NodePtr node = getNode();
    if (!node)
        return;

    mx::ElementPtr currElem = node;
    while (currElem)
    {
        ...
    }

    for (const auto& input : node->getActiveInputs())
    {
        ...
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants