Tremblp/maya 102361/rename fixes #237
Conversation
ppt-adsk
added
the
do-not-merge-yet
| prevents it). | ||
| Two-way map of proxy shape UFE path to corresponding stage. | ||
|
|
||
| We will assume that a USD proxy shape will not be instanced (even though |
There was a problem hiding this comment.
Previous USD stage map was path-based, which was impossible to maintain in the presence of path changes through renaming. Simply use the proxy shape MObject instead.
There was a problem hiding this comment.
Again, slightly concerned about the no instancing assumption, but maybe I'm misunderstanding.
There was a problem hiding this comment.
No, you're not misunderstanding... As per other comment entered as
#256
ppt-adsk
removed
the
do-not-merge-yet
| @@ -22,7 +22,6 @@ | |||
| #include "UsdTransform3dHandler.h" | |||
| #include "UsdSceneItemOpsHandler.h" | |||
|
|
|||
| #include <ufe/rtid.h> | |||
There was a problem hiding this comment.
Looks like we're actually adding a usage of Ufe::Rtid at line 130, so I'm not sure why we're taking this include out? I know it was added to the header, but since there's usage in both files, I'd prefer to see the include in both files.
There was a problem hiding this comment.
Not a huge deal, to be sure, but isn't that violating Don't Repeat Yourself? If maintenance removes the use of Ufe::Rtid, I'd now have two places to remove the include of ufe/rtid.h.
| return handle; | ||
| } | ||
|
|
||
| // Assuming proxy shape nodes cannot be instanced, simply return the first path. |
There was a problem hiding this comment.
I'll admit I'm not sure I fully understand the context here, but is this a safe assumption? In our pipeline, we definitely do instance pxrUsdReferenceAssembly/pxrUsdProxyShape nodes using either nParticles or MASH.
There was a problem hiding this comment.
For some additional context, the scene file for the testPxrUsdMayaGLInstancerDraw unit test is an example of an instancer setup:
https://github.com/Autodesk/maya-usd/blob/dev/plugin/pxr/maya/lib/pxrUsdMayaGL/testenv/InstancerDrawTest/InstancerDrawTest.ma
https://github.com/Autodesk/maya-usd/blob/dev/plugin/pxr/maya/lib/pxrUsdMayaGL/testenv/testPxrUsdMayaGLInstancerDraw.py
There was a problem hiding this comment.
Created an issue describing the problem, and some areas that would need to be worked on:
#256
| // Allow for use of MObjectHandle with std::unordered_map. | ||
| namespace std { | ||
| template <> struct hash<MObjectHandle> { | ||
| std::size_t operator()(const MObjectHandle& handle) const { | ||
| return static_cast<std::size_t>(handle.hashCode()); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
This would be a great candidate for the kind of Maya-as-the-only-dependency utilities library that Animal Logic has been advocating for. We actually had something for this in usdMaya (now migrated to mayaUsd) as well:
Line 105 in 31e6f34
There's nothing USD or UFE related about either implementation.
| prevents it). | ||
| Two-way map of proxy shape UFE path to corresponding stage. | ||
|
|
||
| We will assume that a USD proxy shape will not be instanced (even though |
There was a problem hiding this comment.
Again, slightly concerned about the no instancing assumption, but maybe I'm misunderstanding.
|
@mattyjams you already approved the change, but there are new commits implementing your code review suggestions. Can you re-approve once reviewed (or simply let me know that you are done with the review). |
|
PS: |
lib/ufe/UsdStageMap.cpp
Outdated
| fPathToStage[path] = stage; | ||
| fStageToPath[stage] = path; | ||
| // We expect a path to the proxy shape node, therefore a single segment. | ||
| auto nbSegments = path.nbSegments(); |
There was a problem hiding this comment.
One minor thing that I just noticed, It doesn't look like we are guarding UsdStageMap.cpp against UFE1.0.
See
Line 214 in f53f29c
There was a problem hiding this comment.
Yup, I think that's fine: UsdStageMap is required for proper UFE v1 use, and it doesn't depend on any UFE v2 features, so it should only be protected for UFE compilation, which it currently is, not against UFE v2.
There was a problem hiding this comment.
Makes sense. Thanks
There was a problem hiding this comment.
As discussed, completely missed your point, great catch! Fixed in
e508bdb
HamedSabri-adsk
added
the
do-not-merge-yet
HamedSabri-adsk
removed
the
do-not-merge-yet
lib/ufe/UsdStageMap.cpp
Outdated
| @@ -69,7 +69,12 @@ UsdStageMap g_StageMap; | |||
| void UsdStageMap::addItem(const Ufe::Path& path, UsdStageWeakPtr stage) | |||
| { | |||
| // We expect a path to the proxy shape node, therefore a single segment. | |||
| auto nbSegments = path.nbSegments(); | |||
| auto nbSegments = | |||
| #ifdef UFE_V2_FEATURES_AVAILABLE | |||
There was a problem hiding this comment.
lgtm. Thanks Pierre.
No description provided.