Improve fidelity of multi-pointer drag gestures. by winctoryang · Pull Request #106552 · flutter/flutter

winctoryang · 2022-06-24T11:42:37Z

Improve fidelity of multi-pointer drag gestures.
Hi there!
My name is winctoryang and fixed the related issue below. Please have a review and have a nice one!
Thank you!

As #38926 said:

When multi pointers dragging: delta(combined) = sum of delta(n)
This is very wrong. When multiple pointers are moving in the same direction, the scrollable will be moved at double or triple the speed.
When a pointer leaves: all remaining pointers still work.

The results that I fixed (refer to iOS behavior):

When dragging:
delta(combined) = max(i of n that are positive) delta(i) - max(i of n that are negative) delta(i)
It means that, if two fingers are moving +50 and +10 respectively, it will move +50; if they're moving at +50 and -10 respectively, it will move +40.
When a pointer leaves:
all pointers stop working.

The fixed logic:
A vector stores the EventMove of every pointer, if the EventMove of all pointers reaches, flush the vector and update the scroll delta. It means that, if three fingers (p1, p2, p3) are downing and moving: p1 down, p2 down, p3 down, p1 move +10, p2 move +20, and p3 move +30, only three EventMoves reach(p1 EventMove, p2 EventMove, p3 EventMove), the DragGesture will move +30.

Additionally, if the EventMoves of one pointer reach continually, then flush the vector directly. It means that, if three fingers are downing and moving: p1 down, p2 down, p3 down, p1 move +0, p2 move +0, and p3 move +10, since the move deltas p1 and p2 both are 0, the EventMoves of p1 and p2 may not reach (according to the log of iOS), only the EventMove of p3 reached, the DragGesture will move +10 until the next EventMove of p3 reaches.

The approach will lost the last EventMove, for example, p1 down, p2 down, p1 move +0, p2 move+10, p1 move +0, p2 move +10, p1 move +0, and p2 move +10, the Events: p1 EventDown, p2 EventDown, p2 EventMove, p2 EventMove, and p2 EventMove, the DragGesture will Move +10 and Move+10 finally. Actually, the EventMove of one pointer reaches many times, and the loss of the final EventMove is acceptable.

I modified the unit tests too, please have a review. Thank you!

The result videos have been attached below:
Android before fixed:

andorid_before.mp4

related issue:
Improve fidelity of multi-pointer drag gestures #38926
Scrolling with two fingers scrolls twice as fast #11884

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

merge master

google-cla · 2022-06-24T11:42:42Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

winctoryang · 2022-06-25T05:31:10Z

For the limit of the uploaded video size, I supply other videos here.

Android after fixed:

andrid_after.mp4

winctoryang · 2022-06-25T05:32:00Z

iOS before fixed:

ios_before.mp4

winctoryang · 2022-06-25T05:32:39Z

iOS after fixed:

ios_after.mp4

MaxBittker · 2022-06-26T03:54:02Z

bravo!! thank you for fixing this long standing issue

dkwingsmt

I wonder how panning is affected by this change. I'm mostly worried about the delta no longer being the change of the position, because this change want to be compatible with both the iOS scrolling behavior and the iOS panning behavior. Maybe eventually we should have a "strategy" class, so that iOS monodrag, iOS panning, and Android dragging should have separate strategies.

winctoryang · 2022-06-29T15:44:50Z

I wonder how panning is affected by this change. I'm mostly worried about the delta no longer being the change of the position, because this change wants to be compatible with both the iOS scrolling behavior and the iOS panning behavior. Maybe eventually we should have a "strategy" class, so that iOS monodrag, iOS panning, and Android dragging should have separate strategies.

I am confused about the iOS scrolling, the iOS panning, and the iOS monodrag what you mentioned, I am newer to iOS, after my investigation, it seems only the pan gesture is about scrolling in the picture below (Whether single finger or multiple).UIPanGestureRecognizer. Can you give me more explanation? Thank you for your patience!

Maybe eventually we should have a "strategy" class, so that iOS monodrag, iOS panning, and Android dragging should have separate strategies.

The logic in this fixed refers the iOS because the experience of iOS is better than Android. What I understand is you suggest different strategies (to simulate the Android and the iOS behavior) just like the over scroll physics behavior (the BouncingScrollPhysics and the ClampingScrollPhysics())?

Thank you for your review!

moffatman · 2022-06-30T19:03:26Z

I tried it out in my app, works well 👍

FYI Pan/zoom = trackpad scrolling event. Zoom/scale is not relevant here, only pan, which could also be called scroll. When I implemented it, I summed the deltas as previous behavior, but your solution is better in my opinion. The case of using both touchscreen and trackpad is not well handled by most platforms, trying it on iOS/UIKit causes a lot of flickering and is not correct, so there isn't any break with an existing standard to change it.

winctoryang · 2022-07-02T03:34:37Z

@moffatman

I tried it out in my app, works well 👍

FYI Pan/zoom = trackpad scrolling event. Zoom/scale is not relevant here, only pan, which could also be called scroll. When I implemented it, I summed the deltas as previous behavior, but your solution is better in my opinion.

Thank you for your FYI.

The case of using both touchscreen and trackpad is not well handled by most platforms, trying it on iOS/UIKit causes a lot of flickering and is not correct, so there isn't any break with an existing standard to change it.

So, the problem is the fixed only works well on the iOS and Android screens, but not on trackpad and touchscreen? I did not test on the trackpad, I will test it next week to improve. But I do not understand what's the problem with using a touchscreen, a lot of flickering on Andoird, iOS, or Windows surface? Can you give me some details about the devices to reproduce the problem? Thank you very much!

moffatman · 2022-07-02T17:35:30Z

@xiaoyanermiemie

I think I wasn't clear. There isn't any problem with your PR when using trackpad and touchscreen at the same time. It's all good 👍. I mean that when using trackpad and touchscreen at the same time in other apps (not Flutter), it's almost never handled correctly by other systems.

winctoryang · 2022-07-04T05:16:04Z

@xiaoyanermiemie

I think I wasn't clear. There isn't any problem with your PR when using trackpad and touchscreen at the same time. It's all good 👍. I mean that when using trackpad and touchscreen at the same time in other apps (not Flutter), it's almost never handled correctly by other systems.

@moffatman

Thank you for your reply, however, I am confused about in other apps (not Flutter). What I understand is that some apps use the flutter/flutter repository but are not called the flutter app, am I right？ Could you give me some codes or repository which is flickering and not correctly handled by this PR so that I can fix this issue more efficiently? I do not know how to reproduce the problem.

moffatman · 2022-07-04T05:23:34Z

@xiaoyanermiemie I mean other apps which don't use Flutter at all. When I said other platforms, I meant UIKit, SwiftUI, Windows UWP. See what it looks like using touchscreen and trackpad at the same time on the iPad settings app (noticeable jumping):

RPReplay_Final1656911939.mov

I didn't see any flickering using your PR at all. There is no problem 😄.

MaxBittker · 2022-07-05T15:14:35Z

thank you @xiaoyanermiemie !

winctoryang · 2022-07-28T13:22:18Z

@goderbauer

After the update, it would not hurt scrolling fidelity now. Please have a review. Thank you!

winctoryang · 2022-08-01T02:37:42Z

@dkwingsmt

I have updated the logic according to your endOfBatch, And I updated unit tests, it is more clear now! Please have a review!

@dkwingsmt

Do you have time to review this week?

dkwingsmt · 2022-08-02T00:57:44Z

+          _multiPointerMoveTrackers.add(event);
+          assert (_multiPointerMoveTrackers.length <= _multiPointerStartTrackers.length);
+          // If this event is the last event of a batch, update the pointers.
+          if (event.endOfBatch || _multiPointerMoveTrackers.length == _multiPointerStartTrackers.length) {


Do we still need to keep _multiPointerMoveTrackers.length == _multiPointerStartTrackers.length? We can probably just trust endOfBatch, even if the batch is larger than the active pointers. We can keep track of the movement of each pointer in a map (mapping from pointer ID to the latest event, since later events always override earlier ones in terms of absolute position) and calculate the final result at the final event of the batch.

Yes, we still need _multiPointerMoveTrackers.length == _multiPointerStartTrackers.length in this situation: If two pointers p1 and p2 down, one up (p1 up) and another down(p3 down). Now there are still two pointers down from the platform. It means that if we move p3, there will be two events dispatch (p3 where the endOfBatch is false, and p2 where the endOfBatch is true) from the platform. But in fact, when p1 up, we give up p2 too. And the PointerEvent of p2 does not come HandleEvent because it is given up. It means we will receive a lot of PointerEvent of p3 where the endOfBatch is false. So we still need _multiPointerMoveTrackers.length == _multiPointerStartTrackers.length.

That's a really good point, but what if we do this instead: when p1 becomes up, the recognizer does not call stopTrackingPointer with p2 until it becomes up. In that case, the recognizer will keep receiving events of p2's and p3's.

dkwingsmt · 2022-08-02T01:05:13Z

    if (event is PointerUpEvent || event is PointerCancelEvent || event is PointerPanZoomEndEvent) {
-      _giveUpPointer(event.pointer);
+      if (_pointerMoveAccept) {
+        _velocityTrackers.keys.toList().forEach(_giveUpPointer);


Suggested change

_velocityTrackers.keys.toList().forEach(_giveUpPointer);

_velocityTrackers.keys.forEach(_giveUpPointer);

dkwingsmt · 2022-08-02T01:06:15Z

  });

-  testGesture('Drag with multiple pointers in down behavior', (GestureTester tester) {
+  testGesture('Drag with multiple pointers in down behavior (the endOfBatch of event is always true)', (GestureTester tester) {


Suggested change

testGesture('Drag with multiple pointers in down behavior (the endOfBatch of event is always true)', (GestureTester tester) {

testGesture('Drag with multiple pointers in down behavior (events sent individually)', (GestureTester tester) {

dkwingsmt · 2022-08-02T01:06:27Z

    ]);
  });

+  testGesture('Drag with multiple pointers in down behavior (the endOfBatch of event may be true)', (GestureTester tester) {


Suggested change

testGesture('Drag with multiple pointers in down behavior (the endOfBatch of event may be true)', (GestureTester tester) {

testGesture('Drag with multiple pointers in down behavior (events sent in batches)', (GestureTester tester) {

dkwingsmt · 2022-08-02T01:12:57Z

+    final HorizontalDragGestureRecognizer drag1 =
+    HorizontalDragGestureRecognizer() ..dragStartBehavior = DragStartBehavior.down;


No changes are showing. Have you clicked "Add suggestion to batch" without clicking "commit batch"? Or have you forgot to push the local changes?

dkwingsmt · 2022-08-02T01:35:45Z

    updatedScrollDelta = null;
    expect(didEndPan, isFalse);

+    // one Pointer up, give up all pointers.


Suggested change

// one Pointer up, give up all pointers.

// One pointer up. Give up all pointers.

dkwingsmt · 2022-08-02T03:51:14Z

    updatedScrollDelta = null;
    expect(didEndPan, isFalse);

+    // Let another pointer join.


Suggested change

// Let another pointer join.

// Another pointer joins.

dkwingsmt · 2022-08-02T03:53:27Z

+    // Here are two pointers, so when they both moved, the gesture will move finally.
+    // one pointer moves (10, 10), and another moves (30, 30), the result is (30, 30).


Suggested change

// Here are two pointers, so when they both moved, the gesture will move finally.

// one pointer moves (10, 10), and another moves (30, 30), the result is (30, 30).

// One pointer moves by (10, 10), and another moves by (30, 30). As a result,

// the gesture moves by (30, 30).

dkwingsmt · 2022-08-02T03:55:57Z


  @override
  void handleEvent(PointerEvent event) {
+    // When multiple pointers are down and move, there may be the following


This piece of comment might need an update now that we have endOfBatch.

Yes, it should be updated. Done.

winctoryang

@dkwingsmt

I have updated it according to your comments, please review it again. Thank you!

winctoryang · 2022-07-27T13:16:55Z

    if (event is PointerUpEvent || event is PointerCancelEvent || event is PointerPanZoomEndEvent) {
-      _giveUpPointer(event.pointer);
+      if (_pointerMoveAccept) {
+        _velocityTrackers.keys.toList().forEach(_giveUpPointer);


But there's still toList.

After I tested the code, I found the giveUpPointer will call the didStopTrackingLastPointer and it will clear the _velocityTrackers. This will cause an exception.

winctoryang · 2022-07-27T15:10:14Z

+        _pointerMoveAccept = true;
+        // When a same pointer is detected, update immediately and add this event.
+        final bool eventExists = _multiPointerMoveTrackers.any((PointerEvent element) => element.pointer == event.pointer);
+        if (eventExists) {


Do we still need to check or track eventExists? We probably should just trust endOfBatch. This will make the algorithm much simpler and easier to explain.

On Android, if p1 and p2 down and only p1 leaves, then another p3 down (here are 3 pointers totally) and moves, the p3' endOfBatch always false. Because there are 2 pointer on the screen. So this logic is still useful.

I will find a better way in few days.

winctoryang · 2022-08-02T10:38:34Z


  @override
  void handleEvent(PointerEvent event) {
+    // When multiple pointers are down and move, there may be the following


Yes, it should be updated. Done.

winctoryang · 2022-08-02T11:06:34Z

    updatedScrollDelta = null;
    expect(didEndPan, isFalse);

+    // one Pointer up, give up all pointers.


winctoryang · 2022-08-02T11:07:00Z

    updatedScrollDelta = null;
    expect(didEndPan, isFalse);

+    // Let another pointer join.


winctoryang · 2022-08-02T11:07:38Z

+    // Here are two pointers, so when they both moved, the gesture will move finally.
+    // one pointer moves (10, 10), and another moves (30, 30), the result is (30, 30).


dkwingsmt · 2022-08-02T12:40:03Z

+          _multiPointerMoveTrackers.add(event);
+          assert (_multiPointerMoveTrackers.length <= _multiPointerStartTrackers.length);
+          // If this event is the last event of a batch, update the pointers.
+          if (event.endOfBatch || _multiPointerMoveTrackers.length == _multiPointerStartTrackers.length) {


That's a really good point, but what if we do this instead: when p1 becomes up, the recognizer does not call stopTrackingPointer with p2 until it becomes up. In that case, the recognizer will keep receiving events of p2's and p3's.

dkwingsmt · 2022-08-02T12:56:04Z

+    // When multiple pointers move, they dispatch the PointerEvents. The PointerMoveEvents
+    // of each individual pointer considered a batch. The last PointerEvent.endOfBatch
+    // is true. which means this batch event ends.
+    // When the endOfBatch is true, the gesture updates.


Suggested change

// When multiple pointers move, they dispatch the PointerEvents. The PointerMoveEvents

// of each individual pointer considered a batch. The last PointerEvent.endOfBatch

// is true. which means this batch event ends.

// When the endOfBatch is true, the gesture updates.

// When multiple pointers move together, each pointer dispatches a PointerMoveEvent.

// They constitute a "batch". The last event of a batch has `endOfBatch` true, indicating

// the end of a batch, which is when the gesture updates.

winctoryang · 2022-08-08T10:46:04Z

@dkwingsmt

Do you have time this week for a review? Thank you.

dkwingsmt · 2022-08-08T10:47:05Z

Oh I was waiting for you. Can you take a look at #106552 (comment)?

winctoryang · 2022-08-08T11:02:40Z

Oh I was waiting for you. Can you take a look at #106552 (comment)?

@dkwingsmt

I do not know why no notification to you. I replied to you before😳:

In fact, I tried this logic. But I failed because this will affect more low-level logic, for example, the accept or possible state. And the consequence of affecting more code is that there may be hidden bugs. So I think _multiPointerMoveTrackers.length == _multiPointerStartTrackers.length is a better solution than 'do not give up'. Although it is a little confused.

dkwingsmt · 2022-08-08T11:15:04Z

That's fair, thanks for explaining (and for avoiding we waiting for both each other forever.) This is not crucial anyway.

I think this PR is ready. @goderbauer Can you take another look?

dkwingsmt

LGTM

add comments for toList Co-authored-by: Tong Mu <[email protected]>

dkwingsmt · 2022-08-08T11:22:40Z

Regarding the endOfBatch changes, I'll probably merge #108340 first after resolving the issue about resampling before merging this.

winctoryang · 2022-08-08T11:34:08Z

Regarding the endOfBatch changes, I'll probably merge #108340 first after resolving the issue about resampling before merging this.

That is nice!

winctoryang · 2022-08-10T11:03:24Z

@goderbauer

Do you have time for a review this week?

goderbauer · 2022-08-10T16:27:27Z

Do you have time for a review this week?

I have this in my review queue.

goderbauer · 2022-08-15T17:35:52Z

Actually, before reviewing this one, lets make sure that #108340 is good to go first since this change depends on that one.

goderbauer · 2022-09-20T22:19:09Z

(triage) @xiaoyanermiemie Same comment as on #108340: Do you still have plans for this PR in the near future?

goderbauer · 2022-10-04T22:17:54Z

(Triage): I am going to close this one as there hasn't been any follow-up and the discussion has largely centered around #108340

winctoryang and others added 7 commits June 23, 2022 00:58

modify handle move

d39de62

add event_index

59062f1

remove log

20cd863

Merge pull request #1 from flutter/master

ee32f9d

merge master

for usage

c53b2c8

modify unit tests

821e93f

Merge branch 'flutter:master' into master

fe73c2d

flutter-dashboard Bot added f: gestures flutter/packages/flutter/gestures repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Jun 24, 2022

linux analysis

1897c5c

dkwingsmt reviewed Jun 28, 2022

View reviewed changes

Comment thread packages/flutter/lib/src/gestures/monodrag.dart Outdated

Comment thread packages/flutter/lib/src/gestures/monodrag.dart

Comment thread packages/flutter/test/widgets/overscroll_stretch_indicator_test.dart Outdated

Comment thread packages/flutter/test/gestures/drag_test.dart Outdated

winctoryang added 2 commits June 29, 2022 23:46

for any

74e534b

linux analysis

fa479ac

winctoryang and others added 2 commits July 5, 2022 20:42

modify unit tests

8d7f933

Merge branch 'flutter:master' into master

385bdbd

moffatman reviewed Jul 5, 2022

View reviewed changes

Comment thread packages/flutter/lib/src/gestures/monodrag.dart Outdated

moffatman reviewed Jul 5, 2022

View reviewed changes

Comment thread packages/flutter/lib/src/gestures/monodrag.dart Outdated

winctoryang requested a review from dkwingsmt July 29, 2022 08:11

dkwingsmt requested changes Aug 2, 2022

View reviewed changes

some comments

98e3eda

winctoryang commented Aug 2, 2022

View reviewed changes

Merge branch 'flutter:master' into master

f659109

dkwingsmt reviewed Aug 2, 2022

View reviewed changes

winctoryang added 2 commits August 3, 2022 11:10

some comments

7244105

Merge branch 'master' of https://github.com/xiaoyanermiemie/flutter

0f06ddf

dkwingsmt requested a review from goderbauer August 8, 2022 11:05

dkwingsmt reviewed Aug 8, 2022

View reviewed changes

Comment thread packages/flutter/lib/src/gestures/monodrag.dart

dkwingsmt approved these changes Aug 8, 2022

View reviewed changes

winctoryang and others added 2 commits August 8, 2022 19:17

Update packages/flutter/lib/src/gestures/monodrag.dart

9bb7f7c

add comments for toList Co-authored-by: Tong Mu <[email protected]>

Merge branch 'flutter:master' into master

6b2f503

dkwingsmt mentioned this pull request Aug 11, 2022

Add PointerEvent.endOfBatch #108340

Closed

8 tasks

goderbauer closed this Oct 4, 2022

winctoryang mentioned this pull request Oct 22, 2022

improve modelity of multi finger move flutter-team-archive/engine#36948

Closed

xu-baolin mentioned this pull request Nov 21, 2023

Implement Specific Fixes for Multi-Touch Scrolling Across Platforms #138763

Closed

6 of 8 tasks

	_velocityTrackers.keys.toList().forEach(_giveUpPointer);
	_velocityTrackers.keys.forEach(_giveUpPointer);

	testGesture('Drag with multiple pointers in down behavior (the endOfBatch of event is always true)', (GestureTester tester) {
	testGesture('Drag with multiple pointers in down behavior (events sent individually)', (GestureTester tester) {

		final HorizontalDragGestureRecognizer drag1 =
		HorizontalDragGestureRecognizer() ..dragStartBehavior = DragStartBehavior.down;

	// one Pointer up, give up all pointers.
	// One pointer up. Give up all pointers.

		// Here are two pointers, so when they both moved, the gesture will move finally.
		// one pointer moves (10, 10), and another moves (30, 30), the result is (30, 30).

Conversation

winctoryang commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

google-cla Bot commented Jun 24, 2022

Uh oh!

winctoryang commented Jun 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winctoryang commented Jun 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winctoryang commented Jun 25, 2022

Uh oh!

MaxBittker commented Jun 26, 2022

Uh oh!

dkwingsmt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

winctoryang commented Jun 29, 2022

Uh oh!

moffatman commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winctoryang commented Jul 2, 2022

Uh oh!

moffatman commented Jul 2, 2022

Uh oh!

winctoryang commented Jul 4, 2022

Uh oh!

moffatman commented Jul 4, 2022

Uh oh!

MaxBittker commented Jul 5, 2022

Uh oh!

Uh oh!

Uh oh!

winctoryang commented Jul 28, 2022

Uh oh!

winctoryang commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winctoryang commented Jun 24, 2022 •

edited

Loading

winctoryang commented Jun 25, 2022 •

edited

Loading

winctoryang commented Jun 25, 2022 •

edited

Loading

dkwingsmt left a comment •

edited

Loading

moffatman commented Jun 30, 2022 •

edited

Loading

winctoryang commented Aug 1, 2022 •

edited

Loading

winctoryang commented Aug 8, 2022 •

edited

Loading

dkwingsmt commented Aug 8, 2022 •

edited

Loading