feat(Touch): added touch events to tools #247

Ouwen · 2022-10-17T09:02:13Z

Adds the following cornerstone events:

TOUCH_START,
TOUCH_START_ACTIVATE,
MULTI_TOUCH_START,
MULTI_TOUCH_START_ACTIVATE,
TOUCH_DRAG,
MULTI_TOUCH_DRAG,
TOUCH_END

Currently only uses native browser events (touchstart, touchmove, touchend)

netlify · 2022-10-17T09:02:18Z

✅ Deploy Preview for cornerstone-3d-docs ready!

Name	Link
🔨 Latest commit	`ec264ff`
🔍 Latest deploy log	https://app.netlify.com/sites/cornerstone-3d-docs/deploys/63d41aaedfdab00009c7464f
😎 Deploy Preview	https://deploy-preview-247--cornerstone-3d-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Ouwen · 2022-10-17T18:34:25Z

@sedghi let me know what you think

packages/tools/examples/stackManipulationToolsTouch/index.ts

packages/tools/src/store/removeEnabledElement.ts

packages/tools/src/eventListeners/touch/index.ts

sedghi · 2022-10-19T02:54:05Z

This looks great at first sight. So are you plannig to uncommente the touch event listeners in annotation tools later? Like here https://github.com/cornerstonejs/cornerstone3D-beta/blob/main/packages/tools/src/tools/annotation/LengthTool.ts#L490

Also, are you saying we don't need the hammerJS anymore?

Ouwen · 2022-10-19T14:09:51Z

I think either hammerjs or contactjs will be needed to trigger the pinch/pan functionality

So I had some questions regarding hammerjs usage in the cornerstoneTools v2.

I noticed that hammerjs has a press and tap functionality, but cornerstone touch eventlistener seem to internally implement the press/tap. Was there a reason for this? cornerstonejs/cornerstoneTools#120

Last question in cornerstoneTools v2, there were some touch specific tool copies (pan vs panmultitouch) would it make more sense to have the pan tool have configuration which enables multitooltouch instead of these being two separate tools?

sedghi · 2022-10-20T14:12:59Z

Hammer.js would add MULTI_TOUCH_PAN, PINCH, and ROTATE, PRESS (longer and tap)

sedghi · 2022-10-20T14:14:48Z

Example to get shown need to be added here

sedghi · 2022-10-20T14:16:11Z

In case of changing serve, we need to update the docs

sedghi · 2022-10-25T12:42:12Z

Is this ready to go?

Ouwen · 2022-10-25T23:23:08Z

Not yet, am in the process of adding the multitouch and gestures. I think we can avoid hammerjs dependency as well

Ouwen · 2022-10-27T10:43:41Z

@sedghi this is almost ready for your review. This is becoming a larger change than I initially intended.

Highlights

We don't use the hammerjs dependency
Multi touch is "reduced" to single touch via taking means.
- Distances are calculated as the mean distance of unique unordered points
- In the case of a drag event where the user lifts a finger for example, the deltas of 3 last touch => 2 current touch are calculated as the mean point of the 3touch case minus the mean point of the 2touch case
- Rotation is still todo, but following the case above we can take the mean of the 3 touch case and the mean of the 2 touch case. We can first translate the the 2 touch points by looking at the difference in mean points from 2points => 3points. Then for each matched point identifier we can calculate the theta and take the mean of all thetas.

TODO:

Since touch handles an array of touch points we have an array of IPoints[] as the currentPoints, lastPoints, and startPoints. This doesn't play nice with the mouse interface since IPoints is not an array.
I'll probably keep the IPoints[] under a different key (currentPointsList?). Then I'll reserve the original key with a datatype compatible with IPoints

wayfarer3130 · 2022-11-21T17:11:39Z

packages/tools/src/types/EventTypes.ts

+/**
+ * The normalized mouse event detail
+ */
+type NormalizedTouchEventDetail = {


This doesn't have anything different from the normalized mouse event detail - please re-use the existing type (you can rename it if you want, but only have one instance of it)

wayfarer3130 · 2022-11-21T17:13:13Z

packages/tools/src/types/EventTypes.ts

+/**
+ * EventDetail for touchstart event
+ */
+type TouchStartEventDetail = NormalizedTouchEventDetail & {


Please start with the MouseStartEventDetail, and then add specific behaviour that identifies this as a touch event. That allows leaving most of the tools classes alone, and only tweaking them if needed. There should/must be an identifier that identifies this as a touch event, and probably something that identifies what "drag" type to listen to.

wayfarer3130 · 2022-11-21T17:14:39Z

packages/tools/src/types/EventTypes.ts

+ */
+type TouchStartEventDetail = NormalizedTouchEventDetail & {
+  /** The starting points of the touch event. */
+  startPoints: ITouchPoints;


Let the values that can be inheritted from MouseStartEventDetail be inheritted, and only add the extras as required.

wayfarer3130 · 2022-11-21T17:18:45Z

packages/tools/src/tools/MagnifyTool.ts

@@ -101,6 +103,10 @@ class MagnifyTool extends BaseTool {
    return true;
  };

+  preTouchStartCallback = (evt: EventTypes.TouchStartActivateEventType) => {


You won't need many, if any of this type of change if the preMouseDownCallback just takes a mouse down activate event type, and can check if it is a touch event sub-type.

diegohennrich · 2023-01-24T21:49:22Z

Could you please review it? @Ouwen @sedghi

Ouwen

I did a pretty thorough review for each file. I want to be mindful that this is a very large PR and would prefer it to go in before other tool related PRs if possible @sedghi. Otherwise subsequent changes will need to rebased and force push might sneak diffs.

These were the main changes I found, but many of them are cosmetic which might be better handled in another (smaller) PR.

Ouwen · 2023-01-25T01:14:00Z

packages/tools/src/tools/CrosshairsTool.ts

-    evt: EventTypes.MouseDownActivateEventType,
-    interactionType: string
+    evt: EventTypes.InteractionEventType,
+    interactionType = 'mouse'


I don't think this is used in this function. Minor nit that might be better to do in a separate PR.

Ouwen · 2023-01-25T01:14:13Z

packages/tools/src/tools/CrosshairsTool.ts

@@ -469,7 +469,7 @@ class CrosshairsTool extends AnnotationTool {
  }

  handleSelectedCallback = (
-    evt: EventTypes.MouseDownEventType,
+    evt: EventTypes.InteractionEventType,
    annotation: Annotation,
    handle: ToolHandle,
    interactionType = 'mouse'


Ouwen · 2023-01-25T01:21:05Z

packages/tools/src/tools/annotation/ArrowAnnotateTool.ts

-  doubleClickCallback = (evt: EventTypes.MouseUpEventType) => {
+  touchTapCallback = (evt: EventTypes.TouchTapEventType) => {
+    if (evt.detail.taps == 2) {
+      this.doubleClickCallback(evt as unknown as EventTypes.MouseUpEventType);


this can pass evt without as unknown as EventTypes... with the InteractionEventType change we made

Actually, I think we need to use the specific TouchTapEventDetail here because we don't have the "taps" parameter from InteractionEventType. So "TouchTapEventDetail" is inherited from the interaction.

type TouchTapEventDetail = NormalizedInteractionEventDetail & TouchCustomEventDetail & { currentPointsList: ITouchPoints[]; currentPoints: ITouchPoints; taps: number; <---- };

As we discussed yesterday, this is a special case where we need to use the taps parameter and we don't have it inherited from the InteractionType. So, make sense to use the TouchTapEventDetail at this specific point.

Ouwen · 2023-01-25T01:23:37Z

packages/tools/src/tools/annotation/DragProbeTool.ts

@@ -51,7 +51,7 @@ class DragProbeTool extends ProbeTool {
  }

  postMouseDownCallback = (


Needs a postTouchDownCallback exposed to have touch events dispatched to this tool.

"postTouchDownCallback" or "postTouchStartCallback"?

I believe we're using the "postTouchStartCallback", see below:

packages/tools/src/eventDispatchers/touchEventHandler/touchstart.ts

if (activeTool && typeof activeTool.postTouchStartCallback === 'function') { const consumedEvent = activeTool.postTouchStartCallback(evt); if (consumedEvent) { // If the tool claims it consumed the event, prevent further checks. return; } }

should be postTouchStart callback you are correct. Rather than copying the mouse function you can just call the postMouseDownCallback function inside of postTouchStart callback

Ouwen · 2023-01-25T01:28:58Z

packages/tools/src/tools/base/AnnotationTool.ts

@@ -172,7 +172,8 @@ abstract class AnnotationTool extends AnnotationDisplayTool {
    element: HTMLDivElement,
    annotation: Annotation,
    canvasCoords: Types.Point2,
-    proximity: number
+    proximity: number,
+    interactionType = 'mouse'


This should be removed

Ouwen · 2023-01-25T01:30:41Z

packages/tools/src/tools/segmentation/CircleScissorsTool.ts

-    // element.removeEventListener(Events.TOUCH_END, this._mouseUpCallback)
-    // element.removeEventListener(Events.TOUCH_DRAG, this._mouseDragCallback)
+    // element.removeEventListener(Events.TOUCH_END, this._endCallback)
+    // element.removeEventListener(Events.TOUCH_DRAG, this._dragCallback)


Wrongly commented out same as above.

also removeEventListener to the Events.TOUCH_TAP event

Ouwen · 2023-01-25T01:33:01Z

packages/tools/src/tools/segmentation/RectangleScissorsTool.ts

-    // element.addEventListener(Events.TOUCH_END, this._mouseUpCallback)
-    // element.addEventListener(Events.TOUCH_DRAG, this._mouseDragCallback)
+    // element.addEventListener(Events.TOUCH_END, this._endCallback)
+    // element.addEventListener(Events.TOUCH_DRAG, this._dragCallback)


This should be enabled and include

element.addEventListener(Events.TOUCH_TAP, this._endCallback)

Ouwen · 2023-01-25T01:33:11Z

packages/tools/src/tools/segmentation/RectangleScissorsTool.ts

-    // element.removeEventListener(Events.TOUCH_END, this._mouseUpCallback)
-    // element.removeEventListener(Events.TOUCH_DRAG, this._mouseDragCallback)
+    // element.removeEventListener(Events.TOUCH_END, this._endCallback)
+    // element.removeEventListener(Events.TOUCH_DRAG, this._dragCallback)


Same here
This should be enabled and include

element. removeEventListener(Events.TOUCH_TAP, this._endCallback)

Ouwen · 2023-01-25T01:33:36Z

packages/tools/src/tools/segmentation/SphereScissorsTool.ts

-    // element.addEventListener(Events.TOUCH_END, this._mouseUpCallback)
-    // element.addEventListener(Events.TOUCH_DRAG, this._mouseDragCallback)
+    // element.addEventListener(Events.TOUCH_END, this._endCallback)
+    // element.addEventListener(Events.TOUCH_DRAG, this._dragCallback)


Same as other segmentation tools.

Ouwen · 2023-01-25T01:33:54Z

packages/tools/src/tools/segmentation/SphereScissorsTool.ts

-    // element.removeEventListener(Events.TOUCH_END, this._mouseUpCallback)
-    // element.removeEventListener(Events.TOUCH_DRAG, this._mouseDragCallback)
+    // element.removeEventListener(Events.TOUCH_END, this._endCallback)
+    // element.removeEventListener(Events.TOUCH_DRAG, this._dragCallback)


Same as other segmentation tools

Ouwen · 2023-01-25T01:45:17Z

@sedghi lmk if there are any outstanding changes required aside from the above... thanks again for taking the time to review this large diff which pokes other existing files.

sedghi · 2023-01-25T15:31:43Z

@Ouwen probably yes, but I think I would like to do one last review. Just let me know when it is ready for it

diegohennrich · 2023-01-25T21:44:48Z

Could you review it, please? @Ouwen @sedghi

Ouwen · 2023-01-25T22:09:59Z

packages/tools/src/tools/annotation/ArrowAnnotateTool.ts

    }
  };

-  doubleClickCallback = (evt: EventTypes.InteractionEventType): void => {
+  doubleClickCallback = (evt: EventTypes.TouchTapEventType): void => {


should stay InteractionEventType since taps are not used

Ouwen · 2023-01-25T22:10:06Z

packages/tools/src/tools/annotation/ArrowAnnotateTool.ts

@@ -405,11 +405,11 @@ class ArrowAnnotateTool extends AnnotationTool {

  touchTapCallback = (evt: EventTypes.TouchTapEventType) => {
    if (evt.detail.taps == 2) {
-      this.doubleClickCallback(evt as unknown as EventTypes.MouseUpEventType);
+      this.doubleClickCallback(evt as EventTypes.TouchTapEventType);


should just be evt

should be evt since touchTapCallback = (evt: EventTypes.TouchTapEventType) => {

Ouwen · 2023-01-26T00:00:37Z

packages/tools/src/tools/segmentation/RectangleScissorsTool.ts

@@ -321,8 +321,8 @@ class RectangleScissorsTool extends BaseTool {
    element.removeEventListener(Events.MOUSE_CLICK, this._endCallback);
    element.removeEventListener(Events.TOUCH_TAP, this._endCallback);


let's move the touch_tap back down to after line 325 to match mouse formatting

diegohennrich · 2023-01-26T18:09:52Z

I think everything was done. Could you review it, please? @Ouwen @sedghi

Ouwen · 2023-01-26T19:01:57Z

@sedghi, I went through another pass at it, I think it is ready for review

sedghi · 2023-01-27T17:28:18Z

packages/docs/docs/concepts/cornerstone-tools/touch.md

+toolGroup.setToolActive(WindowLevelTool.toolName, {
+  bindings: [
+    {
+      mouseButton: MouseBindings.Primary, // special condition for one finger touch
+    },
+  ],
+});
+```
+
+The `MouseBindings.Primary` is a special binding type which will
+automatically bind single finger touch.


I don't understand, why the API here is different, is this supposed to do a mouse binding or touch bindings? If it is touch, I expected the following for consistensy

toolGroup.setToolActive(WindowLevelTool.toolName, { bindings: [{ numTouchPoints: 1 }], });

sedghi · 2023-01-27T17:28:34Z

packages/tools/examples/stackManipulationToolsTouch/index.ts

+  toolGroup.setToolActive(WindowLevelTool.toolName, {
+    bindings: [
+      {
+        mouseButton: MouseBindings.Primary, // Left Click
+      },
+    ],
+  });


I remember you said this

Yeah numTouchPoints = 1 is a special case where we also treat it as a "Primary Mouse" event. This way existing modes which just bind a primary mouse get the free touch win without additional explicit config.

But don't think we should treat the numPoint ===1 special, why not for the others? why just 1 ?

packages/tools/src/eventDispatchers/mouseEventHandlers/mouseMove.ts

sedghi

The work done on this PR is truly exceptional and greatly appreciated. Kudos to the Gradient Health team as always 🎉 💯

Ouwen force-pushed the gradienthealth/added_touch_events branch 4 times, most recently from d3827d4 to f2fe364 Compare October 17, 2022 12:36

Ouwen force-pushed the gradienthealth/added_touch_events branch from f2fe364 to 0d58ee2 Compare October 19, 2022 01:25

sedghi reviewed Oct 19, 2022

View reviewed changes

packages/tools/examples/stackManipulationToolsTouch/index.ts Show resolved Hide resolved

sedghi reviewed Oct 19, 2022

View reviewed changes

packages/tools/src/store/removeEnabledElement.ts Show resolved Hide resolved

sedghi reviewed Oct 19, 2022

View reviewed changes

packages/tools/src/eventListeners/touch/index.ts Outdated Show resolved Hide resolved

Ouwen force-pushed the gradienthealth/added_touch_events branch from 0d58ee2 to b14c070 Compare October 27, 2022 09:01

Ouwen force-pushed the gradienthealth/added_touch_events branch 5 times, most recently from 9bac6d5 to a49735b Compare October 28, 2022 21:08

Ouwen force-pushed the gradienthealth/added_touch_events branch from a49735b to 7580eeb Compare November 6, 2022 00:35

Ouwen force-pushed the gradienthealth/added_touch_events branch from 7580eeb to 7219316 Compare November 19, 2022 04:42

wayfarer3130 reviewed Nov 21, 2022

View reviewed changes

diegohennrich force-pushed the gradienthealth/added_touch_events branch 2 times, most recently from 64e932c to 78287da Compare January 24, 2023 21:47

diegohennrich added 2 commits January 24, 2023 19:03

Build conflicts solved

bb546b2

API Updated

b826554

Ouwen commented Jan 25, 2023

View reviewed changes

Tools changes

8f456f9

Ouwen commented Jan 25, 2023

View reviewed changes

Listeners changed

c751d4b

Ouwen commented Jan 26, 2023

View reviewed changes

diegohennrich added 4 commits January 25, 2023 21:32

Event changed

ad43438

Listeners Reorganized

733d81b

postTouchDownCallback function fixed

2be9f4e

postTouchStartCallback function fixed

f07217b

added documentation and minor nit

1b5eb40

sedghi reviewed Jan 27, 2023

View reviewed changes

packages/tools/src/eventDispatchers/mouseEventHandlers/mouseMove.ts Outdated Show resolved Hide resolved

Ouwen added 2 commits January 27, 2023 18:36

docs: added todo and example

d560635

removed import

ec264ff

sedghi approved these changes Jan 27, 2023

View reviewed changes

sedghi changed the title ~~feat: added touch events to @cornerstonejs/tools~~ feat(Touch): added touch events to tools Jan 27, 2023

sedghi merged commit e35f963 into cornerstonejs:main Jan 27, 2023

Ouwen deleted the gradienthealth/added_touch_events branch February 7, 2023 05:51

		@@ -51,7 +51,7 @@ class DragProbeTool extends ProbeTool {
		}

		postMouseDownCallback = (

		@@ -321,8 +321,8 @@ class RectangleScissorsTool extends BaseTool {
		element.removeEventListener(Events.MOUSE_CLICK, this._endCallback);
		element.removeEventListener(Events.TOUCH_TAP, this._endCallback);

feat(Touch): added touch events to tools #247

feat(Touch): added touch events to tools #247

Conversation

Ouwen commented Oct 17, 2022

netlify bot commented Oct 17, 2022 • edited Loading

✅ Deploy Preview for cornerstone-3d-docs ready!

Ouwen commented Oct 17, 2022

sedghi commented Oct 19, 2022

Ouwen commented Oct 19, 2022

sedghi commented Oct 20, 2022 • edited Loading

sedghi commented Oct 20, 2022

sedghi commented Oct 20, 2022

sedghi commented Oct 25, 2022

Ouwen commented Oct 25, 2022

Ouwen commented Oct 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegohennrich commented Jan 24, 2023

Ouwen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ouwen commented Jan 25, 2023 • edited Loading

sedghi commented Jan 25, 2023

diegohennrich commented Jan 25, 2023

Ouwen Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegohennrich commented Jan 26, 2023

Ouwen commented Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sedghi left a comment

Choose a reason for hiding this comment

netlify bot commented Oct 17, 2022 •

edited

Loading

sedghi commented Oct 20, 2022 •

edited

Loading

Ouwen commented Jan 25, 2023 •

edited

Loading

Ouwen Jan 25, 2023 •

edited

Loading

Ouwen commented Jan 26, 2023 •

edited

Loading