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-129061 - Refactor material commands. #3056

Merged
merged 5 commits into from
May 15, 2023

Conversation

frohnej-adsk
Copy link
Collaborator

In UsdUndoMaterialCommands, factor out the code related to materials scopes into separate commands and functions. This will allow reusing them and is required by future work (MAYA-128151).

Also, make UsdUndoAddNewPrimCommand derive from SceneItemResultUndoableCommand. This allows UFE clients to access the newly created prim by calling SceneItemResultUndoableCommand::sceneItem().

Copy link
Collaborator

@JGamache-autodesk JGamache-autodesk left a comment

Choose a reason for hiding this comment

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

Well done! This will help.

seando-adsk
seando-adsk previously approved these changes May 4, 2023
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.

Since this was just refactoring existing code I assume there is no new unit test(s) that are needed?

Copy link
Collaborator Author

@frohnej-adsk frohnej-adsk left a comment

Choose a reason for hiding this comment

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

Yes, the refactored code is covered by the existing tests.

UsdUndoCreateMaterialsScopeCommand is now separated out, so if we want that, we could also test it independently by creating a Python binding for it. However, it is used by UsdUndoAssignNewMaterialCommand and, thus, already covered by e.g. this test.

One addition that is not used or tested yet is UsdUndoAddNewPrimCommand::sceneItem().


Ufe::SceneItem::Ptr UsdUndoAddNewPrimCommand::sceneItem() const
{
return Ufe::Hierarchy::createItem(newUfePath());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This method is currently not used or tested. However, I'm not sure if it can easily be tested right now, since UsdUndoAddNewPrimCommand is tested though Ufe::ContextOps::doOpCmd() (see here), which returns a UndoableCommand::Ptr, not a Ufe::SceneItemResultUndoableCommand().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made the material commands call UsdUndoAddNewPrimCommand::sceneItem() instead of manually calling Ufe::Hierarchy::createItem() on UsdUndoAddNewPrimCommand::newUfePath(). This makes the code a little more tidy and ensures that UsdUndoAddNewPrimCommand::sceneItem() is covered by existing tests. See 088f54ab81ccfb4ddc941d5c120888d34dee9fea.

@frohnej-adsk
Copy link
Collaborator Author

Rebase on dev as an attempt of fixing clang-format issues on code I have not touched.

Make UsdUndoAddNewPrimCommand derive from SceneItemResultUndoableCommand. This
allows UFE clients to access the newly created prim by calling
SceneItemResultUndoableCommand::sceneItem().
In UsdUndoMaterialCommands, factor out the code related to materials scopes into
separate commands and functions. This will allow reusing them and is required by
future work (MAYA-128151).
@frohnej-adsk frohnej-adsk force-pushed the frohnej/MAYA-129061/refactorMaterialCommands branch from fd6d428 to 355deb1 Compare May 12, 2023 11:33
@frohnej-adsk
Copy link
Collaborator Author

Rebase on dev again to fix formating.

@frohnej-adsk frohnej-adsk added the ready-for-merge Development process is finished, PR is ready for merge label May 12, 2023
@seando-adsk seando-adsk added the ufe-usd Related to UFE-USD plugin in Maya-Usd label May 15, 2023
@seando-adsk seando-adsk merged commit 0916bdb into dev May 15, 2023
@seando-adsk seando-adsk deleted the frohnej/MAYA-129061/refactorMaterialCommands branch May 15, 2023 18:45
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 ufe-usd Related to UFE-USD plugin in Maya-Usd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants