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

Added implementation for new UFE unparent interface (MAYA-105276) #665

Merged
merged 3 commits into from
Jul 30, 2020

Conversation

ppt-adsk
Copy link
Collaborator

This pull request implements the Ufe::Hierarchy::defaultParent() method. This is used when unparenting, a.k.a parent -world. In this implementation, the default parent of USD nodes is their proxy shape, so that calling parent -w on them will place them as a child of the corresponding USD stage pseudo-root.

@ppt-adsk
Copy link
Collaborator Author

Hi @pilarmolinalopez , I hope this is of some interest to you and your team.

@pilarmolinalopez
Copy link
Contributor

Thanks @ppt-adsk ! I'll take a look.

Copy link
Contributor

@fowlertADSK fowlertADSK left a comment

Choose a reason for hiding this comment

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

Can we move the insertChild functions next to insertChildCmd in each of the .cpp files instead of having them by themselves at the bottom?

The defaultParent part looks good to me.

Only thing I’m starting to wonder about is the usefulness of the non-cmd functions that implement UFE interfaces but that’s a separate conversation. This review isn't the place to have that conversation, but for someone taking a look at this review it will look odd that "insertChild" can just return null. I believe that this is because everyone goes through the Cmd interface and the Cmd does not necessarily have to use the non-Cmd function as its implementation correct?

@ppt-adsk
Copy link
Collaborator Author

Can we move the insertChild functions next to insertChildCmd in each of the .cpp files instead of having them by themselves at the bottom?

Sounds good, will do. Will wait for suggestions from Pilar and fix eveything in a single commit.

Only thing I’m starting to wonder about is the usefulness of the non-cmd functions that implement UFE interfaces but that’s a separate conversation. This review isn't the place to have that conversation, but for someone taking a look at this review it will look odd that "insertChild" can just return null. I believe that this is because everyone goes through the Cmd interface and the Cmd does not necessarily have to use the non-Cmd function as its implementation correct?

The non-cmd versions are there because if you're writing code that is non-interactive (and therefore does not need undo / redo), it's really awkward to call the cmd version of the interface, call execute on the resulting cmd object, then ask the cmd object for the result of the call, when all you want is to call e.g. insertedChild = parent.insertChild(child, position);

What has been suggested before (and that I like) is that the non-cmd versions be virtual, but have a default implementation right in UFE that does exactly what I mentioned, create the command object, call execute on it, and return the result. That way, if the run-time implementer doesn't want to bother with details, they can just implement the cmd version and that is sufficient.

But as you say, this is a conversation for a later branch.

Copy link
Contributor

@pilarmolinalopez pilarmolinalopez 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 to me! thanks @ppt-adsk !

@kxl-adsk
Copy link

@ppt-adsk I'm marking this PR as do-not-merge-yet since there are changes you still want to make. Please remove that label when PR is ready for merge.

@kxl-adsk kxl-adsk added the do-not-merge-yet Development is not finished, PR not ready for merge label Jul 24, 2020
@ppt-adsk ppt-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 Jul 29, 2020
@ppt-adsk ppt-adsk changed the title Tremblp/maya 105276/unparent implementation Added implementation for new UFE unparent interface (MAYA-105276) Jul 29, 2020
@kxl-adsk kxl-adsk merged commit a87a919 into dev Jul 30, 2020
@kxl-adsk kxl-adsk deleted the tremblp/MAYA-105276/unparent_implementation branch July 30, 2020 14:39
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants