Add PointerEvent.endOfBatch by dkwingsmt · Pull Request #108340 · flutter/flutter

dkwingsmt · 2022-07-26T02:38:48Z

This PR adds a new field bool endOfBatch to PointerEvent and populates it.

Some pointer events are sent in batches. For example, when the device is being operated with multiple fingers, events from each moving finger constitute a batch and is sent together. This information is necessary to implement the iOS behavior of multi-finger scrolling, which needs to know which event is the last of the batch so that it can start processing the entire batch as a whole.

What constitutes a batch? The iOS platform sends a batch of events in the same allTouches, and the iOS embedding is already sending events from the same allTouches in the same PointerDataPacket. The framework just needs to mark all of them as endOfBatch: false except for the last one.

The harder part is when the events are being resampled. If there are multiple batches within two resampling frames, the two batches might not have the same order, or even the same set of devices. This PR considers all events resulting from traversing PointerEventResamplers and calling PointerEventResampler.sample() a single batch. All of these events are collected, and are marked as endOfBatch: false except for the last one.

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.

goderbauer · 2022-07-26T16:23:37Z

Does this play nicely/correctly when pointer events are resampled with PointerEventResampler? Maybe add a test for that case?

dkwingsmt · 2022-08-08T11:21:47Z

Does this play nicely/correctly when pointer events are resampled with PointerEventResampler? Maybe add a test for that case?

That's a good point and I suspect that I do need extra changes.... I'll take a look soon.

…mt/flutter into pointer-add-end-of-batch

goderbauer · 2022-08-12T21:17:09Z

I have this on my list to review early next week. Just a quick fly-by comment, looks like the doc check is unhappy.

goderbauer · 2022-08-15T16:49:55Z

-    return data
+    final List<ui.PointerData> knownData = data
        .where((ui.PointerData datum) => datum.signalKind != ui.PointerSignalKind.unknown)
+        .toList();


This extra toList conversion is unfortunate and could be costly. I wonder if there's a way to avoid it?

goderbauer · 2022-08-15T16:54:20Z

+  ///
+  /// Some pointer events are sent in batches. For example, when the device
+  /// is being operated with multiple fingers, events from each moving finger
+  /// constitute a batch and is sent together. Since pointer events are always


is -> are? since "events" is plural?

goderbauer · 2022-08-15T16:57:43Z

 typedef HandleEventCallback = void Function(PointerEvent event);

-/// Class for pointer event resampling.
+/// A [PointerEvent] that has everything ready except for `endOfBatch`.


Document: What does "everything ready" mean?

goderbauer · 2022-08-15T16:59:42Z

        ? _toMoveEvent(
-            event, position, delta, pointerIdentifier, timeStamp, buttons)
-        : _toHoverEvent(event, position, delta, timeStamp, buttons);
+            event, position, delta, pointerIdentifier, timeStamp, buttons, endOfBatch)


nit: I see this was already incorrectly formatted before, but could you fix formatting by moving this line to the previous one?

goderbauer · 2022-08-15T17:02:02Z

          final Offset delta = position - _position;
-          callback(_toMoveOrHoverEvent(event, position, delta,
-              _pointerIdentifier, sampleTime, wasDown, hadButtons));
+          eventBuilders.add((bool endOfBatch) =>


nit (here and elsewhere): https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#consider-using--for-short-functions-and-methods:

[...] only use ⇒ when everything, including the function declaration, fits on a single line.

goderbauer · 2022-08-15T17:06:25Z

+      eventBuilders.add((bool endOfBatch) =>
+        _toMoveOrHoverEvent(next, position, delta, _pointerIdentifier,
+            sampleTime, _isDown, _hasButtons, endOfBatch),


nit: formatting

goderbauer · 2022-08-15T17:17:54Z

+    // current sample time.
+    final List<BatchEventBuilder> batchBuilder = <BatchEventBuilder>[];
+    for (final PointerEventResampler resampler in _resamplers.values) {
+      batchBuilder.addAll(resampler.sample(sampleTime, nextSampleTime));


While reading this I have been wondering why we need all this indirection via the BatchEventBuilder. For my own understanding: Is it possible that resampler.sample here my return an empty list and the endOfBatch event is actually produced by a Resampler other then the last one in _resamplers.values? If that's not true, could we just tell the last resampler.sample method that it is responsible for producing an endOfBatch event?

If we have to sample all resamplers before we can determine the endOfBatch event, could this be simplified by just having resampler.sample return a list of pointer events and before calling handlePointerEvent below we copy the last event of the collected list to set endOfBatch: true?

goderbauer · 2022-08-15T17:34:05Z

Does this actually help with solving #106552 in all cases? Let's imagine a situation where the screen is split in two halves. One half is covered by a DragGestureRecognizer, the other half isn't. Two finger (F1 and F2) touch the screen and move around, one in each half. F1 moves around in the half covered by the DragGestureRecognizer, which tracks its every move. F2 moves around outside of that area and is not tracked by the DragGestureRecognizer. In a single PointerData packet (Iterable<ui.PointerData>) we now get a move event for F1 and another move event for F2, which are processed by the PointerEventConverter in that order. The F2 event is marked with endOfBatch, the F1 event isn't. Unless I am missing something, the DragGestureRecognizer (which only tracks F1) will never know when the batch ended in this situation because the F2 event is never routed to it... Wouldn't this cause problems for #106552 (I haven't reviewed the latest changes to that PR closely, this is just from conceptually thinking about this)?

dkwingsmt · 2022-08-16T11:14:22Z

Does this actually help with solving #106552 in all cases? Let's imagine a situation where the screen is split in two halves. One half is covered by a DragGestureRecognizer, the other half isn't. Two finger (F1 and F2) touch the screen and move around, one in each half. F1 moves around in the half covered by the DragGestureRecognizer, which tracks its every move. F2 moves around outside of that area and is not tracked by the DragGestureRecognizer. In a single PointerData packet (Iterable<ui.PointerData>) we now get a move event for F1 and another move event for F2, which are processed by the PointerEventConverter in that order. The F2 event is marked with endOfBatch, the F1 event isn't. Unless I am missing something, the DragGestureRecognizer (which only tracks F1) will never know when the batch ended in this situation because the F2 event is never routed to it... Wouldn't this cause problems for #106552 (I haven't reviewed the latest changes to that PR closely, this is just from conceptually thinking about this)?

You're right, thanks for bringing this up. Well, I can't think of a way of solving this without a complete overhaul of our gesture recognizer API by disptaching the filtered batch in a list, because now that the gesture recognizer can receive any portion of the batch, how can the recognizer know that it has received the last of this portion without receiving an event from the next batch?

I would suggest reverting #106552 back to the previous approach, which is not beautiful but totally works (as far as I can see).

winctoryang · 2022-08-16T13:30:30Z

Does this actually help with solving #106552 in all cases? Let's imagine a situation where the screen is split in two halves. One half is covered by a DragGestureRecognizer, the other half isn't. Two finger (F1 and F2) touch the screen and move around, one in each half. F1 moves around in the half covered by the DragGestureRecognizer, which tracks its every move. F2 moves around outside of that area and is not tracked by the DragGestureRecognizer. In a single PointerData packet (Iterable<ui.PointerData>) we now get a move event for F1 and another move event for F2, which are processed by the PointerEventConverter in that order. The F2 event is marked with endOfBatch, the F1 event isn't. Unless I am missing something, the DragGestureRecognizer (which only tracks F1) will never know when the batch ended in this situation because the F2 event is never routed to it... Wouldn't this cause problems for #106552 (I haven't reviewed the latest changes to that PR closely, this is just from conceptually thinking about this)?

You're right, thanks for bringing this up. Well, I can't think of a way of solving this without a complete overhaul of our gesture recognizer API by disptaching the filtered batch in a list, because now that the gesture recognizer can receive any portion of the batch, how can the recognizer know that it has received the last of this portion without receiving an event from the next batch?

I would suggest reverting #106552 back to the previous approach, which is not beautiful but totally works (as far as I can see).

@dkwingsmt

So, the best way is what I said "make every pointer dispatch the event even some pointer not move". Android did this, but iOS not.

I know why only moved pointer dispatches on iOS but not on Android. https://github.com/flutter/engine/blob/main/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm#:~:text=%2D%20(void)dispatchTouches,(UIEvent*)event%20%7B In the dispatchTouches function, it should use the event.alltouches.

If we do this, we will not use endOfBatch.
But the behavior of " the DragGestureRecognizer (which only tracks F1) will never know when the batch ended in this situation because the F2 event is never routed to it." may be still existing. I will check this performance this week. But I think the endOfBatch is not the reason for this behavior. Even we use UIEvent.allTouches, this behaivor do not change, unless we modify the algorithem in monodrag.dart.

All above, the UITouch.allTouches can replace endOfBatch, and it is more clearer (avoid all event adding an attributes). But this replacement can not fix the problem (or we can refer the iOS's behavior).

dkwingsmt · 2022-08-16T20:53:06Z

I see, so you mean, if we switch to using allTouches, then the counting approach will be much more reliable. I agree. @goderbauer What do you think?

winctoryang · 2022-08-18T13:19:35Z

I see, so you mean, if we switch to using allTouches, then the counting approach will be much more reliable. I agree. @goderbauer What do you think?

@goderbauer @dkwingsmt

After I tested, the behavior " the DragGestureRecognizer (which only tracks F1) will never know when the batch ended in this situation because the F2 event is never routed to it." exists indeed.

But my fix code can handle it. Because I both use endOfBatch, and the count of PointerDownEvent'. Which means that, if there are 2 pointers in Recognizer Aand 3 pointers inRecognizer B. the endOfBatch` is false in A, and true in B, but Recognizer A records 2 pointers and B records 3 pointers. So We can use this information.

But I still suggest we can use allTouches than enfOfBatch.

goderbauer · 2022-08-18T18:32:54Z

I see, so you mean, if we switch to using allTouches, then the counting approach will be much more reliable. I agree. @goderbauer What do you think?

Sorry, I don't fully follow what the new proposal is.

dkwingsmt · 2022-08-18T21:35:59Z

I see, so you mean, if we switch to using allTouches, then the counting approach will be much more reliable. I agree. @goderbauer What do you think?

@goderbauer @dkwingsmt

After I tested, the behavior " the DragGestureRecognizer (which only tracks F1) will never know when the batch ended in this situation because the F2 event is never routed to it." exists indeed.

But my fix code can handle it. Because I both use endOfBatch, and the count of PointerDownEvent'. Which means that, if there are 2 pointers in Recognizer Aand 3 pointers inRecognizer B. the endOfBatch` is false in A, and true in B, but Recognizer A records 2 pointers and B records 3 pointers. So We can use this information.

But I still suggest we can use allTouches than enfOfBatch.

Yeah. Can you try allTouches and without endOfBatch?

winctoryang · 2022-08-23T12:41:51Z

@dkwingsmt

I tested that changing touches to allTouches, but it causes more exceptions. there are some logics to match touches. For example the pointer_data_packet_converter.cc holds the state_ map which is influenced by touches. If we modify touches to allTouches only, it will cause bugs. And if we modify all relative logics to match touches to allTouches. It is expensive. So I suggest we use 'endOfBatch' and my fix. This works well. What is your opinion?

dkwingsmt · 2022-08-26T18:53:38Z

@dkwingsmt

I tested that changing touches to allTouches, but it causes more exceptions. there are some logics to match touches. For example the pointer_data_packet_converter.cc holds the state_ map which is influenced by touches. If we modify touches to allTouches only, it will cause bugs. And if we modify all relative logics to match touches to allTouches. It is expensive. So I suggest we use 'endOfBatch' and my fix. This works well. What is your opinion?

Anything that relies on endOfBatch is easily broken when the gesture area does not cover the entire screen. I don't think it's an approach worth discussing any more.

Sorry I was busy with other issues lately but I'll experiment on changing to allTouches soon.

winctoryang · 2022-08-29T07:34:25Z

@dkwingsmt
I tested that changing touches to allTouches, but it causes more exceptions. there are some logics to match touches. For example the pointer_data_packet_converter.cc holds the state_ map which is influenced by touches. If we modify touches to allTouches only, it will cause bugs. And if we modify all relative logics to match touches to allTouches. It is expensive. So I suggest we use 'endOfBatch' and my fix. This works well. What is your opinion?

Anything that relies on endOfBatch is easily broken when the gesture area does not cover the entire screen. I don't think it's an approach worth discussing any more.

Sorry I was busy with other issues lately but I'll experiment on changing to allTouches soon.

Oh, I forget that broken. Thank you for your expriment. I will try to change it too.

flutter-dashboard · 2022-09-18T07:37:42Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

goderbauer · 2022-09-20T22:16:17Z

(triage) @dkwingsmt @xiaoyanermiemie What's the status on this one? Should this be closed or do you still plan to work in this particular PR in the near future?

winctoryang · 2022-09-22T11:57:16Z

(triage) @dkwingsmt @xiaoyanermiemie What's the status on this one? Should this be closed or do you still plan to work in this particular PR in the near future?

I still plan

winctoryang · 2022-09-22T12:59:30Z

Does this actually help with solving #106552 in all cases? Let's imagine a situation where the screen is split in two halves. One half is covered by a DragGestureRecognizer, the other half isn't. Two finger (F1 and F2) touch the screen and move around, one in each half. F1 moves around in the half covered by the DragGestureRecognizer, which tracks its every move. F2 moves around outside of that area and is not tracked by the DragGestureRecognizer. In a single PointerData packet (Iterable<ui.PointerData>) we now get a move event for F1 and another move event for F2, which are processed by the PointerEventConverter in that order. The F2 event is marked with endOfBatch, the F1 event isn't. Unless I am missing something, the DragGestureRecognizer (which only tracks F1) will never know when the batch ended in this situation because the F2 event is never routed to it... Wouldn't this cause problems for #106552 (I haven't reviewed the latest changes to that PR closely, this is just from conceptually thinking about this)?

@dkwingsmt @goderbauer
In this situation, there is a bug I found at the master branch of flutter/engine. The reproduct approach is : Two fingers on two ListViews respectively and up them together.

Here is the exception stack (tips: you can try some times, remember together):

This bug block me, can you fix it firstly?

winctoryang · 2022-09-22T13:02:02Z

you can refer to this demo: two ListViews.

goderbauer · 2022-09-27T22:22:04Z

In this situation, there is a bug I found at the master branch of flutter/engine. The reproduct approach is : Two fingers on two ListViews respectively and up them together.

Can you file a bug for this with some code to reproduce it? Thanks.

winctoryang · 2022-10-22T09:15:28Z

@dkwingsmt @goderbauer Please see the discussion in flutter/engine

winctoryang · 2022-10-22T09:23:28Z

In this situation, there is a bug I found at the master branch of flutter/engine. The reproduct approach is : Two fingers on two ListViews respectively and up them together.

Can you file a bug for this with some code to reproduce it? Thanks.

Sorry, there is no bug. I had a mistake.

dkwingsmt · 2022-10-26T19:21:01Z

@xiaoyanermiemie I tried switching to allTouches at https://github.com/flutter/engine/blob/206ef0f083675072844331b42b06b69429982277/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm#L1119 :

 - (void)touchesBegan:(NSSet*)touches withEvent:(UIEvent*)event {
-  [self dispatchTouches:touches pointerDataChangeOverride:nullptr event:event];
+  [self dispatchTouches:event.allTouches pointerDataChangeOverride:nullptr event:event];
 }

And all similar methods below it.

(I know you talked about changing dispatchTouches instead but I think this is a better place.) But I didn't see any tests broken with this change. Did I make the wrong change?

winctoryang · 2022-10-27T08:14:15Z

@xiaoyanermiemie I tried switching to allTouches at https://github.com/flutter/engine/blob/206ef0f083675072844331b42b06b69429982277/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm#L1119 :
 - (void)touchesBegan:(NSSet*)touches withEvent:(UIEvent*)event {
-  [self dispatchTouches:touches pointerDataChangeOverride:nullptr event:event];
+  [self dispatchTouches:event.allTouches pointerDataChangeOverride:nullptr event:event];
 }
And all similar methods below it.

(I know you talked about changing dispatchTouches instead but I think this is a better place.) But I didn't see any tests broken with this change. Did I make the wrong change?

@dkwingsmt
Maybe tests do not consider this situation (down and move event together in event.allTouches). You can use two fingers to move, and it easily to reproduct. (Remember together down and fast move :)

goderbauer · 2022-11-22T22:56:31Z

(Triage): I am going to close this PR as it is getting stale. I would suggest filing an issue and discussing options there.

Impl and test

0523f8f

flutter-dashboard Bot added f: gestures flutter/packages/flutter/gestures repository. framework flutter/packages/flutter repository. See also f: labels. labels Jul 26, 2022

dkwingsmt mentioned this pull request Jul 26, 2022

Improve fidelity of multi-pointer drag gestures. #106552

Closed

8 tasks done

dkwingsmt added 4 commits August 11, 2022 04:37

Resampler

53d00db

Resampler

3cb4fb8

Merge branch 'pointer-add-end-of-batch' of https://github.com/dkwings…

60bf630

…mt/flutter into pointer-add-end-of-batch

Merge remote-tracking branch 'origin/main' into pointer-add-end-of-batch

0133d5f

dkwingsmt marked this pull request as ready for review August 11, 2022 11:38

dkwingsmt requested review from goderbauer and removed request for goderbauer August 11, 2022 11:38

dkwingsmt marked this pull request as draft August 11, 2022 13:30

MultiDeviceResampler

1ba87c7

dkwingsmt marked this pull request as ready for review August 11, 2022 21:37

dkwingsmt requested a review from goderbauer August 11, 2022 21:40

goderbauer reviewed Aug 15, 2022

View reviewed changes

winctoryang mentioned this pull request Oct 22, 2022

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

Closed

goderbauer closed this Nov 22, 2022

Conversation

dkwingsmt commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

goderbauer commented Jul 26, 2022

Uh oh!

dkwingsmt commented Aug 8, 2022

Uh oh!

goderbauer commented Aug 12, 2022

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!

goderbauer commented Aug 15, 2022

Uh oh!

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

Uh oh!

Uh oh!

winctoryang commented Aug 16, 2022

Uh oh!

dkwingsmt commented Aug 16, 2022

Uh oh!

winctoryang commented Aug 18, 2022

Uh oh!

goderbauer commented Aug 18, 2022

Uh oh!

dkwingsmt commented Aug 18, 2022

Uh oh!

winctoryang commented Aug 23, 2022

Uh oh!

dkwingsmt commented Aug 26, 2022

Uh oh!

winctoryang commented Aug 29, 2022

Uh oh!

flutter-dashboard Bot commented Sep 18, 2022

Uh oh!

goderbauer commented Sep 20, 2022

Uh oh!

winctoryang commented Sep 22, 2022

Uh oh!

winctoryang commented Sep 22, 2022

Uh oh!

winctoryang commented Sep 22, 2022

Uh oh!

goderbauer commented Sep 27, 2022

Uh oh!

winctoryang commented Oct 22, 2022

Uh oh!

winctoryang commented Oct 22, 2022

Uh oh!

dkwingsmt commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winctoryang commented Oct 27, 2022

Uh oh!

goderbauer commented Nov 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dkwingsmt commented Jul 26, 2022 •

edited

Loading

dkwingsmt commented Aug 16, 2022 •

edited

Loading

dkwingsmt commented Oct 26, 2022 •

edited

Loading