Bring base proxy, ufe and vp2renderdelegate from refactoring_sandbox #91

kxl-adsk · 2019-10-30T16:33:10Z

This PR brings the following modules from refactoring_sandbox:

base proxy shape: enables workflows and integrations to be reusable across plugins
vp2renderdelegate: new subscene override which leverages hydra render delegate to feed VP2 with data coming from base proxy shape stage
ufe-usd: plugin to make USD data model visible to different workflows in Maya 2019 and newer

Additionally, we brought changes to enable UFE tests, adsk plugin (only the proxy shape since UFE tests are dependent on it; move will next merges from refactoring_sandbox) and rpath fixes for Linux and OSX

kxl-adsk · 2019-10-30T16:39:51Z

@murphyeoin and @mattyjams this change affects both AL and PXR plugins. Can you help us with reviewing it?

To help in the review, I organized commits per feature, starting with the creation of base proxy shape as a foundational module and continued with connecting it to AL plugin, vp2renderdelegate, and UFE.

The feature set should match what we currently have in refactoring_sandbox. Once this change will be merged, we will move future development in merged areas to the dev branch.

Finally, this merge will break mergeability between dev and refactoring_sandbox. I'm going to restore it by merging and resolving conflicts manually.

robthebloke · 2019-10-31T01:39:15Z

lib/nodes/proxyShapeBase.cpp

+    UsdStageRefPtr usdStage;
+
+    MayaUsdStageData* inData =
+        dynamic_cast<MayaUsdStageData*>(inDataCachedHandle.asPluginData());


This may break us in the short term. static_cast would be preferable for us. (For annoying reasons, we may have to switch over the StageData MPxData object in steps in the rest of our pipeline, so we may have 2 C++ types, both with the same TypeId).

This is rather concerning usage pattern. We explicitly build base proxy shape to own the stage data.

robthebloke · 2019-10-31T01:40:09Z

lib/nodes/proxyShapeBase.cpp

+    // Get the prim
+    // If no primPath string specified, then use the pseudo-root.
+    UsdPrim usdPrim;
+    std::string primPathStr = primPath.asChar();


std::string primPathStr(primPath.asChar(), primPath.length());

Avoids the additional pointless strlen.

This code has been taken from https://github.com/PixarAnimationStudios/USD/blob/4b1162957b2ad219e1635c81de8343be490e41fc/third_party/maya/lib/usdMaya/proxyShape.cpp#L602
I agree with your suggestion, let's improve it in a separate PR.

lib/nodes/proxyShapeBase.cpp

robthebloke · 2019-10-31T01:43:22Z

lib/nodes/proxyShapeBase.cpp

+        dataBlock.inputValue(outStageDataAttr, &localStatus);
+    CHECK_MSTATUS_AND_RETURN(localStatus, usdPrim);
+
+    MayaUsdStageData* outData = dynamic_cast<MayaUsdStageData*>(outDataHandle.asPluginData());


dynamic cast again.

robthebloke · 2019-10-31T01:43:40Z

lib/nodes/proxyShapeBase.cpp

+    CHECK_MSTATUS_AND_RETURN(localStatus, UsdStageRefPtr());
+
+    MayaUsdStageData* outData =
+        dynamic_cast<MayaUsdStageData*>(outDataHandle.asPluginData());


robthebloke · 2019-10-31T04:52:41Z

lib/ufe/private/Utils.cpp

+	static TfToken rotX("xformOp:rotateX");
+	static TfToken rotY("xformOp:rotateY");
+	static TfToken rotZ("xformOp:rotateZ");
+	static TfToken rotXYZ("xformOp:rotateXYZ");


We need support for other rotation orders.

Current limitation. Nothing we will change in this PR.

robthebloke · 2019-10-31T04:53:47Z

lib/ufe/wrapUsdSceneItem.cpp

+
+#include "UsdSceneItem.h"
+
+#if 1


Agreed, dead code in here. We continue developing this, so more cleanup will happen.

robthebloke · 2019-10-31T05:04:07Z

plugin/al/lib/AL_USDMaya/AL/usdmaya/nodes/ProxyShape.cpp

-MObject ProxyShape::m_complexity = MObject::kNullObj;
-MObject ProxyShape::m_outStageData = MObject::kNullObj;
-MObject ProxyShape::m_displayGuides = MObject::kNullObj;
-MObject ProxyShape::m_displayRenderGuides = MObject::kNullObj;


We'll need to have a chat about this. This will break our current production.

All of these attributes are now provided by the base proxy class. Functionality should be exactly the same.

robthebloke · 2019-10-31T05:13:05Z

plugin/al/lib/AL_USDMaya/AL/usdmaya/nodes/ProxyShape.cpp

-  }
+  cachedBBox = MBoundingBox(
+      MPoint(-100000.0f, -100000.0f, -100000.0f),
+      MPoint( 100000.0f,  100000.0f,  100000.0f));


This is a bug. Please evaluate the correct bounding box!!! reasons are:

We have city sized sets. This bounding box is far too small for that case.

We have lego sized bricks, This bounding box is far too big for that case.

Selection speed is already painfully slow for us. Attempting to force every proxy shape into a selection query won't be great for performance. :(

We need the bounding box to remain accurate for animated objects.

See the method name - ProxyShape::CacheEmptyBoundingBox. The calculation is now provided by MayaUsdProxyShapeBase::boundingBox()

Ahh, ok. Ignore me then :)

robthebloke · 2019-10-31T05:16:11Z

plugin/al/mayautils/AL/maya/utils/NodeHelper.h

+  /// \param  longName  long name for the attribute
+  /// \param  flags  a bitfield containing a mask of the AttributeFlags enumeration. Describes if the attribute is an input/output/etc
+  AL_MAYA_UTILS_PUBLIC
+  static void inheritTimeAttr(const char* longName, uint32_t flags);


Aren't all of these gong to be the same code? Is there anything really specific about the fact it's a time v.s. Int32 attr?

I may not be getting the question, but if you compare addTimeAttr vs inheritTimeAttr you will notice one will register attribute and create it on a node, the second one will inherit already existing attribute from the base class and register it internally to AL plugin.

No, I mean that the code for inheritTimeAttr / inheritInt32Attr / inheritFloatAttr / etc, would all end up being identical. You could just do this with a single inheritAttr method (I think the inheritFileAttr is possibly the only exception)

cfmoore007 · 2019-10-31T11:58:47Z

( usability notes )

As for building / compiling ( on linux vs USD 19.11 ), it works here. Unit tests pass, proxy shape displays/loads fine. And, ( as expected ) same bugs are there from refactoring_sandbox.

However, I'm having an error thrown loading the AL plugin ( I do not see this in refactoring_sandbox ) :

Coding Error: in _Load at line 248 of src/USD/pxr/base/lib/plug/plugin.cpp -- Load of 'maya-usd/lib/usd/mayaUsd_ShaderFragments' for 'mayaUsd_ShaderFragments' failed: maya-usd/lib/usd/mayaUsd_ShaderFragments: cannot read file data: Is a directory

kxl-adsk · 2019-10-31T21:08:31Z

@robthebloke thank you for your review and great suggestions. Almost all of them don't look like blocking for the merge and I propose we address them in separate PRs...maybe with exception to your concern about single stage data and dynamic casts. I'm not sure why this would break on you. Let's discuss this offline.

robthebloke

nothing to add beyond comments above :)

mattyjams

There's a bunch of inconsistency in how includes are formatted, so that ends up being the bulk of the comments. Apologies that there were so many, but I wanted to make sure I flagged each one.

That in combination with the use of #pragma once was problematic for us, since our compiler was not always correctly identifying when it had already processed an #include. In particular, I had to replace the #pragma once in UsdSceneItem.h with an #ifndef guard, otherwise we'd get compiler errors about things being redefined. I'd really like to see us remove all of the #pragma once usage in favor of traditional #ifndef-style include guards.

This branch might need to be rebased against dev? I noticed that plugin/pxr/pxr/pxr.h.in has crept back in, so this may be missing @elrond79's fixes in #52.

I also noticed that two pyc files appear to have been added accidentally:

plugin/al/usdtransaction/AL/usd/transaction/tests/testTransaction.pyc
plugin/al/usdtransaction/AL/usd/transaction/tests/testTransactionManager.pyc

I have this building but not yet working correctly at Pixar, so not ready to approve just yet, but I'm hoping to be able to do so soon.

mattyjams · 2019-11-08T19:50:50Z

cmake/utils.cmake

+function(mayaUsd_promoteMayaUsdHeader)
+    set(srcFile ${CMAKE_CURRENT_SOURCE_DIR}/base/mayaUsd.h.src)
+    set(dstFile ${CMAKE_BINARY_DIR}/include/mayaUsd/mayaUsd.h)
+    if (NOT EXISTS ${dstFile})
+        message(STATUS "promoting: " ${srcFile})
+    endif()
+    configure_file(${srcFile} ${dstFile})
+endfunction()
+
+function(mayaUsd_promoteHeaderList)
+    foreach(header ${ARGV})
+        set(srcFile ${CMAKE_CURRENT_SOURCE_DIR}/${header})
+        set(dstFile ${CMAKE_BINARY_DIR}/include/mayaUsd/${header})
+
+        set(content "#pragma once\n#include \"${srcFile}\"\n")
+
+        if (NOT EXISTS ${dstFile})
+            message(STATUS "promoting: " ${srcFile})
+            file(WRITE ${dstFile} "${content}")
+        else()
+            file(READ ${dstFile} oldContent)
+            if (NOT "${content}" STREQUAL "${oldContent}")
+                message(STATUS "Promoting ${srcfile}")
+                file(WRITE ${dstFile} "${content}")
+            endif()
+        endif()
+    endforeach()
+endfunction()


This seems somewhat brittle to me, and a bit odd to have file and directory names specific to the mayaUsd library hard-coded in a top-level CMake file like this.

I would think mayaUsd_promoteMayaUsdHeader() would only ever be called in one place, so that seems like an easy one to just move into the library's CMakeLists.txt

For mayaUsd_promoteHeaderList(), could that not be accomplished using set_target_properties() or install() and PUBLIC_HEADER/PRIVATE_HEADER in the mayaUsd library itself instead? This feels like a reimplementation of something CMake usually takes care of. I'd also advocate against modifying the file contents and instead make sure that the source file is complete (more on #pragma later). If it must stay as is, I would also suggest moving it to the library CMakeLists.txt.

mattyjams · 2019-11-08T19:57:30Z

lib/CMakeLists.txt

@@ -0,0 +1,463 @@
+#


Organizationally, I would love to see all the files that currently live in the "lib" directory to be grouped under "lib/mayaUsd" instead. That would create a clearer mapping between the structure in source and what gets installed. It might also help clean up some of the oddities like the mayaUsd.h.src being in the base directory but then getting lofted out.

I could see us potentially adding other top-level libraries too, possibly one for utilities that don't link USD for example, and the current structure doesn't really lend itself to that.

mattyjams · 2019-11-08T19:59:43Z

lib/CMakeLists.txt

+list(APPEND mayaUsdPxVP20_headers
+    render/px_vp20/utils.h
+    render/px_vp20/utils_legacy.h
+)


These files don't seem to exist. They still live under the pxr library as far as I can tell?

They don't need to be here...it will be in a future merge.

mattyjams · 2019-11-08T20:03:39Z

lib/CMakeLists.txt

+
+    add_library(${UFE_PYTHON_LIBRARY_NAME}
+        SHARED
+        ufe/wrapUtils.cpp


There's a wrapUsdSceneItem.cpp file on disk that's not listed here?

Great catch! Removed the file.

mattyjams · 2019-11-08T20:05:57Z

lib/CMakeLists.txt

+    if (CMAKE_UFE_V2_FEATURES_AVAILABLE)
+        # Maya Attribute Editor python template files
+        install(FILES ae/mesh/ae_template.py
+             DESTINATION ${CMAKE_INSTALL_PREFIX}/lib/python/ufe_ae/usd/nodes/mesh
+        )
+        # We need an empty __init__.py file in each subfolder so it is considered a python module.
+        set(_INIT_PY_DEST ${CMAKE_INSTALL_PREFIX}/lib/python/)
+        foreach(_SUBDIR ufe_ae usd nodes mesh)
+            string(APPEND _INIT_PY_DEST "/${_SUBDIR}")
+            install(FILES ae/__init__.py
+                 DESTINATION ${_INIT_PY_DEST}
+            )
+        endforeach()
+    endif()


I'm not sure whether this chunk is relevant yet with this PR? I'm not seeing any of these attribute editor files.

Nope, removed.

mattyjams · 2019-11-09T01:41:04Z

plugin/pxr/maya/plugin/pxrUsdTranslators/fileTextureWriter.cpp

-#include "usdMaya/util.h"
+#include <mayaUsd/utils/util.h>


Not a system header:

#include "mayaUsd/utils/util.h"

mattyjams · 2019-11-09T01:41:12Z

plugin/pxr/maya/plugin/pxrUsdTranslators/instancerWriter.cpp

-#include "usdMaya/util.h"
+#include <mayaUsd/utils/util.h>


Not a system header:

#include "mayaUsd/utils/util.h"

mattyjams · 2019-11-09T01:41:19Z

plugin/pxr/maya/plugin/pxrUsdTranslators/jointWriter.cpp

-#include "usdMaya/util.h"
+#include <mayaUsd/utils/util.h>


Not a system header:

#include "mayaUsd/utils/util.h"

mattyjams · 2019-11-09T01:41:27Z

plugin/pxr/maya/plugin/pxrUsdTranslators/meshWriter.cpp

-#include "usdMaya/util.h"
+#include <mayaUsd/utils/util.h>


Not a system header:

#include "mayaUsd/utils/util.h"

mattyjams · 2019-11-09T01:42:09Z

plugin/pxr/maya/plugin/pxrUsdTranslators/meshWriter_Primvars.cpp

-#include "usdMaya/colorSpace.h"
+#include <mayaUsd/utils/colorSpace.h>
 #include "usdMaya/roundTripUtil.h"
-#include "usdMaya/util.h"
+#include <mayaUsd/utils/util.h>


Not system headers:

#include "usdMaya/roundTripUtil.h" #include "mayaUsd/utils/colorSpace.h" #include "mayaUsd/utils/util.h"

…_on_windows Review blockers and pixar tests on windows

mattyjams

I caught up with @kxl-adsk offline on this PR. He submitted and merged into this branch #109 which addresses most of the main issues we ran into at Pixar. With that, I'll approve this PR.

One last thing we might want to fix before merging (but not critical), but these two files look like they may have been added accidentally:

plugin/al/usdtransaction/AL/usd/transaction/tests/testTransaction.pyc
plugin/al/usdtransaction/AL/usd/transaction/tests/testTransactionManager.pyc

The only issue left for us (and seemingly only affecting us) is the use of #pragma once specifically in UsdSceneItem.h. We're going to patch that locally for now and hope that when we upgrade our compiler it becomes a non-issue. I still wanted to flag it though since we've been burned in the past by open source users of USD using some exotic compiler that similarly chokes on #pragma once whereas #ifndef-based include guards have been unambiguous and reliable. Even assuming we do continue to use #pragma once, I'd advocate that we remove the bit in cmake/utils.cmake that adds it programatically in mayaUsd_promoteHeaderList() and instead ensure that all source code files are complete.

Also wanted to note two things I'm going to look into following this merge:

So far, using VP2_RENDER_DELEGATE_PROXY=1 with pxrUsdReferenceAssembly nodes does not appear to work, though pxrUsdProxyShape nodes do. I get errors about invalid prim names, so my suspicion is that the introduction of Maya namespaces with : delimiters in them causes the creation of invalid identifiers/prim paths when translated for Hydra. I'm hoping that fixing this is as simple as adding a TfMakeValidIdentifier() somewhere.
I'd like to clean up the plugInfo.json setup for mayaUsd. The combination of relative paths baked into the file itself and the relocation happening in the build is confusing to follow and seems likely to cause issues in the future.

A few other notes I wanted to unbury:

Shader fragment XML files are forked from pxrUsdPreviewSurface, which used to be kept in sync with USD's previewSurface.glslfx. Are we concerned with the two drifting apart if we don't continue to auto-generate the files in lib/render/vp2ShaderFragments?
For the libraries that moved/are moving from pxr to mayaUsd, we should probably move their Python wrapping as well:
- colorSpace
- query
- stageCache

Other Cosmetic stuff we might want to clean up and/or chat about after things start to settle in the dev branch:

Organization of files under the lib directory (e.g. lib/mayaUsd)
Use fully-qualified paths to public header files in #include lines
Angle braces vs. quotes for maya-usd vs. "system" (Maya, UFE) header includes
use of ' = default;' for empty virtual destructor overrides
Clean up old Pixar Doxygen tags in header files

kxl-adsk · 2019-11-11T23:50:48Z

Re Shader fragment XML files - we actually fixed issues with light support in them and would like to keep only one copy. This will be resolved once we will bring the rest of refactoring from sandbox branch.

Re missing python wrappers - the next big merge should address it, but let us know if it's needed sooner.

…dev. Changes based on comments from PR: #91 * For these include file changes below mostly made the change only in 'lib/ufe' ** #include "mayaUsd/xxx" -> #include <mayaUsd/xxx> ** #include "maya/xxx" -> #include <maya/xxx> ** #include "ufe/xxx" -> #include <ufe/xxx> ** #include "pxr/xxx" -> #include <pxr/xxx> * reserve memory for vector

See #91

* MAYA-99741 - Build all 3 plugins build/work in sandbox * Added top-level cmake option "CMAKE_WANT_UFE_BUILD" with default value ON. When enabled, we (quietly) try to find UFE and if available we enable it and use it for building. If not available, then don't compile UFE features. * Being an option means users UFE can be disabled if needed. * Removed logic which looked for UFE only when devkit location was set. * Replaced specialized "HDMAYA_UFE_BUILD" define with our generic "WANT_UFE_BUILD" define. * Replaced xxx_INT vars with cmake configure logic. * Added missing UFE to pxrUsd plugin. * MAYA-99741 - Build all 3 plugins build/work in sandbox * No need for extra #define in the promoted header file hdMaya.h. Just use the standard cmake add_definitions which works on all platforms and is consistent with the rest of our repo.

fill in missing python translator export implementation

Krystian Ligenza added 11 commits October 24, 2019 15:12

Create ProxyShapeBase out of Pixar plugin

0779aca

Refactor AL proxy shape to use ProxyShapeBase class

70efd23

Add VP2RenderDelegate and enable it for both plugins

571c61f

Add UFE-USD plugin

5e5260e

Add test build stage, adsk plugin and UFE plugin tests

c217793

Add CMake changes

a845d5d

Refactor rpaths

f424a5f

Fix OSX build after rpath changes and enabling of AL tests build

77425bc

Fix linux build

4120aae

Fix UFE crashes with AL regression tests

f51d2f2

Add new changes from refactoring_sandbox

a38b845

robthebloke reviewed Oct 31, 2019

View reviewed changes

kxl-adsk mentioned this pull request Nov 5, 2019

[pxr] get tests building / working for pixar plugin #100

Closed

robthebloke approved these changes Nov 6, 2019

View reviewed changes

mattyjams reviewed Nov 9, 2019

View reviewed changes

Krystian Ligenza added 4 commits November 11, 2019 14:57

Get Pixar tests run on Windows

f9c76da

Address blockers for Pixar

1585bcd

Add TODO

0006397

Merge pull request #109 from Autodesk/review_blockers_and_pixar_tests…

d214abe

…_on_windows Review blockers and pixar tests on windows

mattyjams approved these changes Nov 11, 2019

View reviewed changes

huidong-chen approved these changes Nov 11, 2019

View reviewed changes

Remove pyc files added by mistake

6f34227

kxl-adsk merged commit f8c3e2e into dev Nov 11, 2019

kxl-adsk deleted the proxy_ufe_vp2renderdelegate branch November 11, 2019 23:51

seando-adsk mentioned this pull request Nov 29, 2019

Integrate AE work from sandbox into dev #142

Merged

mattyjams mentioned this pull request Dec 14, 2019

Autodesk/ppt/refactoring sandbox to dev 00 #154

Merged

huidong-chen pushed a commit that referenced this pull request Dec 16, 2019

MAYA-102285 minor code refinement from previous review

4c50c61

See #91

huidong-chen mentioned this pull request Dec 16, 2019

MAYA-102285 minor code refinement from previous review #180

Merged

robthebloke pushed a commit to AnimalLogic/maya-usd that referenced this pull request Sep 10, 2020

Merge pull request Autodesk#91 from rnd/pyTranslatorExport1

dd2f9ac

fill in missing python translator export implementation

Bring base proxy, ufe and vp2renderdelegate from refactoring_sandbox #91

Bring base proxy, ufe and vp2renderdelegate from refactoring_sandbox #91

Conversation

kxl-adsk commented Oct 30, 2019

kxl-adsk commented Oct 30, 2019

robthebloke Oct 31, 2019 • 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

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

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

cfmoore007 commented Oct 31, 2019

kxl-adsk commented Oct 31, 2019

robthebloke 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

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

mattyjams left a comment • edited Loading

Choose a reason for hiding this comment

kxl-adsk commented Nov 11, 2019

robthebloke Oct 31, 2019 •

edited

Loading

mattyjams left a comment •

edited

Loading