MaterialX icon indicating copy to clipboard operation
MaterialX copied to clipboard

Graph Editor: Reproducible crash after copying and connecting node

Open jsjtxietian opened this issue 1 year ago • 1 comments

Version: dfffe83f74e742edc3743f004da359c35cef5652 on windows

When I was debugging another issue, I found a crash accidently:

Repoduce step:

  • Launch the Graph Editor with its default marble material.
  • Click on the NG_marble1 node and copy it.
  • Break the connect of he out of the origin NG_marble1 node to SR_marble1's subsurface_color
  • Connect the out of the origin NG_marble1 node to the new created one's base_color_1
  • Connect the new created one's base_color_1 to SR_marble1's subsurface_color

test2

jsjtxietian avatar Jul 11 '24 04:07 jsjtxietian

Hello, I would like to work on this ticket for ASWF Dev Days 2024.

fnRaihanKibria avatar Sep 22 '24 17:09 fnRaihanKibria

Thanks to @fnRaihanKibria for adding new checks in #2029, which reduce the severity of this error when it occurs, and I'm copying the notes from @niklasharrysson that describe next steps for fully resolving the error:

The cause of this issue is a limitation in the support for cascading nodegraphs.

When there are multiple nodegraphs connected to each other these may be flattened into a single large ShaderGraph for codegen. But there are no handling of the case where nodes internal to these graphs may share the same name. So when connections are constructed and nodes are referenced by name this breaks down..

A typical case is copy/paste of a graph as in https://github.com/AcademySoftwareFoundation/MaterialX/issues/1932, where all nodes in the copy share names with the original.

So the symptom is that a cycle is found in the graph, but it's actually caused by nodes not being uniquely named.

I think the best fix for this is to stop flattening nodegraphs, and preserving the nodegraph hierarchies, when creating the codegen ShaderGraph. There are other benefits of this as well, for example an issue we've discussed before with limiting the number of uniforms that are generated for a shader (if nodegraphs are not flattened it's much easier to keep internal parameters private).

I will take a closer look at getting this work started as soon as possible.

jstone-lucasfilm avatar Dec 06 '24 23:12 jstone-lucasfilm