Arnold USD compatibility improvements #5759
Merged
Conversation
This file contains 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
|
LGTM, once you address the failures ( which it sounds like you've got plan for ). |
|
Pushed a fixup for the OSLShader test failures. |
|
LGTM |
OSL shaders have named outputs, and any ShaderNetworks generated in Gaffer will always provide the output name in the connection.
When Arnold first introduced OSL support, an Arnold C++ shader was only able to have a single output. So when OSL shaders had multiple outputs, we had to translate them to multiple Arnold shaders using the `constant STRING output` parameter to tell Arnold which output to use for each one. Arnold shaders can now have multiple outputs (as of at least 7.0, I didn't check further back), so we can use a single Arnold node per OSL shader.
We were previously omitting it for subclasses which didn't use the `out` plug as a container for named shader outputs, and instead used it to represent a single output. Everything is simpler if outputs always have a name, and this aligns much better with USD too where a name is required. Cortex has a nasty workaround with a `DEFAULT_OUTPUT` to round-trip empty names though USD, and this will no longer be needed. What we really want to do as well is enforce that the `out` plug is always just a container, and all outputs must be parented under it. This will also allow us to support multiple Arnold outputs, as requested in GafferHQ#5542. But maintaining backwards compatibility for that change is a bigger problem. This commit means that the `IECoreScene::ShaderNetworks` we generate and export have the format we want, even though we haven't yet adopted the plug format we want yet. Note : the choice of `out` for the default output matches Autodesk's convention for the default (unnamed) output from an Arnold shader, so this means we now export USD using the same conventions as Maya.
We were always connecting from `shader["out"]`, but we should be connecting from `shader["out"]["<name>"]` for well-behaved nodes that represent outputs as we want them.
Otherwise the other parameters we want to set don't exist yet. This didn't occur for any shaders authored in Gaffer, because there we would use the OSLCode node to author a generic shader that comes in via the `isOSLShader` code path. What is being fixed here is Arnold `osl` shaders coming in via USD after being authored somewhere like Houdini.
This matches the networks we now get from `GafferArnold::ArnoldShader` and the convention expected by Arnold-USD. Longer term I think `IECoreScene::ShaderNetwork` will _require_ the output nameas well. Also added a test so that we retain some coverage for legacy outputs serialised into `.scc` and the like.
johnhaddon
force-pushed
the
arnoldUSDConnections
branch
from
e06daad to
8c6fa4b
Compare
johnhaddon
added a commit
to johnhaddon/gaffer
that referenced
this pull request
Apr 24, 2024
This fixes the rendering of shader balls in the Viewer, which was broken by GafferHQ#5759. This really is just a workaround though, and the ideal fix is now documented in ShaderView.cpp, along with the reasons we can't make it immediately. I moved the check for "" and "out" after the search for the valid outputs because of the small chance of there actually being a real output called "out" (and it not being the first one). I think it's unlikely that we need the check for "" as well any more, but I've kept it out of an abundance of caution. Also changed InternedString equality comparisons to use preconstructed strings - comparing to `char *` actually constructs InternedStrings to compare against on the fly, which is unnecessarily slow.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 makes various improvements to our import/export of Arnold shader networks via USD :
outputs:outrather thanoutputs:DEFAULT_OUTPUTin USD. This also applies to the ShaderNetworks we pass around internally, where we now use"out"rather than""to refer to the default output.out:rgb,out:floatetc based on type. I've confirmed that this was not intended to diverge from the Maya/SDR convention, and that it should be fixed in future. In the meantime, we can now translate such outputs to Arnold.oslshaders with inlinecode. We don't author these in Gaffer, but we can now ingest and render them from HtoA and elsewhere.