Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[New feature]Introduce iOS multi-touch drag behavior #141355

Merged
merged 23 commits into from Mar 13, 2024

Conversation

xu-baolin
Copy link
Member

@xu-baolin xu-baolin commented Jan 11, 2024

Fixes #38926

This patch implements the iOS behavior pointed out by @dkwingsmt at #38926 , which is also consistent with the performance of my settings application on the iPhone.

iOS behavior (horizontal or vertical drag)

Algorithm

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.

TODO
Write some test cases

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. f: cupertino flutter/packages/flutter/cupertino repository f: gestures flutter/packages/flutter/gestures repository. labels Jan 11, 2024
@@ -436,7 +436,7 @@ void main() {
),
);

await tester.drag(find.text('9'), const Offset(0.0, 32.0), touchSlopY: 0, warnIfMissed: false); // see top of file
await tester.drag(find.text('9'), const Offset(0.0, 32.0), pointer: 1, touchSlopY: 0, warnIfMissed: false); // see top of file
Copy link
Member Author

@xu-baolin xu-baolin Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do not give the pointer, it will dispatch a new pointer event,
the second drag move offset will be merged by the new multitouch strategy on Apple platforms and cause test failed.

return;
}

final Duration currentSystemFrameTimeStamp = SchedulerBinding.instance.currentSystemFrameTimeStamp;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fortunately, we can use this to identify whether the events are of the same batch. CC @dkwingsmt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant!

@xu-baolin xu-baolin requested a review from Piinks January 11, 2024 12:00
@xu-baolin xu-baolin force-pushed the xbl240105 branch 2 times, most recently from 1b846e5 to f2ce80e Compare January 16, 2024 08:16
MultitouchDragStrategy get multitouchDragStrategy => MultitouchDragStrategy.latestPointer;
/// create drag gestures for non-Apple platforms, and
/// [MultitouchDragStrategy.maxAllPointers] for Apple platforms.
MultitouchDragStrategy get multitouchDragStrategy {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should not be converted to a method so that is can have a BuildContext?

The defaultTargetPlatform is used by ScrollBehavior, but subclasses like MaterialScrollBehavior and CupertinoScrollBehavior use the getPlatform method using context to account for if the Theme is specifying a platform.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.
I'm worried that this will become a breaking change unless we can guarantee that this will be released at the same time as the previous PR. What do you think?

Copy link
Contributor

@Piinks Piinks Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They won't be released at the same time. The other PR has already been cut into multiple beta releases I think.
It feels like a worthwhile breaking change though with low risk. Not a lot of folks will have had the opportunity to use the new getter, so following up with a small change to make it a method should be ok. I can help with a migration guide.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think dart fix will be able to help too.


if (_activePointer == pointer) {
_activePointer =
_acceptedActivePointers.isNotEmpty ? _acceptedActivePointers.first : null;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refer to Android RecyclerView

image

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked closely into the logic yet, just touching on the comments part.

packages/flutter/lib/src/cupertino/app.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/gestures/monodrag.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/gestures/recognizer.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/gestures/recognizer.dart Outdated Show resolved Hide resolved
@dkwingsmt
Copy link
Contributor

dkwingsmt commented Jan 23, 2024

Another question: If I remember correctly, you said that you used the event timestamp to determine whether the current event belongs to the batch of the previous event. Can you briefly explain how this is done? Specifically, I asked this because previously we wanted to do the similar thing in the pointer event converter and was concerned that, this approach requires that any calculation on a batch can not be started until the next pointer.

   event1    event2    event3    event4
   batch1    batch1    batch1    batch2
                                  ^
    wait...   wait...   wait...   | Hey a new timestamp! Time to process batch 1

This not only introduces delay but also prevents the last batch from taking effect.

I assume your algorithm takes a different approach and processes each event as soon as they arrive, and calculate incrementally based on whatever already is sent out on this batch?

@xu-baolin
Copy link
Member Author

xu-baolin commented Jan 25, 2024

I assume your algorithm takes a different approach and processes each event as soon as they arrive, and calculate incrementally based on whatever already is sent out on this batch?

@dkwingsmt, you are right.
In fact, all events before the next Vsync signal arrives are treated as a batch here. Each event will be processed immediately, without delay, and does not depend on the arrival of the last event. In other words, any event may be the last one in the batch. Since there's no real start frame yet, we have the opportunity to merge handling of the input events.
Image_20240124103122
Image_20240125171649
Image_20240124103132

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read the algorithm in detail and gave some comments. Most of them are "this logic can be simplified" but I raised a major question in L448.

packages/flutter/lib/src/gestures/monodrag.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/gestures/monodrag.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/gestures/monodrag.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/gestures/monodrag.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly ok, but another test case should be added.

packages/flutter/lib/src/gestures/monodrag.dart Outdated Show resolved Hide resolved
packages/flutter/test/gestures/drag_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/gestures/drag_test.dart Show resolved Hide resolved
tester.route(down6);

log.add('-b');
// #6 pointer move (110.0, 110.0), received delta should be (10, 10).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the same as the panning algorithm I described in #38926, although I might have been wrong. Can you verify the native behavior of panning on iOS and see if it only checks the border pointers or if it averages all pointers? Also, what if the border pointers for x axis are different from those for y axis, for example:

5: Offset(-100, 0),
6: Offset(100, 0),
7: Offset(0, -100),
8: Offset(0, 100),

what's the behavior on native for cases like this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I test the UIPanGestureRecognizer for iOS and indeed like you said, it takes the average of all fingers.
Here is the test code,

//
//  ViewController.swift
//  panning
//
//  Created by 徐宝林(Ava) on 2024/2/18.
//

import UIKit

class ViewController: UIViewController {
    
    private let pannableView: UIView = {
        // Initialize View
        let view = UIView(frame: CGRect(origin: .zero,
                                        size: CGSize(width: 300.0, height: 300.0)))

        // Configure View
        view.backgroundColor = .blue
        view.translatesAutoresizingMaskIntoConstraints = false

        return view
    }()
    
    @objc private func didPan(_ sender: UIPanGestureRecognizer) {
//        pannableView.center = sender.location(in: view)
        let center = pannableView.center
        let trans = sender.translation(in: view)
        pannableView.center = CGPointMake(center.x + trans.x, center.y + trans.y)
        sender.setTranslation(CGPointZero, in: view)
    }

    override func viewDidLoad() {
        super.viewDidLoad()

        // Add to View Hierarchy
        view.addSubview(pannableView)

        // Center Pannable View
        pannableView.center = view.center

        // Initialize Swipe Gesture Recognizer
        let panGestureRecognizer = UIPanGestureRecognizer(target: self, action: #selector(didPan(_:)))

        // Add Swipe Gesture Recognizer
        pannableView.addGestureRecognizer(panGestureRecognizer)
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5: Offset(-100, 0),
6: Offset(100, 0),
7: Offset(0, -100),
8: Offset(0, 100),

what's the behavior on native for cases like this?

The draggable object will remain stationary.

@xu-baolin
Copy link
Member Author

Ping @dkwingsmt : )

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

/// If the user is dragging with 5 pointers at the same time, each having
/// \[+10, +20, +33, -1, -12\] pixels of offset, the recognizer will report a
/// delta of (+33) + (-12) = 21 pixels.
///
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mention the panning algorithm here, and change the first paragraph.

And I feel like the name maxAllPointers doesn't apply to panning, but I can't think of a better name either.

Maybe marginPointers, in contrast to latestPointer, to indicate that the result comes from all the outermost ones. For one-dimensional movement, they're positive-most one and the negavie-most one. For two-dimensional movement, they're essentially all pointers.

This is my suggestion of the entire documentation:

  /// All active pointers will be tracked, and the result is computed from the "outermost" pointers.
  ///
  /// The scrolling offset is determined by the maximum deltas of both directions.
  ///
  /// If the user is dragging with 3 pointers at the same time, each having
  /// \[+10, +20, +33\] pixels of offset, the recognizer will report a delta of 33 pixels.
  ///
  /// If the user is dragging with 5 pointers at the same time, each having
  /// \[+10, +20, +33, -1, -12\] pixels of offset, the recognizer will report a
  /// delta of (+33) + (-12) = 21 pixels.
  ///
  /// The panning offset is the average of all pointers.
  ///
  /// If the user is dragging ... [insert an example here].
  ///
  /// This is the behavior typically seen on iOS.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need a native speaker for the final verdict. @Piinks May we have your idea? Does the doc and the variable name look clear enough to you?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also provide two names for reference, mergePointers or conbinePointers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I've been thinking on this.

I think the doc explains this really well. The name can be misleading though in my opinion.

maxAllPointers, mergePointers, and combinePointers imply to me that all the values are included in the computation. But it is the greatest value in each direction, so not all pointers are included.

Words like extremes, boundaries, range come to mind... do any of these sound right?

  • sumExtremePointers
  • sumBoundaryPointers
  • sumDirectionalMaxPointers

WDYT?

Copy link
Contributor

@dkwingsmt dkwingsmt Mar 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest against the word sum, because this rules is not a summation. The rule sumAllPointers literally adds all deltas together, which leads to twice the scrolling delta for two pointers. Other than that, I think either extremePointers or boundaryPointers sound ok. (directionalMaxPointers sounds a little too lengthy for me for no extra benefits.)

Copy link
Contributor

@dkwingsmt dkwingsmt Mar 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want a verb, maybe we can use averageBoundaryPointers, since both the one-directial algorithm and the panning algorithm can be seen as averaging.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 13, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 14, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 15, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App f: cupertino flutter/packages/flutter/cupertino repository f: gestures flutter/packages/flutter/gestures repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve fidelity of multi-pointer drag gestures
3 participants