-
-
Notifications
You must be signed in to change notification settings - Fork 790
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
Fix bad curve on easeTo/flyTo in constrained space #3793
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3793 +/- ##
==========================================
- Coverage 86.64% 86.41% -0.23%
==========================================
Files 240 240
Lines 32273 32289 +16
Branches 1977 1959 -18
==========================================
- Hits 27963 27903 -60
- Misses 3376 3463 +87
+ Partials 934 923 -11 ☔ View full report in Codecov by Sentry. |
I would feel better if there was a test specifically to cover this bug fix. If you can add some videos of before and after for both cases it would be amazing. |
ease.before.after.mp4 |
As I said, I don't think it's testable. In case you are worried about easing/flying to the side globes, I think it's fully covered in I've added missing 2 test cases that easeTo doesn't cross antimeridian on a single-globe map. (Though this is really not what this pr is about). Similar tests for flyTo were already there. |
CHANGELOG.md
under the## main
section.I know there's a lot to review now so don't haste with this )
This PR involves several methods that animate the map's scale and center:
flyTo
(and thereforefitBounds
andfitScreenCoordinates
)easeTo
(and thereforepanTo
andpanBy
)These methods don't animate well when the destination point is near the bounds: those set by maxBounds, by vertical edges of the world, by antimeridian on single-globe map.
What is apparently incorrect in current easeTo and flyTo algorithms is that they don't apply these constraints when deciding on the destination. They just take the destination lngLat as it was provided by a user.
The result is that this "optimistic" animation hits the bounds somewhere in the process, and a smooth curve breaks into something more linear.
This PR fixes this problem. It runs the code from
transform._constrain
in advance, before the animation starts.For that I had to extract most of
transform._constrain
code into a newgetConstrained
method that doesn't set anything on Transform instance but only calculates zoom and center for a given point. There are a lot of changes in this method but it's mostly just renaming the magic variables.The main meaningful change there is that I deal not with actual
transform.worldSize
but with a "theoretical"worldSize
that corresponds to a requested zoom level.Also, the requested zoom level is now clamped to [
minZoom
,maxZoom
] insidegetConstrained
. (Instead of doing that ineaseTo
/flyTo
). I think it just makes sense because zoom bounds are closely related to coord bounds.This change in itself is non-destructive and it surely changes the situation for the better.
The implications are:
easeTo is always smooth near the bounds:
https://github.com/maplibre/maplibre-gl-js/assets/11789697/b5347134-a529-4575-993c-e7dd1883cf95
flyTo is smooth near the bounds only in some cases - when there is no significant zooming out (on low zoom levels or when a user sets
curve
option to something small).In other cases flyTo trajectory gets only slightly better but is still broken.
The problem of flyTo is not only the final destination but intermidate frames too because of zooming out. Ideal solution to this will be to calculate the animation path with respect of constraints. But this will be a lot of engineering with little impact.
I see a couple of (supposedly) simple changes here. They may look a bit controversial but I think they will provide a better UX. They are not included in this PR.
This will result in:
The last problem is of course not that small. Though still we are in a better position: bad behaviour will happen only on interaction and in a situation where animation was ugly anyway.
In terms of code, I tried it and it looks easy.
Result:
I can't say yet anything about code complexity of this step.
As for tests, I didn't add any. The only thing that changes here in terms of UX is the trajectory of animation. (Final point doesn't change).
Now it's more possible to test
getConstrained
in isolation but IDK if it's desirable.