Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAYA-125039 handle rename and reparent in the orphaned nodes manager #2690

Merged
merged 7 commits into from
Nov 23, 2022

Conversation

pierrebai-adsk
Copy link
Collaborator

  • Handle the reparent and rename UFE notifications.
  • Rename the orphan manager trie nodes.
  • Rename the variant descriptors.
  • Rewrite the pull info on the USD node to reflect the new UFE paths.
  • Added util code to print the orphan pulled info, useful when debugging.
  • Add two unit tests

- Handle the reparent and rename UFE notifications.
- Rename the orphan manager trie nodes.
- Rename the variant decriptors.
- Rewrite the pull info on the USD node to reflect the new UFE paths.
- Added util code to print the orphan pulled info, useful when debugging.
- Add two unit tests
@pierrebai-adsk pierrebai-adsk added do-not-merge-yet Development is not finished, PR not ready for merge adsk Related to Autodesk plugin labels Oct 28, 2022
@ppt-adsk ppt-adsk self-requested a review November 2, 2022 20:23
Copy link
Collaborator

@ppt-adsk ppt-adsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks pretty good to me! Thanks for the great work. Just not too sure about the additional information passed around into the Trie methods in the orphaned nodes manager. Perhaps something to discuss live.


UFE_NS_DEF { class Path; }

PXR_NAMESPACE_OPEN_SCOPE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a brand new file, can we try not putting the new code in the Pixar namespace, and put it into the MAYAUSD_NS_DEF namespace instead? Don't think it would be too much work, right? This is what I did for the orphaned nodes manager.


namespace MAYAUSD_NS_DEF {

void OrphanedNodesManagerPullInfoToText(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function name should not be capitalized.

@@ -0,0 +1,57 @@
//
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this code used or tested in this pull request?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is part of this #2684 PR, on which this is baed. The order the PR will go in is:

#2682
#2684
and this one last.

The code is tested (somewhat indirectly) by the tests named testHideOnNestedVariantSwitchOnReload, which verifies that the orphan nodes manager state is correct when Maya scene is cleared and then a scene with an edited object is reloaded.

MGlobal::executeCommand(cmd, result, display, undoable);

return result != 0;
return depNode.hasAttribute(attrName);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these changes. Might have been nice to put them up as a separate pull request, just to make this one lighter.

if (!trieNode)
return;

Ufe::TrieNode<PullVariantInfo>& node = *trieNode;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit-pick: should be const ref. And could be const auto& if you like :)


namespace {

Ufe::Path trieNodeToPullePrimUfePath(Ufe::TrieNode<PullVariantInfo>::Ptr trieNode)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in function name: should be "trieNodeToPulledPrimUfePath".

Also should pass the trie node ptr by const ref, to avoid needless inc / dec ref count of temporary object.

bool visibility)
namespace ufe {

extern Ufe::Rtid g_MayaRtid;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awkward, this is an implementation detail. Should use functions from Global.h.

lib/mayaUsd/fileio/orphanedNodesManager.cpp Outdated Show resolved Hide resolved
with mayaUsd.lib.OpUndoItemList():
aMayaItem = ufe.GlobalSelection.get().front()
aMayaPath = aMayaItem.path()
self.assertTrue(mayaUsd.lib.PrimUpdaterManager.mergeToUsd(ufe.PathString.string(aMayaPath)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent.


validateEditAsMayaMetadata()

# Note: the parent command changes the selection, to preserve and restore it.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"to preserve" --> "so preserve".

@pierrebai-adsk
Copy link
Collaborator Author

Ah, I had 3 PR, each one built on top of the previous, and this one was the last of the three. So, it contains the code of all three. I understand that by reviewing this one you review all three at the same time, which saves time, but should I still do 3 PR or just this massive one?

@seando-adsk
Copy link
Collaborator

@pierrebai-adsk Different JIRA items each require their own PR. We should not mix work from different tasks together.

@pierrebai-adsk
Copy link
Collaborator Author

Half the review comments have been addressed in #2684 since the code was introduced in that PR. The other half were about code that is only in this PR and will be updated once I merge that PR into this one (and fix any conflict.)

@pierrebai-adsk pierrebai-adsk marked this pull request as draft November 7, 2022 18:12
- Fix typos.
- Make a trie node ref constant.
@pierrebai-adsk pierrebai-adsk marked this pull request as ready for review November 21, 2022 14:09
@pierrebai-adsk pierrebai-adsk added ready-for-merge Development process is finished, PR is ready for merge and removed do-not-merge-yet Development is not finished, PR not ready for merge labels Nov 21, 2022
@seando-adsk seando-adsk merged commit c69365c into dev Nov 23, 2022
@seando-adsk seando-adsk deleted the bailp/MAYA-125039/rename-vs-orphaned-nodes branch November 23, 2022 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adsk Related to Autodesk plugin ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants