Conversation
The `implname` attribute lets an <implementation> declare that a port defined in the <nodedef> is called something else in the implementation's code, which the specification describes as a way to avoid clashes with reserved words in the target shading language. The standard libraries already used it - every <image> implementation declares that the nodedef's `default` input is called `default_value` - but shader generation ignored it entirely. Those ports were instead named by appending a counter, so the `mix` input of <mix> became `mix1` and the `normal` input of a BSDF became `normal1`: names that carry no meaning, and that shift if the reserved word list changes. Shader generation ----------------- Port name remappings are now read from the implementation element and applied when naming variables in generated code. * `ShaderNodeImpl` stores the remappings declared by its implementation element and exposes them through a new `getPortName()`. * `ShaderNode::getPortName()` forwards to its implementation, and `ShaderGraph` overrides it with its own set, for generators that build a graph directly from a nodedef rather than from a node. `ShaderNode::getPortVariableName()` is the single source of truth for the variable naming formula and is used at every site that names a port. * `ShaderGenerator::getImplementation()` populates the remappings via the new `addPortImplementationNames()` helper, once per implementation element before it is cached. It reads both the resolved element and the unresolved declaration: an <implementation> that references a nodegraph is resolved to that nodegraph before shader generation sees it, so an `implname` on the <implementation> itself was being discarded. Reading both, with the declaration taking precedence, allows a nodegraph shared by every target to be given target-specific port names. * `CompoundNode` forwards its remappings to `ShaderGraph::create()`, which applies them before finalizing the graph, since finalizing is what assigns the variable names that become a function's parameter names. * A shader generated for a single node is the implementation of that node's nodedef, so its interface is published under that implementation's names as well. * Nodes inserted by `inlineNodeBeforeOutput()` are created after `finalize()` has run and named their own port variables, bypassing the remapping. They now use the same formula. Variable names remain unique, as they still pass through `Syntax::getVariableName()`. Data libraries -------------- Audited every genosl <implementation>, checking each input name of its <nodedef> against the reserved words registered by OslSyntax - the OSL keywords and types, the OSL standard library function names, and the type name that registering a type syntax reserves. Only the 9 `default` inputs of the <image> and hextiled nodes were declared. 163 more are added here, each declaring an `implname` of the input name with a `_value` suffix, following the convention already established by `default` -> `default_value`. 78 of them are on implementations backed by OSL source or by an inline expression. The remaining 85 belong to nodedefs whose OSL implementation is a <nodegraph> - standard_surface, open_pbr_surface, gltf_pbr, UsdPreviewSurface, disney_principled, the lama nodes and others. That nodegraph's interface is shared by every target, so the declaration cannot go on the nodegraph itself; each is declared instead on a new target-specific <implementation> that references the existing nodegraph, leaving the nodegraph untouched and every other target unaffected. For the bxdf library these live in a new `bxdf/genosl` folder, alongside the equivalent folders of the other data libraries. The clashing inputs that remain undeclared all belong to nodedefs with no genosl implementation at all, mostly the light and volume material nodes. Generated output ---------------- The parameters of a shader generated from a single nodedef are renamed: <overlay> now takes `mix_value` rather than `mix1`, <image> takes `default_value` and `layer_value`, and <standard_surface> takes `subsurface_value`, `sheen_value`, `emission_value` and `normal_value`. Calls between nodes are emitted positionally, so nothing else in the generated source changes. This renames the parameters of the reference shaders and of the oso libraries built from them. Anything binding to one of those osos by the previous names has to be updated. OSL network target ------------------ The OSL network generator emits `param` and `connect` statements that bind by name against the compiled oso for each node, making it the one place where a port name crosses a real interface boundary. Both sides of a binding are now resolved through `getPortName()`. `LibsToOso` generates the <implementation> elements for the genoslnetwork target, and these carried none of the port remappings declared by the genosl implementation they are derived from. They now carry them, so that a consumer of an oso can bind against the names its parameters actually have. Without this the network generator emitted a connection to `surfaceshader1` on a shader whose parameter is `surfaceshader_value`, which testrender rejects. Tests ----- `GenShader: Implementation Names` checks that a remapped input is named by its `implname` while an input without one keeps the nodedef name, for every source code target. `GenShader: OSL Implicit Surfacematerial Port Names` covers the node that the OSL generator inserts after finalization. The implementation coverage check now tests an <implementation> that references a nodegraph through that nodegraph, since it is the nodegraph that gets recorded as used, and logs each one together with the nodegraph so that the indirection is visible in the report rather than silently trusted. Test suite coverage is added for nodes that no material exercised, and that were therefore never shader generated by the generator tests: <overlay> in each of its three variants, <ramp> (the multi interval ramp, as distinct from ramplr / ramptb / ramp4), <randomfloat> for a float and an integer input, <bump>, <refract>, <latlongimage>, the glTF PBR image nodes not reached by the glTF PBR examples, and the three shader translation graphs. All are implemented by nodegraphs, which is why the gap went unnoticed: the coverage check only counts implementation elements, and a nodedef implemented by a nodegraph has none. Like the other unit test files for individual nodes these declare nodegraph outputs rather than materials, so they are exercised by the shader generation tests of every target.
ShaderGraph::create() accepts outputs and shader nodes, both of which describe a node in the context of a document that binds its inputs. Generating a shader for a nodedef on its own has no such context: there is no node instance, and the shader's interface should be the nodedef's own interface. Add a nodedef branch to ShaderGraph::create() that builds a graph wrapping a single instance of the node the nodedef declares, publishing the nodedef's inputs and outputs as the graph interface and connecting them to that node. This is what lets a library shader be generated for a nodedef in isolation, which the OSL network generator uses to synthesize an oso per nodedef. The graph interface is published under the names declared by the implementation's 'implname' attributes, so the parameters of the generated shader match the names the implementation uses and the result is interchangeable with a hand written shader. Those names are read from the node rather than from the <implementation> element, since a nodedef implemented by a nodegraph has no <implementation> to read and its remappings have already been resolved onto the node. Graph output sockets are matched to the node's outputs by name. Both are derived from the nodedef, but by separate traversals of it - getActiveOutputs() for the sockets and getActiveValueElements() for the node's ports - so relying on the two agreeing in order is an assumption that is not stated anywhere. Every nodedef in the data library does satisfy it today, so this is hardening rather than a bug fix, but matching by name also replaces an unchecked index into the socket vector with an explicit error. applyInputTransforms() is deliberately not called. There is no node instance declaring colorspaces or units to convert from, and a shader generated for a nodedef must not bake transforms into the library implementation it represents. Tests ----- `GenShader: NodeDef Shader Generation` runs against every generator that is built, MDL and Slang included, and covers: - the published interface and its wiring, including the bind input state, for nodedefs with enum, filename and uniform inputs, for a multi-output nodedef, and for a nodedef implemented by a nodegraph, which is the case the implname handling is written for. Each input socket is checked against the node input it connects to on both type and uniform state, since addInputSockets() and ShaderNode::create() apply enum remapping independently and a divergence would produce a type mismatched connection rather than an error. Each output socket is checked to reach the node output of the same name. - the published interface reaching the emitted source - nodedefs whose only node is elided during finalize(), which is the case for constant and filename dot nodedefs. The graph ends up with no nodes at all and the shader body comes from the value the elision pushed downstream, so the emitted source is checked to still carry that value. Naming that value through the input socket's variable rather than by a literal also covers OSL renaming the `in` input of <dot> to `in_`. - a nodedef with no implementation for the target reporting an error `GenShader: NodeDef Metadata` covers metadata declared on the nodedef and on its inputs reaching the graph and its sockets. This needs a populated metadata registry on the context, without which the graph's metadata is simply null and any assertion on it passes for the wrong reason. `GenShader: OSL NodeDef Port Names` covers implnames on both inputs and outputs reaching the graph interface. It lives with the OSL tests because only the OSL implementations in the data library declare implnames, and no implementation declares one on an output, so the test adds one to a real implementation rather than leaving the output half of the handling uncovered. Two of the checks hold only for the generators that emit a named output variable, and are written for what every target actually guarantees instead: hardware generators publish input sockets of their own, such as the uv scale and offset of a texture node, so the nodedef's inputs are a subset of a graph's; and MDL emits a material assigning finalOutput__ rather than a shader with a named output parameter.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(This work is extracted and refined from #2739)
Stacked on #3044.
This branch contains that PR's commit as its parent, so the diff shown here until #3044 lands includes both. Please review only the second commit, Support shader generation from a nodedef — the implname work is already under review separately. I'll rebase onto main once #3044 merges. The dependency is real rather than just a merge convenience: publishing a nodedef's interface under its implementation's port names relies on the
getPortName()machinery #3044 introduces.Summary
ShaderGraph::create()accepts<output>and shader node elements. Both describe a node in the context of a document that binds its inputs. Generating a shader for a<nodedef>on its own has no such context: there is no node instance, and the shader's interface should be the nodedef's own interface.This adds a nodedef branch to
ShaderGraph::create()that builds a graph wrapping a single instance of the node the nodedef declares, publishing the nodedef's inputs and outputs as the graph interface and connecting them to that node. That is what lets a library shader be generated for a nodedef in isolation, which the OSL network generator uses to synthesize one .oso per nodedef.Purely additive to codegen: it's a new else if branch on an element type that previously threw, so no existing generation path changes behaviour.
Design notes
Interface naming. The graph interface is published under the names declared by the implementation's implname attributes, so the parameters of the generated shader match the names the implementation uses and the result is interchangeable with a hand-written shader. Those names are read from the node rather than from the element, since a nodedef implemented by a nodegraph has no
<implementation>to read and its remappings have already been resolved onto the node.Output socket matching. Output sockets are matched to the node's outputs by name. Both are derived from the nodedef, but by separate traversals of it —
getActiveOutputs()for the sockets andgetActiveValueElements()for the node's ports — so relying on the two agreeing in order is an assumption stated nowhere. Every nodedef in the data library satisfies it today, so this is hardening rather than a bug fix, but matching by name also replaces an unchecked index into the socket vector with an explicit error.