cornerstone3D icon indicating copy to clipboard operation
cornerstone3D copied to clipboard

feat: added touch events to @cornerstonejs/tools

Open Ouwen opened this issue 1 year ago • 2 comments

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)

Ouwen avatar Oct 17 '22 09:10 Ouwen

Deploy Preview for cornerstone-3d-docs ready!

Name Link
Latest commit ec264ffe988bd235f32c0f259b5c50948cf45ff8
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...

QR Code

Use your smartphone camera to open QR code link.

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

netlify[bot] avatar Oct 17 '22 09:10 netlify[bot]

@sedghi let me know what you think

Ouwen avatar Oct 17 '22 18:10 Ouwen

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?

sedghi avatar Oct 19 '22 02:10 sedghi

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? https://github.com/cornerstonejs/cornerstoneTools/pull/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?

Ouwen avatar Oct 19 '22 14:10 Ouwen

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

sedghi avatar Oct 20 '22 14:10 sedghi

Example to get shown need to be added here

sedghi avatar Oct 20 '22 14:10 sedghi

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

sedghi avatar Oct 20 '22 14:10 sedghi

Is this ready to go?

sedghi avatar Oct 25 '22 12:10 sedghi

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

Ouwen avatar Oct 25 '22 23:10 Ouwen

@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

Ouwen avatar Oct 27 '22 10:10 Ouwen

Hey all,

Is there any news on this PR? This is a very handy feature.

BardiaKh avatar Dec 20 '22 02:12 BardiaKh

@BardiaKh currently in progress!

Ouwen avatar Dec 20 '22 14:12 Ouwen

@sedghi @wayfarer3130 this should be ready for review. Main changes to note:

  • TouchEvents and MouseEvents Types have been better merged to be InteractionEvent types. Tools that are agnostic to Touch and Mouse can use this event type instead. Touch and Mouse events then inherit from the interaction event in case there are specific needs (mouseButton, touch pressure, etc).

  • We no longer use hammerjs for touch compared to v2

Ouwen avatar Jan 15 '23 17:01 Ouwen

@diegohennrich @Ouwen Let me know when this is ready for review Also you need to remove the type, we don't need that anymore

image

sedghi avatar Jan 20 '23 03:01 sedghi

Could you please review it? @Ouwen @sedghi

diegohennrich avatar Jan 24 '23 21:01 diegohennrich

@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.

Ouwen avatar Jan 25 '23 01:01 Ouwen

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

sedghi avatar Jan 25 '23 15:01 sedghi

Could you review it, please? @Ouwen @sedghi

diegohennrich avatar Jan 25 '23 21:01 diegohennrich

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

diegohennrich avatar Jan 26 '23 18:01 diegohennrich

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

Ouwen avatar Jan 26 '23 19:01 Ouwen