Conversation
fallenmi
left a comment
fallenmi
left a comment
There was a problem hiding this comment.
I reviewed exact head 0a93a590dcca71cf89e9d56ced3bc53a18f081ad and the current GitHub merge aad34959fa1452b327280ff21bfdd9c233962e67. I found no blocking issue.
The flattening pass preserves the graph's ownership and connection invariants while expanding nested compound implementations: copied nodes keep their implementation, classification, metadata, port values and authored flags; external values, pass-through sockets, value-only outputs and output fan-out are all spliced before the compound nodes are removed. The new duplicate-ID rejection and batched removal also keep the owning map and raw-pointer order synchronized.
I built the exact head and current merge with AppleClang 21 and ran the four owning cases ten times on each tree (40/40 per tree). The full local GenShader slice passed 11 cases / 3,509 assertions on the head and 13 cases / 3,525 assertions on the current merge. A separate public-API oracle registered CompoundFlatteningRefactor on the generator before graph construction, expanded a two-level nested compound graph to zero remaining compound nodes, and preserved distinct node IDs and both interior values in 10/10 merge runs. git diff --check is clean. Live CI is fully green across all 35 check runs plus EasyCLA, including the dynamic-analysis and cross-platform build jobs.
Disclosure: I used OpenAI Codex to assist with exact-diff inspection, builds, tests, the independent registered-refactor oracle, live policy and interaction checks, and drafting this review. I independently verified the cited revisions, outputs, and conclusion.
Compound nodes wrap a nodegraph and are emitted as a function call by backends that support them. Backends that cannot do so need the nodegraph inlined into the parent graph instead. CompoundFlatteningRefactor expands each compound node in place, replacing it with copies of the nodes inside its graph rewired onto the compound node's external connections. Nested compound graphs are expanded in the same run, using a worklist seeded from the graph and extended as copies that are themselves compound are created. Compound nodes are identified through the existing ShaderNodeImpl::getGraph() virtual, so the pass has no dependency on any concrete implementation type. The pass is deliberately not registered by default, since inlining a nodegraph discards the emission behavior of its compound implementation. Registering it puts the expansion in ShaderGraph::finalize() ahead of removeUnusedNodes(), topologicalSort() and setVariableNames(). Copies are keyed by unique id rather than by name, since "<parent>_<child>" concatenates ambiguously and two distinct copies can produce the same key. Names are separately uniquified against the names already in use, since backends key on them. Values crossing the compound boundary are propagated in both directions, and in each case the authored state of the source port is preserved, since backends use it to decide which parameters to emit. A port carrying no value leaves the target's own default alone, and unit, color space and element path override the target only where the source has something to say. Inbound, a compound node input with no upstream connection applies its value to the interior inputs it feeds. Outbound, an output socket can be left carrying a value rather than a connection: eliding a constant pushes the constant's value onto whatever it fed and breaks the connection, and NodeElisionRefactor runs by default while the compound graph is finalized. A socket like that has no interior node to splice onto the external consumers, so its value is forwarded to them before they are disconnected. Both cases mirror how ShaderGraph::bypass() pushes a value downstream when it has no upstream connection to re-route. Supporting changes: - ShaderGraph::createNode() gains an overload taking a resolved node implementation, and both overloads now route through addNode(), which rejects duplicate unique ids. Previously a duplicate silently replaced the owning entry in the node map while the node order kept a raw pointer to the released node. - ShaderGraph::removeNode() and removeNodes() erase nodes and break their connections, the latter compacting the node order in a single pass. - ShaderPort::copyToPort() copies a port's value, flags and descriptive data, applying the flags last so an unauthored value stays unauthored. - ShaderNode::getImplementationPtr() exposes the implementation so copies can share it.
0a93a59 to
292d858
Compare
|
I'm tagging @bernardkwok for his input as well. How does this new workflow work with existing MaterialX/source/MaterialXCore/Node.cpp Line 251 in d23766b Wouldn't the flattenSubgraphs (byfilter) -> codegen be better that doing this in Shadergraph? |
|
Also in the context where we're surfacing the MaterialX shader generation system via a visitor pattern at some point in the future - we don't want to be dependent on logic that only operates on the document model. |
|
@ld-kerley, @ashwinbhat : I think I mentioned this in some earlier TSC meeting. I believe you can create it already "pre-flattened" by not introducing compounds except the top level container. To a certain extent this already occurs for The ability to flatten a specific compound still feels like a useful mechanism to have and also agree pre-document manipulation is possible but can't really comment as to what may be "better" - but I think we'd want a check to make The flip-side that I believe I mentioned is enforcing hierarchys of compounds for nested nodegraphs (if that ever get's in :)). |
|
BTW, It would be useful to have this for the PR I put up for shadergraph API access. @ld-kerley : having Python / JS wrapper access would allow for flattening to occur before calling to get shadergraph connectivity info. @ashwinbhat, |
4 participants
(This work is extracted and refined from #2739)
Compound nodes wrap a nodegraph and are emitted as a function call by backends that support them. Backends that cannot do so need the nodegraph inlined into the parent graph instead.
CompoundFlatteningRefactor expands each compound node in place, replacing it with copies of the nodes inside its graph rewired onto the compound node's external connections. Nested compound graphs are expanded in the same run, using a worklist seeded from the graph and extended as copies that are themselves compound are created. Compound nodes are identified through the existing ShaderNodeImpl::getGraph() virtual, so the pass has no dependency on any concrete implementation type.
The pass is deliberately not registered by default, since inlining a nodegraph discards the emission behavior of its compound implementation. Registering it puts the expansion in ShaderGraph::finalize() ahead of removeUnusedNodes(), topologicalSort() and setVariableNames().
Values crossing the compound boundary are propagated in both directions, and in each case the authored state of the source port is preserved, since backends use it to decide which parameters to emit. A port carrying no value leaves the target's own default alone, and unit, color space and element path override the target only where the source has something to say.
Inbound, a compound node input with no upstream connection applies its value to the interior inputs it feeds. Outbound, an output socket can be left carrying a value rather than a connection: eliding a constant pushes the constant's value onto whatever it fed and breaks the connection, and NodeElisionRefactor runs by default while the compound graph is finalized. A socket like that has no interior node to splice onto the external consumers, so its value is forwarded to them before they are disconnected. Both cases mirror how ShaderGraph::bypass() pushes a value downstream when it has no upstream connection to re-route.