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

Fix bad curve on easeTo/flyTo in constrained space #3793

Merged
merged 19 commits into from
Mar 10, 2024

Conversation

sbachinin
Copy link
Collaborator

@sbachinin sbachinin commented Mar 5, 2024

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to 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 therefore fitBounds and fitScreenCoordinates)
  • easeTo (and therefore panTo and panBy)

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 new getConstrained 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] inside getConstrained. (Instead of doing that in easeTo/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:

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.

  1. Disable constraints for the time of flyingTo.
    This will result in:
  • proper animation path
  • "restricted" space (possibly white bg) will be visible for a little while. (I believe this is not essential).
  • panning / zooming / whatever during the unfinished "flight" outside the bounds will cause an abrupt return into the bounds.
    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.
  1. It must be possible to ignore user actions when there is an unfinished flight and it's happening outside the bounds.
    Result:
  • animation always looks good.
  • user can't pan/zoom/... during this "overflow". IDK if it's a big problem.
    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.

@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 98.50746% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 86.41%. Comparing base (ee0450c) to head (4ec497f).
Report is 19 commits behind head on main.

Files Patch % Lines
src/geo/transform.ts 98.27% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

src/geo/transform.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Collaborator

HarelM commented Mar 7, 2024

I would feel better if there was a test specifically to cover this bug fix.
Might be a stupid question, but did you make sure fly to and ease to work as expected if renderWorldCopies is ture?

If you can add some videos of before and after for both cases it would be amazing.
Thanks for all the work on this!!

@sbachinin
Copy link
Collaborator Author

ease.before.after.mp4

@sbachinin
Copy link
Collaborator Author

sbachinin commented Mar 9, 2024

I would feel better if there was a test specifically to cover this bug fix.

As I said, I don't think it's testable.
The bug was only in the trajectory of easeTo/flyTo, not in the final destination.
So the fix shows up only in the intermediate frames.
Perhaps it's technically possible to check intermediate values but such test will surely be very unstable.

In case you are worried about easing/flying to the side globes, I think it's fully covered in camera.test.ts, for example

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.

@HarelM HarelM merged commit b68f6a3 into maplibre:main Mar 10, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants