[core][macOS] Compute camera center and zoom for geometry with fixed heading by asheemmamoowala · Pull Request #10107 · mapbox/mapbox-gl-native

asheemmamoowala · 2017-10-02T22:18:25Z

Implement a Map:: cameraForLatLngs variant that allows specifying a heading and returns CameraOptions with zoom, center, and angle.

jfirebaugh

👍 core changes

1ec5

Thanks for running with this feature. In addition to the comments below, would you do the honors and add a blurb to the iOS and macOS changelogs?

1ec5 · 2017-10-03T17:42:42Z

 @class MGLAnnotationImage;
 @class MGLMapCamera;
 @class MGLStyle;
+@class MGLShape;


Can you make these changes to the iOS implementation of MGLMapView also? Thanks!

1ec5 · 2017-10-03T17:48:18Z

+    around the given coordinate bounds.
+ @param animated Specify `YES` to animate the change by smoothly scrolling and
+    zooming or `NO` to immediately display the given bounds.
+ */


This method is reminiscent of -showAnnotation:, which also adds the passed-in annotation (which may be a shape) if it isn't already on the map. It might be nice to generalize that method with these additional parameters, but I'm not sure it's necessary to have one that only changes the camera, since the developer can readily compose -cameraThatFitsShape:heading:edgePadding: with -setCamera:animated:. The reason -setVisibleCoordinateBounds:edgePadding:animated: exists is that there's already a visibleCoordinateBounds property. But there isn't inherently a "visible shape" the way there's a visible coordinate bounds.

@1ec5 I added this method to help with testing in the macOS demo app. and didn't expect it to be added to the mobile SDKs. Maybe there is a case to add this functionality to -showAnnotation but I'd prefer to scope this PR down to just the -cameraTheFitsShape changes.
//cc @d-prukop

We try to keep the iOS and macOS SDKs as close as possible. In fact, there’s an entire project tracking macOS SDK parity with the iOS SDK, but it’s generally assumed that the iOS SDK would get new features first because it’s a bigger priority for Mapbox.

Ideally we would find a way to make -cameraThatFitsShape:heading:edgePadding: compatible with one of the -setCamera:… methods, perhaps -setCamera:withDuration:animationTimingFunction:edgePadding:completionHandler:, such that we could add the same method to both SDKs publicly.

1ec5 · 2017-10-03T17:51:11Z

+    Transform transform(impl->transform.getState());
+    transform.setAngle(angle);
+    CameraOptions options = mbgl::cameraForLatLngs(latLngs, transform, padding);
+    options.angle = angle;


The return value inherits the current pitch, but the developer may want to animate to an arbitrary pitch for the same reason that they'd want to animate to an arbitrary angle.

Opened this as a separate issue #10115. It depends on us first getting a fix for #2259.

1ec5 · 2017-10-03T17:53:12Z

+}
+
+CameraOptions Map::cameraForLatLngs(const std::vector<LatLng>& latLngs, double heading, const EdgeInsets& padding) const {
+    double angle = -heading * util::DEG2RAD;


The convention throughout mbgl seems to be to use the term "angle" for what might be called "heading" or "bearing" elsewhere.

1ec5 · 2017-10-03T17:57:32Z

+    with zoom level as high (close to the ground) as possible while still
+    including the entire coordinate bounds. The camera object uses the current
+    direction and pitch.
+ */


MGLMapView uses the term "direction" instead of "heading", because "heading" is part of a 3D camera system that's encapsulated in MGLMapCamera. If we allow the heading to be configured but not the pitch, we should use "direction" here for consistency with -setDirection:. On the other hand, if we make the pitch configurable, as suggested below, then this method might take a camera (as matchingCamera:) instead of separate heading and pitch parameters.

Given #2259, I didn't want to add pitch just yet.
👍 on direction

1ec5 · 2017-10-03T17:58:11Z

+    around the returned camera object if it were set as the receiver’s camera.
+ @return A camera object centered on the same location as the coordinate bounds
+    with zoom level as high (close to the ground) as possible while still
+    including the entire coordinate bounds. The camera object uses the current


This method doesn't use the current direction, and it includes the entire shape, not an entire coordinate bounds.

1ec5 · 2017-10-03T18:00:02Z

+        MGLShapeCollection *shapeCollection = (MGLShapeCollection *)shape;
+        for(MGLShape *s in shapeCollection.shapes) {
+            auto shapeLatLngs = coordinatesForShape(s);
+            latLngs.insert(latLngs.end(), shapeLatLngs.begin(), shapeLatLngs.end());


Consider having the mbgl::Map method take a Geometry; each MGLShape subclass already has an internal method for converting to a Geometry.

1ec5

Added some final comments regarding the iOS and macOS changes. I’ll leave it to @tobrun to look at the Android side in detail.

1ec5 · 2017-10-04T22:29:39Z

Is MapboxMap::getCameraForGeometry() the right syntax for referring to an instance method in Java? I thought it was MapboxMap.getCameraForGeometry().

1ec5 · 2017-10-04T22:31:48Z

We just branched for iOS SDK v3.7.0 and macOS SDK v0.6.0 (in release-agua). Would you mind retitling the “master” section and adding a new “master” section above it?

1ec5 · 2017-10-04T22:33:21Z

The iOS and macOS SDKs always say “zoom level” and “center coordinate”, to differentiate from the act of zooming and the view’s center (point).

1ec5 · 2017-10-04T23:49:43Z

I continue to think this would be a poor API to expose publicly, per #10107 (comment). For one thing, it isn’t possible to customize the duration or timing function or provide a completion handler, all options that come with -setCamera:withDuration:animationTimingFunction:edgePadding:completionHandler: on iOS.

If it’s really necessary to achieve a padding-less camera fitting a shape, can we remove this method, replace it with -setCamera:withDuration:animationTimingFunction:edgePadding:completionHandler: from iOS, and use the following code to achieve the same effect for the time being?

MGLMapCamera *camera = [mapView cameraThatFitsShape:shape direction:direction edgePadding:edgePadding]; NSEdgeInsets edgePadding = contentInsets; edgePadding.left *= -1; edgePadding.right *= -1; edgePadding.top *= -1; edgePadding.bottom *= -1; [mapView setCamera:camera withDuration:5 animationTimingFunction:nil edgePadding:edgePadding completionHandler:nil];

1ec5 · 2017-10-07T01:19:16Z

 * Increased the default maximum zoom level from 20 to 22. ([#9835](https://github.com/mapbox/mapbox-gl-native/pull/9835))
 * Added an `overlays` property to `MGLMapView`. ([#8617](https://github.com/mapbox/mapbox-gl-native/pull/8617))
 * Selecting an annotation no longer sets the user tracking mode to `MGLUserTrackingModeNone`. ([#10094](https://github.com/mapbox/mapbox-gl-native/pull/10094))
+* Added `-[MGLMapView cameraThatFitsShape:direction:padding:]` to get a camera with zoom level and center coordinate computed to fit a shape. ([#10107](https://github.com/mapbox/mapbox-gl-native/pull/10107))


This should say edgePadding:, not padding:.

tobrun · 2017-10-09T11:58:30Z

This is working great @asheemmamoowala 👍

I might revisit the public api with overloaded methods (so bearing and padding are optional)
Let's ship this and iterate on it further.

…. On macOS, also added -[MGLMapView setCamera: withDuration: animationTimingFunction: edgePadding: completionHandler:] for parity with iOS

asheemmamoowala requested review from 1ec5 and jfirebaugh October 2, 2017 22:18

asheemmamoowala changed the title ~~9809 camera for geom~~ [core][macOS] Compute camera center and zoom for geometry with fixed heading Oct 2, 2017

jfirebaugh approved these changes Oct 2, 2017

View reviewed changes

1ec5 reviewed Oct 3, 2017

View reviewed changes

asheemmamoowala requested a review from tobrun October 4, 2017 22:07

1ec5 reviewed Oct 4, 2017

View reviewed changes

asheemmamoowala force-pushed the 9809-camera-for-geom branch from a7b7f9a to 71f6649 Compare October 5, 2017 00:26

lilykaiser added the Core The cross-platform C++ core, aka mbgl label Oct 5, 2017

1ec5 mentioned this pull request Oct 7, 2017

Reinvent the course tracking wheel mapbox/mapbox-navigation-ios#402

Merged

12 tasks

1ec5 approved these changes Oct 7, 2017

View reviewed changes

tobrun approved these changes Oct 9, 2017

View reviewed changes

Asheem Mamoowala added 2 commits October 9, 2017 11:04

[core] Add cameraForLatLngs() method with modified bearing

30dadf9

[android][ios][macOS] Implement bindings for Map::cameraForGeometry…

900faca

…. On macOS, also added -[MGLMapView setCamera: withDuration: animationTimingFunction: edgePadding: completionHandler:] for parity with iOS

asheemmamoowala force-pushed the 9809-camera-for-geom branch from 71f6649 to 900faca Compare October 9, 2017 18:08

asheemmamoowala merged commit 9e79659 into master Oct 9, 2017

asheemmamoowala deleted the 9809-camera-for-geom branch October 9, 2017 18:42

asheemmamoowala mentioned this pull request Oct 9, 2017

Cherry pick #10107: Map::cameraForGeometry() #10166

Merged

Guardiola31337 mentioned this pull request Oct 19, 2017

Release Android v5.2.0-beta.2 #10239

Closed

20 tasks

Guardiola31337 mentioned this pull request Oct 26, 2017

Release Android v5.2.0 beta.3 #10297

Closed

20 tasks

tobrun mentioned this pull request Nov 3, 2017

Release Android v5.2.0-beta.4 #10379

Closed

21 tasks

This was referenced Nov 14, 2017

Release Android v5.2.0-beta.5 #10463

Closed

Release Android v5.2.0 #10486

Closed

Conversation

asheemmamoowala commented Oct 2, 2017

Uh oh!

jfirebaugh left a comment

Choose a reason for hiding this comment

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobrun commented Oct 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

1ec5 Oct 4, 2017 •

edited

Loading