VelocityTracker isn't reset in between draggable gestures [199921305]

Bug

Status Update

No update yet.

Description

ch...@gmail.com

created issue #1

Sep 15, 2021 10:44AM

I'm currently writing a custom FlingBehavior which handles flings from the Lazy layouts. Unfortunately I'm seeing weirdness in the velocity values provided.

It seems that if you perform a number of flings in alternating directions (left then right, repeat), the velocities accumulate.

I had a quick look through draggable, and I'm not seeing anywhere where the VelocityTracker is being reset. I'd expect a call to velocityTracker.resetTracking() somewhere around here: https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/Draggable.kt;l=329

Is this intentional?

Comments

ch...@gmail.com <ch...@gmail.com> #2Sep 15, 2021 10:46AM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

ae...@google.com <ae...@google.com> #3Sep 15, 2021 07:07PM

Reassigned to ae...@google.com.

I have a WIP patch aosp/1826965 which greatly increases the volume and accuracy of data fed to VelocityTracker. It doesn't include any additional reset calls right now, but it resolves a lot of the weird behavior in fling velocity.

Examining the original logic in HorizontalScrollView, it looks like we ought to add clear() calls when the last finger is released and also when the maximum scroll extent is reached. I can see if it's easy to make those changes as part of my patch. Although, just the additional samples alone in my current WIP already make it much more unlikely for the direction to be reversed, and I can't repro that problem with my patch applied right now.

ch...@gmail.com <ch...@gmail.com> #4Sep 16, 2021 09:32AM

I had a chat with Matvei yesterday and I don't think my hypothesis about the "VelocityTracker not being reset" is correct. He pointed out that draggable uses a new tracker for each gesture (here), so resetting shouldn't be necessary.

I'll try aosp/1826965 once it lands. Thanks!

ap...@google.com <ap...@google.com> #5Sep 22, 2021 01:28AM

Project: platform/frameworks/support
Branch: androidx-main

commit 9446c48c42cd9583bf86249eda1aa2e1bb3c59d5
Author: Alexandre Elias <aelias@google.com>
Date: Thu Sep 16 20:15:37 2021

Velocity tracking for historical changes

This forwards the additional "historical" pointer information in
MotionEvent to the fling velocity tracker, allowing more accurate fling
detection.

It also forwards ordinary pointer information for the
first DOWN event and first (slop) MOVE event, matching the framework
velocity tracker.

Test: MoveWithHistoryTest.flingScrollableWithHistorical
Fixes: 197553056
Fixes: 199914454
Bug: 199921305
Relnote: Added experimental historical pointers to PointerEventChange.

Change-Id: Ic1fd82fd3f2335a9289cc1fc96c35e89ec9b90ee

M compose/ui/ui-test/src/androidMain/kotlin/androidx/compose/ui/test/AndroidInputDispatcher.android.kt
M compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/input/pointer/MotionEventAdapter.android.kt
M compose/ui/ui-test/api/public_plus_experimental_current.txt
M compose/ui/ui-test/src/commonMain/kotlin/androidx/compose/ui/test/MultiModalInjectionScope.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/Draggable.kt
M compose/ui/ui-test/src/commonMain/kotlin/androidx/compose/ui/test/TouchInjectionScope.kt
A compose/ui/ui-test/src/androidAndroidTest/kotlin/androidx/compose/ui/test/injectionscope/touch/MoveWithHistoryTest.kt
M compose/ui/ui/api/current.txt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/input/pointer/PointerInputEventProcessorTest.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/pointer/util/VelocityTracker.kt
M compose/ui/ui-test/src/commonMain/kotlin/androidx/compose/ui/test/InputDispatcher.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/pointer/PointerInputEventProcessor.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/pointer/PointerEvent.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ScrollableTest.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/pointer/HitPathTracker.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/pointer/InternalPointerInput.kt
M compose/ui/ui/api/restricted_current.txt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/gesture/OverScrollTest.kt
M compose/ui/ui-test/src/desktopMain/kotlin/androidx/compose/ui/test/DesktopInputDispatcher.desktop.kt
M compose/ui/ui/api/public_plus_experimental_current.txt

https://android-review.googlesource.com/1826965

ma...@gmail.com <ma...@gmail.com> #6Jan 27, 2022 10:00AM

Weirdness = velocities in the wrong direction to what I just dragged

I suspect this is because the velocity calculation is based on touch position relative to the component handling the pointer input.

Associated issue in the Jetpack Compose - UI | Issue Tracker

Message last modified on Jan 27, 2022 12:28PM

is...@google.com <is...@google.com> Mar 11, 2024 07:07AM

Status: New

Issue 199921305