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

Add option to fallback to mayaDefaultStandardSurface shader fragment #3957

Merged
merged 7 commits into from
Nov 25, 2024

Conversation

kpolonski-atomic
Copy link
Contributor

In maya versions 2024 and greater, the default shader for geometry without a shader assignment has been switched to use the standardSurface material which shades correctly under the new default light intensity of 3.14 of VP2. This makes the current maya-usd fallback material shade very bright compared to maya geometry in the same scene that uses vertex colors for it's material.

Within our studio, I have implemented a Tf environment setting MAYAUSD_VP2_USE_STANDARDSURFACE_FALLBACK that can be set to true to use the 'mayaDefaultStandardSurface' shader fragment graph as a fallback shader that works properly with usd displayColor and displayOpacity primvars from geometry. This then gives consistent shading across geometry rendered in VP2 from both maya geometry and geometry from a usd proxy shape and is opt in.

Please see my attached images and example maya scene displaying the inconsistency of shader defaults between maya geometry and VP2 hydra delegate meshes in maya 2024. NOTE: to use this example maya scene PIXMAYA_LINEAR_COLORS=true must be set as the color values are linear rec709 space.

I have emailed a corporate CLA from Atomic Cartoons Inc

I have also submitted a support ticket to Autodesk and got back code BSPR-110507 from them.

2024-09-19_12-35

2024-09-19_12-37

maya_scene_and_usd.zip

= shaderMgr->getStockShader(MHWRender::MShaderManager::k3dDefaultMaterialShader);
= TfGetEnvSetting(MAYAUSD_VP2_USE_STANDARDSURFACE_FALLBACK) ?
shaderMgr->getStockShader(MHWRender::MShaderManager::k3dStandardSurfaceShader) :
shaderMgr->getStockShader(MHWRender::MShaderManager::k3dDefaultMaterialShader);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would say lets live in the future and not use an environment variable in this situation. There is a MEL call that will tell you what the default shader name is:

    _3dDefaultMaterialShader
        = (MGlobal::executeCommandStringResult("defaultShaderName()") != "lambert1") ?
        shaderMgr->getStockShader(MHWRender::MShaderManager::k3dStandardSurfaceShader) :
        shaderMgr->getStockShader(MHWRender::MShaderManager::k3dDefaultMaterialShader);

If you are curious or want to test that everything still works in a legacy universe, you can either open an old scene that was saved when Lambert was the default, or use the following env var to temporarily restore Lambert as the default: set MAYA_DEFAULT_SURFACE_SHADER=lambert.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh. That brings back bad memories of PR #2049. The number of unit test results to update would be quite important if you modernize the code.

That would be asking a lot, so let's try a progressive solution: introduce an env var, but instead of having it introduce the new behavior, use it to get back to the previous one:

    bool useStandardSurface = (MGlobal::executeCommandStringResult("defaultShaderName()") != "lambert1");
    if (TfGetEnvSetting(MAYAUSD_VP2_USE_LAMBERT_FALLBACK) {
        // Explicit request for legacy Lambert:
        useStandardSurface  = false;
    }
    _3dDefaultMaterialShader
        = useStandardSurface ?
        shaderMgr->getStockShader(MHWRender::MShaderManager::k3dStandardSurfaceShader) :
        shaderMgr->getStockShader(MHWRender::MShaderManager::k3dDefaultMaterialShader);

This way you can add that env var to test\lib\mayaUsd\render\vp2RenderDelegate\CMakeLists.txt to not have to update all results, and remove the env var for unit tests that would shine with your new code.

One unit test I have in mind would be test\lib\mayaUsd\render\vp2RenderDelegate\testVP2RenderDelegateDisplayColors.py with a standardSurface-based scene when mel.eval("defaultShaderName()") != "lambert1". Would require a different set of baseline images in the modern code path. Just add a _standardSurface suffix to them like we do in other modernization situations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's a great idea to live in the future and have the env var work the other way around. Thanks for the hint of the mel command to get the default shader name, this is a good way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Changed to MAYAUSD_VP2_USE_LAMBERT_FALLBACK and comparison of the default shader name.
  • Updated tests cmake file to use this environment for legacy tests.

@kpolonski-atomic
Copy link
Contributor Author

Thanks for the feedback, will get around to changing the implementation at some point this week. Cheers!

Copy link
Collaborator

@JGamache-autodesk JGamache-autodesk left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for the update!

@kpolonski-atomic
Copy link
Contributor Author

kpolonski-atomic commented Oct 18, 2024

Ran clang-format on the files build_preflight complained about

@seando-adsk
Copy link
Collaborator

Almost every build failed during testing. I'll re-run the preflight to see if it was just a system glitch.

@kpolonski-atomic
Copy link
Contributor Author

I didn't see the pre-flight get run again for this again @seando-adsk . Curious what the issue could be. Are there any relevant logs to take a look at?

@seando-adsk
Copy link
Collaborator

@kpolonski-atomic Every build in the preflight failed again during the testing stage. Different tests are failing on different platforms. For example:

2024 Linux:

testUsdMayaGetVariantSetSelections (Failed)
testUsdMayaReferenceAssemblyEdits (Failed)
testUsdReferenceAssemblyChangeRepresentations (Failed)
TestALUSDMayaPython_ImportModule (SEGFAULT)

2024 Windows:

testUtilsFileSystem (Exit code 0xc0000142)
testTargetLayer (Exit code 0xc0000142)
test_ShaderGenUtils (Exit code 0xc0000142)
testUsdMayaGetVariantSetSelections (Failed)
testUsdMayaReferenceAssemblyEdits (Failed)
testUsdReferenceAssemblyChangeRepresentations (Failed)
TestALUSDMayaPython_ImportModule (Failed)

The failing tests just seem to be crashing. Not much output, example:

 99/266 Test #158: testUtilsFileSystem .............................Exit code 0xc0000142
***Exception:   1.23 sec

@kpolonski-atomic
Copy link
Contributor Author

@kpolonski-atomic Every build in the preflight failed again during the testing stage. Different tests are failing on different platforms. For example:

interesting, I ran the tests last night (linux maya-2024) and didn't get any failures involving any segfaults or crashes. I got one from mtoh which I have to fix but I wouldn't expect this many failures, especially on 2024 Linux build.

one suspect I have is my FloatToFloat3 shader fragment and possible language choices between Cg, HLSL and GLSL. I'm not familiar with how maya might fail in the case of bad syntax here nor have I worked with any shader fragments previously. The only reason I made that addition was that the maya provided mayaFloat3Joiner was giving me very odd results when I was attempting to use it for alpha to opacity.

I'll do some investigating. Thanks

@seando-adsk
Copy link
Collaborator

@kpolonski-atomic I will ask one of the internal Autodesk developers to compile your branch locally and run the tests to see if we can replicate the failures.

@kpolonski-atomic
Copy link
Contributor Author

sounds like a plan @seando-adsk , thanks for that!

In the meantime I peered very closely into the FloatToFloat3 shade fragment but don't see an issue there. The constructors I am using to form the array for each language match that of other shade fragments that maya provides.

@seando-adsk
Copy link
Collaborator

@kpolonski-atomic I compiled your branch locally and ran the tests - they all failed for me (on Windows) just like the preflight. I debugged it and found the problem is from calling WantStandardSurfaceFallback() to set in a global variable. You cannot do that as Maya hasn't initialized yet (so you cannot use MString, etc). I reworked the code to move those into the initialize() method and then all the tests passed.

fix_crash.patch

Sean

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.

Unit tests are crashing.

@kpolonski-atomic
Copy link
Contributor Author

@kpolonski-atomic I compiled your branch locally and ran the tests - they all failed for me (on Windows) just like the preflight. I debugged it and found the problem is from calling WantStandardSurfaceFallback() to set in a global variable. You cannot do that as Maya hasn't initialized yet (so you cannot use MString, etc). I reworked the code to move those into the initialize() method and then all the tests passed.

That would make sense. Somehow my environment is allowing for this to happen so I was unable to reproduce the crash. Thanks so much for taking the time to check on that for me @seando-adsk . I've applied your patch and pushed it up.

@kpolonski-atomic kpolonski-atomic removed their assignment Oct 31, 2024
@kpolonski-atomic
Copy link
Contributor Author

@seando-adsk could we get the CI to pick up on this again? requested changes have been completed

@seando-adsk
Copy link
Collaborator

@kpolonski-atomic Sorry for the delay. I'm just waiting to hear back from our legal department about the copyright changes. Since we moved to the Autodesk maya-usd github repo we have never had an external user add a copyright so we just wanted to confirm its okay.

Sean

@kpolonski-atomic
Copy link
Contributor Author

kpolonski-atomic commented Nov 4, 2024

Ah I see, thanks for the update. I merely followed the instructions in your codingGuidelines.md file under the Copyright notice section, which states:

Every file should contain at least one Copyright line at the top, which can be to Autodesk or the individual/company that contributed the code.

Keep me posted as to whether this presents an issue or not.

@seando-adsk
Copy link
Collaborator

@kpolonski-atomic Sorry for the delay. We got the answer back from our Legal and the copyright is fine. I'll re-run the preflight.

@kpolonski-atomic
Copy link
Contributor Author

Excellent! thanks @seando-adsk

seando-adsk
seando-adsk previously approved these changes Nov 18, 2024
@kpolonski-atomic
Copy link
Contributor Author

@seando-adsk it looks like the workflow requires an approval in order to run, if you wouldn't mind taking a look whenever you have a moment.

@seando-adsk
Copy link
Collaborator

@kpolonski-atomic Unfortunately the preflight failed again on our Windows 2022 build which ran your new test. The scene file you created was for Maya 2024 and won't load in Maya 2022. Is it reasonable to only run this test for Maya 2024 and greater? If so you can adjust the test to only run in 2024 and above. Grep for either skipIf or skipUnless for an example on how to do that.

    self._StartTest('ColorTestStandardSurface')
  File ".../ecg-mayausd-branch-preflight-2022-python2-pxrusd-min-interactive-windows/ecg-maya-usd/maya-usd/test/lib/mayaUsd/render/vp2RenderDelegate\testVP2RenderDelegateDisplayColorsStandardSurface.py", line 67, in _StartTest
    cmds.file(testFile, o=True)
RuntimeError: file: .../ecg-mayausd-branch-preflight-2022-python2-pxrusd-min-interactive-windows/ecg-maya-usd/maya-usd/test/testSamples/displayColors/ColorTestStandardSurface.ma line 5: Unknown Maya file version: 2024.
file: .../ecg-mayausd-branch-preflight-2022-python2-pxrusd-min-interactive-windows/ecg-maya-usd/maya-usd/test/testSamples/displayColors/ColorTestStandardSurface.ma line 364: The displayLayer 'defaultLayer' has no '.ufem' attribute.
file: .../ecg-mayausd-branch-preflight-2022-python2-pxrusd-min-interactive-windows/ecg-maya-usd/maya-usd/test/testSamples/displayColors/ColorTestStandardSurface.ma line 364: setAttr: No object matches name: .ufem

@kpolonski-atomic
Copy link
Contributor Author

@seando-adsk I think it is reasonable to skip the added test in maya2022 as the default shader in that version is not the standardSurface. I have added the unittest skipIf decorator to the test method to skip if the version is less than 2024.

@seando-adsk seando-adsk added vp2renderdelegate Related to VP2RenderDelegate ready-for-merge Development process is finished, PR is ready for merge labels Nov 25, 2024
@seando-adsk seando-adsk merged commit f3b37ff into Autodesk:dev Nov 25, 2024
11 checks passed
@seando-adsk
Copy link
Collaborator

@kpolonski-atomic As you can see your PR is merged. Thanks for your patience and getting everything fixed for this PR.

Sean

@kpolonski-atomic
Copy link
Contributor Author

Thank you for your patience as well @seando-adsk ! Glad we were able to get this one merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge Development process is finished, PR is ready for merge vp2renderdelegate Related to VP2RenderDelegate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants