MAYA-108549: Fix undo crash when switching between target layers in a stage #965
Conversation
HamedSabri-adsk
added
core
lib/mayaUsd/undo/UsdUndoManager.cpp
Outdated
| layer->SetStateDelegate(_layerStateDelegate); | ||
| } | ||
|
|
||
| void UsdUndoManager::trackStatesOnEditTargetChange(const SdfLayerHandle& layer) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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);
}
lib/mayaUsd/undo/UsdUndoManager.cpp
Outdated
| { | ||
| layer->SetStateDelegate(UsdUndoStateDelegate::New()); | ||
| _layerStateDelegate = TfCreateRefPtr(new UsdUndoStateDelegate); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
This reverts commit 120ae47.
…dy registered with our UsdUndoStateDelegate. If the cast fails, that means it needs to be set with a new one.
| @@ -88,7 +88,7 @@ class MAYAUSD_CORE_PUBLIC StagesSubject : public TfWeakBase | |||
| void onStageInvalidate(const MayaUsdProxyStageInvalidateNotice& notice); | |||
|
|
|||
| // Array of Notice::Key for registered listener | |||
| #if UFE_PREVIEW_VERSION_NUM >= 2029 | |||
| #if UFE_PREVIEW_VERSION_NUM >= 2025 | |||
There was a problem hiding this comment.
@ppt-adsk Based on our internal discussion, lowering the version to cover existing shipped PR.
| = TfDynamic_cast<UsdUndoStateDelegatePtr>(layer->GetStateDelegate()); | ||
| if (!usdUndoStateDelegatePtr) { | ||
| layer->SetStateDelegate(UsdUndoStateDelegate::New()); | ||
| } |
There was a problem hiding this comment.
@ppt-adsk Thanks for great feedback. Using a dynamic_cast to determine if a layer is already registered with our UsdUndoStateDelegate. If the cast fails, that means it needs to be set with a new one. The UsdUndoStateDelegate is stateless and doesn't need to be shared among layers.
HamedSabri-adsk
deleted the
sabrih/MAYA-108549/fix_undo_crash_switching_layers
branch
This PR fixes the crash when switching between target layers in a stage.