Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] easeTo: linear interpolation over zoom instead of scale #15281

Merged
merged 3 commits into from
Aug 8, 2019

Conversation

astojilj
Copy link
Contributor

@astojilj astojilj commented Jul 31, 2019

Captured screen video could be compared against videos #15144.
Final version (reusing fly to calculation for constant movement) as in https://github.com/mapbox/mapbox-gl-native/files/3460613/Archive.zip

Image

Version after first patch:
Image

Linear interpolation over zoom looks more natural, as the rate of change looks constant to user: for every zoom change of 1, view displays 2 times wider area.

Problem if using scale could be described by this graph:

http://www.mathopenref.com/graphfunctions.html?fx=log(x)&xh=1000000&xl=0&yh=20&yl=0
Horizontal axis: scale, vertical axis: zoom:

Screen Shot 2019-07-31 at 23 36 02

When linearly interpolating over scale, zoom changes very slowly when zoomed in. When zooming out (values of scale towards 0), as reported in #15144, it feels like a sudden zoom out.

The transition could be further improved, by moving center coordinate slower for higher zoom values, to present linear interpolation of center coordinate on the screen, but this improvement already feels like fixing original issue.

Manual verification using modified iosapp:

diff --git a/platform/ios/app/MBXViewController.m b/platform/ios/app/MBXViewController.m
index 2fb95e1b1..bba60c64b 100644
--- a/platform/ios/app/MBXViewController.m
+++ b/platform/ios/app/MBXViewController.m
@@ -1784,11 +1784,28 @@ - (void)addAnnotations:(NSInteger)numAnnotations aroundCoordinate:(CLLocationCoo
 
 - (void)randomWorldTour {
     // Consistent initial conditions (consider setting these by test params)
-    srand48(0);
-    [self.mapView removeAnnotations:self.mapView.annotations];
-    [self.mapView setCenterCoordinate:CLLocationCoordinate2DMake(31, -100) zoomLevel:3 animated:NO];
-
-    [self randomWorldTourInternal];
+//    srand48(0);
+//    [self.mapView removeAnnotations:self.mapView.annotations];
+//    [self.mapView setCenterCoordinate:CLLocationCoordinate2DMake(31, -100) zoomLevel:3 animated:NO];
+
+//    [self randomWorldTourInternal];
+
+    static double altitude = 50000.0;
+    CLLocationCoordinate2D target;
+    if (altitude == 50000.0) {
+        altitude = 10.0;
+        target.latitude = 37.80296867678598;
+        target.longitude = -122.2494649887085;
+    } else {
+        altitude = 50000.0;
+        target.latitude = 37.85296867678598;
+        target.longitude = -122.2494649887085;
+    }
+    MGLMapCamera *camera = [MGLMapCamera cameraLookingAtCenterCoordinate:target
+                                                                altitude:altitude
+                                                                   pitch:0
+                                                                 heading:0];
+    [self.mapView setCamera:camera withDuration:2 animationTimingFunction:nil];
 }
 
 - (void)randomWorldTourInternal {

Fixes: #15144

@astojilj astojilj added the Core The cross-platform C++ core, aka mbgl label Jul 31, 2019
@astojilj astojilj requested review from 1ec5 and chloekraw July 31, 2019 20:50
@astojilj astojilj self-assigned this Jul 31, 2019
@astojilj astojilj marked this pull request as ready for review July 31, 2019 20:51
@astojilj
Copy link
Contributor Author

@1ec5 ,
PTAL.

@chloekraw chloekraw added the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 1, 2019
@chloekraw chloekraw added this to the release-queso milestone Aug 1, 2019
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

This change does look to be somewhat consistent with how flyTo() works. (I’d be more hesitant to change how flyTo() calculates the zoom level, because I’m pretty sure it’s now aligned with the paper describing the optimal path, following #3301 and #3355.)

We should check whether the trajectory looks good if the origin is at a low latitude and the destination is at a high latitude or vice versa. For example, how does it look if the camera eases from Null Island to Greenwich, England, with identical MGLMapCamera.altitude and thus different zoom levels?

Suggested changelog entry:

  • Fixed an issue where animated camera transitions zoomed in or out too dramatically.

@mourner
Copy link
Member

mourner commented Aug 1, 2019

The transition could be further improved, by moving center coordinate slower for higher zoom values, to present linear interpolation of center coordinate on the screen, but this improvement already feels like fixing original issue.

The video currently looks like the camera is going in a curve, not as a straight line, which I'd consider a bug for easeTo — how easy it would be to fix?

@astojilj astojilj force-pushed the astojilj-easeto-over-zoom branch from f8117eb to eb99e9c Compare August 2, 2019 09:32
@astojilj
Copy link
Contributor Author

astojilj commented Aug 2, 2019

@mourner , @1ec5 , @chloekraw , @ansis

Patch eb99e9c
reuses flyTo for easeTo implementation:

  • Limit zoom out to min(startZoom, targetZoom).
  • Linear zoom interpolation.

The patch also sets linear interpolation for flyTo edgeInsets animation, as it looks more appropriate to follow the approach taken for pitch and bearing.

Attached videos use this easeTo (patch in issue description) with two different set of coordinates.

Archive.zip

@astojilj
Copy link
Contributor Author

astojilj commented Aug 2, 2019

The transition could be further improved, by moving center coordinate slower for higher zoom values, to present linear interpolation of center coordinate on the screen, but this improvement already feels like fixing original issue.

The video currently looks like the camera is going in a curve, not as a straight line, which I'd consider a bug for easeTo — how easy it would be to fix?

Curve related to set latitude and longitude or altitude?

Could you compare it to videos attached to #15281 (comment)? One of the videos uses the same coordinates as in the gif.

@astojilj astojilj force-pushed the astojilj-easeto-over-zoom branch from eb99e9c to 9c9a034 Compare August 2, 2019 10:22
@astojilj astojilj requested a review from mourner August 2, 2019 11:32
@mourner
Copy link
Member

mourner commented Aug 2, 2019

@astojilj oh, yeah, I can see the "curve" in the camera path in those videos too (camera not moving in a straight line). I just checked in GL JS and there it is too. I guess it's an unrelated bug that was there before and I didn't pay attention. An example of a straight line camera path would be scroll-zooming with a mouse on a GL JS map.

@astojilj astojilj force-pushed the astojilj-easeto-over-zoom branch 3 times, most recently from 0710ed4 to 7cc6f6d Compare August 2, 2019 13:22
@astojilj astojilj requested a review from alexshalamov August 4, 2019 08:41
@astojilj
Copy link
Contributor Author

astojilj commented Aug 6, 2019

@alexshalamov PTAL

src/mbgl/map/transform.cpp Outdated Show resolved Hide resolved
@astojilj astojilj force-pushed the astojilj-easeto-over-zoom branch from 7cc6f6d to 7727324 Compare August 7, 2019 17:04
@astojilj astojilj requested a review from a team August 7, 2019 17:04
@astojilj
Copy link
Contributor Author

astojilj commented Aug 7, 2019

@1ec5 ,
Thanks.

  • Fixed an issue where animated camera transitions zoomed in or out too dramatically.

Done.

…plementation.

When using flyTo for easeTo:
Limit zoom out to min(startZoom, targetZoom).
Reduce cinematic zoom in animation at the end of transition,
Linear zoom interpolation.

The patch also sets linear interpolation for flyTo edgeInsets animation, as it looks more appropriate to follow the approach taken for pitch and bearing.

Fixes: #15144
@astojilj astojilj force-pushed the astojilj-easeto-over-zoom branch from b2454a8 to 03f64a6 Compare August 8, 2019 07:25
@astojilj astojilj merged commit 7cdeed7 into master Aug 8, 2019
@astojilj astojilj deleted the astojilj-easeto-over-zoom branch August 8, 2019 09:14
@chloekraw chloekraw removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core] easeTo camera animation looks buggy when camera needs to zoom out
6 participants