feat(touch): more optimized touch interactions #461

Ouwen · 2023-03-05T01:23:54Z

Removes start lag in touch
Swipe is optimized to have no lag and is only active for the first 200ms (Tap Swipe is more common than random swipe)
Tap occurs with Touch Start / Touch End

Test Swipe:

Launch the example page on a touch device.
Quickly swipe over the image with a single finger.
An info message should be displayed indicating that a swipe occurred.

netlify · 2023-03-05T01:23:59Z

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

Name	Link
🔨 Latest commit	`cd866cc`
🔍 Latest deploy log	https://app.netlify.com/sites/cornerstone-3d-docs/deploys/64124ac62a014c0008dbd7cb
😎 Deploy Preview	https://deploy-preview-461--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.

jbocce · 2023-03-06T13:25:36Z

Hi @Ouwen . Thanks for submitting this PR. Could you please add details as to how this PR can be tested? Thanks.

Ouwen · 2023-03-07T05:58:33Z

@jbocce I've updated the PR. The changes can be tested by running the example:
yarn run example stackManipulationToolsTouch

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

jbocce · 2023-03-07T03:20:02Z

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

-    state = JSON.parse(JSON.stringify(defaultState));
-  }, state.touchStartDelay - 10);
+    tapState = JSON.parse(JSON.stringify(defaultTapState));
+  }, tapState.tapWindow);


Should not the timeout time here be tapState.tapWindow - (currentTime - tapState.startTime.getTime())? Having the timeout time as just tapState.tapWindow would mean that the tap event would be (unnecessarily) delayed and thus the ultimate (UI/user) action would be delayed. Take this (exaggerated) example: take a single tap where the tap actually occurs at tapState.tapWindow - 1ms. Then a timeout would be set for tapState.tapWindowms to fire the actual tap event. Thus the total time for the event to fire from the touch start is almost 2 * tapState.tapWindowms. Please verify that I have this correct. Apologies if I do not.

So the reason for the window is to aggregate multiple taps. If the user taps, there will always be a delay of tapWindowMs before the actual tap event fires.

In the case the user taps again within the TapWindow, the setTimeout would clear and wait for another tapWindowMs to pass before firing an event that 2 taps were heard.

The alternative approach to this is that we fire events every time a tap happens. This means a triple tap would actually be 3 events. This would prevent lag, but the tap events can get noisy. The windowing logic would then need to be pushed down for any listener of the tap event to set their own windows + timeouts.

Tap(1)
Tap(2)
Tap(3)

Yes I understand why the aggregation and NO, we do NOT want the alternative approach you suggested.

I apologize because I think I misread the code before. However having both lines 379 and 391 are a bit confusing to me.

Line 379 together with the setTimeout on line 417 sort of suggests to me that a single tap has to fall within 300ms (i.e. tapState.tapWindow), double tap has to fall within 600ms, etc. However line 391 to me reads, for example, if the second tap happens > 300ms after the first tap (and say 400ms) from the first tap , then do not do the double tap (i.e. return). JSYK I verified this via debugging whereby the debugger showed me that I did two taps within 351ms and I did NOT get a double tap. I honestly do not think we need both checks, but I could be wrong.

jbocce · 2023-03-07T03:34:21Z

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

+  };
+  if (Math.abs(x) > state.swipeDistanceThreshold) {
+    eventDetail.swipe = x > 0 ? Swipe.RIGHT : Swipe.LEFT;
+    triggerEventCallback(eventDetail.element, TOUCH_SWIPE, eventDetail);


If I read the code correct, it seems that both a TOUCH_SWIPE and a TOUCH_DRAG can be fired during the same interaction. Is that correct? If so, then that seems a bit surprising - I would expect them to be mutually exclusive. For example, one may want to swipe to navigate a stack - not, navigate and, say, pan. If I am mistaken, then ignore this comment. I suppose firing both events leaves it up to the embedding application whether both should be handled or just one. Thoughts?

That's right, previously TOUCH_SWIPE and TOUCH_DRAG were mutually exclusive; however, this implementation required a setTimeout delay which makes either drag or swipe feel laggy. (Currently this is the case with Tap Event, see comment regarding whether to push down to embedding application layer)

Application layer will need to manage SWIPE/DRAG events happening together to have responsive touch.

For most use cases, drag and swipe gestures don't really conflict. And since drag event is clamped to 10 the affect is not that bad for a mis swipe or mis drag.

Having them mutually exclusive is fine - leave this as is.

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

jbocce · 2023-03-07T13:20:08Z

@jbocce I've updated the PR. The changes can be tested by running the example: yarn run example stackManipulationToolsTouch

Thanks @Ouwen for the updates - they definitely helped.

Could you please add specific test cases to the description of the PR. For example...

Test Swipe

Launch the example page on a touch device.
Quickly swipe over the image with a single finger.
An info message should be displayed indicating that a swipe occurred.

I tried the example on my phone and I found the following...

I cannot get a swipe info message to display ever. What am I doing wrong?
If I tap twice on two locations that are greater than 48px but within the double tap time tolerance (300ms), I do NOT get two individual taps. I was expecting one tap event for each location. Does this make sense?
Note that our double (mouse) click tolerance is 400ms - we should likely be consistent here.

Overall good work. Mouse and touch interactions are difficult things to get correct - so far so good.

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

Ouwen · 2023-03-07T17:30:08Z

@jbocce thanks for taking the time to a through code review on this PR. Let me know what you think of my responses and I can make those changes.

Main changes I'll be making:

Rename vars and update stale comments
Change distance tolerance from 48px to 32px and create a TODO for more configurable touch thresholds

Open discussion:

Should tap logic be pushed to embedding application or should multiple taps be handled in this API?
I would push for it being handled in this API. In the future, the user can set a configuration to be exposed to every tap event. If they need better latency for added complexity.

jbocce · 2023-03-08T14:25:40Z

@jbocce thanks for taking the time to a through code review on this PR. Let me know what you think of my responses and I can make those changes.

Main changes I'll be making:

Rename vars and update stale comments

Change distance tolerance from 48px to 32px and create a TODO for more configurable touch thresholds

Open discussion:

Should tap logic be pushed to embedding application or should multiple taps be handled in this API?
I would push for it being handled in this API. In the future, the user can set a configuration to be exposed to every tap event. If they need better latency for added complexity.

Yes please make the changes.

I am not sure what you mean by the tap logic... you mean swipe logic right? If so then yes I agree.

Also, in case you missed them please have a look at these overall comments I made #461 (comment).

Ouwen · 2023-03-09T16:58:36Z

I cannot get a swipe info message to display ever. What am I doing wrong?

Hmmm, that's strange it might require faster swiping? Which device are you on?

Note that our double (mouse) click tolerance is 400ms - we should likely be consistent here.

We can change the tap to 400ms

If I tap twice on two locations that are greater than 48px but within the double tap time tolerance (300ms), I do NOT get two individual taps. I was expecting one tap event for each location. Does this make sense?

Hmmm I think this makes sense. The tap logic currently waits for "tap chains" to complete. Instead, if a tap is outside the 48px it should start a new "tap chain" (as the follow up tap could make it a double tap)

Regarding tap chains, would it be acceptable to include this as a TODO?

jbocce · 2023-03-09T17:17:07Z

@Ouwen

I am using a Pixel 5 phone. I have (finally) managed to register swipes, but NOT consistently. It just does not feel quite right to me. Perhaps increase the time tolerance (slightly)? I feel like I am doing the same swipe gesture, yet it only detects an actual swipe like 1 out of 25 or so times.

jbocce · 2023-03-09T17:25:38Z

@Ouwen

I am not entirely sure what you mean by creating a TODO for tap chains. However if it means that a new tap chain/sequence should be started once the previous distance tolerance has been exceeded then yes. I would say create an issue maybe?

Here is what I would put as the issue details (feel free to edit/modify as you wish). I am providing this here just to be sure we are one the same page with "tap chains". 😊

Issue: Performing two taps within the (double) tap time tolerance but outside the tap distance tolerance should be registered as two individual taps.

Detailed description: Whenever a tap falls within an existing tap sequence time tolerance BUT outside the tap distance tolerance, a new tap chain/sequence should be started.

jbocce · 2023-03-09T17:28:56Z

With regards to increasing the (double) tap tolerance to 400ms. Yes it makes sense for double tap, but a single tap should likely be much less than this. A similar thing is done for mouse events in mouseDownListener.ts. There is a clickDelay (i.e. single click time tolerance) of 200ms and the double click time tolerance is 400ms. Does this make sense?

Ouwen · 2023-03-13T01:06:17Z

With regards to increasing the (double) tap tolerance to 400ms. Yes it makes sense for double tap, but a single tap should likely be much less than this. A similar thing is done for mouse events in mouseDownListener.ts. There is a clickDelay (i.e. single click time tolerance) of 200ms and the double click time tolerance is 400ms. Does this make sense?

Ah I see, in this case it would make sense for touch to be 200ms in our use case. Since touch is also a little slower than click I think it may make sense to keep 300ms for sngle tap and thus 600ms for double tap (using the same language as the click implementation in this thread)

Ouwen · 2023-03-13T01:26:33Z

@Ouwen

I am using a Pixel 5 phone. I have (finally) managed to register swipes, but NOT consistently. It just does not feel quite right to me. Perhaps increase the time tolerance (slightly)? I feel like I am doing the same swipe gesture, yet it only detects an actual swipe like 1 out of 25 or so times.

I've increased the swipe tolerance from 200ms => 300ms which might help

Ouwen · 2023-03-13T01:28:58Z

#474

Issue regarding "Tap Chains" created and I've added a comment in the code!

Ouwen · 2023-03-13T01:58:01Z

@jbocce ready for your review!

jbocce

Thanks for all the changes @Ouwen. There is really only one left that concerns me slightly - but perhaps I am missing something.

jbocce · 2023-03-14T14:08:14Z

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

@@ -104,9 +104,13 @@ const defaultState: ITouchStartListenerState = {

  swipeDistanceThreshold: 48,
  swiped: false,
-  swipeWindow: 200, // 200ms to swipe after touch start or no swipe trigger
+  swipeToleranceMs: 300, // user has 300ms to swipe after touch start or no swipe will trigger


The new value does not help my phone, nor another Android phone I tried and a Chromebook that I tried. So feel free to revert this change, but it is ok as it as well. Up to you. Thanks for trying it for me.

hmmm that's really strange, if I manually open a browser window and go into mobile mode it can trigger

Well maybe the new tolerance is slightly better, but it still feels difficult to trigger. Let's just leave this as is for now.

jbocce · 2023-03-14T14:15:16Z

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

@@ -376,7 +386,7 @@ function _onTouchEnd(evt: TouchEvent): void {
 function _checkTouchTap(evt: TouchEvent): void {
  const currentTime = new Date().getTime();
  const startTime = state.startTime.getTime();
-  if (currentTime - startTime >= tapState.tapWindow) return;
+  if (currentTime - startTime > tapState.tapToleranceMs) return;


I am still not convinced that both this line AND 401-402 are both needed. Why would they both be needed? They are practically identical considering line 435 resets the tap start time. I tried removing each and noticed no difference. Perhaps I am missing something?

hmmmm yeah, I think you are right we can just rely on the touch start start time for tolerance. We can remove new Date() tracking for the tap state

Ouwen · 2023-03-15T22:23:29Z

@jbocce this is ready for your review!

jbocce

Thank you @Ouwen for your diligence with this PR. It is approved.

@sedghi please merge at your convenience.

sedghi requested a review from jbocce March 6, 2023 13:20

Ouwen force-pushed the gradienthealth/optimized_touch_fix branch from 70be5fa to 7af3534 Compare March 7, 2023 05:48

jbocce reviewed Mar 7, 2023

View reviewed changes

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

jbocce reviewed Mar 7, 2023

View reviewed changes

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

feat: optimized touch

dd0ca48

jbocce reviewed Mar 14, 2023

View reviewed changes

Ouwen force-pushed the gradienthealth/optimized_touch_fix branch from 7c90c00 to 52f3968 Compare March 15, 2023 22:21

feat: removed time from tap state

cd866cc

Ouwen force-pushed the gradienthealth/optimized_touch_fix branch from 52f3968 to cd866cc Compare March 15, 2023 22:46

jbocce approved these changes Mar 16, 2023

View reviewed changes

sedghi changed the title ~~feat: optimized touch~~ feat(touch): more optimized touch interactions Mar 16, 2023

sedghi merged commit f79f29a into cornerstonejs:main Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(touch): more optimized touch interactions #461

feat(touch): more optimized touch interactions #461

Ouwen commented Mar 5, 2023 •

edited

Loading

netlify bot commented Mar 5, 2023 •

edited

Loading

jbocce commented Mar 6, 2023

Ouwen commented Mar 7, 2023

jbocce Mar 7, 2023

Ouwen Mar 7, 2023 •

edited

Loading

jbocce Mar 8, 2023 •

edited

Loading

jbocce Mar 7, 2023

Ouwen Mar 7, 2023

jbocce Mar 8, 2023

jbocce commented Mar 7, 2023

Ouwen commented Mar 7, 2023 •

edited

Loading

jbocce commented Mar 8, 2023

Ouwen commented Mar 9, 2023 •

edited

Loading

jbocce commented Mar 9, 2023

jbocce commented Mar 9, 2023

jbocce commented Mar 9, 2023

Ouwen commented Mar 13, 2023 •

edited

Loading

Ouwen commented Mar 13, 2023

Ouwen commented Mar 13, 2023

Ouwen commented Mar 13, 2023

jbocce left a comment

jbocce Mar 14, 2023

Ouwen Mar 14, 2023

jbocce Mar 15, 2023

jbocce Mar 14, 2023

Ouwen Mar 15, 2023

Ouwen commented Mar 15, 2023

jbocce left a comment

feat(touch): more optimized touch interactions #461

feat(touch): more optimized touch interactions #461

Conversation

Ouwen commented Mar 5, 2023 • edited Loading

Test Swipe:

netlify bot commented Mar 5, 2023 • edited Loading

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

jbocce commented Mar 6, 2023

Ouwen commented Mar 7, 2023

Choose a reason for hiding this comment

Ouwen Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

jbocce Mar 8, 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

jbocce commented Mar 7, 2023

Test Swipe

Ouwen commented Mar 7, 2023 • edited Loading

jbocce commented Mar 8, 2023

Ouwen commented Mar 9, 2023 • edited Loading

jbocce commented Mar 9, 2023

jbocce commented Mar 9, 2023

jbocce commented Mar 9, 2023

Ouwen commented Mar 13, 2023 • edited Loading

Ouwen commented Mar 13, 2023

Ouwen commented Mar 13, 2023

Ouwen commented Mar 13, 2023

jbocce 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

Ouwen commented Mar 15, 2023

jbocce left a comment

Choose a reason for hiding this comment

Ouwen commented Mar 5, 2023 •

edited

Loading

netlify bot commented Mar 5, 2023 •

edited

Loading

Ouwen Mar 7, 2023 •

edited

Loading

jbocce Mar 8, 2023 •

edited

Loading

Ouwen commented Mar 7, 2023 •

edited

Loading

Ouwen commented Mar 9, 2023 •

edited

Loading

Ouwen commented Mar 13, 2023 •

edited

Loading