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

Graph Editor: Reproducible crash after renaming node #1912

Closed
jstone-lucasfilm opened this issue Jun 30, 2024 · 2 comments · Fixed by #1930
Closed

Graph Editor: Reproducible crash after renaming node #1912

jstone-lucasfilm opened this issue Jun 30, 2024 · 2 comments · Fixed by #1930

Comments

@jstone-lucasfilm
Copy link
Member

Repro steps:

  • Launch the Graph Editor with its default marble material.
  • Click on the NG_marble1 node and rename it to NewMarble.
  • Double click on the NewMarble node to view its graph.
  • Navigate back to the original view by clicking on the left angle bracket.
  • Connect the output of NewMarble to the base_color input of SR_marble1.
@jsjtxietian
Copy link
Contributor

jsjtxietian commented Jul 9, 2024

Hi I'd like to tackle this one!

Since the crash is vertex out of bounds, a quick fix would be like this, if any node is invalid, just early out in deleteLinkInfo:

diff --git a/source/MaterialXGraphEditor/Graph.cpp b/source/MaterialXGraphEditor/Graph.cpp
index f2382e28..04ec66d8 100644
--- a/source/MaterialXGraphEditor/Graph.cpp
+++ b/source/MaterialXGraphEditor/Graph.cpp
@@ -2753,6 +2753,10 @@ void Graph::deleteLinkInfo(int startAttr, int endAttr)
     int upNode = getNodeId(startAttr);
     int downNode = getNodeId(endAttr);
 
+    if (upNode == -1 || downNode == -1) {
+        return ;
+    }
+
     // Change input to default value
     if (_graphNodes[downNode]->getNode())
     {

But the root cause seems to be that when navigate back to the original view after renaming and viewing, some connections are lost.

Update:
Looks like in PortElement's _attributeMap, it still hold the old name, so getConnectedOutput failed. I have an idea to fix it, will update it tmr.

@jstone-lucasfilm
Copy link
Member Author

Thanks for taking a look, @jsjtxietian, and a fix for that root cause would be much appreciated!

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

Successfully merging a pull request may close this issue.

2 participants