Fix MGLMapCamera initialization and viewing distance calculations #12966

1ec5 · 2018-09-25T20:54:50Z

MGLMapCamera can now be expressed in terms of viewing distance. +[MGLMapCamera cameraLookingAtCenterCoordinate:fromDistance:pitch:heading:] incorrectly treated the distance as an altitude. Due to backwards compatibility concerns, this method can’t be fixed in place until the next semver-major release. Instead, separate +cameraLookingAtCenterCoordinate:correctlyFromDistance:pitch:heading: and +cameraLookingAtCenterCoordinate:altitude:pitch:heading: methods have been added, and the original method has been deprecated to head-off continuing confusion.

A viewingDistance property has been added to MGLMapCamera. This property is computed from the altitude and pitch properties, so there isn’t any parallel state to keep track of. The benefit of this approach over #12938 is that there are fewer, more testable code paths, and developers using this SDK can officially take advantage of the viewingDistance property without having to forward-declare anything. One can imagine an eyeCoordinate property being added along the same lines in the future.

Rewrote +[MGLMapCamera cameraLookingAtCenterCoordinate:fromEyeCoordinate:eyeAltitude:] to produce the right heading and pitch (and thus respect the specified eye coordinate).

Please double-check my trigonometry. ~~I still need to add unit tests before this PR can land.~~

Fixes #12943 and fixes #12967. Supersedes #12938 and part of #12518.

/cc @d-prukop @fabian-guerra @julianrex

d-prukop

I can't wait to see if this works with my animations. A few fixes to make. Let me know when you're ready

d-prukop · 2018-09-25T22:28:36Z

platform/darwin/src/MGLMapCamera.mm

+    CLLocationDirection eyeAngle = 360 - groundAngle - pitch;
+    CLLocationDistance altitude = self.altitude;
+    // Solution of triangles given angle-side-angle.
+    return altitude * sin(groundAngle) / (sin(groundAngle) * cos(eyeAngle) + sin(eyeAngle) * cos(groundAngle));


These angles need to be converted to radians

Good catch – more reason for me to unit test this thing. 😄

Spotted #12967 along the way.

d-prukop · 2018-09-25T23:03:46Z

platform/darwin/src/MGLMapCamera.mm

+    CLLocationDirection eyeAngle = 360 - groundAngle - pitch;
+    CLLocationDistance altitude = self.altitude;
+    // Solution of triangles given angle-side-angle.
+    return altitude * sin(groundAngle) / (sin(groundAngle) * cos(eyeAngle) + sin(eyeAngle) * cos(groundAngle));


groundAngle = PI/2, sin(PI/2) = 1 and cos(PI/2) = 0.
This equation is effectively altitude / cos(eyeAngle) which produces a negative number.
I think eyeAngle is meant to be the difference between the pitch value and 90.

Try this:

- (CLLocationDistance)viewingDistance { CLLocationDirection eyeAngleFromGround = 90 - self.pitch; CGFloat eyeAngleFromGroundRadians = eyeAngleFromGround * M_PI_2 / 180; CLLocationDistance altitude = self.altitude; return altitude / sin(eyeAngleFromGroundRadians) }

platform/darwin/src/MGLMapCamera.mm

1ec5 · 2018-09-26T02:13:56Z

Thanks for your feedback, @d-prukop. I’ve fixed the issues you pointed out and added tests.

1ec5 · 2018-09-26T04:06:17Z

platform/darwin/test/MGLMapCameraTests.m

+                                                   correctlyFromDistance:10000
+                                                                   pitch:0
+                                                                 heading:0];
+    MKMapCamera *mkCamera = [MKMapCamera cameraLookingAtCenterCoordinate:fountainSquare


As far as I can tell, this is the first time we’re explicitly testing the iOS/macOS SDK against MapKit. I think we should try to test against MapKit like this whenever an API is intended to match a MapKit API one-for-one.

1ec5 · 2018-09-27T18:20:38Z

I went ahead and fixed #12967 too.

d-prukop · 2018-09-27T19:22:59Z

This doesn't quite address my problems with MGLCamera. There is still wierd behavior when animating viewingDistance and pitch at the same time. I'm going to ask that we still merge in #12938 until we can come up with a more comprehensive solution.

fabian-guerra · 2018-09-27T19:53:51Z

platform/darwin/src/MGLMapCamera.h

@@ -63,10 +78,47 @@ MGL_EXPORT
    The value `180` means the top of the map points due south, and so on.
 */
 + (instancetype)cameraLookingAtCenterCoordinate:(CLLocationCoordinate2D)centerCoordinate
-                                   fromDistance:(CLLocationDistance)distance
+                          correctlyFromDistance:(CLLocationDistance)distance


Would it be possible avoid the correctly word? what are your thoughts on fromLineOfSightDistance?

I considered fromLineOfSightDistance, but it bridges to Swift as fromLineOfSight, which is unfortunate. “Correctly” is also unfortunate, but I’m hopeful that it would be temporary and that we’d swap it in for fromDistance at the next available opportunity.

Another possibility would be acrossDistance, which bridges to Swift elegantly as across.

I'm inclined for acrossDistance instead.

I like having the word distance in there - can we use NS_SWIFT_NAME in this case? To throw some more out there, perhaps withSubjectDistance, fromViewDistance, withCameraAtDistance.

I like having the word distance in there

Forcing the word “distance” would violate Swift API naming guidelines. The Swift compiler is doing the right thing here, since the type is CLLocationDistance. I’ll go with acrossDistance in Objective-C, which bridges to Swift as across; none of the other suggestions to a better job of distinguishing between viewing distance and viewing altitude anyways.

1ec5 · 2018-09-27T21:01:23Z

There is still wierd behavior when animating viewingDistance and pitch at the same time.

Is this the same behavior seen in the “before” screen recording in #12938 (comment)? If so, can you ensure that pitch is always set before viewingDistance? The conversion from viewingDistance to altitude is dependent on the pitch. If this is infeasible when working with Core Animation, then it would be trivial to add a -setViewingDistance:pitch: method to ensure the correct order.

julianrex

It would be great if we could find an alternative to across (which isn't particularly clear to me - it feels more like the "adjacent" rather than the hypotenuse).

julianrex · 2018-10-01T12:57:01Z

platform/darwin/src/MGLMapCamera.h

@@ -63,10 +85,46 @@ MGL_EXPORT
    The value `180` means the top of the map points due south, and so on.
 */
 + (instancetype)cameraLookingAtCenterCoordinate:(CLLocationCoordinate2D)centerCoordinate
-                                   fromDistance:(CLLocationDistance)distance
+                                 acrossDistance:(CLLocationDistance)distance


Can we make it clear that the across distance here is the same as the viewingDistance above? Either by documentation, by an alternative parameter name?

If we don't want to explicitly use viewingDistance due to Swift bridging, are there any alternatives that would be clearer? radius, orbit, span? Actually I quite like radius in this context.

I’m not sure any of those alternatives distinguishes between the hypotenuse and adjacent any more than acrossDistance would, but I just don’t think that distinction is very important. Consider that the original method – which I want to return to eventually, once we remove the incorrect implementation – calls it fromDistance, for simplicity and consistency with MapKit. The point is to distinguish from the opposite (altitude), which all these options accomplish to varying degrees. (The property, meanwhile, is still called viewingDistance – do you think it needs to be revisited?)

If acrossDistance doesn’t work, then my next suggestion would be -cameraLookingAtDistance:toCoordinate:pitch:heading: (camera(lookingAt:to:pitch:heading:)) or something like that. But honestly, I keep getting frustrated because all the good ways to express this concept in English require putting a word after the quantity (e.g., “from … away”), which doesn’t work here.

Yep - I totally hear you about language frustrations. I think the truth is that we have the best one, it's just that we need to deprecate it first before re-using it! 🔀

Let's get this one in 👍

Thanks, for what it’s worth, there is a more detailed explanation of this parameter a few lines further up:

mapbox-gl-native/platform/darwin/src/MGLMapCamera.h

Lines 71 to 73 in 44aa4f4

This method interprets the distance as a straight-line distance from the

viewpoint to the center coordinate. To specify the altitude of the viewpoint,

use the `-cameraLookingAtCenterCoordinate:altitude:pitch:heading:` method.

fabian-guerra

👍🏼

+[MGLMapCamera cameraLookingAtCenterCoordinate:fromDistance:pitch:heading:] incorrectly treated the distance as an altitude. Due to backwards compatibility concerns, this method can’t be fixed in place. Instead, separate +[MGLMapCamera cameraLookingAtCenterCoordinate:correctlyFromDistance:pitch:heading:] and +[MGLMapCamera cameraLookingAtCenterCoordinate:altitude:pitch:heading:] methods have been added.

Co-authored-by: Dave Prukop <dave.prukop@mapbox.com>

Rewrote the code that calculates the heading and pitch of the camera when given an eye coordinate.

1ec5 added bug iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS navigation For the Mapbox Navigation SDK for Android or iOS or navigation use cases in general MapKit parity For feature parity with MapKit on iOS or macOS labels Sep 25, 2018

1ec5 self-assigned this Sep 25, 2018

1ec5 requested review from fabian-guerra, julianrex and d-prukop September 25, 2018 20:54

1ec5 mentioned this pull request Sep 25, 2018

[iOS] Map view can now be optionally configured to make pitch adjustments without affecting altitude #12938

Closed

d-prukop suggested changes Sep 25, 2018

View reviewed changes

1ec5 force-pushed the 1ec5-camera-distance-12943 branch 2 times, most recently from 34c2425 to 4413e82 Compare September 26, 2018 02:12

1ec5 requested a review from d-prukop September 26, 2018 02:13

1ec5 force-pushed the 1ec5-camera-distance-12943 branch from 4413e82 to 8577a32 Compare September 26, 2018 04:02

1ec5 commented Sep 26, 2018

View reviewed changes

1ec5 changed the title ~~Allow expressing MGLMapCamera in terms of viewing distance~~ Fix MGLMapCamera initialization and viewing distance calculations Sep 27, 2018

d-prukop approved these changes Sep 27, 2018

View reviewed changes

fabian-guerra reviewed Sep 27, 2018

View reviewed changes

1ec5 force-pushed the 1ec5-camera-distance-12943 branch from f42bbd4 to 4a13bd7 Compare September 28, 2018 19:36

julianrex approved these changes Oct 1, 2018

View reviewed changes

fabian-guerra approved these changes Oct 1, 2018

View reviewed changes

1ec5 and others added 3 commits October 1, 2018 17:07

[ios, macos] Added MGLMapCamera.viewingDistance property

fe5ad23

Co-authored-by: Dave Prukop <dave.prukop@mapbox.com>

[ios, macos] Fixed camera initialization with eye coordinate

44aa4f4

Rewrote the code that calculates the heading and pitch of the camera when given an eye coordinate.

1ec5 force-pushed the 1ec5-camera-distance-12943 branch from 4a13bd7 to 44aa4f4 Compare October 2, 2018 00:07

1ec5 merged commit a9dc1b5 into master Oct 2, 2018

1ec5 deleted the 1ec5-camera-distance-12943 branch October 2, 2018 01:12

1ec5 mentioned this pull request Nov 6, 2018

Fix camera initializer deprecation warnings mapbox/mapbox-navigation-ios#1815

Merged

1ec5 mentioned this pull request May 3, 2019

Deprecation warning in first steps tutorial — MGLCamera(lookingAtCenter:fromDistance:pitch:heading:) mapbox/ios-sdk-examples#298

Open

1ec5 mentioned this pull request Jul 18, 2019

Maximum pitch should vary by zoom level #6908

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix MGLMapCamera initialization and viewing distance calculations #12966

Fix MGLMapCamera initialization and viewing distance calculations #12966

1ec5 commented Sep 25, 2018 •

edited

Loading

d-prukop left a comment

d-prukop Sep 25, 2018

1ec5 Sep 25, 2018

1ec5 Sep 26, 2018

d-prukop Sep 25, 2018

1ec5 commented Sep 26, 2018

1ec5 Sep 26, 2018

julianrex Oct 1, 2018

1ec5 commented Sep 27, 2018

d-prukop commented Sep 27, 2018

fabian-guerra Sep 27, 2018

1ec5 Sep 27, 2018 •

edited

Loading

1ec5 Sep 27, 2018

fabian-guerra Sep 27, 2018

julianrex Sep 28, 2018

1ec5 Sep 28, 2018 •

edited

Loading

1ec5 commented Sep 27, 2018

julianrex left a comment

julianrex Oct 1, 2018

1ec5 Oct 1, 2018 •

edited

Loading

julianrex Oct 1, 2018

1ec5 Oct 2, 2018

fabian-guerra left a comment

	This method interprets the distance as a straight-line distance from the
	viewpoint to the center coordinate. To specify the altitude of the viewpoint,
	use the `-cameraLookingAtCenterCoordinate:altitude:pitch:heading:` method.

Fix MGLMapCamera initialization and viewing distance calculations #12966

Fix MGLMapCamera initialization and viewing distance calculations #12966

Conversation

1ec5 commented Sep 25, 2018 • edited Loading

d-prukop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Sep 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Sep 27, 2018

d-prukop commented Sep 27, 2018

Choose a reason for hiding this comment

1ec5 Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

1ec5 commented Sep 27, 2018

julianrex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabian-guerra left a comment

Choose a reason for hiding this comment

1ec5 commented Sep 25, 2018 •

edited

Loading

1ec5 Sep 27, 2018 •

edited

Loading

1ec5 Sep 28, 2018 •

edited

Loading

1ec5 Oct 1, 2018 •

edited

Loading