fix: fix LayerDM Pipeline cleanup

Add several steps to avoid crash in pipelines during or just after cleanup:

- Add BlockUpdateObserver to block triggers to the OnUpdate callback
- Add BlockInteractionProcessing to block pipeline from being processed during interactions
- Add SetFrozen to freeze all interactions with pipeline during and after removal
- Add LoseInteraction when node is removed during interaction process

Author: Thibault-Pelletier

LayerDM/MRMLDM/vtkMRMLLayerDMInteractionLogic.cxx

@@ -64,6 +64,11 @@ std::tuple<double, int> vtkMRMLLayerDMInteractionLogic::PrioritizeCanProcessPipe
   int maxState = this->MinWidgetState();
   for (const auto& pipeline : m_pipelines)
   {
+    if (pipeline->IsInteractionProcessingBlocked())
+    {
+      continue;
+    }
+
     double pipelineDistance = std::numeric_limits<double>::max();
     if (pipeline->CanProcessInteractionEvent(eventData, pipelineDistance))
     {
@@ -102,6 +107,10 @@ void vtkMRMLLayerDMInteractionLogic::AddPipeline(const vtkSmartPointer<vtkMRMLLa
 
 void vtkMRMLLayerDMInteractionLogic::RemovePipeline(const vtkSmartPointer<vtkMRMLLayerDMPipelineI>& pipeline)
 {
+  if (this->m_prevFocusedPipeline == pipeline)
+  {
+    this->LoseFocus();
+  }
   this->m_pipelines.erase(std::find(this->m_pipelines.begin(), this->m_pipelines.end(), pipeline));
 }
 
@@ -133,6 +142,11 @@ bool vtkMRMLLayerDMInteractionLogic::ProcessInteractionEvent(vtkMRMLInteractionE
 {
   for (const auto& pipeline : m_canProcess)
   {
+    if (pipeline->IsInteractionProcessingBlocked())
+    {
+      continue;
+    }
+
     // If pipeline can process, store pipeline for further interaction events
     if (pipeline->ProcessInteractionEvent(eventData))
     {

LayerDM/MRMLDM/vtkMRMLLayerDMPipelineI.cxx

@@ -59,11 +59,33 @@ void vtkMRMLLayerDMPipelineI::SetRenderer(vtkRenderer* renderer)
 
 bool vtkMRMLLayerDMPipelineI::BlockResetDisplay(bool isBlocked)
 {
+  if (this->m_isFrozen)
+  {
+    return true;
+  }
+
   bool prev = this->m_isResetDisplayBlocked;
   this->m_isResetDisplayBlocked = isBlocked;
   return prev;
 }
 
+bool vtkMRMLLayerDMPipelineI::BlockInteractionProcessing(bool isBlocked)
+{
+  if (this->m_isFrozen)
+  {
+    return true;
+  }
+
+  const auto prev = this->m_isInteractionProcessingBlocked;
+  this->m_isInteractionProcessingBlocked = isBlocked;
+  return prev;
+}
+
+bool vtkMRMLLayerDMPipelineI::IsInteractionProcessingBlocked() const
+{
+  return this->m_isInteractionProcessingBlocked;
+}
+
 bool vtkMRMLLayerDMPipelineI::CanProcessInteractionEvent(vtkMRMLInteractionEventData* eventData, double& distance2)
 {
   return false;
@@ -142,6 +164,37 @@ vtkMRMLAbstractViewNode* vtkMRMLLayerDMPipelineI::GetViewNode() const
   return this->m_viewNode;
 }
 
+bool vtkMRMLLayerDMPipelineI::BlockUpdateObserver(bool isBlocked) const
+{
+  if (this->m_isFrozen)
+  {
+    return true;
+  }
+
+  return this->m_obs->SetBlocked(isBlocked);
+}
+
+void vtkMRMLLayerDMPipelineI::SetFrozen(bool isFrozen)
+{
+  if (this->m_isFrozen == isFrozen)
+  {
+    return;
+  }
+
+  // Block states are only updated when the pipeline is not frozen.
+  // Unfreeze to update all before saving the frozen state.
+  this->m_isFrozen = false;
+  this->BlockInteractionProcessing(isFrozen);
+  this->BlockResetDisplay(isFrozen);
+  this->BlockUpdateObserver(isFrozen);
+  this->m_isFrozen = isFrozen;
+}
+
+bool vtkMRMLLayerDMPipelineI::IsFrozen() const
+{
+  return this->m_isFrozen;
+}
+
 vtkMRMLNode* vtkMRMLLayerDMPipelineI::GetDisplayNode() const
 {
   return this->m_displayNode;
@@ -182,6 +235,8 @@ vtkMRMLLayerDMPipelineI::vtkMRMLLayerDMPipelineI()
   , m_displayNode{ nullptr }
   , m_renderer{ nullptr }
   , m_isResetDisplayBlocked{ false }
+  , m_isFrozen{ false }
+  , m_isInteractionProcessingBlocked{ false }
   , m_obs(vtkSmartPointer<vtkMRMLLayerDMObjectEventObserver>::New())
   , m_pipelineManager(nullptr)
 {

LayerDM/MRMLDM/vtkMRMLLayerDMPipelineI.h

@@ -123,6 +123,26 @@ class VTK_SLICER_LAYERDM_MODULE_MRMLDISPLAYABLEMANAGER_EXPORT vtkMRMLLayerDMPipe
   /// If \param isBlocked is true, \sa UpdatePipeline is not called during \sa ResetDisplay.
   bool BlockResetDisplay(bool isBlocked);
 
+  /// If \param isBlocked is true, blocks \sa CanProcessInteractionEvent and \sa ProcessInteractionEvent to be called.
+  /// \sa vtkMRMLLayerDMInteractionLogic
+  bool BlockInteractionProcessing(bool isBlocked);
+
+  /// Current state of interaction processing blocking.
+  bool IsInteractionProcessingBlocked() const;
+
+  /// If \param isBlocked is true, blocks \sa OnUpdate method from being triggered by external changes.
+  bool BlockUpdateObserver(bool isBlocked) const;
+
+  /// @{
+  /// If \param isFrozen is true, blocks all reactiveness from the pipeline (ResetDisplay, Interaction and Update).
+  /// Reactiveness cannot be toggled back on unless the pipeline is unfrozen first.
+  /// Used to deactivate pipelines during removal.
+  ///
+  /// \sa BlockResetDisplay \sa BlockInteractionProcessing \sa BlockUpdateObserver
+  void SetFrozen(bool isFrozen);
+  bool IsFrozen() const;
+  /// @}
+
   /// Returns the current display node.
   vtkMRMLNode* GetDisplayNode() const;
 
@@ -189,6 +209,8 @@ class VTK_SLICER_LAYERDM_MODULE_MRMLDISPLAYABLEMANAGER_EXPORT vtkMRMLLayerDMPipe
   vtkWeakPointer<vtkMRMLNode> m_displayNode;
   vtkWeakPointer<vtkRenderer> m_renderer;
   bool m_isResetDisplayBlocked;
+  bool m_isFrozen;
+  bool m_isInteractionProcessingBlocked;
   vtkSmartPointer<vtkMRMLLayerDMObjectEventObserver> m_obs;
   vtkWeakPointer<vtkMRMLLayerDMPipelineManager> m_pipelineManager;
   vtkWeakPointer<vtkMRMLScene> m_scene;

LayerDM/MRMLDM/vtkMRMLLayerDMPipelineManager.cxx

@@ -128,8 +128,10 @@ bool vtkMRMLLayerDMPipelineManager::RemovePipeline(vtkMRMLNode* displayNode)
   }
 
   RequestRenderOnceGuard renderGuard{ *this };
-  this->m_layerManager->RemovePipeline(pipeline);
+  pipeline->SetFrozen(true);
+  // Let interaction logic process the removal first if the pipeline needs to lose focus.
   this->m_interactionLogic->RemovePipeline(pipeline);
+  this->m_layerManager->RemovePipeline(pipeline);
   this->m_pipelineMap.erase(displayNode);
   this->InvokeEvent(vtkCommand::ModifiedEvent);
   return true;

LayerDM/Testing/Python/InteractionLogicTest.py

@@ -105,3 +105,33 @@ def test_on_new_interaction_different_from_focused_loses_focus(self):
         p2.mockProcess.assert_called_once_with(self.event)
 
         assert self.logic.GetLastFocusedPipeline() == p2
+
+    def test_pipelines_with_blocked_interactions_are_not_called_during_can_process(self):
+        p1 = MockPipeline(canProcess=True, processDistance=0)
+        self.logic.AddPipeline(p1)
+
+        assert not p1.BlockInteractionProcessing(True)
+        self.logic.CanProcessInteractionEvent(self.event, self.distance)
+        p1.mockCanProcess.assert_not_called()
+
+        assert p1.BlockInteractionProcessing(False)
+        self.logic.CanProcessInteractionEvent(self.event, self.distance)
+        p1.mockCanProcess.assert_called_once()
+
+    def test_pipelines_with_blocked_interactions_are_not_called_during_process(self):
+        # Blocking can happen after the pipeline had claimed it could process the interaction.
+        # Make sure that it doesn't actually process it if blocked afterwards.
+        p1 = MockPipeline(canProcess=True, didProcess=True, processDistance=0)
+        self.logic.AddPipeline(p1)
+
+        assert self.logic.CanProcessInteractionEvent(self.event, self.distance)
+
+        assert not p1.BlockInteractionProcessing(True)
+        assert not self.logic.ProcessInteractionEvent(self.event)
+        p1.mockProcess.assert_not_called()
+
+        assert p1.BlockInteractionProcessing(False)
+        assert self.logic.CanProcessInteractionEvent(self.event, self.distance)
+        assert self.logic.ProcessInteractionEvent(self.event)
+
+        p1.mockProcess.assert_called_once()

LayerDM/Testing/Python/PipelineManagerTest.py

@@ -296,3 +296,21 @@ def test_notifies_pipelines_when_references_are_added_or_removed(self):
         # Unset the display node and expect the pipeline to have been notified
         markups.SetAndObserveDisplayNodeID("")
         m1.mockOnReferenceToDisplayNodeRemoved.assert_called_once_with(markups, "display")
+
+    def test_pipelines_removed_are_frozen_during_cleanup(self):
+        m1 = self.triggerMockPipelineCreation(MockPipeline())
+        assert not m1.IsFrozen()
+        self.pipelineManager.RemoveNode(m1.GetDisplayNode())
+        assert m1.IsFrozen()
+
+    def test_pipelines_removed_while_having_interaction_lose_focus_during_cleanup(self):
+        m1 = self.triggerMockPipelineCreation(MockPipeline())
+        m1.mockCanProcess.return_value = (True, 0.0)
+        m1.mockProcess.return_value = True
+        distance = ref(0.0)
+        self.pipelineManager.CanProcessInteractionEvent(vtkMRMLInteractionEventData(), distance)
+        self.pipelineManager.ProcessInteractionEvent(vtkMRMLInteractionEventData())
+        m1.mockProcess.assert_called_once()
+        m1.mockLoseFocus.assert_not_called()
+        self.pipelineManager.RemoveNode(m1.GetDisplayNode())
+        m1.mockLoseFocus.assert_called_once()