Improve Event Delegation in qMRMLThreeDView and qMRMLSliceView #7311

jcfr · 2023-10-30T06:47:06Z

This pull request is aimed at streamlining the review process and testing by consolidating several independent changes, each of which will be contributed separately.

The primary focus for review in this pull request is the last commit, titled ENH: Improve Event Delegation in qMRMLThreeDView and qMRMLSliceView.

In this commit, we've refactored vtkMRMLThreeDViewInteractorStyle and vtkMRMLSliceViewInteractorStyle to inherit from vtkObject and have implemented event delegation to MRML displayable managers and their associated widgets by observing the interactor style.

This enhancement allows for the association of specialized interactor styles, such as vtkOpenXRInteractorStyle or vtkOpenVRInteractorStyle, with the render window while still maintaining MRML-specific event delegation.

Dependent Changes

The introduction of the commit ENH: Allow virtual func to be called during MRML(ThreeD|Slice)View pimpl init necessitates an update to CTK, which can be found in the following pull request:

Ensure virtual functions can be called from derived ctkVTK view pimpl commontk/CTK#1153

Update: CTK pull request commontk/CTK#1153 has been integrated upstream and corresponding External_CTK.cmake update has been added to this pull request by integrating the commits being contributed in the following pull request:

ENH: Allow virtual func to be called during MRML(ThreeD|Slice)View pimpl init #7325

Associated independent changes are being contributed in these pull requests:

Refactor vtkMRMLThreeDViewInteractorStyle and vtkMRMLSliceViewInteractorStyle to derive from vtkObject and implement event delegation to MRML displayable managers and associated widgets by observing the interactor style. This will allows to associate specialized interactor styles like vtkOpenXRInteractorStyle or vtkOpenVRInteractorStyle with render window while maintaining the MRML specific event delegation. Co-authored-by: Lucas Gandel <lucas.gandel@kitware.com>

lassoan

I don’t like going back to using interactor style, as it is just an unnecessary additional layer that adds complexity and not sufficient to actually handle all interactions in complex applications. Interactor styles should be rather removed from VTK, too (and let VTK widgets handle camera movement).

However, I cannot pinpoint any functional errors with the changes and I see that you worked a lot on this and would not want to hold up things that depend on these changes, so I'm OK if the changes are merged.

lassoan · 2023-11-22T20:11:01Z

Libs/MRML/DisplayableManager/vtkMRMLViewInteractorStyle.cxx

-    return;
-    }
-  this->Superclass::OnKeyPress();
+  this->GetInteractorStyle()->OnKeyPress();


If all we do is calling the Superclass implementation then it is better to remove the method completely.

This is intended to keep the same behavior as we had before, I will create a follow-up pull request and/or issue to move forward with simplification

lassoan · 2023-11-22T20:11:24Z

Libs/MRML/DisplayableManager/vtkMRMLViewInteractorStyle.h

@@ -37,43 +40,44 @@ class vtkTimerLog;
 /// Some additional high-level events (such as click and double-click)
 /// are generated here.
 class VTK_MRML_DISPLAYABLEMANAGER_EXPORT vtkMRMLViewInteractorStyle :
-  public vtkInteractorStyle3D
+  public vtkObject


Why it is not based on an interactor style?

Because our vtkMRMLViewInteractorStyle are technically observer and not interactor style anymore. For more details, see section Refactor Interactor Styles in #7310

For sake of reducing the amount of changes and allowing easy review, I didn't rename our classes. For more details, see section Renaming Interactor Styles in #7310.

lassoan · 2023-11-22T20:13:42Z

Libs/MRML/DisplayableManager/vtkMRMLThreeDViewInteractorStyle.cxx

@@ -110,7 +111,8 @@ bool vtkMRMLThreeDViewInteractorStyle::DelegateInteractionEventToDisplayableMana
  // Get display and world position
  int* displayPositionInt = this->GetInteractor()->GetEventPosition();
  vtkRenderer* pokedRenderer = this->GetInteractor()->FindPokedRenderer(displayPositionInt[0], displayPositionInt[1]);


FindPokedRenderer is error-prone. If somebody adds any content to the render window in a new renderer then it may break picking and or interaction with other widgets. It would be safer to pass the main 3D content renderer, not whatever renderer happened to be found at that position.

Agreed, this can be improved and I will create a dedicated issue to improve and address this. In prefer to keep the existing behavior and focus this pull request on only revisiting the observation of the interactor style events.

jcfr · 2023-11-22T21:12:03Z

I don’t like going back to using interactor style, as it is just an unnecessary additional layer that adds complexity and not sufficient to actually handle all interactions in complex applications.

After discussion with @LucasGandel, we indeed acknowledge that the VTK interactor style currently implemented in the vtkRenderingVR (and OpenVR and OpenXR specialization) could be revisited to avoid having some much "logic" associated with it.

I cannot pinpoint any functional errors with the changes and I see that you worked a lot on this and would not want to hold up things that depend on these changes, so I'm OK if the changes are merged.

Thanks for reviewing and provided detailed feedback. As you indicated, while this is not the "best" path forward, it allows to move forward 🙏

Now that I commented inline, let me know if you have other questions before approving ✔️

jcfr · 2023-11-22T23:07:34Z

going back to using interactor style, as it is just an unnecessary additional layer that adds complexity

As a point of clarification, this pull-request does not add any functional changes.
Instead of having:

vtkMRMLViewInteractorStyle derives from vtkInteractorStyle3D, it now derives from vtkObject
vtkMRMLViewInteractorStyle override callback, it calls corresponding functions by observing the interactor style associated with the render window
the qMRMLThreeDView class set vtkMRMLThreeDViewInteractorStyle as interactor style, it sets an instance of vtkInteractorStyle3D
the qMRMLSliceView class set vtkMRMLSliceViewInteractorStyle as interactor style, it sets an instance of vtkInteractorStyleUser

Doing so allows to associate the OpenVR or OpenXR rendering windows with their corresponding interactor style while maintaining the delegation of event to our displayable manager working as expected.

lassoan

Thanks for the responses, I have no further questions.

jcfr linked an issue Oct 30, 2023 that may be closed by this pull request

Improve Event Delegation in qMRMLThreeDView and qMRMLSliceView #7310

Closed

jcfr force-pushed the refactor-qMRMLThreeDView-and-qMRMLSliceView-event-delegation branch 5 times, most recently from 6b58b0e to d7492b3 Compare November 6, 2023 15:23

jcfr force-pushed the refactor-qMRMLThreeDView-and-qMRMLSliceView-event-delegation branch from d7492b3 to 2f7a6cc Compare November 14, 2023 16:15

jcfr added this to the Slicer 5.5 milestone Nov 16, 2023

jcfr force-pushed the refactor-qMRMLThreeDView-and-qMRMLSliceView-event-delegation branch from 2f7a6cc to 80fba35 Compare November 17, 2023 06:45

jcfr requested review from lassoan, pieper and sjh26 November 21, 2023 23:52

jcfr self-assigned this Nov 21, 2023

lassoan reviewed Nov 22, 2023

View reviewed changes

jcfr requested a review from lassoan November 22, 2023 21:12

lassoan approved these changes Nov 23, 2023

View reviewed changes

jcfr merged commit 1cc3b2c into Slicer:main Nov 23, 2023
5 checks passed

jcfr deleted the refactor-qMRMLThreeDView-and-qMRMLSliceView-event-delegation branch November 23, 2023 02:44

pieper mentioned this pull request Nov 26, 2023

Doubleclick to maximize sliceViews and threeDViews stopped working #7429

Closed

This was referenced Dec 21, 2023

BUG: Fix integration with updated Slicer event delegation and VTK OpenVR API KitwareMedical/SlicerVirtualReality#131

Merged

Add support for OpenXR & OpenXRRemoting KitwareMedical/SlicerVirtualReality#80

Closed

jcfr mentioned this pull request Dec 30, 2023

Enhancements and Refactoring for XR Runtimes Integration KitwareMedical/SlicerVirtualReality#149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Event Delegation in qMRMLThreeDView and qMRMLSliceView #7311

Improve Event Delegation in qMRMLThreeDView and qMRMLSliceView #7311

jcfr commented Oct 30, 2023 •

edited

lassoan left a comment

lassoan Nov 22, 2023

jcfr Nov 22, 2023

lassoan Nov 22, 2023

jcfr Nov 22, 2023

lassoan Nov 22, 2023

jcfr Nov 22, 2023

jcfr commented Nov 22, 2023

jcfr commented Nov 22, 2023 •

edited

lassoan left a comment

Improve Event Delegation in qMRMLThreeDView and qMRMLSliceView #7311

Improve Event Delegation in qMRMLThreeDView and qMRMLSliceView #7311

Conversation

jcfr commented Oct 30, 2023 • edited

Dependent Changes

lassoan left a comment

Choose a reason for hiding this comment

lassoan Nov 22, 2023

Choose a reason for hiding this comment

jcfr Nov 22, 2023

Choose a reason for hiding this comment

lassoan Nov 22, 2023

Choose a reason for hiding this comment

jcfr Nov 22, 2023

Choose a reason for hiding this comment

lassoan Nov 22, 2023

Choose a reason for hiding this comment

jcfr Nov 22, 2023

Choose a reason for hiding this comment

jcfr commented Nov 22, 2023

jcfr commented Nov 22, 2023 • edited

lassoan left a comment

Choose a reason for hiding this comment

jcfr commented Oct 30, 2023 •

edited

jcfr commented Nov 22, 2023 •

edited