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

MAYA-108549: Fix undo crash when switching between target layers in a stage #965

Merged
merged 4 commits into from
Dec 4, 2020
Merged

MAYA-108549: Fix undo crash when switching between target layers in a stage #965

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
120ae47
Fix undo/redo crash when switching between layers.
Dec 3, 2020
2981cca
Revert "Fix undo/redo crash when switching between layers."
Dec 4, 2020
a87fc0b
Make sure the USD state delegate is registered for current existing PR.
Dec 4, 2020
677db40
Address feedback. Use a dynamic_cast to determine if a layer is alrea…
Dec 4, 2020
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
2 changes: 1 addition & 1 deletion lib/mayaUsd/python/wrapUsdUndoManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class PythonUndoBlock

void _trackLayerStates(const SdfLayerHandle& layer)
{
MayaUsd::UsdUndoManager::instance().trackLayerStates(layer);
MayaUsd::UsdUndoManager::instance().trackStatesOnNewStage(layer);
}

} // namespace
Expand Down
4 changes: 2 additions & 2 deletions lib/mayaUsd/ufe/StagesSubject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ void StagesSubject::stageEditTargetChanged(
UsdStageWeakPtr const& sender)
{
// Track the edit target layer's state
UsdUndoManager::instance().trackLayerStates(notice.GetStage()->GetEditTarget().GetLayer());
UsdUndoManager::instance().trackStatesOnEditTargetChange(notice.GetStage()->GetEditTarget().GetLayer());
}
#endif

Expand All @@ -319,7 +319,7 @@ void StagesSubject::onStageSet(const MayaUsdProxyStageSetNotice& notice)
// invalid stage.
if (noticeStage) {
// Track the edit target layer's state
UsdUndoManager::instance().trackLayerStates(noticeStage->GetEditTarget().GetLayer());
UsdUndoManager::instance().trackStatesOnNewStage(noticeStage->GetEditTarget().GetLayer());
}
#endif

Expand Down
11 changes: 9 additions & 2 deletions lib/mayaUsd/undo/UsdUndoManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,16 @@ UsdUndoManager& UsdUndoManager::instance()
return undoManager;
}

void UsdUndoManager::trackLayerStates(const SdfLayerHandle& layer)
void UsdUndoManager::trackStatesOnNewStage(const SdfLayerHandle& layer)
{
layer->SetStateDelegate(UsdUndoStateDelegate::New());
_layerStateDelegate = TfCreateRefPtr(new UsdUndoStateDelegate);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we save this as a data member? Since the UsdUndoManager is a singleton, doesn't that mean that more than one layer can share the same UsdUndoStateDelegate?

Thinking out loud here, could it be that the UsdUndoManager should track a set of layers, and when it sees one that isn't in its set, whether than be through a layer creation notification or an edit target change notification, it will give the unknown layer a newly-created UsdUndoStateDelegate, then will add the layer to its set of known layers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we save this as a data member?

The main reason I need to save _layerStateDelegate as a data member is for undo/redo purposes between layers itself.

Since the UsdUndoManager is a singleton, doesn't that mean that more than one layer can share the same UsdUndoStateDelegate?

Exactly, more than one layer can share the same UsdUndoStateDelegate in a same stage.

it will give the unknown layer a newly-created UsdUndoStateDelegate, then will add the layer to its set of known layers?

Let's have a chat around this. I would like to understand this approach better.


layer->SetStateDelegate(_layerStateDelegate);
}

void UsdUndoManager::trackStatesOnEditTargetChange(const SdfLayerHandle& layer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit perplexed here. If we set a new state delegate on layer creation, then why do we have to set it on edit target change? Is it because of initialization order and the fact that certain layers may have been created before the UsdUndoManager starts tracking changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every time we change the target edit layer, UsdUndoStateDelegate::_OnSetLayer is invoked where it
needs to set the _layer for tracking changes.

We do need to call layer->SetStateDelegate() when we switch between layers otherwise how would the layer know what state delegate needs to use?

Notice that in trackStatesOnEditTargetChange, I am only passing the _layerStateDelegate object that was created on stage set.

More than one layer needs to share the same StateDelegate .

void UsdUndoManager::trackStatesOnEditTargetChange(const SdfLayerHandle& layer) 
{
    layer->SetStateDelegate(_layerStateDelegate);
}

{
layer->SetStateDelegate(_layerStateDelegate);
}

void UsdUndoManager::addInverse(InvertFunc func)
Expand Down
10 changes: 8 additions & 2 deletions lib/mayaUsd/undo/UsdUndoManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#define MAYAUSD_UNDO_UNDOMANAGER_H

#include "UsdUndoableItem.h"
#include "UsdUndoStateDelegate.h"

#include <mayaUsd/base/api.h>

Expand Down Expand Up @@ -52,8 +53,11 @@ class MAYAUSD_CORE_PUBLIC UsdUndoManager
UsdUndoManager(UsdUndoManager&&) = delete;
UsdUndoManager& operator=(UsdUndoManager&&) = delete;

// tracks layer states by spawning a new UsdUndoStateDelegate
void trackLayerStates(const SdfLayerHandle& layer);
// tracks layer states on a new stage
void trackStatesOnNewStage(const SdfLayerHandle& layer);

// tracks layer states when switching between stage's layers
void trackStatesOnEditTargetChange(const SdfLayerHandle& layer);

private:
friend class UsdUndoStateDelegate;
Expand All @@ -68,6 +72,8 @@ class MAYAUSD_CORE_PUBLIC UsdUndoManager

private:
InvertFuncs _invertFuncs;

UsdUndoStateDelegateRefPtr _layerStateDelegate;
};

} // namespace MAYAUSD_NS_DEF
Expand Down
5 changes: 0 additions & 5 deletions lib/mayaUsd/undo/UsdUndoStateDelegate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,6 @@ UsdUndoStateDelegate::UsdUndoStateDelegate()

UsdUndoStateDelegate::~UsdUndoStateDelegate() { }

UsdUndoStateDelegateRefPtr UsdUndoStateDelegate::New()
{
return TfCreateRefPtr(new UsdUndoStateDelegate());
}

bool UsdUndoStateDelegate::_IsDirty() { return _dirty; }

void UsdUndoStateDelegate::_MarkCurrentStateAsClean() { _dirty = false; }
Expand Down
2 changes: 0 additions & 2 deletions lib/mayaUsd/undo/UsdUndoStateDelegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,6 @@ class MAYAUSD_CORE_PUBLIC UsdUndoStateDelegate : public SdfLayerStateDelegateBas
UsdUndoStateDelegate();
~UsdUndoStateDelegate() override;

static UsdUndoStateDelegateRefPtr New();

private:
void invertSetField(const SdfPath& path, const TfToken& fieldName, const VtValue& inverse);
void invertCreateSpec(const SdfPath& path, bool inert);
Expand Down