Autodesk/ppt/refactoring sandbox to dev 00 #154

ppt-adsk · 2019-12-04T19:46:25Z

This is the second large pull request to move code from the Pixar plugin to the mayaUsd core library and translator plugins. Impact on the AL plugin is minimal. All Pixar and mayaUsd core tests pass on my CentOS 7 Linux box (albeit with some difficult to reproduce random failures). All AL tests using the AL test plugin pass when run inside an interactive Maya. When running the test stage of the build script, the following tests fail, but these are apparently known to fail in the dev branch at this time:
74 - AL_MayaUtilsTests (Failed)
77 - TestAL_USDMaya (Failed)
78 - TestUSDMayaPython (Failed)
80 - TestAdditionalTranslators (Failed)
81 - TestPxrUsdTranslators (Failed)

ppt-adsk · 2019-12-04T19:52:21Z

Tests also pass on OS X and Windows.

robthebloke

Behind our firewall, trying to review a PR this size is a painfully slow experience :(

I think I'd prefer changes such as the copyright notices, changes to the export macro, changes to the file paths, and then the actual code changes split out into their own commits in the future.

I'm really not a fan of the change to use relative include paths (especially since half the PR uses absolute paths, the other half uses relative paths). It's a change that only serves to introduce more git noise than is really needed (and also worth mentioning that both the pixar and animal coding conventions already use absolute include paths everywhere)

robthebloke · 2019-12-09T04:12:59Z

lib/fileio/primUpdater.cpp

+{
+    try {
+        const MFnDagNode& dagNodeFn =
+            dynamic_cast<const MFnDagNode&>(depNodeFn);


If i pass an MFnSet to this method, dagNodeFn will be initialised to NULL right? You then next call:

const MDagPath dagPath = dagNodeFn.dagPath(&status);

Which would crash horribly. Dynamic casting a reference is not the safest way of programming if you ask me :/

dynamic_cast of a reference throws bad_cast when it fails, which is caught at line 48. Therefore, in that case, line 33 won't be reached.

robthebloke · 2019-12-11T03:33:50Z

lib/fileio/jobs/jobArgs.cpp

-#include "usdMaya/registryHelper.h"
-#include "usdMaya/shadingModeRegistry.h"
+#include "../registryHelper.h"
+#include "../shading/shadingModeRegistry.h"


Any reason you've decided to go for relative include paths everywhere? (I must confess, I'm not a big fan of this style)

I obviously agree with @robthebloke, given the stink I raised in #91.

We let it slide there, but in my opinion this all really needs to be cleaned up before this change gets merged in. Otherwise, we're significantly backsliding with this change.

robthebloke · 2019-12-11T03:42:36Z

lib/fileio/primUpdaterContext.cpp

+UsdMayaPrimUpdaterContext::GetUsdStage() const 
+{
+    return _stage;
+}


DSO exporting these getters is madness. make them inline.

robthebloke · 2019-12-11T03:44:43Z

lib/fileio/shading/shadingModeRegistry.h

@@ -60,28 +60,32 @@ class UsdMayaShadingModeRegistry : public TfWeakBase
 {
 public:

+    MAYAUSD_CORE_PUBLIC
    static UsdMayaShadingModeExporterCreator GetExporter(const TfToken& name) {
        return GetInstance()._GetExporter(name);
    }


Is this an inline function, or an exported one? Please remove the export macros. This change is a change for the worse imho.

Agreed. The export macros should not have been added for the functions defined here in the header.

robthebloke · 2019-12-11T04:02:31Z

lib/python/wrapUserTaggedAttribute.cpp

@@ -14,7 +14,7 @@
 // limitations under the License.
 //
 #include "pxr/pxr.h"
-#include "usdMaya/userTaggedAttribute.h"
+#include <mayaUsd/fileio/utils/userTaggedAttribute.h>


The rest of this PR turns all absolute paths into relative ones (bad choice if you ask me), so why does it differ here?

No reason, both are valid, since these changes were done manually I will assume it was an on the spot decision.

robthebloke · 2019-12-11T04:13:53Z

plugin/adsk/plugin/importTranslator.cpp

+        const MString& optionsString,
+        MPxFileTranslator::FileAccessMode  /*mode*/)
+{
+    std::string fileName(file.fullName().asChar());


std::string fileName(file.fullName().asChar(), file.fullName().length());

robthebloke · 2019-12-11T04:14:47Z

plugin/adsk/plugin/importTranslator.cpp

+                continue;
+            }
+
+            std::string argName(theOption[0].asChar());


std::string argName(theOption[0].asChar(), theOption[0].length());

robthebloke · 2019-12-11T04:15:32Z

plugin/adsk/plugin/importTranslator.cpp

+        MStringArray optionList;
+        MStringArray theOption;
+        optionsString.split(';', optionList);
+        for(int i=0; i<(int)optionList.length(); ++i) {


for(unsigned i=0, n = optionList.length(); i<n; ++i)

robthebloke · 2019-12-11T04:16:50Z

lib/utils/undoHelperCommand.cpp

+#include <maya/MFnPlugin.h>
+
+namespace {
+#define CMD_NAME "usdUndoHelperCmd"


const char* const CMD_NAME = "usdUndoHelperCmd";

Won't be doing this one, prevents C literal string concatenation, which is used in 5 places in this file. I agree that in general macros are to be avoided, but this one is a nice convenience.

robthebloke · 2019-12-11T04:17:32Z

lib/utils/undoHelperCommand.cpp

+UsdMayaUndoHelperCommand::initialize(MFnPlugin& plugin)
+{
+    // If we're already registered, do nothing.
+    if (_registrationCount++ > 0) {


This pattern is still a bug ;)

Armor plated this.

mattyjams

I think I've read through all of the changes at this point. I'm still working on adapting our internal build to deal with all of the moves, so I've not yet been able to run our tests on it. I'll be continuing to work on that next week.

There are some important problems in lib/CMakeLists.txt that need fixing.

I let the header include paths issue go in #91 but I really can't let it go here. Those need to be fixed.

I'd love to hear what others think about the mayaUsd.lib.<...> Python package path. Do we expect a lot of other stuff to end up under the mayaUsd namespace? Why the extra lib step? We have to touch up a lot of internal code to account for these moves as other might have to also, so I want to try to make sure we only need to do it once.

There are a handful of other minor cosmetic issues, and then a few cases where isolated, unrelated changes crept in. It would really be helpful if future PRs could be smaller and more focused, or at the very least be organized somewhat into a series of commits that clarify what's changing. This and #91 were painful to review on GitHub even if you're not on the other side of the Earth. 😛

lib/usd/CMakeLists.txt

mattyjams · 2019-12-14T00:17:45Z

lib/CMakeLists.txt

+    fileio/registryHelper.cpp
+    fileio/primReaderContext.cpp
+    fileio/primReaderArgs.cpp
+    fileio/primReader.cpp
+    fileio/primReaderRegistry.cpp
+    fileio/functorPrimReader.cpp
+    fileio/fallbackPrimReader.cpp
+    fileio/primWriterContext.cpp
+    fileio/primWriterArgs.cpp
+    fileio/primWriter.cpp
+    fileio/primWriterRegistry.cpp
+    fileio/functorPrimWriter.cpp
+    fileio/writeJobContext.cpp
+    fileio/instancedNodeWriter.cpp
+    fileio/transformWriter.cpp
+    fileio/shaderWriter.cpp
+    fileio/primUpdater.cpp
+    fileio/primUpdaterContext.cpp
+    fileio/primUpdaterRegistry.cpp


Would be nice to keep each of these subgroups ordered lexicographically, and have the groups themselves ordered that way as well

mattyjams · 2019-12-14T00:21:07Z

lib/CMakeLists.txt

    utils/stageCache.cpp
+    utils/query.cpp


utils/query.cpp is included twice in this block.

mattyjams · 2019-12-14T00:31:57Z

lib/CMakeLists.txt

    utils/query.h
    utils/util.h
    utils/utilFileSystem.h
+    utils/blockSceneModificationContext.h
    utils/stageCache.h
    utils/query.h


utils/query.h listed twice here too, though not as a result of this change.

lib/CMakeLists.txt

plugin/pxr/maya/lib/usdMaya/readJobWithSceneAssembly.cpp

plugin/pxr/maya/lib/usdMaya/readJobWithSceneAssembly.h

plugin/pxr/maya/lib/usdMaya/testenv/testUsdImportMesh.py

plugin/pxr/maya/lib/usdMaya/testenv/testUsdMayaDiagnosticDelegate.py

test/lib/ufe/testGroupCmd.py

ppt-adsk · 2019-12-20T21:40:39Z

Thanks all for the detailed comments. Back on January 6th, will pick things up at that point.

ppt-adsk · 2020-01-06T16:03:04Z

Hey all, thanks for the careful reviews. Correct me if I'm wrong, but I believe that two issues would be considered blocking:

Relative include path changes: we would like to make a targeted branch to remove relative include paths later on.
mayaUsd package structure: in refactoring_sandbox, we have two sub-packages, mayaUsd.lib for core code, and mayaUsd.ufe for Python bindings to UFE interface implementations.

All other suggested changes appear reasonable to me, and I think should be implemented. However, our goal is to minimize difficulties in merging back dev to refactoring_sandbox. This merge is needed to bring refactoring_sandbox and all branches created from it that are still in flight up to date, so that they can in turn be merged back to dev and ultimately clean out and close down refactoring_sandbox development.
Our hope is therefore to implement the suggestions made here in a later dev branch.

@mattyjams did you run your internal tests with this branch? Let us know the results.

mattyjams · 2020-01-07T19:47:56Z

lib/fileio/jobs/readJob.cpp

    mArgs(iArgs),
    mFileName(iFileName),
-    mPrimPath(iPrimPath),
    mVariants(iVariants),
+    mPrimPath(iPrimPath),
    mDagModifierUndo(),
    mDagModifierSeeded(false),
    mMayaRootDagPath()


Given the ordering or properties in the header, these initializers need to be re-ordered to avoid tripping over -Werror=reorder:

mArgs(iArgs), mFileName(iFileName), mVariants(iVariants), mMayaRootDagPath(), mPrimPath(iPrimPath), mDagModifierUndo(), mDagModifierSeeded(false)

mattyjams · 2020-01-07T19:50:21Z

lib/fileio/primUpdater.h

+    UsdMayaPrimUpdater() = default;
+    virtual ~UsdMayaPrimUpdater() = default;
+
+    MAYAUSD_CORE_PUBLIC


This export macro should be removed. Our build fails because of -Werror=attributes:

maya_open_source/maya-usd/lib/fileio/primUpdater.h:45:16: error: attribute ignored in declaration of 'enum class UsdMayaPrimUpdater::Supports' [-Werror=attributes]

mattyjams · 2020-01-08T02:56:41Z

lib/fileio/chaser/chaser.cpp

@@ -0,0 +1,42 @@
+//


This file was newly added when it should have been moved from plugin/pxr/maya/lib/usdMaya.

The file is still present in the old location and should be removed:
https://github.com/Autodesk/maya-usd/blob/Autodesk/ppt/refactoring_sandbox_to_dev_00/plugin/pxr/maya/lib/usdMaya/chaser.cpp

ppt-adsk · 2020-01-08T15:21:51Z

Hello again all,

After the discussion that this PR has triggered, and because of the fact that some of the problems raised cause build breaks with Pixar's internal builds, I've decided that it would be too much work to manage those changes that need to be done in this PR because they're blocking, versus those that could be postponed to a later branch as non-blocking. Since implementing all suggested changes was the intent all along, I will therefore do them immediately, except for include path changes. This should reduce e-mail discussions and tracking of which suggested changes were done in which branch, while keeping the merge back to refactoring_sandbox efficient (I hope). I will proceed with this approach immediately. Let me know if you still have questions.

lib/fileio/jobs/readJob.cpp

ppt-adsk · 2020-01-08T21:53:40Z

lib/fileio/primUpdater.h

@@ -41,7 +41,6 @@ class UsdMayaPrimUpdater
    UsdMayaPrimUpdater() = default;
    virtual ~UsdMayaPrimUpdater() = default;

-    MAYAUSD_CORE_PUBLIC


Symbol visibility attribute warning fix.

ppt-adsk · 2020-01-08T21:53:57Z

lib/render/vp2RenderDelegate/mesh.cpp

@@ -304,7 +304,7 @@ namespace {
        const VtIntArray &faceVertexCounts = topology.GetFaceVertexCounts();

        unsigned int numIndex = 0;
-        for (int i = 0; i < faceVertexCounts.size(); i++)
+        for (std::size_t i = 0; i < faceVertexCounts.size(); i++)


Signed / unsigned comparison warning fix.

ppt-adsk · 2020-01-08T21:54:14Z

lib/usd/translators/CMakeLists.txt

@@ -96,11 +96,6 @@ target_include_directories(${TRANSLATORS_PLUGIN}
    ${CMAKE_BINARY_DIR}
 )

-target_include_directories(${TRANSLATORS_PLUGIN}


api.h is not an include directory warning fix.

ppt-adsk · 2020-01-09T14:16:22Z

Unless otherwise noted, changes in commit
b10bf8e
are to fix unsigned vs. signed comparison warnings.

ppt-adsk · 2020-01-09T14:16:49Z

plugin/al/lib/AL_USDMaya/AL/usdmaya/CodeTimings.cpp

@@ -160,6 +160,8 @@ void Profiler::popTime()
  timespec endtime;
  #ifdef _WIN32
  while(clock_gettime(CLOCK_REALTIME_COARSE, &endtime) != 0) /* deliberately empty */;
+  #else
+  timespec_get(&endtime, TIME_UTC);


Fixes endtime uninitialized warning.

plugin/al/mayautils/AL/maya/utils/PluginTranslatorOptions.cpp

plugin/al/plugin/AL_USDMayaTestPlugin/test_translators_AnimationTranslator.cpp

plugin/al/usdmayautils/AL/usdmaya/utils/DgNodeHelper.cpp

mattyjams · 2020-01-09T19:44:00Z

lib/usd/translators/CMakeLists.txt

+    usdSkel
+    usdUtils
+    vt
+    ${Boost_PYTHON_LIBRARY}


I don't think Boost is required for this library?

…a namespace Pull request Autodesk#154 relocates a bunch of Python bindings from pxr.UsdMaya to mayaUsd.lib. To ease the transition for Python code that uses the bindings from the old package namespace, this change reintroduces the migrated names back into the pxr.UsdMaya namespace by importing them from mayaUsd.lib. It is wrapped in a try/except so that it will work both before and after PR Autodesk#154 is merged.

mattyjams

Thanks for addressing all of the notes @ppt-adsk!

With those changes, I've now been able to get this PR to build internally and pass all of our tests, so we're ready to take this change.

There were a few minor things I needed to touch up, one of which submitted as #199. The changes here end up being a breaking change for any Python code that used any of the UsdMaya functionality that moved to mayaUsd.lib, so #199 keeps old code working and allows some additional time to revisit it. The goal is definitely for that to be temporary though. I'm not sure how much others outside of Pixar are affected by this, but we have a fair bit of clean-up to do there.

The other two things had to do with how the Python bindings for mayaUsd.lib.mayaUsd and mayaUsd.ufe get built (making them consistent with other Python binding libs), and how the plugInfo.json files are arranged. I'll wait until the smoke clears from this PR going in (along with the backlog behind it) before I try to clean up our internal changes for those and put them up as PRs.

Thanks again!

murphyeoin

LGTM

Methods to query information about renderer plugins are now static

ppt-adsk added 2 commits December 4, 2019 10:45

Applied changes from refactoring_sandbox up to 5a5392c to dev.

9a5a9eb

Addressed internal code review comments.

fd049a5

ppt-adsk requested review from mattyjams, murphyeoin and kxl-adsk December 4, 2019 19:46

kxl-adsk approved these changes Dec 9, 2019

View reviewed changes

robthebloke reviewed Dec 11, 2019

View reviewed changes

mattyjams suggested changes Dec 14, 2019

View reviewed changes

ppt-adsk mentioned this pull request Jan 6, 2020

modernize and clean up mayaUsd and mayaUsdPlugin targets! #191

Closed

This was referenced Jan 6, 2020

[pxr] when loading the pixar plugin, trigger an immediate fail #193

Merged

change HdxPickHit "ndcDepth" property to "normalizedDepth" for core USD after 19.11 #186

Merged

[pxr] fix for export of namespaced point instancers #184

Merged

mattyjams reviewed Jan 7, 2020

View reviewed changes

mattyjams reviewed Jan 8, 2020

View reviewed changes

ppt-adsk added 2 commits January 8, 2020 13:48

Core changes to compile with -Werror

f243a41

AL plugin changes to compile with -Werror

b10bf8e

ppt-adsk commented Jan 8, 2020

View reviewed changes

ppt-adsk commented Jan 9, 2020

View reviewed changes

mattyjams reviewed Jan 9, 2020

View reviewed changes

ppt-adsk added 4 commits January 9, 2020 13:10

Addressed code review comments.

1940ac1

Addressed code review comments part 2.

5f83412

Fixes for compilation on macOS with warnings as errors.

c55c976

Addressed code review comments part 3.

3e8c1a1

mattyjams mentioned this pull request Jan 14, 2020

reintroduce migrated UsdMaya Python bindings back into the pxr.UsdMaya namespace #199

Merged

mattyjams approved these changes Jan 14, 2020

View reviewed changes

robthebloke approved these changes Jan 14, 2020

View reviewed changes

murphyeoin approved these changes Jan 14, 2020

View reviewed changes

ppt-adsk merged commit 0e21c26 into dev Jan 14, 2020

ppt-adsk deleted the Autodesk/ppt/refactoring_sandbox_to_dev_00 branch January 14, 2020 14:45

asluk pushed a commit to asluk/maya-usd that referenced this pull request Jan 18, 2020

Merge pull request Autodesk#154 from LumaPictures/pr/no_dummy_engine

ffca007

Methods to query information about renderer plugins are now static

pmolodo pushed a commit to LumaPictures/maya-usd that referenced this pull request Jan 22, 2020

MAYA-101127 Run transparency test through opacity array (Autodesk#154)

4794218

Autodesk/ppt/refactoring sandbox to dev 00 #154

Autodesk/ppt/refactoring sandbox to dev 00 #154

Conversation

ppt-adsk commented Dec 4, 2019

ppt-adsk commented Dec 4, 2019

robthebloke 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

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

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

ppt-adsk commented Dec 20, 2019

ppt-adsk commented Jan 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppt-adsk commented Jan 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppt-adsk commented Jan 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattyjams left a comment

Choose a reason for hiding this comment

murphyeoin left a comment

Choose a reason for hiding this comment