-
Notifications
You must be signed in to change notification settings - Fork 579
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.
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
Improve Event Delegation in qMRMLThreeDView and qMRMLSliceView #7310
Comments
We got rid of interactor styles in Slicer (just kept a sham class that just forwards events to widgets) because they did not allow dynamic mapping of GUI events to actions; and the hardcoded event mappings often conflicted with event mappings in VTK widgets. VTK VR developers realized the rigidity of interactor styles, too, and implemented a very basic event translation mechanism in We also improved the widget event translation mechanism to allow specifying widget state for an event translation rule. For example, DEL key press event can be mapped to delete point when the current widget state is "point selected"; and it can be mapped to delete the entire markup if the state is "whole markup is selected"). We also added interaction Returning to the old event handling based on monolithic interaction styles classes would be a huge step backward in Slicer. I think the simplest would be to keep Slicer and VTK interaction handling separate (maybe factor out some common logic in helper classes). We just need to avoid past mistakes when VTK core developers kept refactoring virtual reality implementation in VTK in a way that it breaks Slicer. The problem may get better by itself, because as the implementation matures there should be less need for breaking changes. We could also facilitate keeping important APIs in VTK functional by adding VTK tests for features that we rely on in Slicer. I'm really sorry for not being able to propose any simple way of harmonizing implementations for this in VTK and Slicer, but 3D widgets is one of the very few parts of VTK that I find unusable for anything more than toy examples (single-view manipulation of a few selected widget types). It is not just me. You can find that there are about 3 generations of 3D widgets in VTK, all with serious limitations, so it is quite obvious that VTK developers struggled with this. There were 4-5 attempts (and $500k+ spent) trying to use VTK widgets in Slicer without major redesign and all those attempts have failed. When we permitted ourselves to break away from the VTK design then suddenly we could implement features that seemed impossible before. |
Is your feature request related to a problem? Please describe.
The current event delegation mechanism in Slicer is based on Slicer-specific interactor styles, namely
vtkMRMLThreeDViewInteractorStyle
andvtkMRMLSliceViewInteractorStyle
, which are derived fromvtkMRMLViewInteractorStyle
and, ultimately,vtkInteractorStyle3D
.These styles handle interaction events by adding observers for events like
EnterEvent
,LeaveEvent
, andMouseMoveEvent
, and subsequently call functions likevtkMRMLViewInteractorStyle::OnEnter()
,vtkMRMLViewInteractorStyle::OnLeave()
, orvtkMRMLViewInteractorStyle::OnMouseMove()
.In each of these
On<EventName>()
functions, delegation to displayable managers and their associated widgets is performed, and if a given event isn't intercepted, the default behavior for<EventName>
implemented invtkInteractorStyle3D
is executed by callingthis->Superclass::On<EventName>()
. The snippet below illustrates this:However, a limitation arises when default event handling implemented in specific interactor styles (e.g.,
vtkOpenXRInteractorStyle
orvtkOpenVRInteractorStyle
) needs to be executed when not delegated.Describe the solution you'd like
Refactor Interactor Styles
Refactor
vtkMRMLThreeDViewInteractorStyle
andvtkMRMLSliceViewInteractorStyle
to derive fromvtkObject
and implement event delegation to MRML displayable managers and associated widgets by observing the interactor style. Depending on the observed event, default interactor style behavior would be executed using one of the following approaches:Move3D
,Button3D
,Menu3D
, and others where thevtkInteractorStyle
class checks if the event was aborted, we will set an abort flag.On<EventName>()
: For other events, we would explicitly call the correspondingOn<EventName>()
function. To support this, the existing methodvtkMRMLAbstractThreeDViewDisplayableManager::PassThroughInteractorStyleEvent
will be used and updated as needed.The interactor style will be associated with its corresponding displayable manager group, allowing access to the MRML scene and registered displayable managers.
Renaming Interactor Styles
To faciliate review of changes introduced in the first phase of the refactoring, the rename the classes will happen in in a second phase. This will help avoid future confusion.
More specifically, we will ename the classes
vtkMRMLThreeDViewInteractorStyle
tovtkMRMLThreeDViewInteractorObserver
andvtkMRMLSliceViewInteractorStyle
tovtkMRMLSliceViewInteractorObserver
.Access to
CameraNode
fromvtkMRMLThreeDViewInteractorStyle
Access to the
CameraNode
will be updated in the following places:qMRMLThreeDView
, the code can be refactored to retrieve the camera node from thevtkMRMLCameraDisplayableManager
by callingqMRMLThreeDView::displayableManagerByClassName()
.vtkMRMLThreeDViewInteractorStyle
, the camera node will be accessed through the displayable manager group following the refactoring described above.Default Interactor Styles
vtkInteractorStyleTrackballCamera
.vtkInteractorStyleUser
.Whenever possible, Slicer-specific behavior will be added either to
vtkMRMLThreeDViewInteractorStyle
(renamedvtkMRMLThreeDViewInteractorObserver
in Phase 2) or to displayable managers to ensure that switching the interactor style does not affect critical functionality.Describe alternatives you've considered
An alternative would be duplicating the code currently implemented in VTK-specific interactor styles. However, this approach is error-prone and unsustainable.
Additional context
This proposal was created following discussion with @LucasGandel while discussing the following issue:
The text was updated successfully, but these errors were encountered: