extract RenderMan for Maya shading export from "pxrRis" shadingMode for use with "useRegistry" #787
Conversation
mattyjams
force-pushed
the
pr/convert_pxrRis_shadingMode_into_useRegistry
branch
from
e2c1564 to
865c047
Compare
| TF_DEFINE_PRIVATE_TOKENS( | ||
| _tokens, | ||
|
|
||
| ((conversionName, "rendermanForMaya")) |
There was a problem hiding this comment.
@mattyjams Would it be problematic if we make this translator a separate plugin?
There was a problem hiding this comment.
@kxl-adsk: I had thought of that, but I was a little reluctant to do so right now. My short term goal here is to get rid of the pxrRis shadingMode and replace it with this. Anyone who had been using that would need to start using the useRegistry shadingMode instead, so I was hesitant to force them to make that change and load something else. Especially so since with the use of the genericShaderWriter, there would be almost nothing to the plugin. If you all have strong feelings that this be separated out though, I could do that. I'd just need some guidance as to what structure/naming would make sense for that.
I think ideally, this could one day be owned by the RenderMan team and just ship as part of their plugin once this shading-related API stabilizes and is available in public releases.
There was a problem hiding this comment.
I see your argument for not having much left in the plugin. Moving it over to RenderMan team would match our own discussions around Arnold and translators for Arnold.
How about we then make this translator conditionally compiled? The point to make here is that not everyone has access to RenderMan and we are considering taking it out of the binaries we release.
There was a problem hiding this comment.
Yeah, that makes sense. I can see not necessarily wanting to ship RenderMan-related stuff with the base mayaUsd installer, though there isn't currently a way to cull the pxrRis shadingMode. But hopefully we're killing that soon anyway!
In the rebased commits I just pushed, you'll see a BUILD_RFM_TRANSLATORS build option was added in fc8efb0. This needed to go at the top-level since the option is used in both the lib/... and test/... trees, and you'll see the option checked again to conditionally add the test in 2d951e4.
There was a problem hiding this comment.
Thank you for adding BUILD_RFM_TRANSLATORS, @mattyjams.
I'm definitely for killing pxrRis shadingMode - is this something you are planning on doing in the near future or it's more on us to execute on it?
There was a problem hiding this comment.
Yeah, I'm planning on doing that once useRegistry provides a complete replacement for the functionality pxrRis provided. We don't actually use the pxrRis shadingMode internally, so most of the work is probably just making sure that anyone external who does use it is aware that it's being removed.
I'm planning to kill the displayColor shadingMode too, though that might be slightly more contentious since it seems more likely that people actively use that one. Hopefully not though. With Jerry's work to support Maya-native materials, I don't see much of a need to keep displayColor anymore.
There was a problem hiding this comment.
I'm planning on doing that once useRegistry provides a complete replacement for the functionality pxrRis provided
What would you consider as still missing?
There was a problem hiding this comment.
This PR only adds export support, so I have a follow-up one I'm working on that'll take care of import.
A few nuances off the top of my head that we'll still need to deal with:
- exporting needs to be able to consider non-built-in shading plugs that some renderers use on the
shadingEnginenode:
RenderMan for Maya obviously does this, but I think Arnold might as well? I think the intent is to help differentiate between the shader used for previewing in the Maya viewport and the renderer-specific shader used by the renderer. - Similarly, import likely needs to support creating these non-built-in plugs and making shading connections to them, for example when a Material prim has both an
outputs:surfaceand anoutputs:ri:surfaceterminal, or potentially if we want to start considering materialPurpose (e.g. preview vs. full). useRegistrycurrently only supports importing the universal renderContext terminals of Material prims. The export of RfM shaders here writes them into therirenderContext, souseRegistrywill not currently see them on import. This will tie into the feature above so that the import knows which plugs to connect these shaders to on theshadingEngine.
I'm working on (or will be working on) all of these things and will put up PRs for them, and happy to discuss the intricacies of each as we dig into them.
There was a problem hiding this comment.
Seems we need to get in touch as I am also looking into expanding the material import code to support an import path similar to the materialConversion that was added recently on export.
Some information:
- Arnold does use the same shading group attachment points as regular surface materials, so it does not allow a material purpose concept in Maya.
- We would like to be able to pass multiple shadingMode to the import command to allow specifying a list of locations where we want to search for materials in USD. I thought the search would end on the first surface material found, but having custom entry points for RfM might mean reconsidering how the search terminates. The import options would be simplified to try everywhere, and a user wanting full control over where to search could do so at the command level.
- The usdListShadingModes command would be expanded to also list materialConversions that have an import path. This means expanding the materialConversion registry with import information.
There was a problem hiding this comment.
@JGamache-autodesk: Yup, definitely. Sorry, I didn't mean to suggest that any of the things I mentioned were solved problems with well understood solutions that just needed implementing. I'm sure we'll be talking about all of these things and more as we keep going. PRs (or maybe draft PRs?) might be a good way to anchor some of the discussion around something concrete as we break down each piece, but I'm expecting we'll need to iterate a bit to find the best way forward.
Good to know about Arnold, as well as the other ideas you have in mind. All sounds great to me. Catch up with you about it soon!
|
Looks good. Should not be too difficult to move to the Renderman plugin once the API stabilizes. At that moment, the Pxr part of the RfmNodesToShaderIds table should probably be auto-discovered from the plugin instead of copied in the table. The genericShaderWriter is quite nice. It obsoletes 90% of the code in maya-usd/dev/test/lib/usd/plugin/mayaGenericWriter.cpp |
…lity function This function did not actually use any state from the shadingMode export context, and its functionality could be useful for shader readers and writers. It was removed from UsdMayaShadingModeExportContext as a class member and moved to the UsdMayaShadingUtil namespace.
… to USD It turns out that there is very little specific to RenderMan for Maya being done in the "pxrRis" shadingMode, so this change extracts the shader export translation from there into a re-usable shader writer for use with the "useRegistry" shadingMode. This new shader writer performs a "literal" translation of the Maya shading node to USD. Input and output attributes on the Maya node translate directly to inputs and outputs on the exported UsdShadeShader with the same names. The shader writer can optionally specify a material conversion that must be set in order for the writer to return `Supported` from its `CanExport()` function. If no conversion name is supplied, `CanExport()` will always return `Supported`. If a conversion name is supplied, `Supported` is returned if the conversion name matches the one specified in the export args, and `Unsupported` is returned otherwise. A static `RegisterWriter()` function is provided to simplify the registration of writers that use this new class. `RegisterWriter()` should be called inside a `TF_REGISTRY_FUNCTION(UsdMayaShaderWriterRegistry)` block.
…ngMode Ignoring DAG nodes causes us not to export certain shading node types such as place3dTexture.
This mapping is now made available in a publicly installed header file.
There were a few additional node-to-ID mappings for built-in Maya nodes added since the table was last generated. This change also adds exact mappings for all Maya nodes provided by RenderMan for Maya. These will be used by the generic shader writers that are registered for the "useRegistry" shadingMode, since unlike with the "pxrRis" shadingMode, we need to enumerate all possible node type names at registration time. This should still be safe for "pxrRis" though since it just makes explicit the mapping it would have previously generated anyway.
…rMan for Maya These new shader writers and the "rendermanForMaya" material conversion are intended to replace the export side of the "pxrRis" shadingMode. This is work towards the ultimate goal of removing the "pxrRis" shadingMode. These shader writers hook into the "useRegistry" shadingMode and are active when the "convertMaterialsTo" export argument is set to "rendermanForMaya". Shading networks output by these writers are bound to the Material using the "ri" renderContext. The build of the RenderMan for Maya shader writers is enabled by default, but it can be disabled by setting the build option BUILD_RFM_TRANSLATORS=OFF.
…adingMode This test is largely copied from the existing test that uses "pxrRis" shadingMode. Note that the test scene file only uses Maya-native nodes, so there is no need that RenderMan for Maya be installed or loaded. This test is added when the BUILD_RFM_TRANSLATORS build option is enabled.
mattyjams
force-pushed
the
pr/convert_pxrRis_shadingMode_into_useRegistry
branch
from
865c047 to
2d951e4
Compare
Sounds good to me. I wasn't quite sure what to call this thing, but "symmetric" does seem to fit a bit better than "generic". I rebased these changes and updated the naming in 35d7ba7. The only other difference from before is that I noticed you were using That seemed like a good practice, so I applied that in the symmetric writer here too. Once this goes in, I can put up a follow-up change that revisits that Thanks @JGamache-autodesk! |
|
Hey @kxl-adsk, I ran one last pre-flight check on this PR but it looks like the master/Windows/Python 3 build hit some sort of failure. It looks like all of the tests passed, but the process failed during some packaging step towards the end, maybe where it archives build artifacts?: All the other platforms and build configurations look fine. Let me know if I should kick off another one for this PR. Thanks! |
|
@mattyjams no need to run it again. It's something our DevOps team is tracking and working hard to eliminate. |
kxl-adsk
added
the
ready-for-merge
mattyjams
deleted the
pr/convert_pxrRis_shadingMode_into_useRegistry
branch
There was actually hardly anything RfM-specific happening in the shader export side of the
pxrRisshadingMode, so this change pulls all of that out into a generic shader writer. These then get registered per node-type-name-to-shader-ID mapping with the material conversionrendermanForMaya, and the exported shaders are bound using therirenderContext.The eventual goal is that this plus a forthcoming rewrite of the import side will allow us to remove the
pxrRisshadingMode.Note that none of this actually requires RenderMan for Maya to be loaded or even installed. The
pxrRisshadingMode and now these new translators support mapping some Maya-native node types toPxr-prefixed shader IDs.I'm working on the import side of this as well, but there are a few extra complications there that will likely require some more infrastructure-y changes to the
useRegistryshadingMode itself, as well as some more convenient API inUsdShade.Looking at the commit history might make it a bit easier to follow the progression here than just looking at the flat diffs, but let me know if there are any questions or concerns!