-
Notifications
You must be signed in to change notification settings - Fork 203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MAYA-102319 Basic useRegistry import support #621
Conversation
Add the useRegistry import mode which uses an info:id-based registry to process individual UsdShade nodes. Implemented basic import of UsdPreviewSurface and UsdUVTexture nodes to plugin pxrUsdPreviewSurface materials. Split monolithic materialWriter file into proper C++ hierarchy. Note: does not convert to Maya native materials (yet).
@@ -129,7 +129,7 @@ MStatus | |||
MayaUsdProxyShapePlugin::finalize(MFnPlugin& plugin) | |||
{ | |||
// If more than one plugin still has us registered, do nothing. | |||
if (_registrationCount-- > 1) { | |||
if (!_registrationCount || _registrationCount-- > 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request from previous review: fix negative registration counts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (_registrationCount == 0 || ...)
might be more clear here.
instancerWriter.cpp | ||
jointWriter.cpp | ||
lightReader.cpp | ||
lightWriter.cpp | ||
locatorWriter.cpp | ||
materialReader.cpp | ||
materialWriter.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As requested in a previous review: split the monolithic materialWriter.cpp file per dependency node.
class UsdTimeCode; | ||
|
||
/// Shader writer for exporting Maya's material shading nodes to USD. | ||
class PxrUsdTranslators_MaterialWriter : public UsdMayaShaderWriter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Utility base class for often-used functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good idea, just please add documentation to these often-used functions.
return false; | ||
} | ||
|
||
// File |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the exact reverse of usdFileTextureWriter.
MFnDependencyNode depFn; | ||
if (!(UsdMayaTranslatorUtil::CreateShaderNode( | ||
MString(prim.GetName().GetText()), | ||
PxrMayaUsdPreviewSurfaceTokens->MayaTypeName.GetText(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Importing back to pxrUsdPreviewSurface plugin material at this point int time.
Importing to Maya native materials to happen in the next PR.
if (mayaAttrName == PxrMayaUsdPreviewSurfaceTokens->DiffuseColorAttrName | ||
|| mayaAttrName == PxrMayaUsdPreviewSurfaceTokens->EmissiveColorAttrName | ||
|| mayaAttrName == PxrMayaUsdPreviewSurfaceTokens->SpecularColorAttrName) { | ||
// These values are exported unscaled, so they also reimport unscaled: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will have to check to see if we want them scaled on export.
return false; | ||
} | ||
|
||
// Connect manually (fileTexturePlacementConnect is not available in batch): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code from MEL script fileTexturePlacementConnect copied here since the script is not sourced in batch.
class UsdTimeCode; | ||
|
||
/// Shader writer for exporting Maya's material shading nodes to USD. | ||
class PxrUsdTranslators_MaterialWriter : public UsdMayaShaderWriter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good idea, just please add documentation to these often-used functions.
test/lib/usd/pxrUsdPreviewSurface/testPxrUsdPreviewSurfaceExportImportRoundtrip.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good to me! Thanks for all the great work here @JGamache-autodesk!
I left a bunch of comments and suggestions, but not really much that I think would gate merging this. It's a great start to continue building on, and I think we'll just have to work our way through some of the issues we've been discussing as they come to light.
It might have been nice to have the export changes separated out into their own commits/PR, but it's all been reviewed at this point, so maybe moot now.
test/lib/usd/pxrUsdPreviewSurface/testPxrUsdPreviewSurfaceExportImportRoundtrip.py
Outdated
Show resolved
Hide resolved
test/lib/usd/pxrUsdPreviewSurface/testPxrUsdPreviewSurfaceExportImportRoundtrip.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Excited to dig into this and continue to flesh this all out. Thanks @JGamache-autodesk!
@@ -82,7 +82,8 @@ def testPxrUsdPreviewSurfaceRoundtrip(self): | |||
cmds.connectAttr(file_node + ".outColor", | |||
material_node + ".diffuseColor", f=True) | |||
|
|||
txfile = "PxrUsdPreviewSurfaceExportTest/Brazilian_rosewood_pxr128.png" | |||
txfile = "/".join(["UsdExportImportRoundtripPreviewSurface", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.path.sep
here instead of "/"
, or just use os.path.join()
instead?
MStatus status; | ||
MFnDependencyNode depFn; | ||
if (!(UsdMayaTranslatorUtil::CreateShaderNode( | ||
MString(prim.GetName().GetText()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's less about trying to ensure that the node names are unique, and more important that the translation is as consistent as possible across multiple roundtrips. When there are potentially existing overs on the prims being exported, it would be easy for those to fall off if a name collision on import caused the node name to change, which then affects the prim path when re-exported.
And yeah, I think material deduplication would be an interesting feature to pursue if we can detect that the "source" materials in USD are all the same material (e.g. bindings from multiple gprims across references that all resolve to what is ultimately a single material).
usdBlinnWriter.cpp | ||
usdFileTextureWriter.cpp | ||
usdLambertWriter.cpp | ||
usdMaterialWriter.cpp | ||
usdPhongWriter.cpp | ||
usdPhongEWriter.cpp | ||
usdReflectWriter.cpp | ||
usdStandardSurfaceWriter.cpp | ||
usdUVTextureReader.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I'm not quite sure I'm following yet how the selection of an export "dialect" impacts what ultimately gets authored in USD, and how/if there's an overlap there with UsdShade's concept of renderContext. There ongoing discussion here, so I guess I wouldn't worry too much about the naming for now.
Add the useRegistry import mode which uses an info:id-based registry to process individual UsdShade nodes.
Implemented basic import of UsdPreviewSurface and UsdUVTexture nodes to plugin pxrUsdPreviewSurface materials.
Split monolithic materialWriter file into proper C++ hierarchy.
Note 1: does not convert to Maya native materials (yet).
Note 2: does not solve the issue of having multiple importers for a single info:id (for commonly used names like "standard_surface")