Making visibility Undo/Redo correct. #1056

HamedSabri-adsk · 2021-01-11T17:44:57Z

This PR makes the "visibility" in USD data model Undo/Redo correct.

HamedSabri-adsk · 2021-01-11T17:46:48Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+        auto object3d = UsdObject3d::create(fItem);
+        auto current = object3d->visibility();
+        auto cmd = object3d->setVisibleCmd(!current);
+        cmd->execute();


Make sure setting the visibility goes through the new "setVisibleCmd"

Don't need to call execute(), that's done for you by Maya. Just

return object3d->setVisibleCmd(!current);

Also you need to guard this with the UFE_PREVIEW_VERSION_NUM awful pre-processor macro, and keep the old code around... :(

Addressed in 7004631

HamedSabri-adsk · 2021-01-11T17:48:41Z

lib/mayaUsd/ufe/UsdObject3d.h

+#if UFE_PREVIEW_VERSION_NUM >= 2034
+    Ufe::UndoableCommand::Ptr setVisibleCmd(bool vis) override;
+#endif
+


Added guard around "setVisibleCmd" since this feature is available in UFE V.2034 or later.

HamedSabri-adsk · 2021-01-11T17:52:06Z

lib/mayaUsd/ufe/UsdUndoVisibleCommand.cpp

+void UsdUndoVisibleCommand::redo() { _undoableItem.redo(); }
+
+void UsdUndoVisibleCommand::undo() { _undoableItem.undo(); }
+


On execute(), we create an undoBlock to capture the edit and then visibility is set via UsdGeomImageable MakeVisible()/MakeInvisible() methods.

HamedSabri-adsk · 2021-01-11T18:20:20Z

As a FYI, working on the unit-test part for this PR. Should be coming up today.

seando-adsk · 2021-01-11T18:28:30Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+        auto object3d = UsdObject3d::create(fItem);
+        auto current = object3d->visibility();
+        auto cmd = object3d->setVisibleCmd(!current);
+        cmd->execute();


Doesn't this new code need to be protected by UFE_PREVIEW_VERSION_NUM >= 2034 and leave the old code for the else?

Ugh.... Good catch! Indeed. :)

Addressed in 7004631

ppt-adsk

A couple of things to fix, but really what I feel is missing from this PR are the automated tests. You can write an automated test that first tests the UsdUndoVisibleCommand, and calls undo and redo on that object, for the lowest-level test.

Then, I'd like to see tests with the Maya hide and showHidden commands, with calls to Maya undo and redo, to demonstrate the full integration of the UFE command into Maya.

Thanks for doing this!

ppt-adsk · 2021-01-11T18:21:50Z

lib/mayaUsd/ufe/UsdContextOps.cpp

-            current == UsdGeomTokens->invisible ? UsdGeomTokens->inherited
-                                                : UsdGeomTokens->invisible);
+        auto object3d = UsdObject3d::create(fItem);
+        auto current = object3d->visibility();


Shouldn't we check that the scene item supports the Object3d interface? At this point it should, because we put up the menu entry for it, so perhaps we can just assert.

A couple of things to fix, but really what I feel is missing from this PR are the automated tests. You can write an automated test that first tests the UsdUndoVisibleCommand, and calls undo and redo on that object, for the lowest-level test.

Great point. I am working on it as we speak.

Addressed in 7004631

@ppt-adsk @seando-adsk

Added a test for backing the new UsdUndoVisibleCommand :

865d479

Added a new test for backing up the UFE command integration on Maya side for both "hide" and "showHidden" commands :

43bf1b1

ppt-adsk · 2021-01-11T18:24:43Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+        auto object3d = UsdObject3d::create(fItem);
+        auto current = object3d->visibility();
+        auto cmd = object3d->setVisibleCmd(!current);
+        cmd->execute();


Don't need to call execute(), that's done for you by Maya. Just

return object3d->setVisibleCmd(!current);

ppt-adsk · 2021-01-11T18:26:02Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+        auto object3d = UsdObject3d::create(fItem);
+        auto current = object3d->visibility();
+        auto cmd = object3d->setVisibleCmd(!current);
+        cmd->execute();


Also you need to guard this with the UFE_PREVIEW_VERSION_NUM awful pre-processor macro, and keep the old code around... :(

ppt-adsk · 2021-01-11T18:27:47Z

lib/mayaUsd/ufe/UsdUndoVisibleCommand.cpp

+
+#include <pxr/usd/usdGeom/tokens.h>
+
+#include <ufe/log.h>


Addressed in 7004631

ppt-adsk · 2021-01-11T18:27:55Z

lib/mayaUsd/ufe/UsdUndoVisibleCommand.cpp

+#include <mayaUsd/undo/UsdUndoBlock.h>
+#include <mayaUsdUtils/util.h>
+
+#include <pxr/usd/usdGeom/tokens.h>


ppt-adsk · 2021-01-11T18:28:12Z

lib/mayaUsd/ufe/UsdUndoVisibleCommand.cpp

+#include "private/Utils.h"
+
+#include <mayaUsd/undo/UsdUndoBlock.h>
+#include <mayaUsdUtils/util.h>


Do we need this?

Nope they can go away. Addressed in 7004631

ppt-adsk · 2021-01-11T18:28:19Z

lib/mayaUsd/ufe/UsdUndoVisibleCommand.cpp

+//
+#include "UsdUndoVisibleCommand.h"
+
+#include "private/Utils.h"


Do we need this?

ppt-adsk · 2021-01-11T20:21:22Z

lib/mayaUsd/ufe/UsdContextOps.cpp

        auto object3d = UsdObject3d::create(fItem);
+        assert(object3d);


Elsewhere we've been using TF_AXIOM, might want to consider that.

Addressed in 8bac473

ppt-adsk · 2021-01-11T20:22:43Z

lib/mayaUsd/ufe/UsdUndoVisibleCommand.cpp


 #include <pxr/usd/usdGeom/tokens.h>
-
-#include <ufe/log.h>
+#include <pxr/usd/usdGeom/xformCommonAPI.h>


You sure we need this? The common API doesn't feel like it has much to do with visibility...

@ppt-adsk yes it is needed for UsdGeomImageable. It usually brings other stuff which we may need.

But in this case, I don't think we need them. I will replace it with the below header if it is confusing.

#include <pxr/usd/usdGeom/imageable.h>

Addressed in :
88a3a5f
1a2008a

…fter performing undo/redo.

…y via Maya hide and showHidden commands.

ppt-adsk · 2021-01-12T01:53:12Z

test/lib/ufe/testObject3d.py

+        # This time, expect the visibility token to exists in the USD data model
+        self.assertTrue(bool(primSpecCapsule and UsdGeom.Tokens.visibility in primSpecCapsule.attributes))
+        self.assertTrue(bool(primSpecCylinder and UsdGeom.Tokens.visibility in primSpecCylinder.attributes))
+


Would have been nice to undo / redo showHidden --- should be quick to add and we'd put this one to rest.

Good point. Addressed in 585cca0

Making visibility Undo/Redo correct.

b24e690

HamedSabri-adsk added core Related to core library workflows Related to in-context workflows labels Jan 11, 2021

HamedSabri-adsk requested review from ppt-adsk and seando-adsk January 11, 2021 17:44

HamedSabri-adsk commented Jan 11, 2021

View reviewed changes

seando-adsk reviewed Jan 11, 2021

View reviewed changes

ppt-adsk requested changes Jan 11, 2021

View reviewed changes

Address feedback.

7004631

HamedSabri-adsk requested review from ppt-adsk and seando-adsk January 11, 2021 19:31

ppt-adsk reviewed Jan 11, 2021

View reviewed changes

Hamed Sabri added 3 commits January 11, 2021 16:53

Added a test to back the visibility token / attribute value changes a…

865d479

…fter performing undo/redo.

Include imageable header directly.

88a3a5f

Remove unused tokens.h

1a2008a

HamedSabri-adsk requested a review from ppt-adsk January 11, 2021 22:01

Hamed Sabri added 2 commits January 11, 2021 20:26

Added a new test to verify the token / attribute values for visibilit…

43bf1b1

…y via Maya hide and showHidden commands.

replace assert with TF_AXIOM

8bac473

ppt-adsk reviewed Jan 12, 2021

View reviewed changes

Add undo/redo after calling showHidden command.

585cca0

ppt-adsk approved these changes Jan 12, 2021

View reviewed changes

ppt-adsk assigned ecp-maya-devops-adsk Jan 12, 2021

Guard against UFE version since 0.2.34 is not blessed yet.

16457bf

HamedSabri-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jan 12, 2021

ppt-adsk approved these changes Jan 12, 2021

View reviewed changes

seando-adsk approved these changes Jan 12, 2021

View reviewed changes

HamedSabri-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Jan 13, 2021

kxl-adsk merged commit 6180c3f into dev Jan 13, 2021

kxl-adsk deleted the sabrih/MAYA-108423/undo_correct_visibility branch January 13, 2021 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making visibility Undo/Redo correct. #1056

Making visibility Undo/Redo correct. #1056

HamedSabri-adsk commented Jan 11, 2021

HamedSabri-adsk Jan 11, 2021 •

edited

Loading

ppt-adsk Jan 11, 2021

ppt-adsk Jan 11, 2021

HamedSabri-adsk Jan 11, 2021

HamedSabri-adsk Jan 11, 2021

HamedSabri-adsk Jan 11, 2021

HamedSabri-adsk commented Jan 11, 2021

seando-adsk Jan 11, 2021

HamedSabri-adsk Jan 11, 2021

HamedSabri-adsk Jan 11, 2021

ppt-adsk left a comment

ppt-adsk Jan 11, 2021

HamedSabri-adsk Jan 11, 2021

HamedSabri-adsk Jan 11, 2021 •

edited

Loading

ppt-adsk Jan 11, 2021

ppt-adsk Jan 11, 2021

ppt-adsk Jan 11, 2021

HamedSabri-adsk Jan 11, 2021

ppt-adsk Jan 11, 2021

ppt-adsk Jan 11, 2021

HamedSabri-adsk Jan 11, 2021

ppt-adsk Jan 11, 2021

ppt-adsk Jan 11, 2021

HamedSabri-adsk Jan 12, 2021

ppt-adsk Jan 11, 2021

HamedSabri-adsk Jan 11, 2021 •

edited

Loading

ppt-adsk Jan 12, 2021

HamedSabri-adsk Jan 12, 2021

		void UsdUndoVisibleCommand::redo() { _undoableItem.redo(); }

		void UsdUndoVisibleCommand::undo() { _undoableItem.undo(); }

		auto object3d = UsdObject3d::create(fItem);
		assert(object3d);

Making visibility Undo/Redo correct. #1056

Making visibility Undo/Redo correct. #1056

Conversation

HamedSabri-adsk commented Jan 11, 2021

HamedSabri-adsk Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk commented Jan 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppt-adsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk Jan 11, 2021 •

edited

Loading

HamedSabri-adsk Jan 11, 2021 •

edited

Loading

HamedSabri-adsk Jan 11, 2021 •

edited

Loading