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

EMSUSD-1000 - Implement new Hierarchy Cmd. #3597

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

alicedegirolamo
Copy link
Collaborator

Notes: Implementing the new cmd added in UFE PR.
Marking this as a Draft PR as we will need it for next Maya release since it is breaking the Hierarchy interface by adding a new command. Pushing it to not lose progress and be ready to jump on it for the next release.
related JIRA ticket

The new command will allow us to not drop connected items in the Outliner. Please refer to the related story ticket for more details.

@alicedegirolamo alicedegirolamo self-assigned this Feb 6, 2024
Ufe::InsertChildCommand::Ptr
UsdHierarchy::appendChildVerifyRestrictionsCmd(const Ufe::SceneItem::Ptr& child)
{
const auto childCmd = appendChildCmd(child);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A better and cleaner solution would be to create a new cmd that inherits from UsdUndoInsertChildCommand and in the new constructor check the restrictions and throw the exception.

//! Check if the src and dst attributes are connected.
//! \return True, if they are connected.
MAYAUSD_CORE_PUBLIC
bool isConnected(const PXR_NS::UsdAttribute& srcUsdAttr, const PXR_NS::UsdAttribute& dstUsdAttr);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moving it to usdUfe/ufe as I see this function doesn't need the maya and mayaUsd .h included in lib/mayaUsd/ufe/Utils.h

//! Check if the usdItem is connected (i.e. if there are in or out connections).
//! \return True, if it is connected.
USDUFE_PUBLIC
bool isConnected(const UsdSceneItem::Ptr& usdItem);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moving out this function from LookdevXUsd

const auto childCmd = appendChildCmd(child);

if (childCmd && isConnected(downcast(child))) {
throw std::runtime_error("The node you're trying to move has connections.");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should update this message according to the updated and approved UI linked in the related story

Comment on lines +259 to +261
if (childCmd && isConnected(downcast(child))) {
throw std::runtime_error("The node you're trying to move has connections.");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

In your Ufe PR it states "Returns a null pointer if the restrictions are not observed." - but here you are throwing. It should be one of the. Either return nullptr here or in Ufe change the doc to state that it will throw runtime_error if the restrictions are not observed.

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.

I have a general comment about the need for this new method. The JIRA item mentions "modify Outliner code to not allow dropped items if they are connected". Where is this new method called from? Wouldn't users be able to override the restriction by simply calling appendChildCmd? Therefore was any thought given to adding the restriction to that existing method?

@seando-adsk seando-adsk added the do-not-merge-yet Development is not finished, PR not ready for merge label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge-yet Development is not finished, PR not ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants