Graph Editor : Crashes when selecting nodes in functional graphs

[kwokcb]

Issue

Graph Editor will crash if you open the implementation graph (functional) for a node and then select any node in that graph.

Steps to Reproduce

If you start up and open the marble shader implementation graph, and select the output node then it crashes. Tried with head as of time of writing this. This is on Windows and Mac.

Cause

Seems to be this code which now crashes: https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXGraphEditor/Graph.cpp#L804 I don't recall this crashing before so could be a regression, or did exist but is now very easy to repro.

[hybridherbst]

I've seen this sporadically as well but wasn't able to reliably reproduce. Do you happen to have repro steps?

[kwokcb]

I've seen this sporadically as well but wasn't able to reliably reproduce. Do you happen to have repro steps?

I've put in a fuller description.

[MeghaS94]

Hello! I tried to look at this issue, the crash occurs because graphDoc->getDescendant(...) returns a nullptr when called from nodes inside functional/implementation graphs, because _childMap in the Element class does not have entries for these graph names. Most nodes seem to avoid this by checking if the parent graph has a nodeDef and returning early https://github.com/AcademySoftwareFoundation/MaterialX/blob/dd699d89009e9f39399c063c5ba64347e23b02ef/source/MaterialXGraphEditor/Graph.cpp#L786 Output nodes don't have this check. Should output nodes also check for parent graph nodeDef and return early? This seems to fix the crash, but I wanted to check if this made sense before submitting a fix.

[weta-sfinnie]

Hi I'd like to pick this up for dev days.

[jstone-lucasfilm]

Welcome, @weta-sfinnie, and it's all yours!

[weta-sfinnie]

I agree with @MeghaS94 analysis on this problem. The getNodeDef() check seems to be called to catch when something is in a functional graph, but the check only applies to NodePtr which the output node is not. And so something inside a functional graph is getting past the check and triggering a nullptr error down the line. Adding the check in solves the problem.