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-123720 - As a user, I'd like Display Layer membership to survive the 'Edit As Maya Data' workflow #2628

Merged
merged 10 commits into from
Oct 14, 2022

Conversation

vlasovi
Copy link
Collaborator

@vlasovi vlasovi commented Sep 27, 2022

No description provided.

@vlasovi vlasovi requested a review from seando-adsk September 28, 2022 16:15
@@ -231,5 +231,25 @@ Ufe::Selection recreateDescendants(const Ufe::Selection& src, const Ufe::Path& f
MAYAUSD_CORE_PUBLIC
std::vector<std::string> splitString(const std::string& str, const std::string& separators);

class PullExtras
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need two different classes? How about a single class that can handle both cases (Maya->USD and USD->Maya). Just have two sections of methods for each case. And I guess you were thinking that in the future we could add other types of extras (now we have DisplayLayers)? The names "Pull/Push" are specific to your operation, but we'll add this for duplicate usd prim. Can we have a generic name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@seando-adsk The two classes share neither data members nor methods, so I don't see a reason to merge them. As for renaming, would ReplicateExtrasToUSD/ReplicateExtrasFromUSD wouldwork for you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes those names are better.

Comment on lines 237 to 238
void initRecursive(Ufe::SceneItem::Ptr);
void processItem(const Ufe::Path& path, const MObject& mayaObject);
Copy link
Collaborator

Choose a reason for hiding this comment

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

These methods need comments as to what they are doing.

Copy link
Collaborator

@seando-adsk seando-adsk left a comment

Choose a reason for hiding this comment

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

As a general comment you need some method documentation and unit tests.

seando-adsk
seando-adsk previously approved these changes Sep 29, 2022
Copy link
Collaborator

@seando-adsk seando-adsk left a comment

Choose a reason for hiding this comment

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

Looks good thanks.

@vlasovi vlasovi added the ready-for-merge Development process is finished, PR is ready for merge label Sep 29, 2022
@seando-adsk
Copy link
Collaborator

I'll wait until @ppt-adsk has had a chance to review this before merging.

@seando-adsk seando-adsk added workflows Related to in-context workflows and removed ready-for-merge Development process is finished, PR is ready for merge labels Oct 7, 2022
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.

Great work, see my suggestion and let me know if it makes sense.

@@ -67,6 +68,9 @@ class UsdMayaPrimUpdaterContext
MAYAUSD_CORE_PUBLIC
MDagPath MapSdfPathToDagPath(const SdfPath& sdfPath) const;

mutable MayaUsd::ufe::ReplicateExtrasFromUSD _pullExtras;
Copy link
Collaborator

Choose a reason for hiding this comment

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

These mutable data members feel uncomfortable. Does the following make any sense:

  • initRecursive() writes into the map. Can we transform it into a static creator method?
  • processItem() and finalize() read from the map. They can be made const methods.
    That way, you can pass in the ReplicateExtras objects as const into the context, make them const data members, and provide const accessors to them, keeping the PrimUpdaterContext read-only.

Hope I didn't skip over some crucial detail...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Your plan will not fully work because ReplicateExtrasToUSD::processItem writes into the map.

Copy link
Collaborator

Choose a reason for hiding this comment

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

O.K., FromUSD works, ToUSD does not :) Let me think about it some more.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here's a tweak to my suggestion to get it to work: everything as per above, but ReplicateExtrasToUSD::processItem() does a const cast if it needs to write into the map. Alternately, the map is mutable. I think this is acceptable, as the fact that processItem() is writing into the map is an invisible implementation detail. In that way, the prim updater context can appear to be conceptually read-only. What do you think?

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.

Thanks for the changes, looks good!

@vlasovi vlasovi added the ready-for-merge Development process is finished, PR is ready for merge label Oct 12, 2022
@neilh-adsk neilh-adsk merged commit 1cc2962 into dev Oct 14, 2022
@neilh-adsk neilh-adsk deleted the vlasovi/MAYA-123720 branch October 14, 2022 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge Development process is finished, PR is ready for merge workflows Related to in-context workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants