Issues with flattenSubGraphs()

[MasterZap]

When flattening a multiple instances of a subgraph, duplicate names are updated to unique names and the linkage between the nodes are adjusted. However, if these graphs come from a namespace, this breaks, and the links between the nodes are never updated. The cause for this is the function getDownStreamPorts(), which returns nothing for a node inside a namespace.

Trying to flatten the "my_nodegraph" in the attached file flatten-problem2.mtlx, will give the wrong result. But with the attached flatten-noproblem2.mtlx, everything works correctly. Only difference - in one file, the nodes are in a namespace called "myspace".

flatten-problem.zip

The post-flattened result is wrong:

See slack discussion here for more details: https://academysoftwarefdn.slack.com/archives/C0230LWBE2X/p1646646149250959

[MasterZap]

@kwokcb writes in the thread:

"At first glance, this is a patch which is hiding another problem. When something get's converted from a definition to an graph instance the namespace qualifiers should be removed but it seems they are not. Basically the nodegraph nodes should be copied out, and new non-namespaced names should be generated. Then there would be no cached reference with a namespaced name and hence the correct downstream match can be found. (Aside: the downstream traversal is pretty "ugly" as it uses a string name match on the entire document's inputs at runtime vs an explicit link. Guess part of enhancements to have non-named based port connections... )"

Unfortunately, I'm not sure how to actually do that :)

[MasterZap]

@kwokcb Are you sure my suggested fix is as dangerous as you seemed to think in the slack thread? The cache is explicitly created using the qualified name, so I don't really see the harm in looking it up using the qualified name too? In that case, the question if the name should have a namespace or not becomes academic and a separate question? Or what am I missing :)

[MasterZap]

@kwokcb @jstone-lucasfilm I'm gonna make a PR with my original fix, because I don't think it's broken per-se. Scream if you disagree :)

[jstone-lucasfilm]

@MasterZap That sounds like a good step forward to me, and having a concrete PR to test would be valuable.

[ZapAndersson]

sorry for the PR spamming had to get an account that had a contributors agreement set up :)

[bernardkwok]

Sorry for the delay in getting back to you @ZapAndersson. I thought that the logic should have create a duplicate of the graph and hence you don't need this, but it seems this is used to parse the original nodedef file. I'll leave comments in the PR.