Tremblp/maya 102361/rename fixes #237
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,43 @@ | |
| // | ||
|
|
||
| #include "UsdStageMap.h" | ||
| #include "Utils.h" | ||
|
|
||
| #include <maya/MFnDagNode.h> | ||
|
|
||
| #include <cassert> | ||
|
|
||
| namespace { | ||
|
|
||
| MObjectHandle proxyShapeHandle(const Ufe::Path& path) | ||
| { | ||
| // Get the MObjectHandle from the tail of the MDagPath. Remove the leading | ||
| // '|world' component. | ||
| auto noWorld = path.popHead().string(); | ||
| auto dagPath = MayaUsd::ufe::nameToDagPath(noWorld); | ||
| MObjectHandle handle(dagPath.node()); | ||
| if (!handle.isValid()) { | ||
| TF_CODING_ERROR("'%s' is not a path to a proxy shape node.", | ||
| noWorld.c_str()); | ||
| } | ||
| 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 There was a problem hiding this comment. For some additional context, the scene file for the There was a problem hiding this comment. Created an issue describing the problem, and some areas that would need to be worked on: |
||
| Ufe::Path firstPath(const MObjectHandle& handle) | ||
| { | ||
| if (!TF_VERIFY(handle.isValid(), | ||
| "Cannot get path from invalid object handle")) { | ||
| return Ufe::Path(); | ||
| } | ||
|
|
||
| MDagPath dagPath; | ||
| auto status = MFnDagNode(handle.object()).getPath(dagPath); | ||
| CHECK_MSTATUS(status); | ||
| return MayaUsd::ufe::dagPathToUfe(dagPath); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| MAYAUSD_NS_DEF { | ||
| namespace ufe { | ||
|
|
@@ -31,32 +68,57 @@ 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 = | ||
| #ifdef UFE_V0_2_6_FEATURES_AVAILABLE | ||
| path.nbSegments(); | ||
| #else | ||
| path.getSegments().size(); | ||
| #endif | ||
| if (nbSegments != 1) { | ||
| TF_CODING_ERROR("A proxy shape node path can have only one segment, path '%s' has %lu", path.string().c_str(), nbSegments); | ||
| return; | ||
| } | ||
|
|
||
| // Convert the tail of the UFE path to an MObjectHandle. | ||
| auto proxyShape = proxyShapeHandle(path); | ||
| if (!proxyShape.isValid()) { | ||
| return; | ||
| } | ||
|
|
||
| // Could get the stage from the proxy shape object in the stage() method, | ||
| // but since it's given here, simply store it. | ||
| fObjectToStage[proxyShape] = stage; | ||
| fStageToObject[stage] = proxyShape; | ||
| } | ||
|
|
||
| UsdStageWeakPtr UsdStageMap::stage(const Ufe::Path& path) const | ||
| { | ||
| auto proxyShape = proxyShapeHandle(path); | ||
| if (!proxyShape.isValid()) { | ||
| return nullptr; | ||
| } | ||
|
|
||
| // A stage is bound to a single Dag proxy shape. | ||
| auto iter = fObjectToStage.find(proxyShape); | ||
| if (iter != std::end(fObjectToStage)) | ||
| return iter->second; | ||
| return nullptr; | ||
| } | ||
|
|
||
| Ufe::Path UsdStageMap::path(UsdStageWeakPtr stage) const | ||
| { | ||
| // A stage is bound to a single Dag proxy shape. | ||
| auto iter = fStageToObject.find(stage); | ||
| if (iter != std::end(fStageToObject)) | ||
| return firstPath(iter->second); | ||
| return Ufe::Path(); | ||
| } | ||
|
|
||
| void UsdStageMap::clear() | ||
| { | ||
| fObjectToStage.clear(); | ||
| fStageToObject.clear(); | ||
| } | ||
|
|
||
| } // namespace ufe | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,22 +22,37 @@ | |
| #include <pxr/usd/usd/stage.h> | ||
| #include <pxr/base/tf/hash.h> | ||
|
|
||
| #include <maya/MObjectHandle.h> | ||
|
|
||
| #include <unordered_map> | ||
|
|
||
| // 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()); | ||
| } | ||
| }; | ||
| } | ||
|
Comment on lines
+29
to
+36
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. |
||
|
|
||
| PXR_NAMESPACE_USING_DIRECTIVE | ||
|
|
||
| MAYAUSD_NS_DEF { | ||
| namespace ufe { | ||
|
|
||
| //! \brief USD Stage Map | ||
| /*! | ||
| 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 |
||
| nothing in the data model prevents it). To support renaming and repathing, | ||
| we store an MObjectHandle in the maps, which is invariant to renaming and | ||
| repathing, and compute the path on access. This is slower than a scheme | ||
| where we cache using the Ufe::Path, but such a cache must be refreshed on | ||
| rename and repath, which is non-trivial, since there is no guarantee on the | ||
| order of notification of Ufe observers. An earlier implementation with | ||
| rename observation had the Maya Outliner (which observes rename) access the | ||
| UsdStageMap on rename before the UsdStageMap had been updated. | ||
| */ | ||
| class MAYAUSD_CORE_PUBLIC UsdStageMap | ||
| { | ||
|
|
@@ -64,8 +79,10 @@ class MAYAUSD_CORE_PUBLIC UsdStageMap | |
|
|
||
| private: | ||
| // We keep two maps for fast lookup when there are many proxy shapes. | ||
| using ObjectToStage = std::unordered_map<MObjectHandle, UsdStageWeakPtr>; | ||
| using StageToObject = TfHashMap<UsdStageWeakPtr, MObjectHandle, TfHash>; | ||
| ObjectToStage fObjectToStage; | ||
| StageToObject fStageToObject; | ||
|
|
||
| }; // UsdStageMap | ||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're actually adding a usage of
Ufe::Rtidat 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.