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.

Sign up for GitHub

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

Making visibility Undo/Redo correct. #1056

Merged
merged 9 commits into from
Jan 13, 2021
Merged

Making visibility Undo/Redo correct. #1056

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b24e690
Making visibility Undo/Redo correct.
Jan 5, 2021
7004631
Address feedback.
Jan 11, 2021
865d479
Added a test to back the visibility token / attribute value changes a…
Jan 11, 2021
88a3a5f
Include imageable header directly.
Jan 11, 2021
1a2008a
Remove unused tokens.h
Jan 11, 2021
43bf1b1
Added a new test to verify the token / attribute values for visibilit…
Jan 12, 2021
8bac473
replace assert with TF_AXIOM
Jan 12, 2021
585cca0
Add undo/redo after calling showHidden command.
Jan 12, 2021
16457bf
Guard against UFE version since 0.2.34 is not blessed yet.
Jan 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions lib/mayaUsd/ufe/UsdContextOps.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@

#include "private/UfeNotifGuard.h"

#if UFE_PREVIEW_VERSION_NUM >= 2034
#include <mayaUsd/ufe/UsdObject3d.h>
#endif
#include <mayaUsd/ufe/UsdSceneItem.h>
#include <mayaUsd/ufe/UsdUndoAddNewPrimCommand.h>
#include <mayaUsd/ufe/Utils.h>
Expand All @@ -36,7 +38,9 @@
#include <maya/MGlobal.h>
#include <ufe/attribute.h>
#include <ufe/attributes.h>
#if UFE_PREVIEW_VERSION_NUM >= 2034
#include <ufe/object3d.h>
#endif
#include <ufe/path.h>

#include <algorithm>
Expand Down Expand Up @@ -569,10 +573,23 @@ Ufe::UndoableCommand::Ptr UsdContextOps::doOpCmd(const ItemPath& itemPath)
return std::make_shared<SetVariantSelectionUndoableCommand>(prim(), itemPath);
} // Variant sets
else if (itemPath[0] == kUSDToggleVisibilityItem) {
#if UFE_PREVIEW_VERSION_NUM < 2034
auto attributes = Ufe::Attributes::attributes(sceneItem());
assert(attributes);
auto visibility = std::dynamic_pointer_cast<Ufe::AttributeEnumString>(
attributes->attribute(UsdGeomTokens->visibility));
assert(visibility);
auto current = visibility->get();
return visibility->setCmd(
current == UsdGeomTokens->invisible ? UsdGeomTokens->inherited
: UsdGeomTokens->invisible);
#else
auto object3d = UsdObject3d::create(fItem);
assert(object3d);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 8bac473

auto current = object3d->visibility();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@HamedSabri-adsk HamedSabri-adsk Jan 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@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

auto cmd = object3d->setVisibleCmd(!current);
cmd->execute();
return object3d->setVisibleCmd(!current);
#endif

} // Visibility
else if (itemPath[0] == kUSDToggleActiveStateItem) {
return std::make_shared<ToggleActiveStateCommand>(prim());
Expand Down
6 changes: 1 addition & 5 deletions lib/mayaUsd/ufe/UsdUndoVisibleCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,10 @@
//
#include "UsdUndoVisibleCommand.h"

#include "private/Utils.h"

#include <mayaUsd/undo/UsdUndoBlock.h>
#include <mayaUsdUtils/util.h>

#include <pxr/usd/usdGeom/tokens.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unused.


#include <ufe/log.h>
#include <pxr/usd/usdGeom/xformCommonAPI.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@HamedSabri-adsk HamedSabri-adsk Jan 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@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


namespace MAYAUSD_NS_DEF {
namespace ufe {
Expand Down