MAYA-104984 MAYA-104641 - Material export via registry #574

JGamache-autodesk · 2020-06-09T16:05:32Z

Changes:

Exposed the "useRegistry" shading export mode in the export dialog.
Wrote registry classes to export Maya lambert, phong, blinn, standard_surface materials to USDPreviewSurface.
Moved the listShadingModes command from pxr to usdMayaPlugin
Moved the pxrUsdPreviewSurface plugin to usdMayaPlugin.
The shading node overrides uses the same VP2 fragments as the Vp2 render delegate.

JGamache-autodesk · 2020-06-09T16:06:35Z

lib/mayaUsd/fileio/shading/CMakeLists.txt

@@ -3,6 +3,7 @@
 # -----------------------------------------------------------------------------
 target_sources(${PROJECT_NAME} 
    PRIVATE
+        listShadingModesCommand.cpp
        shadingModeDisplayColor.cpp


Moved from pxr plugin.

lib/mayaUsd/render/vp2ShaderFragments/shaderFragments.h

JGamache-autodesk · 2020-06-09T16:14:34Z

lib/usd/translators/materialWriter.cpp

+static
+bool
+_AuthorShaderInputFromShadingNodeAttr(
+        const MFnDependencyNode& depNodeFn,


Greatly inspired by the UsdPreviewSurface writer.

JGamache-autodesk · 2020-06-09T16:17:46Z

test/lib/usd/pxrUsdPreviewSurface/testPxrUsdPreviewSurfaceDraw.py

@@ -43,8 +44,6 @@ def setUp(self):
    def _WriteViewportImage(self, outputImageName, suffix):
        # Make sure the hardware renderer is available
        MAYA_RENDERER_NAME = 'mayaHardware2'
-        mayaRenderers = cmds.renderer(query=True, namesOfAvailableRenderers=True)
-        self.assertIn(MAYA_RENDERER_NAME, mayaRenderers)



This command only seem to work in interactive. We can still continue to the ogsRender command.

kxl-adsk

Looking good. I left a few comments, the biggest one being:

coupling of Pixar and adsk plugins which we want to avoid
the move of image comparison tests which we are not running

lib/usd/pxrUsdPreviewSurface/usdPreviewSurfaceShadingNodeOverride.h

lib/usd/pxrUsdPreviewSurface/usdPreviewSurfaceWriter.cpp

plugin/adsk/plugin/plugin.cpp

kxl-adsk · 2020-06-10T18:13:49Z

plugin/adsk/scripts/mayaUsdTranslatorExport.mel

+                    $niceName = "Material Colors";
+                } else if ($modeName == "pxrRis") {
+                    $niceName = "RfM Shaders";
+                } else if ($modeName == "useRegistry") {


One can implement an exporter using a shader registry which doesn't translate to preview surface. We have more work in here - do you agree?

Yes, the exporter should register with a name that reflects the target, not the process. Follow-up exporters using the registry should do the same. They should probably be expected to provide their own nice name as well.

I think I'm second'ing @kxl-adsk's point here. My intention with the addition of the useRegistry shading mode was that it would be the extensible registry for adding arbitrary exporters for whatever shading node types you wanted to write. With the addition of that, I would not expect that we would ever add any more shading modes, and in fact, over time I'd hope that we'd eventually remove the concept of shading modes.

It so happens that the only exporters currently supported by the useRegistry shading mode generate UsdPreviewSurface, but that would definitely not always be the case, so the nice name doesn't really seem appropriate to me.

plugin/pxr/maya/plugin/pxrUsd/plugin.cpp

plugin/pxr/maya/plugin/pxrUsdPreviewSurface/plugin.cpp

test/lib/usd/pxrUsdPreviewSurface/CMakeLists.txt

kxl-adsk · 2020-06-10T18:26:40Z

test/lib/usd/pxrUsdPreviewSurface/testPxrUsdPreviewSurfaceDraw.py

@@ -29,8 +30,9 @@ def setUpClass(cls):
        # that way too.
        cmds.upAxis(axis='z')

-        cls._testDir = os.path.abspath('.')
-
+        cls._testDir = os.path.join(os.path.abspath('.'),


We are currently not running draw tests...I will let @mattyjams comment on the move of this file + all the supporting image files.

I'm going to have to do something about this internally anyway, since we don't run CMake/CTest at all and instead have a parallel setup through SCons. Luckily, pxrUsdPreviewSurface is not (yet) a critical piece of any real workflows, so it's not absolutely necessary that the test be addressed immediately, but it would obviously be better for it to get running again.

I would love to figure out how to get drawing/interactive tests running in this repo though, both for this test and for the others that are still in pxrUsdMayaGL.

test/lib/usd/pxrUsdPreviewSurface/testPxrUsdPreviewSurfaceDraw.py

HamedSabri-adsk

Hi @JGamache-autodesk ,

mayaUsd_copyFiles is redundant and should be removed. mayaUsd_copyDirectory will copy everything from maya-usd/test/lib/usd/pxrUsdPreviewSurface which is your SOURCE to DESTINATION .

You would also want to exclude any *.txt files since they are not needed to be copied over to the build area.

# copy all the resources and python scripts to build directory
mayaUsd_copyDirectory(${TARGET_NAME}
    SOURCE ${CMAKE_CURRENT_SOURCE_DIR}
    DESTINATION ${CMAKE_CURRENT_BINARY_DIR}
    EXCLUDE "*.txt"
)

Other than this, everything else looks good. Thanks.

HamedSabri-adsk

Lgtm! thanks.

One very minor thing, I noticed the original pxr_register_test has MAYA_DISABLE_CIP=1, which is used to avoid fatal crash on start-up. I wonder if we still need this to be set?

I also think we should set MAYA_DISABLE_CER=1 in test.cmake for all the tests to avoid getting the Customer Error Reporting in case of test failures. Again, something for another PR.

kxl-adsk · 2020-06-12T14:47:31Z

test/lib/usd/pxrUsdPreviewSurface/testPxrUsdPreviewSurfaceExport.py

@@ -32,23 +31,27 @@ class testPxrUsdPreviewSurfaceExport(unittest.TestCase):

    @classmethod
    def setUpClass(cls):
-        standalone.initialize('usd')
+        testDir = os.path.join(os.path.abspath('.'),


@ppt-adsk You are moving tests out of Pixar plugin for import/export - where should we be saving these temporary files? Should it go under temp folder (so follow AL example and usage of NamedTemporaryFile) or maybe you found a different solution?

I believe I'm doing something similar to this, creating an output directory for the test if it is not already created, and emptying it out if it is. This is in the build area.

Why not leverage NamedTemporaryFile? (https://docs.python.org/3/library/tempfile.html)

Why use that? If I want to look at files after a test run, I know exactly where to look in the build area. This is often very useful to diagnose test problems.

The existing Pixar test run setup used temporary files, did not clean them up, and this left an increasing amount of useless files in the temporary area.

Knowing where to look for them in a very good argument. I like it, but let's be a bit more specific about:

where in the build folder we should expect to find these temporary test files

rules about leaving and clearing the temporary test files folder

As my implementation currently stands, the output files are placed in a per-test subdirectory of the build importExport directory, where all the import / export tests live, and where the .pyc files are written. The subdirectory is called Output, one per test. The files are always left after the test run, and always cleaned at the start of the test run. If you feel this deserves further conversation, we can do so outside this review.

I would like us to align in how tests are dealing with it.

The subdirectory is called Output, one per test. The files are always left after the test run, and always cleaned at the start of the test run.

This is great. @kxl-adsk Pierre's points makes a lot of sense to me. We can talk about this internally later so we are all aligned.

kxl-adsk

Thank you for addressing my comments @JGamache-autodesk. The last thing that remains is the move of .png files and draw test. I will let @mattyjams comment on it.

mattyjams

A handful of small notes and ideas, but two main points:

I think it's important that the useRegistry shading mode not be thought of as the mode to use to export everything as UsdPreviewSurface, even though that may be what it currently does today based on the exporters that we're registering. Eventually, hopefully other shading node types that might not generate UsdPreviewSurface in USD could be exported through the same mechanism. Ideally, we wouldn't see the "shading mode" concept extended any further to include any additional shading modes, and we would instead represent all shading import and export through useRegistry.
Somewhat to that end, it feels like all of the shading node type or UsdPreviewSurface specific stuff in materialWriter.cpp should be factored out into discrete files and classes, leaving the base material writer class as a type-agnostic framework that can be extended to support individual node types.

It's not critical that that be done before this gets merged, so I'll mark this as approved, but it would be great to address that soon after with a follow-up PR.

For the unit test, I think I'm ok with it moving and not yet being run through the usual CTest harness, as I'm going to have to do some work internally to get it running in our SCons-based system anyway.

lib/mayaUsd/commands/baseListShadingModesCommand.cpp

lib/usd/pxrUsdPreviewSurface/CMakeLists.txt

lib/mayaUsd/render/vp2ShaderFragments/shaderFragments.h

lib/usd/pxrUsdPreviewSurface/CMakeLists.txt

test/lib/usd/pxrUsdPreviewSurface/testPxrUsdPreviewSurfaceDraw.py

plugin/pxr/maya/plugin/pxrUsdPreviewSurface/CMakeLists.txt

mattyjams · 2020-06-13T00:43:42Z

lib/usd/translators/materialWriter.cpp

+PXRUSDMAYA_REGISTER_WRITER(
+    lambert,
+    PxrUsdTranslators_MaterialWriter);
+
+PXRUSDMAYA_REGISTER_WRITER(
+    blinn,
+    PxrUsdTranslators_MaterialWriter);
+
+PXRUSDMAYA_REGISTER_WRITER(
+    phong,
+    PxrUsdTranslators_MaterialWriter);
+
+PXRUSDMAYA_REGISTER_WRITER(
+    phongE,
+    PxrUsdTranslators_MaterialWriter);
+
+PXRUSDMAYA_REGISTER_WRITER(
+    standardSurface,
+    PxrUsdTranslators_MaterialWriter);


It feels to me like all of the shading node type-specific stuff should not live in this file, and should probably be distinct files and probably even classes per type. This file/class would then provide the basis for and common functionality between all shader writers, which they can extend to support whatever type-specific stuff they want to do.

There should be nothing in here that's specific to any particular type of Maya shading node, nor anything having to do with UsdPreviewSurface.

mattyjams · 2020-06-13T00:50:10Z

lib/usd/translators/materialWriter.cpp

+            MPlug scalingPlug =
+                depNodeFn.findPlug(
+                    depNodeFn.attribute(scalingAttrName.GetText()),
+                    /* wantNetworkedPlug = */ true,
+                    &status);
+            if (status == MS::kSuccess) {
+                VtValue vtScale =
+                    UsdMayaWriteUtil::GetVtValue(
+                        scalingPlug,
+                        SdfValueTypeNames->Float);
+
+                if (vtScale.IsHolding<float>()) {
+                    colorScale = vtScale.UncheckedGet<float>();
+                }
+            }


This scaling stuff again seems specific to a particular subset of shading node types.

mattyjams · 2020-06-13T00:52:42Z

lib/usd/translators/materialWriter.cpp

+        return;
+    }
+
+    if (GetMayaObject().hasFn(MFn::Type::kBlinn)) {


The battery of MFn::Type-based checks below seem useful maybe as individual helper functions that discrete node-specific exporters could compose as appropriate, rather than having every node pass through every possible MFn::Type.

# Conflicts: # plugin/pxr/maya/plugin/pxrUsdPreviewSurface/pxrUsdPreviewSurfaceGenFragments.py

JGamache-autodesk · 2020-06-15T21:35:48Z

lib/usd/pxrUsdPreviewSurface/plugInfo.json

-            "Type": "resource"
+            "LibraryPath": "../../../@CMAKE_SHARED_LIBRARY_PREFIX@@TARGET_NAME@@CMAKE_SHARED_LIBRARY_SUFFIX@",
+            "Name": "@TARGET_NAME@",
+            "Type": "library"
        }


Based on plugInfo.json in lib/usd/translators.
How do we handle the fact that 2 Maya plugins will provide the same translator? Should I use mayaUsdPlugin instead at line 7?

I think the change in structure actually means that you could leave the mayaPlugin field above empty. The node and the translator are now getting bundled into the mayaUsd library itself, right?

maya-usd/lib/mayaUsd/fileio/registryHelper.cpp

Line 170 in 1a5d926

if (!mayaPlugin.empty()) {

The mayaUsd library itself is what provides the translator, so it's not actually necessary to load any plugins to ensure that the translator has been registered, since the library will already have been loaded.

As it is now, I think if you only had the adsk mayaUsdPlugin loaded and you did an export, it would needlessly load the pxr pxrUsdPreviewSurface plugin thinking it needs to do so in order to make sure the translator is available.

@mattyjams We want to keep the original plugin structure for preview surface, i.e. the node and the translator is not part of mayaUsd library. This is why we put it in lib/usd and not lib/mayaUsd.

What Jerry talks about is the current limitation where you can have only one translator registered per node type. We will need to support more than one.

As for line 7 - pxrUsdPreviewSurface is a plugin that shouldn't be dependent on ADSK, AL, nor PXR plugins. Why would we need to put mayaUsdPlugin in there?

Indeed the plugInfo.json file is not necessary. The writer will get registered as soon as the basePxrUsdPreviewSurface DLL gets loaded in memory, which will happen as soon as one of the two plugins using UsdPreviewSurface gets loaded.

The plugInfo.json is probably not strictly necessary for export, no, since as you mentioned, the library that provides the translator will already have been loaded to make the node itself available.

It would definitely be necessary for import though, albeit that's not implemented yet. You'll need to loadPlugin something in order to have that library be loaded in the case where you're importing into an empty scene and the importer encounters a shader of that type. And if we're going to (eventually) include an entry in the plugInfo.json for import, it would be nice to have an entry for export as well, just for completeness.

pxrUsdPreviewSurface will live in lib/usd with this change, but what could cause that library to be loaded other than doing a loadPlugin of either mayaUsdPlugin or pxrUsdPreviewSurface (the Maya plugin from pxr)?

lib/mayaUsd/render/vp2ShaderFragments/shaderFragments.cpp

MAYA-104984 MAYA-104641 - Material export via registry

bcc1a7d

JGamache-autodesk requested review from mattyjams and kxl-adsk June 9, 2020 16:05

JGamache-autodesk commented Jun 9, 2020

View reviewed changes

lib/mayaUsd/render/vp2ShaderFragments/shaderFragments.h Show resolved Hide resolved

JGamache-autodesk commented Jun 9, 2020

View reviewed changes

JGamache-autodesk assigned ecp-maya-devops-adsk Jun 9, 2020

MAYA-104984 - Prevent running draw test if no VP2

1163289

kxl-adsk added the import-export Related to Import and/or Export label Jun 10, 2020

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jun 10, 2020

MAYA-104984 - Skip draw test in batch

7aff920

kxl-adsk suggested changes Jun 10, 2020

View reviewed changes

JGamache-autodesk added 3 commits June 11, 2020 14:39

MAYA-104984 - Reworked following review comments

d86a66e

Merge branch 'dev' into t_gamaj/MAYA-104984/use_registry_export

6118519

MAYA-104984 - Added test labels

975a571

JGamache-autodesk commented Jun 11, 2020

View reviewed changes

test/lib/usd/pxrUsdPreviewSurface/testPxrUsdPreviewSurfaceDraw.py Show resolved Hide resolved

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jun 11, 2020

JGamache-autodesk requested review from kxl-adsk and HamedSabri-adsk June 11, 2020 19:16

HamedSabri-adsk suggested changes Jun 11, 2020

View reviewed changes

MAYA-104984 - Remove unnecessary copies

2ae96bc

JGamache-autodesk requested a review from HamedSabri-adsk June 12, 2020 13:30

HamedSabri-adsk approved these changes Jun 12, 2020

View reviewed changes

kxl-adsk reviewed Jun 12, 2020

View reviewed changes

kxl-adsk approved these changes Jun 12, 2020

View reviewed changes

mattyjams approved these changes Jun 13, 2020

View reviewed changes

kxl-adsk unassigned ecp-maya-devops-adsk Jun 14, 2020

kxl-adsk assigned ecp-maya-devops-adsk Jun 14, 2020

JGamache-autodesk added 2 commits June 15, 2020 16:04

MAYA-104984 - First batch of reworks

215727f

Merge branch 'dev' into t_gamaj/MAYA-104984/use_registry_export

c3afce8

# Conflicts: # plugin/pxr/maya/plugin/pxrUsdPreviewSurface/pxrUsdPreviewSurfaceGenFragments.py

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jun 15, 2020

MAYA-104984 - Fix incomplete plugInfo.json

cc5286c

JGamache-autodesk commented Jun 15, 2020

View reviewed changes

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jun 15, 2020

MAYA-104984 - Fix over-deregistration.

e926c70

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jun 15, 2020

kxl-adsk reviewed Jun 16, 2020

View reviewed changes

lib/mayaUsd/render/vp2ShaderFragments/shaderFragments.cpp Show resolved Hide resolved

kxl-adsk merged commit f45f050 into dev Jun 17, 2020

kxl-adsk deleted the t_gamaj/MAYA-104984/use_registry_export branch June 17, 2020 15:16

mattyjams mentioned this pull request Jun 29, 2020

pxrUsdPreviewSurface opacity support broken following PR #574 #607

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAYA-104984 MAYA-104641 - Material export via registry #574

MAYA-104984 MAYA-104641 - Material export via registry #574

JGamache-autodesk commented Jun 9, 2020 •

edited

Loading

JGamache-autodesk Jun 9, 2020

JGamache-autodesk Jun 9, 2020

JGamache-autodesk Jun 9, 2020

kxl-adsk left a comment

kxl-adsk Jun 10, 2020

JGamache-autodesk Jun 11, 2020

mattyjams Jun 13, 2020

kxl-adsk Jun 10, 2020

mattyjams Jun 13, 2020

HamedSabri-adsk left a comment

HamedSabri-adsk left a comment

kxl-adsk Jun 12, 2020

ppt-adsk Jun 12, 2020

kxl-adsk Jun 12, 2020

ppt-adsk Jun 12, 2020

ppt-adsk Jun 12, 2020

kxl-adsk Jun 12, 2020

ppt-adsk Jun 12, 2020

kxl-adsk Jun 12, 2020

HamedSabri-adsk Jun 12, 2020

kxl-adsk left a comment

mattyjams left a comment

mattyjams Jun 13, 2020

mattyjams Jun 13, 2020

mattyjams Jun 13, 2020

JGamache-autodesk Jun 15, 2020 •

edited

Loading

mattyjams Jun 16, 2020

kxl-adsk Jun 16, 2020

JGamache-autodesk Jun 16, 2020

mattyjams Jun 16, 2020 •

edited

Loading

MAYA-104984 MAYA-104641 - Material export via registry #574

MAYA-104984 MAYA-104641 - Material export via registry #574

Conversation

JGamache-autodesk commented Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxl-adsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk left a comment

Choose a reason for hiding this comment

HamedSabri-adsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxl-adsk left a comment

Choose a reason for hiding this comment

mattyjams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JGamache-autodesk Jun 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattyjams Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

JGamache-autodesk commented Jun 9, 2020 •

edited

Loading

JGamache-autodesk Jun 15, 2020 •

edited

Loading

mattyjams Jun 16, 2020 •

edited

Loading