ENH: Simplify logic in vtkMRMLViewInteractorStyle::CustomProcessEvents
#7495
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Considering that: 1. we always have to first attempt to delegate to our displayable managers, and then call the "regular" interactor style processing only if these are not "delegated", checking for the interaction state is not needed. 2. we are not observing events like InteractionEvent, StartInteractionEvent or EndInteractionEvent currently used to process interaction state in regular VTK widgets. ... we remove the check of the interaction state. For reference,this logic was initially introduced in 364ff7b (`ENH: Use common interactor style for slice and 3D views`). At the time of that commit[^1], the function `DelegateInteractionEventToDisplayableManagers` was invoked once at the beginning of `CustomProcessEvents` for each events only if "state" was not VTKIS_NONE. It was also systematically invoked in the overridden `OnXXX()` functions, called as a side effect of `Superclass::ProcessEvents`. Following the refactoring introduced in commit 1cc3b2c (`ENH: Improve Event Delegation in qMRMLThreeDView and qMRMLSliceView`), the function `DelegateInteractionEventToDisplayableManagers` now ends up being called only once for all observed events and then the "original" interactor style function are called from `vtkMRMLViewInteractorStyle::ProcessEvents()`. [^1]: https://github.com/Slicer/Slicer/blob/364ff7b6d4758629b8659712fdec990d8bddd81b/Libs/MRML/DisplayableManager/vtkMRMLViewInteractorStyle.cxx#L351-L360
jcfr
changed the title
vtkMRMLViewInteractorStyle::CustomProcessEvents
jcfr
force-pushed
the
remove-obsolete-check-from-vtkMRMLViewInteractorStyle-CustomProcessEvents
branch
from
7f18f6b
to
abcf009
Compare
|
I just understood something critical related to how event handling was implemented the way it was. I will be addressing the issue and add documentation as expected. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Considering that:
... we remove the check of the interaction state.
For reference,this logic was initially introduced in 364ff7b (
ENH: Use common interactor style for slice and 3D views). At the time of that commit1, the functionDelegateInteractionEventToDisplayableManagerswas invoked once at the beginning ofCustomProcessEventsfor each events only if "state" was not VTKIS_NONE. It was also systematically invoked in the overriddenOnXXX()functions, called as a side effect ofSuperclass::ProcessEvents.Following the refactoring introduced in commit 1cc3b2c (
ENH: Improve Event Delegation in qMRMLThreeDView and qMRMLSliceView), the functionDelegateInteractionEventToDisplayableManagersnow ends up being called only once for all observed events and then the "original" interactor style function are called fromvtkMRMLViewInteractorStyle::ProcessEvents().Footnotes
https://github.com/Slicer/Slicer/blob/364ff7b6d4758629b8659712fdec990d8bddd81b/Libs/MRML/DisplayableManager/vtkMRMLViewInteractorStyle.cxx#L351-L360 ↩