software-mansion / react-native-gesture-handler

Declarative API exposing platform native touch and gesture system to React Native.
https://docs.swmansion.com/react-native-gesture-handler/
MIT License
5.83k stars 952 forks source link

Make FlingHandler use velocity as the activation metric. #2796

Closed latekvo closed 1 month ago

latekvo commented 1 month ago

Description

Fixed fling component handler to activate based on fling velocity instead of fling distance. The fling also no longer has to be performed from start to finish entirely within the component's bounds. This feature was tested on both emulated and live android phones.

Test plan

Web

Android

IOS

Reference

https://github.com/software-mansion/react-native-gesture-handler/assets/74246391/6868ff68-4e6e-41fb-93a7-0cd35e85cd6e

j-piasecki commented 1 month ago

I'm not sure if that's what we actually want 😅 Handlers have property called shouldCancelWhenOutside, so behavior of moving out of the component's bounds should match this property

I haven't looked at the code yet, but is there a reason why we cannot have both behaviors depending on shouldCancelWhenOutside?

If we can only choose one behavior I'd argue that, especially in the case of fling, having it not cancel when moving outside the view would be more desirable but having both would be great.

m-bert commented 1 month ago

I agree that those changes in Vector could also be done on web (cc @j-piasecki).

latekvo commented 1 month ago

Give me one minute, i forgot to rewrite the JS vector as well.

latekvo commented 1 month ago

Looks good, besides the little thing with VelocityTracker!

I have one more thing: right now the the vectors are considered aligned when the angle between them is less than ~41 degrees. It works great, but when setting the direction of UP | RIGHT (or any other 2 adjacent directions) it leaves a cone of ~18 degrees where the gesture doesn't activate. I think we can change the DEFAULT_MIN_DIRECTION_ALIGNMENT to 0.707 which would mean considering two vectors aligned when the angle between them is less than 45.008 degrees, so there would not be a no-gesture-recognized cone in-between. What do you think?

Ideally, we would want to reduce the cone for a particular direction to like 60-45 deg and expand them when 2 adjacent directions are set. We may also consider going in the direction of iOS, which does a weird thing of rotating the cone (I think? cc. m-bert) when adjacent directions are set. That's something up for discussion though, and definitely for another PR.

I believe when user enables more than 1 direction, it is very likely they intend them to serve separate functions. We're still waiting for @m-bert 's opinion, and his approve on changes.

m-bert commented 1 month ago

I don't think that trying to mimic iOS behavior is a good idea. We will have some cross-platform inconsistency, but still we will achieve something what I believe can be called "expected behavior" on web and android.

I think we can change the DEFAULT_MIN_DIRECTION_ALIGNMENT to 0.707 which would mean considering two vectors aligned when the angle between them is less than 45.008 degrees, so there would not be a no-gesture-recognized cone in-between.

I agree with that 👍

Ideally, we would want to reduce the cone for a particular direction to like 60-45 deg and expand them when 2 adjacent directions are set.

If we consider expanding the cone, 45 deg sounds a bit too much for me. In that case I would set it to ~30 deg for one direction and expand it when we set adjacent directions. We can discuss it next time though.

latekvo commented 1 month ago

Alright, I'm increasing the constant to 0.86 (30 deg), and adding angle expansion for adjacent enabled directions. (we're moving this issue to a different PR)

m-bert commented 1 month ago

:shipit::shipit::shipit: