Skip to content
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-133932 - Implement OpenPBR import/export #3990

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

JGamache-autodesk
Copy link
Collaborator

Add support for backported OpenPBR from MaterialX
Add unit test.

Add support for backported OpenPBR from MaterialX
Add unit test.
cmake/modules/FindUSD.cmake Show resolved Hide resolved
@@ -715,6 +715,26 @@ class UseRegistryShadingModeImporter
}

if (!mayaAttr.isNull()) {
// Connecting the R component of a color to a color should be possible. Check
// if there is a compatible parent in case of type mismatch.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My test case has a few monochrome textures connected to float inputs. Added support for that.

@@ -37,6 +37,17 @@ target_sources(${PROJECT_NAME}
)
endif()

if(USD_HAS_BACKPORTED_MX39_OPENPBR)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backported OpenPBR code requires injecting Maya lighting code at the exact right moment. This is done by creating a special closure node to handle to handle the two MaterialX nodes that require updated lighting.

@@ -0,0 +1,112 @@
#ifdef MAYA_MX39_USING_ENVIRONMENT_FIS
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same code as found in the regular Maya lighting files, but adapted to use Mx39FresnelData and mx39compute_fresnel.

@@ -335,21 +335,27 @@ struct _MaterialXData
{
_MaterialXData()
{
_mtlxSearchPath = HdMtlxSearchPaths();
try {
_mtlxSearchPath = HdMtlxSearchPaths();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any error in this code path will throw a C++ exception that will completely break the Maya viewport. Making sure we catch any issue here.

PXR_NAMESPACE_OPEN_SCOPE

/// Shader reader for importing UsdPreviewSurface to Maya's openPBRShader material nodes
class PxrUsdTranslators_OpenPBRSurfaceReader : public PxrUsdTranslators_MaterialReader
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also identical code to StandardSurface here, but since there was a lot less code, I did not extract a translation table reader. I suspect we would have a lot of similarities in the other surface readers as well.

const auto coatDarkening
= _GetMayaAttributeValue<float>(depNodeFn, TrMayaOpenPBRTokens->coatDarkening);

// Diffuse: Converting from OpenPBR to StandardSurface requires a bit of math to compute coat
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very differenct from StandardSurface here. Code was found in the MaterialX converter networks that remap OpenPBR surface to Standard surface, chained into the Standard surface to UsdPreviewSurface network.

There are a few tweaks here that should be backported to usdStandardSurfaceWriter.cpp:

  • How coat affects emissionColor
  • How subsurface affects diffuseColor

@@ -184,7 +184,7 @@ void PxrUsdTranslators_StandardSurfaceWriter::Write(const UsdTimeCode& usdTime)
shaderSchema,
PxrMayaUsdPreviewSurfaceTokens->NormalAttrName,
usdTime,
/* ignoreIfUnauthored = */ false,
/* ignoreIfUnauthored = */ true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found while debugging. Do not author an empty normals input if we are not connecting anything to it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As good as it gets. Issues with 2nd row 2nd column is known. The OpenGL shader should divide the emissionLuminance by 1000 to get reasonable colors along the range. Same issue with 2nd row last square, which is also emission based.

Issue in 2nd row 3rd square is also known. Exporting normal maps is not yet fully supported.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we are exporting to UsdPreviewSurface, so the roundtrip will lose information, as seen in rows 1 versus 3 and 4 versus 6.

Copy link
Collaborator

@seando-adsk seando-adsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't review the actual code changes as I'm not familiar with this area at all. I only reviewed the cmake and refactoring. Looks okay to me.

optionMenuGrp -l `getMayaUsdString("kImportMaterialConvLbl")` -cw 1 $cw1 -ann `getMayaUsdString("kImportMaterialConvAnn")` mayaUsdTranslator_MaterialsConversionMenu;
menuItem -l `getMayaUsdString("kImportMaterialConvNoneLbl")` -ann "none";
if ((int)$apiStringFull >= 20250300) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is equivalent to the cmake check for OpenPBR

Copy link
Collaborator

@pierrebai-adsk pierrebai-adsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work. Obviously, I won't review the maths :)

@JGamache-autodesk JGamache-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Nov 8, 2024
@samuelliu-adsk samuelliu-adsk merged commit 9711854 into dev Nov 12, 2024
12 of 13 checks passed
@seando-adsk seando-adsk added the import-export Related to Import and/or Export label Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
import-export Related to Import and/or Export ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants