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.

Sign up for GitHub

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

Jump to bottom

Improve fling animation #963

Merged
merged 4 commits into from
Apr 3, 2023
Merged

Improve fling animation #963

merged 4 commits into from
Apr 3, 2023

Conversation

Helium314
Copy link
Contributor

PR for #927

This improves the fling animation with the main goal of the animation being similar to other maps.
See #927 (comment) for the reasons.
Short:

  • UnitBezier parameters are adjusted for a less sudden stop of the fling animation
  • Offset is now velocity / animationTime (with some mysterious factor, chosen so that animation starts at same velocity as the user moved the map)
  • Animation base time and fling threshold can be set in UiSettings. In my opinion default threshold is too high, and animation time too short, making the map feel "sticky".

@Helium314
improve fling animation
4ef144b 
* change offset to velocity / animationTime (with some mysterious factor)
* allow setting animation base time and fling threshold in UiSettings
* adjust UnitBezier parameters for less sudden stop of fling animation
@louwers louwers requested a review from artakka March 31, 2023 20:54
@louwers louwers added android enhancement New feature or request labels Mar 31, 2023
@louwers
Copy link
Collaborator

louwers commented Mar 31, 2023

Neat! Could you update the android CHANGELOG.md?

@louwers louwers requested a review from ovivoda March 31, 2023 20:55
@Helium314
Copy link
Contributor Author

Helium314 commented Apr 1, 2023 •
edited
Loading

Sure!

Did you test it? I am not 100% sure the factor 0.28 will work on all screenDensities / tilts.
Especially with tilt testing is necessary (wasn't able to tilt, as mentioned in #927 (comment)

What else is to be done?

  • javadoc for changes in UI settings
  • moving UI settings changes to "correct" position (not sure if there is any ordering / grouping of settings)
  • maybe increasing ANIMATION_DURATION_FLING_BASE from ANIMATION_DURATION_SHORTto ANIMATION_DURATION (150 ms is much too short in my opinion)
  • and maybe reduce the default fling threshold?

ovivoda
ovivoda approved these changes Apr 1, 2023
View reviewed changes
Copy link
Contributor

@ovivoda ovivoda left a comment

Choose a reason for hiding this comment

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

This looks good! Are you able to provide a video of the change in action on one of the native platforms?

@Helium314
Copy link
Contributor Author

This looks good! Are you able to provide a video of the change in action on one of the native platforms?

Same app on Android as in #927 (comment), but with the updated library with flingAnimationBaseTime set to 300 ms and flingThreshold set to 300:

ml_new.mp4

@ovivoda ovivoda merged commit a454f26 into maplibre:main Apr 3, 2023
artakka
artakka reviewed Apr 3, 2023
View reviewed changes
Copy link
Collaborator

@artakka artakka left a comment

Choose a reason for hiding this comment

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

Looks like a great cleanup. I have just one minor comment.

...m/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapGestureDetector.java
// factor 1000 because speed is in pixels/s
// and the factor 0.28 was determined by testing: panning the map and releasing
// should result in fling animation starting at same speed as the move before
double offsetX = velocityX * animationTime * 0.28 / 1000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should 0.28 / 1000 also be a constant defined either in uiSettings or in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The factor 0.28 should be a constant, as I found just ~10-20% changed value results in the animation feeling wrong (e.g. with 0.4 the animation starts noticeably faster than the the map was moved before taking the finger off the display).

1/1000 could be some MILLISECONDS_TO_SECONDS constant. This is used to convert animationTime to seconds (it's mapbox source but this helped me here: https://github.com/mapbox/mapbox-gestures-android/blob/master/library/src/main/java/com/mapbox/android/gestures/ProgressiveGesture.java#L82 means that velocity is computed per 1000 ms).

@ysmaherzi
Copy link

ysmaherzi commented Jan 24, 2024 •
edited
Loading

Hi,
This fix was published in the version 10.1.0 and is still present to this day on master but it's unavaiable on the latest stable 10.2.0.
Does someone knows why ?

Thank you,

@louwers
Copy link
Collaborator

louwers commented Jan 24, 2024 •
edited
Loading

@ysmaherzi 10.2.0 reverted 10.1.0 because it contained a breaking change and should have been a major release.

Sorry for the confusion. This change will be included in the 11.0.0 release (again).

@ysmaherzi
Copy link

Thank you @louwers :)

louwers pushed a commit to louwers/maplibre-native that referenced this pull request Feb 23, 2024
@Helium314 @ovivoda @louwers
Improve fling animation (maplibre#963)
71983d8 
Co-authored-by: Ovidiu Voda <ovi.voda@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artakka artakka artakka left review comments

@ovivoda ovivoda ovivoda approved these changes

Assignees
No one assigned
Labels
android enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Helium314 @louwers @ysmaherzi @artakka @ovivoda