Hide orphaned pulled nodes on ancestor structure change. #2576
Conversation
| @@ -78,6 +80,53 @@ class PrimUpdaterManager : public PXR_NS::TfWeakBase | |||
| bool hasPulledPrims() const { return _hasPulledPrims; } | |||
|
|
|||
| private: | |||
| struct VariantSelection | |||
There was a problem hiding this comment.
To detect when a change in variant should hide or show a pulled hierarchy, we save the complete list of variant sets and their variant selection along the ancestor chain. If this variant configuration matches, we show the pulled hierarchy, if it doesn't, we hide the pulled hierarchy. We cannot rely only on the path, because the path may be identical in two different variants, e.g. all variants of a cup might have a top-level prim called "Cup", so that the path to the selected variant is the same.
| void endManagePulledPrims(); | ||
|
|
||
| // Member function to access private nested classes. | ||
| static std::list<VariantSetDescriptor> variantSetDescriptors(const Ufe::Path& path); |
There was a problem hiding this comment.
Minor: having a class named VariantSetDescriptors and a function named variantSetDescriptors is a bit confusing. Maybe a "get"or "compute" prefix on the function?
| // descendants of the argument path. The trie node that corresponds to | ||
| // the added path is the starting point. It may be an internal node, | ||
| // without data. | ||
| auto ancestorNode = _pulledPrims.node(op.path); |
There was a problem hiding this comment.
I"m not going to try to figure it out now, but a given prim could be hidden both because one of its variant change and it got deleted afterwards. So I think restoring visibility (here and for variant switch below in case multiple variants were switched...) need to check that all original causes why it was hidden are now gone.
There was a problem hiding this comment.
That's a great catch. I think that scope has grown enough in this pull request that I want to do this one separately. Entered as MAYA-125101.
| case Ufe::SceneChanged::SubtreeInvalidate: { | ||
| // On subtree invalidate, the scene item itself has not had a structure | ||
| // change, but its children have changed. There are two cases: | ||
| // - the node has children: from a variant switch, or from a payload |
There was a problem hiding this comment.
Ugh, unfortunate that we need to have such intimate knowledge of cause-and-effect of these notifications.
| } | ||
| } | ||
| if (!foundChild) { | ||
| // Following a subtree invalidate, if none of the now-valid |
There was a problem hiding this comment.
I wonder if a new variant could have some ancestor in the trie, just by happenstance because another pulled object happens to have the same ancestor? I'll be honest and I'm a bit foggy on what scenarios are possible or not.
There was a problem hiding this comment.
Not sure how this would be possible, given different pull paths.
| const bool variantSetsMatch | ||
| = (trieNode->data().variantSetDescriptors == variantSetDescriptors(ufePath.pop())); | ||
| const bool visibility = (pulledNode && variantSetsMatch); | ||
| TF_VERIFY(setVisibilityPlug(trieNode, visibility)); |
There was a problem hiding this comment.
Again, there maybe multiple causes of invisiblity, so we should check that all have been restored before making a node visible.
There was a problem hiding this comment.
Don't think so. If the variant configuration matches, then we're good to go. I think it's the use of unconditional visibility setting by object add and object delete that is incorrect, because it ignores variant configuration.
| @@ -0,0 +1,374 @@ | |||
| // | |||
There was a problem hiding this comment.
Factored out from primUpdaterManager, but with undo / redo fixes.
| pulledPrims().add(pulledPath, PullVariantInfo(pullParentPath, vsd)); | ||
| } | ||
|
|
||
| OrphanedNodesManager::Memento OrphanedNodesManager::remove(const Ufe::Path& pulledPath) |
There was a problem hiding this comment.
Now returning the previous Trie of pull info on remove, for undo.
| public: | ||
| typedef std::shared_ptr<OrphanedNodesManager> Ptr; | ||
|
|
||
| class Memento { |
There was a problem hiding this comment.
Opaque orphaned node manager state, that only it can create and restore (see Memento Pattern). The orphaned node manager is the originator of the Memento.
| @@ -828,22 +820,73 @@ bool pushCustomize( | |||
| class PushPullScope | |||
| { | |||
| public: | |||
| PushPullScope(bool& controlingFlag) | |||
| PushPullScope(bool& controllingFlag) | |||
There was a problem hiding this comment.
Couldn't help myself but to fix the spelling mistake.
| bool* _controllingFlag { nullptr }; | ||
| }; | ||
|
|
||
| class RemovePullPathUndoItem : public MayaUsd::OpUndoItem |
There was a problem hiding this comment.
Undo item for removal of a pull path. Add is not the undo of remove, because on add, the USD hierarchy is present. When a node is orphaned, the USD hierarchy is either changed or absent, so add will fail. Simply save the previous state of the pull info on remove, so that it can be undone even with orphaned nodes.
| @@ -1013,7 +1057,7 @@ bool PrimUpdaterManager::mergeToUsd( | |||
| } | |||
|
|
|||
| if (!isCopy) { | |||
| if (!TF_VERIFY(removePullParent(pullParentPath))) { | |||
| if (!TF_VERIFY(removePullParent(pullParentPath, pulledPath))) { | |||
There was a problem hiding this comment.
At the point where removePullParent is called, the pulled node itself is gone, so the pull infomation cannot be retrieved from it. We simply pass it in.
| @@ -1139,14 +1183,14 @@ bool PrimUpdaterManager::canEditAsMaya(const Ufe::Path& path) const | |||
|
|
|||
| bool PrimUpdaterManager::discardEdits(const MDagPath& dagPath) | |||
| { | |||
| Ufe::Path primPath; | |||
| if (!PXR_NS::PrimUpdaterManager::readPullInformation(dagPath, primPath)) | |||
| Ufe::Path pulledPath; | |||
There was a problem hiding this comment.
Standardize naming, for better understanding.
| handleOp(sceneNotification); | ||
| #else | ||
| // UFE v3: convert to op ourselves. Only convert supported |
There was a problem hiding this comment.
Maya 2023 UFE v3 did not have nice notification to notification op conversion.
| } | ||
| } | ||
|
|
||
| void OrphanedNodesManager::handleOp(const Ufe::SceneCompositeNotification::Op& op) | ||
| { | ||
| switch (op.opType) { | ||
| case Ufe::SceneChanged::ObjectAdd: { | ||
| case Ufe::SceneCompositeNotification::ObjectAdd: { |
There was a problem hiding this comment.
In UFE v3 the enumerant was in the SceneCompositeNotification derived class, in UFE v4 it's move to the base class, but the derived class scope still works, so using the derived class is UFE v3/v4 compatible, a requirement for 2023 compatibility.
| @@ -217,35 +236,28 @@ void OrphanedNodesManager::handleOp(const Ufe::SceneCompositeNotification::Op& o | |||
| } | |||
| } | |||
| } break; | |||
| default: { | |||
There was a problem hiding this comment.
UNIX (Linux, macOS) warning as error fix.
|
Still not 2023 compatible: missing crucial Ufe::TrieNode::childrenComponents() method, which will have to be added to UFE v3 for 2023 Update 3 (and it can be, as it's a non-virtual). |
No description provided.