Add pitch argument to cameraThatFits functions #12213

bsudekum · 2018-06-25T19:48:32Z

In the navigation sdk, we use MGLMapView.cameraThatFitsShape(_:direction: edgePadding) to go from navigation mode to overhead mode. However, since this function does not provide a pitch argument, the function relies on the mapview's current pitch to calculate the new camera. This does not work well because we want to end up overhead, looking straight down.

Because of this, we have to jarringly set the pitch to 0, then find the appropriate camera that fits the current route line. If we could pass in the desired pitch, this first camera animation would not be required.

This change allows for developers to pass in a desired pitch for a few of the cameraThatFites... functions.

/cc @asheemmamoowala @1ec5 @frederoni

todo:

add documentation

asheemmamoowala · 2018-06-25T23:23:57Z

src/mbgl/map/map.cpp

-CameraOptions Map::cameraForLatLngs(const std::vector<LatLng>& latLngs, const EdgeInsets& padding, optional<double> bearing) const {
-    if(bearing) {
+CameraOptions Map::cameraForLatLngs(const std::vector<LatLng>& latLngs, const EdgeInsets& padding, optional<double> bearing, optional<double> pitch) const {
+    if (bearing && pitch) {


This will provide incorrect results if only one of bearing or pitch is provided. The if clause would need to handle each of bearing and pitch individually.

asheemmamoowala · 2018-06-26T17:48:31Z

src/mbgl/map/map.cpp

+        options.angle = transform.getAngle();
+    }
+    if (pitch) {
+        options.angle = transform.getAngle();


Should set pitch here.

asheemmamoowala · 2018-06-26T17:49:51Z

src/mbgl/map/map.cpp

+
+    CameraOptions options = mbgl::cameraForLatLngs(latLngs, transform, padding);
+
+    if (bearing) {


Angle and pitch can be set on options without the additional if blocks.

asheemmamoowala · 2018-06-27T17:15:50Z

test/map/map.test.cpp

@@ -84,7 +84,7 @@ TEST(Map, LatLngBoundsToCameraWithAngle) {
    CameraOptions virtualCamera = test.map.cameraForLatLngBounds(bounds, {}, 35);
    ASSERT_TRUE(bounds.contains(*virtualCamera.center));
    EXPECT_NEAR(*virtualCamera.zoom, 1.21385, 1e-5);
-    EXPECT_DOUBLE_EQ(virtualCamera.angle.value_or(0), -35 * util::DEG2RAD);
+    EXPECT_NEAR(virtualCamera.angle.value_or(0), -35 * util::DEG2RAD, 1e-6);


May need this on Line 96 below as well. While you're at it, could you also add a test with pitch and bearing, and oe with just pitch.

bsudekum · 2018-06-27T18:23:46Z

src/mbgl/map/map.cpp

+        transform.setAngle(angle);
+    }
+    if (pitch) {
+        double pitchAsRadian = *pitch * util::DEG2RAD; // Convert to radians


Unlike bearing, negative does not work here. Does this look right to you @asheemmamoowala?

Yes thats correct. Bearing is counter clockwise from North,

1ec5

This PR implements #12238. Please make any corresponding changes to the macOS implementation of MGLMapView, then add a blurb to the iOS and macOS map SDKs noting the addition of these methods.

1ec5 · 2018-06-27T20:04:10Z

platform/ios/src/MGLMapView.h

@@ -926,6 +926,23 @@ MGL_EXPORT IB_DESIGNABLE
 */
 - (MGLMapCamera *)cameraThatFitsCoordinateBounds:(MGLCoordinateBounds)bounds edgePadding:(UIEdgeInsets)insets;

+/**
+ Returns the camera that best fits the given coordinate bounds, optionally with
+ some additional padding on each side.


This sentence should indicate that the method lets the developer specify a direction and pitch.

1ec5 · 2018-06-27T20:11:23Z

platform/ios/src/MGLMapView.h

+ @return A camera object centered on the shape's center with zoom level as high
+    (close to the ground) as possible while still including the entire shape.
+ */
+- (MGLMapCamera *)cameraThatFitsShape:(MGLShape *)shape direction:(CLLocationDirection)direction pitch:(CGFloat)pitch edgePadding:(UIEdgeInsets)insets;


There are some problems with the design of this API:

This represents the first time we’re exposing pitch as an option in MGLMapView as opposed to MGLMapCamera. Up to now, we’ve deliberately excluded pitch parameters from MGLMapView, because they make more sense in MGLMapCamera’s 3D context than in MGLMapView’s largely 2D context. For example, MGLMapCamera offers two frames of reference (viewer and focal point), whereas MGLMapView can only express the focal point.

We’re trying to limit the number of viewport-related methods in MGLMapView, to avoid combinatorial explosion. What if the developer wants to specify a direction but pitch but not a direction?

From the method name (and even from the documentation), the relationship between the shape and the direction is unclear, and so is the relationship between the shape and the pitch.

+[MGLMapCamera cameraLookingAtCenterCoordinate:fromDistance:pitch:heading:] does a better job of describing the relationship between the center coordinate and the other parameters. Can we do something similar for shapes and coordinate bounds? Maybe something that explicitly returns a copy of an MGLMapCamera, like -[MGLMapView camera:fittingShape:edgePadding:]? We could deprecate -[MGLMapView cameraThatFitsShape:direction:edgePadding:] at the same time, since MGLMapCamera has a convention where values like −1 are ignored.

1ec5 · 2018-06-27T20:25:53Z

platform/ios/src/MGLMapView.mm

    mbgl::EdgeInsets padding = MGLEdgeInsetsFromNSEdgeInsets(insets);
    padding += MGLEdgeInsetsFromNSEdgeInsets(self.contentInset);
+    mbgl::CameraOptions cameraOptions = _mbglMap->cameraForLatLngBounds(MGLLatLngBoundsFromCoordinateBounds(bounds), padding, direction, pitch);


Does mbgl::Map::cameraForLatLngBounds() correctly handle the situation where either direction or pitch is negative? A negative value in an MGLMapCamera property normally means that the property is ignored.

mapbox-gl-native/platform/ios/src/MGLMapView.mm

Lines 3400 to 3407 in 3946841

if (camera.heading >= 0)

{

options.angle = MGLRadiansFromDegrees(-camera.heading);

}

if (camera.pitch >= 0)

{

options.pitch = MGLRadiansFromDegrees(camera.pitch);

}

All the mbgl::Map::cameraFor... methods end up in mbgl::Map::cameraForLatLngs. The input values are wrapped and clamped appropriately before use.

Before clamping, we need to check for < 0, which means “ignore” only on iOS and macOS, but not on other platforms.

bsudekum · 2018-06-28T21:35:30Z

@1ec5 updated and changed this PR to add a single new function, -[MGLMapView camera:fittingShape:edgePadding:]

1ec5 · 2018-06-29T01:49:29Z

platform/ios/src/MGLMapView.h

-    `0` results in a camera pointed straight down at the map. Angles greater
-    than `0` result in a camera angled toward the horizon.
+ @param camera The camera that the return camera should adhere to. All values
+    on this camera will be manipulated except for pitch and direction


Nit: missing a period at the end.

1ec5 · 2018-06-29T01:50:53Z

platform/ios/src/MGLMapView.mm

-    return [self cameraForCameraOptions:cameraOptions];
+
+    CGFloat pitch = camera.pitch;
+    CLLocationDirection direction = camera.heading;


When pitch or heading is negative, it should be ignored in favor of the most appropriate value (which can be the current value).

Hmm this brings up a good point. I feel like the lowest level function should be responsible for normalizing the direction.

This is the lowest-level method at which the normalization is relevant. The negative-means-ignored behavior is specific to Apple platforms.

1ec5 · 2018-06-29T01:51:54Z

platform/ios/src/MGLMapView.h

 */
- (MGLMapCamera *)cameraThatFitsCoordinateBounds:(MGLCoordinateBounds)bounds direction:(CLLocationDirection)direction pitch:(CGFloat)pitch edgePadding:(UIEdgeInsets)insets;
+- (MGLMapCamera *)camera:(MGLMapCamera *)camera fittingShape:(MGLShape *)shape edgePadding:(UIEdgeInsets)insets;


Is there a need for a -camera:fittingCoordinateBounds:edgePadding: as well?

Sure, will add.

1ec5 · 2018-06-29T01:52:33Z

Also, remember to make any corresponding changes to the macOS implementation of MGLMapView, then add a blurb to the iOS and macOS map SDKs noting the addition of these methods.

1ec5 · 2018-06-30T01:15:34Z

platform/ios/CHANGELOG.md

@@ -2,6 +2,10 @@

 Mapbox welcomes participation and contributions from everyone. Please read [CONTRIBUTING.md](../../CONTRIBUTING.md) to get started.

+# master
+
+* Added `-[MGLMapView camera:fittingShape:edgePadding:]` and `-[MGLMapView camera:fittingCoordinateBounds:edgePadding:]` allowing you specify the pitch and direction for the calculated camera.


Also add a link to this PR in parentheses.

1ec5 · 2018-06-30T01:17:04Z

platform/ios/src/MGLMapView.mm

-    return [self cameraForCameraOptions:cameraOptions];
+
+    CGFloat pitch = camera.pitch;
+    CLLocationDirection direction = camera.heading;


This is the lowest-level method at which the normalization is relevant. The negative-means-ignored behavior is specific to Apple platforms.

1ec5

One minor change to make, but otherwise good to go.

1ec5 · 2018-07-02T17:30:19Z

platform/macos/src/MGLMapView.mm

+    padding += MGLEdgeInsetsFromNSEdgeInsets(self.contentInsets);
+
+    CGFloat pitch = camera.pitch < 0 ? _mbglMap->getPitch() : camera.pitch;
+    CLLocationDirection direction = camera.heading < 0 ? _mbglMap->getBearing() : camera.heading;


When possible, use existing property getters to ensure that normalization is applied consistently:

MGLMapCamera *currentCamera = self.camera; CGFloat pitch = camera.pitch < 0 ? currentCamera.pitch : camera.pitch; CLLocationDirection direction = camera.heading < 0 ? currentCamera.heading : camera.heading;

(There’s also an MGLMapView.direction, but we have to call -camera anyways to get the current pitch.)

×4

Add pitch argument to cameraThatFits functions

cf43870

bsudekum requested review from 1ec5 and asheemmamoowala June 25, 2018 19:48

check if nil

073e497

bsudekum added iOS Mapbox Maps SDK for iOS navigation For the Mapbox Navigation SDK for Android or iOS or navigation use cases in general labels Jun 25, 2018

asheemmamoowala suggested changes Jun 25, 2018

View reviewed changes

Change around

74a166a

asheemmamoowala suggested changes Jun 26, 2018

View reviewed changes

Bobby Sudekum added 3 commits June 27, 2018 08:42

fix copy pasta

6de4b0a

Merge branch 'master' into cameraThatFitsWithPitch

8e04e6b

Round

d72d77b

asheemmamoowala reviewed Jun 27, 2018

View reviewed changes

Bobby Sudekum added 2 commits June 27, 2018 11:08

use rad

3c273bc

sp

f5f7397

bsudekum commented Jun 27, 2018

View reviewed changes

asheemmamoowala approved these changes Jun 27, 2018

View reviewed changes

asheemmamoowala mentioned this pull request Jun 27, 2018

Add pitch argument to MapboxMap.getCameraFor.. functions #12238

Closed

retest

3946841

1ec5 mentioned this pull request Jun 27, 2018

Add ability for cameraForShape to fix the pitch #10115

Closed

1ec5 reviewed Jun 27, 2018

View reviewed changes

Bobby Sudekum added 2 commits June 28, 2018 14:25

Update

92dac01

fix

0dca9d8

1ec5 reviewed Jun 29, 2018

View reviewed changes

Bobby Sudekum added 3 commits June 29, 2018 13:39

add camera fitting bounds, add macos

2dab59c

Merge branch 'master' into cameraThatFitsWithPitch

aa4dc6a

Changelog

28d2cd7

1ec5 reviewed Jun 30, 2018

View reviewed changes

Bobby Sudekum added 3 commits July 2, 2018 08:23

add link

cd5828c

Dont allow for negative values

71720d0

do everywhere

d862574

1ec5 approved these changes Jul 2, 2018

View reviewed changes

Use current camera

110c70b

bsudekum merged commit 4286d58 into master Jul 2, 2018

bsudekum deleted the cameraThatFitsWithPitch branch July 2, 2018 23:30

1ec5 added the macOS Mapbox Maps SDK for macOS label Jul 12, 2018

1ec5 mentioned this pull request Oct 3, 2018

Added cameraThatFitsShape: direction: pitch: edgePadding: method #13007

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pitch argument to cameraThatFits functions #12213

Add pitch argument to cameraThatFits functions #12213

bsudekum commented Jun 25, 2018 •

edited

Loading

asheemmamoowala Jun 25, 2018

asheemmamoowala Jun 26, 2018

asheemmamoowala Jun 26, 2018

asheemmamoowala Jun 27, 2018

bsudekum Jun 27, 2018

asheemmamoowala Jun 27, 2018

1ec5 left a comment •

edited

Loading

1ec5 Jun 27, 2018

1ec5 Jun 27, 2018

1ec5 Jun 27, 2018

asheemmamoowala Jun 27, 2018

1ec5 Jun 27, 2018

bsudekum commented Jun 28, 2018

1ec5 Jun 29, 2018

1ec5 Jun 29, 2018

bsudekum Jun 29, 2018

1ec5 Jun 30, 2018

1ec5 Jun 29, 2018

bsudekum Jun 29, 2018

1ec5 commented Jun 29, 2018

1ec5 Jun 30, 2018

1ec5 Jun 30, 2018

1ec5 left a comment

1ec5 Jul 2, 2018


		CameraOptions options = mbgl::cameraForLatLngs(latLngs, transform, padding);

		if (bearing) {

	if (camera.heading >= 0)
	{
	options.angle = MGLRadiansFromDegrees(-camera.heading);
	}
	if (camera.pitch >= 0)
	{
	options.pitch = MGLRadiansFromDegrees(camera.pitch);
	}

Add pitch argument to cameraThatFits functions #12213

Add pitch argument to cameraThatFits functions #12213

Conversation

bsudekum commented Jun 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsudekum commented Jun 28, 2018

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

Choose a reason for hiding this comment

1ec5 commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsudekum commented Jun 25, 2018 •

edited

Loading

1ec5 left a comment •

edited

Loading