From 31dd2c553c033c64b21a8acbc4d0feba441c227f Mon Sep 17 00:00:00 2001 From: Alexis Girault Date: Tue, 30 Jan 2018 17:24:30 -0500 Subject: [PATCH 1/5] style(RenderWindowInteractor): rename eventsWeHandle to handledEvents --- Sources/Rendering/Core/RenderWindowInteractor/index.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Sources/Rendering/Core/RenderWindowInteractor/index.js b/Sources/Rendering/Core/RenderWindowInteractor/index.js index 1ddb9df9b98..9a3214b3e6b 100644 --- a/Sources/Rendering/Core/RenderWindowInteractor/index.js +++ b/Sources/Rendering/Core/RenderWindowInteractor/index.js @@ -19,7 +19,7 @@ const deviceInputMap = { ], }; -const eventsWeHandle = [ +const handledEvents = [ 'Animation', 'Enter', 'Leave', @@ -581,7 +581,7 @@ function vtkRenderWindowInteractor(publicAPI, model) { }; // create the generic Event methods - eventsWeHandle.forEach((eventName) => { + handledEvents.forEach((eventName) => { const lowerFirst = eventName.charAt(0).toLowerCase() + eventName.slice(1); publicAPI[`${lowerFirst}Event`] = (arg) => { if (!model.enabled) { @@ -927,7 +927,7 @@ export function extend(publicAPI, model, initialValues = {}) { macro.obj(publicAPI, model); macro.event(publicAPI, model, 'RenderEvent'); - eventsWeHandle.forEach((eventName) => + handledEvents.forEach((eventName) => macro.event(publicAPI, model, eventName) ); @@ -981,4 +981,4 @@ export const newInstance = macro.newInstance( // ---------------------------------------------------------------------------- -export default Object.assign({ newInstance, extend }, Constants); +export default Object.assign({ newInstance, extend, handledEvents }, Constants); From 6b88b4c2d047b3bde956a0f51083f44ea7beb90a Mon Sep 17 00:00:00 2001 From: Alexis Girault Date: Tue, 30 Jan 2018 17:47:39 -0500 Subject: [PATCH 2/5] fix(AbstractWidget): implement listenEvents and setInteractor --- .../Interaction/Widgets/AbstractWidget/api.md | 6 ++- .../Widgets/AbstractWidget/index.js | 40 ++++++++++++++- .../Interaction/Widgets/HandleWidget/api.md | 9 +--- .../Interaction/Widgets/HandleWidget/index.js | 47 ------------------ .../ImageCroppingRegionsWidget/index.js | 49 ------------------- .../Interaction/Widgets/LineWidget/index.js | 47 ------------------ 6 files changed, 43 insertions(+), 155 deletions(-) diff --git a/Sources/Interaction/Widgets/AbstractWidget/api.md b/Sources/Interaction/Widgets/AbstractWidget/api.md index 8bf19961e99..ef6a8e755b4 100644 --- a/Sources/Interaction/Widgets/AbstractWidget/api.md +++ b/Sources/Interaction/Widgets/AbstractWidget/api.md @@ -21,8 +21,10 @@ It defines the representation of the widget ## listenEvents() -Virtual method, needs to be overrides in derived class. -It defined which methods will be invoked for each event (see vtkHandleWidget) +Attaches interactor events to callbacks. Events are defined in +vtkRenderWindowInteractor.handledEvents, and callbacks need to be formatted +as handle${eventToHandle} and implemented in derived classes. +example: handleMouseMove, handleLeftButtonPress... ## render() diff --git a/Sources/Interaction/Widgets/AbstractWidget/index.js b/Sources/Interaction/Widgets/AbstractWidget/index.js index 75c9064b0ff..aead4838fe2 100644 --- a/Sources/Interaction/Widgets/AbstractWidget/index.js +++ b/Sources/Interaction/Widgets/AbstractWidget/index.js @@ -1,5 +1,6 @@ import macro from 'vtk.js/Sources/macro'; import vtkInteractorObserver from 'vtk.js/Sources/Rendering/Core/InteractorObserver'; +import vtkRenderWindowInteractor from 'vtk.js/Sources/Rendering/Core/RenderWindowInteractor'; const { vtkErrorMacro } = macro; @@ -14,8 +15,43 @@ function vtkAbstractWidget(publicAPI, model) { // Virtual method publicAPI.createDefaultRepresentation = () => {}; - // Virtual method - publicAPI.listenEvents = () => {}; + publicAPI.listenEvents = () => { + if (!model.interactor) { + vtkErrorMacro('The interactor must be set before listening events'); + return; + } + vtkRenderWindowInteractor.handledEvents.forEach((eventName) => { + if (publicAPI[`handle${eventName}`]) { + model.unsubscribes.push( + model.interactor[`on${eventName}`]( + publicAPI[`handle${eventName}`], + model.priority + ) + ); + } + }); + }; + + publicAPI.setInteractor = (i) => { + if (i === model.interactor) { + return; + } + + // if we already have an Interactor then stop observing it + if (model.interactor) { + while (model.unsubscribes.length) { + model.unsubscribes.pop().unsubscribe(); + } + } + + model.interactor = i; + + if (i) { + publicAPI.listenEvents(); + } + + publicAPI.modified(); + }; publicAPI.render = () => { if (!model.parent && model.interactor) { diff --git a/Sources/Interaction/Widgets/HandleWidget/api.md b/Sources/Interaction/Widgets/HandleWidget/api.md index 8c681de6fa1..31d44bdff30 100644 --- a/Sources/Interaction/Widgets/HandleWidget/api.md +++ b/Sources/Interaction/Widgets/HandleWidget/api.md @@ -8,7 +8,7 @@ General widget for moving handles (translate and scale) ## Caught events -List of event catch by the widget : +List of events caught by the widget : - MouseMove - LeftButtonPress - LeftButtonRelease @@ -21,13 +21,6 @@ List of event catch by the widget : Create a vtkSphereHandleRepresentation as a default representation. -## listenEvents() - -For each events, define a method called when event is caught : -- onMouseMove then call handleMouseMove -- onLeftButtonPress then call handleLeftButtonPress -- ... - ## allowHandleResize (set/get bool) Define if the widget representation can be resized (default is true) \ No newline at end of file diff --git a/Sources/Interaction/Widgets/HandleWidget/index.js b/Sources/Interaction/Widgets/HandleWidget/index.js index 52233425127..709201b8963 100644 --- a/Sources/Interaction/Widgets/HandleWidget/index.js +++ b/Sources/Interaction/Widgets/HandleWidget/index.js @@ -6,22 +6,11 @@ import Constants from 'vtk.js/Sources/Interaction/Widgets/HandleWidget/Constants const { InteractionState } = vtkHandleRepresentation; const { WidgetState } = Constants; -const { vtkErrorMacro } = macro; // ---------------------------------------------------------------------------- // vtkHandleWidget methods // ---------------------------------------------------------------------------- -const events = [ - 'MouseMove', - 'LeftButtonPress', - 'LeftButtonRelease', - 'MiddleButtonPress', - 'MiddleButtonRelease', - 'RightButtonPress', - 'RightButtonRelease', -]; - function vtkHandleWidget(publicAPI, model) { // Set our className model.classHierarchy.push('vtkHandleWidget'); @@ -53,42 +42,6 @@ function vtkHandleWidget(publicAPI, model) { } }; - // Implemented method - publicAPI.listenEvents = () => { - if (!model.interactor) { - vtkErrorMacro('The interactor must be set before listening events'); - return; - } - events.forEach((eventName) => { - model.unsubscribes.push( - model.interactor[`on${eventName}`](() => { - if (publicAPI[`handle${eventName}`]) { - publicAPI[`handle${eventName}`](); - } - }) - ); - }); - }; - - publicAPI.setInteractor = (i) => { - if (i === model.interactor) { - return; - } - - // if we already have an Interactor then stop observing it - if (model.interactor) { - while (model.unsubscribes.length) { - model.unsubscribes.pop().unsubscribe(); - } - } - - model.interactor = i; - - if (i) { - publicAPI.listenEvents(); - } - }; - publicAPI.handleMouseMove = () => { publicAPI.moveAction(); }; diff --git a/Sources/Interaction/Widgets/ImageCroppingRegionsWidget/index.js b/Sources/Interaction/Widgets/ImageCroppingRegionsWidget/index.js index 59973ee3673..b53b3f02094 100644 --- a/Sources/Interaction/Widgets/ImageCroppingRegionsWidget/index.js +++ b/Sources/Interaction/Widgets/ImageCroppingRegionsWidget/index.js @@ -6,16 +6,6 @@ import Constants from 'vtk.js/Sources/Interaction/Widgets/ImageCroppingRegionsWi const { vtkErrorMacro, VOID, EVENT_ABORT } = macro; const { WidgetState, CropWidgetEvents, Orientation } = Constants; -const events = [ - 'MouseMove', - 'LeftButtonPress', - 'LeftButtonRelease', - 'MiddleButtonPress', - 'MiddleButtonRelease', - 'RightButtonPress', - 'RightButtonRelease', -]; - // ---------------------------------------------------------------------------- // vtkImageCroppingRegionsWidget methods // ---------------------------------------------------------------------------- @@ -64,45 +54,6 @@ function vtkImageCroppingRegionsWidget(publicAPI, model) { } }; - // Implemented method - publicAPI.listenEvents = () => { - if (!model.interactor) { - vtkErrorMacro('The interactor must be set before listening events'); - return; - } - events.forEach((eventName) => { - model.unsubscribes.push( - model.interactor[`on${eventName}`](() => { - if (publicAPI[`handle${eventName}`]) { - return publicAPI[`handle${eventName}`](); - } - return true; - }, model.priority) - ); - }); - }; - - publicAPI.setInteractor = (i) => { - if (i === model.interactor) { - return; - } - - // if we already have an Interactor then stop observing it - if (model.interactor) { - while (model.unsubscribes.length) { - model.unsubscribes.pop().unsubscribe(); - } - } - - model.interactor = i; - - if (i) { - publicAPI.listenEvents(); - } - - publicAPI.modified(); - }; - publicAPI.setVolumeMapper = (volumeMapper) => { if (volumeMapper !== model.volumeMapper) { model.volumeMapper = volumeMapper; diff --git a/Sources/Interaction/Widgets/LineWidget/index.js b/Sources/Interaction/Widgets/LineWidget/index.js index c7a6be45de9..a850184d63d 100644 --- a/Sources/Interaction/Widgets/LineWidget/index.js +++ b/Sources/Interaction/Widgets/LineWidget/index.js @@ -6,7 +6,6 @@ import vtkLineRepresentation from 'vtk.js/Sources/Interaction/Widgets/LineRepres import { State } from 'vtk.js/Sources/Interaction/Widgets/LineRepresentation/Constants'; import Constants from './Constants'; -const { vtkErrorMacro } = macro; const { WidgetState } = Constants; const { InteractionState } = HandleRepConstants; @@ -14,16 +13,6 @@ const { InteractionState } = HandleRepConstants; // vtkHandleWidget methods // ---------------------------------------------------------------------------- -const events = [ - 'MouseMove', - 'LeftButtonPress', - 'LeftButtonRelease', - 'MiddleButtonPress', - 'MiddleButtonRelease', - 'RightButtonPress', - 'RightButtonRelease', -]; - function vtkLineWidget(publicAPI, model) { // Set our className model.classHierarchy.push('vtkLineWidget'); @@ -42,42 +31,6 @@ function vtkLineWidget(publicAPI, model) { } } - // Implemented method - publicAPI.listenEvents = () => { - if (!model.interactor) { - vtkErrorMacro('The interactor must be set before listening events'); - return; - } - events.forEach((eventName) => { - model.unsubscribes.push( - model.interactor[`on${eventName}`](() => { - if (publicAPI[`handle${eventName}`]) { - publicAPI[`handle${eventName}`](); - } - }) - ); - }); - }; - - publicAPI.setInteractor = (i) => { - if (i === model.interactor) { - return; - } - - // if we already have an Interactor then stop observing it - if (model.interactor) { - while (model.unsubscribes.length) { - model.unsubscribes.pop().unsubscribe(); - } - } - - model.interactor = i; - - if (i) { - publicAPI.listenEvents(); - } - }; - publicAPI.setEnable = (enabling) => { const enable = model.enabled; superClass.setEnable(enabling); From 0d2e6b75189624836fdeaec9d30b9879e41cbb88 Mon Sep 17 00:00:00 2001 From: Alexis Girault Date: Wed, 31 Jan 2018 10:37:48 -0500 Subject: [PATCH 3/5] fix(HandleWidget): handle event aborting if needed --- .../Interaction/Widgets/HandleWidget/index.js | 80 +++++++++---------- 1 file changed, 36 insertions(+), 44 deletions(-) diff --git a/Sources/Interaction/Widgets/HandleWidget/index.js b/Sources/Interaction/Widgets/HandleWidget/index.js index 709201b8963..8cc6cc76b82 100644 --- a/Sources/Interaction/Widgets/HandleWidget/index.js +++ b/Sources/Interaction/Widgets/HandleWidget/index.js @@ -4,6 +4,7 @@ import vtkSphereHandleRepresentation from 'vtk.js/Sources/Interaction/Widgets/Sp import vtkHandleRepresentation from 'vtk.js/Sources/Interaction/Widgets/HandleRepresentation'; import Constants from 'vtk.js/Sources/Interaction/Widgets/HandleWidget/Constants'; +const { VOID, EVENT_ABORT } = macro; const { InteractionState } = vtkHandleRepresentation; const { WidgetState } = Constants; @@ -42,33 +43,19 @@ function vtkHandleWidget(publicAPI, model) { } }; - publicAPI.handleMouseMove = () => { - publicAPI.moveAction(); - }; + publicAPI.handleMouseMove = () => publicAPI.moveAction(); - publicAPI.handleLeftButtonPress = () => { - publicAPI.selectAction(); - }; + publicAPI.handleLeftButtonPress = () => publicAPI.selectAction(); - publicAPI.handleLeftButtonRelease = () => { - publicAPI.endSelectAction(); - }; + publicAPI.handleLeftButtonRelease = () => publicAPI.endSelectAction(); - publicAPI.handleMiddleButtonPress = () => { - publicAPI.translateAction(); - }; + publicAPI.handleMiddleButtonPress = () => publicAPI.translateAction(); - publicAPI.handleMiddleButtonRelease = () => { - publicAPI.endSelectAction(); - }; + publicAPI.handleMiddleButtonRelease = () => publicAPI.endSelectAction(); - publicAPI.handleRightButtonPress = () => { - publicAPI.scaleAction(); - }; + publicAPI.handleRightButtonPress = () => publicAPI.scaleAction(); - publicAPI.handleRightButtonRelease = () => { - publicAPI.endSelectAction(); - }; + publicAPI.handleRightButtonRelease = () => publicAPI.endSelectAction(); publicAPI.selectAction = () => { const pos = model.interactor.getEventPosition( @@ -83,13 +70,13 @@ function vtkHandleWidget(publicAPI, model) { ]; model.widgetRep.computeInteractionState(position); if (model.widgetRep.getInteractionState() === InteractionState.OUTSIDE) { - return; + return VOID; } - model.widgetRep.startComplexWidgetInteraction(position); model.widgetState = WidgetState.ACTIVE; model.widgetRep.setInteractionState(InteractionState.SELECTING); genericAction(); + return EVENT_ABORT; }; publicAPI.translateAction = () => { @@ -105,43 +92,47 @@ function vtkHandleWidget(publicAPI, model) { ]; model.widgetRep.startComplexWidgetInteraction(position); if (model.widgetRep.getInteractionState() === InteractionState.OUTSIDE) { - return; + return VOID; } model.widgetState = WidgetState.ACTIVE; model.widgetRep.setInteractionState(InteractionState.TRANSLATING); genericAction(); + return EVENT_ABORT; }; publicAPI.scaleAction = () => { - if (model.allowHandleResize) { - const pos = model.interactor.getEventPosition( - model.interactor.getPointerIndex() - ); - const boundingContainer = model.interactor - .getCanvas() - .getBoundingClientRect(); - const position = [ - pos.x - boundingContainer.left, - pos.y + boundingContainer.top, - ]; - model.widgetRep.startComplexWidgetInteraction(position); - if (model.widgetRep.getInteractionState() === InteractionState.OUTSIDE) { - return; - } - model.widgetState = WidgetState.ACTIVE; - model.widgetRep.setInteractionState(InteractionState.SCALING); - genericAction(); + if (!model.allowHandleResize) { + return VOID; + } + const pos = model.interactor.getEventPosition( + model.interactor.getPointerIndex() + ); + const boundingContainer = model.interactor + .getCanvas() + .getBoundingClientRect(); + const position = [ + pos.x - boundingContainer.left, + pos.y + boundingContainer.top, + ]; + model.widgetRep.startComplexWidgetInteraction(position); + if (model.widgetRep.getInteractionState() === InteractionState.OUTSIDE) { + return VOID; } + model.widgetState = WidgetState.ACTIVE; + model.widgetRep.setInteractionState(InteractionState.SCALING); + genericAction(); + return EVENT_ABORT; }; publicAPI.endSelectAction = () => { if (model.widgetState !== WidgetState.ACTIVE) { - return; + return VOID; } model.widgetState = WidgetState.START; model.widgetRep.highlight(0); publicAPI.invokeEndInteractionEvent(); publicAPI.render(); + return EVENT_ABORT; }; publicAPI.moveAction = () => { @@ -165,12 +156,13 @@ function vtkHandleWidget(publicAPI, model) { ) { publicAPI.render(); } - return; + return VOID; } model.widgetRep.complexWidgetInteraction(position); publicAPI.invokeInteractionEvent(); publicAPI.render(); + return EVENT_ABORT; }; } From 73c48a4bfbccec6ffb03f786da06877cc3832a8d Mon Sep 17 00:00:00 2001 From: Alexis Girault Date: Wed, 31 Jan 2018 10:17:45 -0500 Subject: [PATCH 4/5] fix(macro): fix callback infinite loop If new callbacks were added to the array during a callback (example: when adding a widget that also listens to events), and if these callbacks have a higher priority, the sorting of those callbacks would prepend them to the array, which means the previous callback would be triggered again. Using a copied array defined when `invoke` is called addresses that issue. --- Sources/macro.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Sources/macro.js b/Sources/macro.js index 01345819096..260cb6220e5 100644 --- a/Sources/macro.js +++ b/Sources/macro.js @@ -736,8 +736,11 @@ export function event(publicAPI, model, eventName) { return; } /* eslint-disable prefer-rest-params */ - for (let index = 0; index < callbacks.length; ++index) { - const [, cb, priority] = callbacks[index]; + // Go through a copy of the callbacks array in case new callbacks + // get prepended within previous callbacks + const currentCallbacks = callbacks.slice(); + for (let index = 0; index < currentCallbacks.length; ++index) { + const [, cb, priority] = currentCallbacks[index]; if (priority < 0) { setTimeout(() => cb.apply(publicAPI, arguments), 1 - priority); } else if (cb) { From 93be5c3088fbcaf8df51eddcb9b7a3f25c162c1d Mon Sep 17 00:00:00 2001 From: Alexis Girault Date: Wed, 31 Jan 2018 11:40:17 -0500 Subject: [PATCH 5/5] fix(AbstractWidget): Ensure we do not re-register callbacks `listenEvents` would usually be called by `setEnable` and `setInteractor`, which would register callbacks twice. Also, `listenEvents` is part of the publicAPI, so callbacks could have been registered again by explicitly calling that function. We now ensure that those callbacks are unsubscribed from the event before subscribing again. --- .../Widgets/AbstractWidget/index.js | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/Sources/Interaction/Widgets/AbstractWidget/index.js b/Sources/Interaction/Widgets/AbstractWidget/index.js index aead4838fe2..51f5f76f1b4 100644 --- a/Sources/Interaction/Widgets/AbstractWidget/index.js +++ b/Sources/Interaction/Widgets/AbstractWidget/index.js @@ -20,6 +20,13 @@ function vtkAbstractWidget(publicAPI, model) { vtkErrorMacro('The interactor must be set before listening events'); return; } + + // Remove current events + while (model.unsubscribes.length) { + model.unsubscribes.pop().unsubscribe(); + } + + // Check what events we can handle and register callbacks vtkRenderWindowInteractor.handledEvents.forEach((eventName) => { if (publicAPI[`handle${eventName}`]) { model.unsubscribes.push( @@ -36,17 +43,9 @@ function vtkAbstractWidget(publicAPI, model) { if (i === model.interactor) { return; } - - // if we already have an Interactor then stop observing it - if (model.interactor) { - while (model.unsubscribes.length) { - model.unsubscribes.pop().unsubscribe(); - } - } - model.interactor = i; - if (i) { + if (i && model.enabled) { publicAPI.listenEvents(); } @@ -60,11 +59,11 @@ function vtkAbstractWidget(publicAPI, model) { }; publicAPI.setEnable = (enable) => { + if (enable === model.enabled) { + return; + } + if (enable) { - if (model.enabled) { - // widget already enabled - return; - } if (!model.interactor) { vtkErrorMacro( 'The interactor must be set prior to enabling the widget' @@ -97,13 +96,9 @@ function vtkAbstractWidget(publicAPI, model) { model.widgetRep.buildRepresentation(); model.currentRenderer.addViewProp(model.widgetRep); } else { - if (!model.enabled) { - // already - return; - } model.enabled = 0; - // Don't listen events + // Don't listen to events while (model.unsubscribes.length) { model.unsubscribes.pop().unsubscribe(); }