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

Fix MGLMapCamera initialization and viewing distance calculations #12966

Merged
merged 3 commits into from
Oct 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 61 additions & 3 deletions platform/darwin/src/MGLMapCamera.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,27 @@ MGL_EXPORT
*/
@property (nonatomic) CGFloat pitch;

/** Meters above ground level. */
/**
The altitude (measured in meters) above the map at which the camera is
situated.

The altitude is the distance from the viewpoint to the map, perpendicular to
the map plane. This property does not account for physical elevation.

This property’s value may be less than that of the `viewingDistance` property.
Setting this property automatically updates the `viewingDistance` property
based on the `pitch` property’s current value.
*/
@property (nonatomic) CLLocationDistance altitude;

/**
The straight-line distance from the viewpoint to the `centerCoordinate`.

Setting this property automatically updates the `altitude` property based on
the `pitch` property’s current value.
*/
@property (nonatomic) CLLocationDistance viewingDistance;

/** Returns a new camera with all properties set to 0. */
+ (instancetype)camera;

Expand All @@ -49,7 +67,11 @@ MGL_EXPORT

/**
Returns a new camera with the given distance, pitch, and heading.


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.

@param centerCoordinate The geographic coordinate on which the map should be
centered.
@param distance The straight-line distance from the viewpoint to the
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@1ec5 1ec5 Oct 1, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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.

pitch:(CGFloat)pitch
heading:(CLLocationDirection)heading;

/**
Returns a new camera with the given altitude, pitch, and heading.

@param centerCoordinate The geographic coordinate on which the map should be
centered.
@param altitude The altitude (measured in meters) above the map at which the
camera should be situated. The altitude may be less than the distance from
the camera’s viewpoint to the camera’s focus point.
@param pitch The viewing angle of the camera, measured in degrees. A value of
`0` results in a camera pointed straight down at the map. Angles greater
than `0` result in a camera angled toward the horizon.
@param heading The camera’s heading, measured in degrees clockwise from true
north. A value of `0` means that the top edge of the map view corresponds to
true north. The value `90` means the top of the map is pointing due east.
The value `180` means the top of the map points due south, and so on.
*/
+ (instancetype)cameraLookingAtCenterCoordinate:(CLLocationCoordinate2D)centerCoordinate
altitude:(CLLocationDistance)altitude
pitch:(CGFloat)pitch
heading:(CLLocationDirection)heading;

/**
@note This initializer incorrectly interprets the `distance` parameter. To
specify the straight-line distance from the viewpoint to `centerCoordinate`,
use the `-cameraLookingAtCenterCoordinate:acrossDistance:pitch:heading:`
method. To specify the altitude of the viewpoint, use the
`-cameraLookingAtCenterCoordinate:altitude:pitch:heading:` method, which has
the same behavior as this initializer.
*/
+ (instancetype)cameraLookingAtCenterCoordinate:(CLLocationCoordinate2D)centerCoordinate
fromDistance:(CLLocationDistance)distance
pitch:(CGFloat)pitch
heading:(CLLocationDirection)heading
__attribute__((deprecated("Use -cameraLookingAtCenterCoordinate:acrossDistance:pitch:heading: "
"or -cameraLookingAtCenterCoordinate:altitude:pitch:heading:.")));

/**
Returns a Boolean value indicating whether the given camera is functionally
equivalent to the receiver.
Expand Down
69 changes: 56 additions & 13 deletions platform/darwin/src/MGLMapCamera.mm
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
#import "MGLMapCamera.h"
#import "MGLGeometry_Private.h"

#include <mbgl/util/projection.hpp>
#import <CoreLocation/CoreLocation.h>

#include <mbgl/math/wrap.hpp>

BOOL MGLEqualFloatWithAccuracy(CGFloat left, CGFloat right, CGFloat accuracy)
{
Expand All @@ -27,17 +29,15 @@ + (instancetype)cameraLookingAtCenterCoordinate:(CLLocationCoordinate2D)centerCo
CLLocationDirection heading = -1;
CGFloat pitch = -1;
if (CLLocationCoordinate2DIsValid(centerCoordinate) && CLLocationCoordinate2DIsValid(eyeCoordinate)) {
mbgl::LatLng centerLatLng = MGLLatLngFromLocationCoordinate2D(centerCoordinate);
mbgl::LatLng eyeLatLng = MGLLatLngFromLocationCoordinate2D(eyeCoordinate);

mbgl::ProjectedMeters centerMeters = mbgl::Projection::projectedMetersForLatLng(centerLatLng);
mbgl::ProjectedMeters eyeMeters = mbgl::Projection::projectedMetersForLatLng(eyeLatLng);
heading = std::atan((centerMeters.northing() - eyeMeters.northing()) /
(centerMeters.easting() - eyeMeters.easting()));
heading = MGLDirectionBetweenCoordinates(eyeCoordinate, centerCoordinate);

double groundDistance = std::hypot(centerMeters.northing() - eyeMeters.northing(),
centerMeters.easting() - eyeMeters.easting());
pitch = std::atan(eyeAltitude / groundDistance);
CLLocation *centerLocation = [[CLLocation alloc] initWithLatitude:centerCoordinate.latitude
longitude:centerCoordinate.longitude];
CLLocation *eyeLocation = [[CLLocation alloc] initWithLatitude:eyeCoordinate.latitude
longitude:eyeCoordinate.longitude];
CLLocationDistance groundDistance = [eyeLocation distanceFromLocation:centerLocation];
CGFloat radianPitch = atan2(eyeAltitude, groundDistance);
pitch = mbgl::util::wrap(90 - MGLDegreesFromRadians(radianPitch), 0.0, 360.0);
}

return [[self alloc] initWithCenterCoordinate:centerCoordinate
Expand All @@ -47,16 +47,45 @@ + (instancetype)cameraLookingAtCenterCoordinate:(CLLocationCoordinate2D)centerCo
}

+ (instancetype)cameraLookingAtCenterCoordinate:(CLLocationCoordinate2D)centerCoordinate
fromDistance:(CLLocationDistance)distance
acrossDistance:(CLLocationDistance)distance
pitch:(CGFloat)pitch
heading:(CLLocationDirection)heading
{
// Angle at the viewpoint formed by the straight lines running perpendicular
// to the ground and toward the center coordinate.
CLLocationDirection eyeAngle = 90 - pitch;
CLLocationDistance altitude = distance * sin(MGLRadiansFromDegrees(eyeAngle));

return [[self alloc] initWithCenterCoordinate:centerCoordinate
altitude:altitude
pitch:pitch
heading:heading];
}

+ (instancetype)cameraLookingAtCenterCoordinate:(CLLocationCoordinate2D)centerCoordinate
altitude:(CLLocationDistance)altitude
pitch:(CGFloat)pitch
heading:(CLLocationDirection)heading
{
return [[self alloc] initWithCenterCoordinate:centerCoordinate
altitude:distance
altitude:altitude
pitch:pitch
heading:heading];
}

+ (instancetype)cameraLookingAtCenterCoordinate:(CLLocationCoordinate2D)centerCoordinate
fromDistance:(CLLocationDistance)distance
pitch:(CGFloat)pitch
heading:(CLLocationDirection)heading
{
// TODO: Remove this compatibility shim in favor of `-cameraLookingAtCenterCoordinate:acrossDistance:pitch:heading:.
// https://github.com/mapbox/mapbox-gl-native/issues/12943
return [MGLMapCamera cameraLookingAtCenterCoordinate:centerCoordinate
altitude:distance
pitch:pitch
heading:heading];
}

- (instancetype)initWithCenterCoordinate:(CLLocationCoordinate2D)centerCoordinate
altitude:(CLLocationDistance)altitude
pitch:(CGFloat)pitch
Expand Down Expand Up @@ -102,6 +131,20 @@ - (id)copyWithZone:(nullable NSZone *)zone
heading:_heading];
}

+ (NSSet<NSString *> *)keyPathsForValuesAffectingViewingDistance {
return [NSSet setWithObjects:@"altitude", @"pitch", nil];
}

- (CLLocationDistance)viewingDistance {
CLLocationDirection eyeAngle = 90 - self.pitch;
return self.altitude / sin(MGLRadiansFromDegrees(eyeAngle));
}

- (void)setViewingDistance:(CLLocationDistance)distance {
CLLocationDirection eyeAngle = 90 - self.pitch;
self.altitude = distance * sin(MGLRadiansFromDegrees(eyeAngle));
}

1ec5 marked this conversation as resolved.
Show resolved Hide resolved
- (NSString *)description
{
return [NSString stringWithFormat:@"<%@: %p; centerCoordinate = %f, %f; altitude = %.0fm; heading = %.0f°; pitch = %.0f°>",
Expand Down
2 changes: 1 addition & 1 deletion platform/darwin/src/MGLMapSnapshotter.h
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ typedef void (^MGLMapSnapshotCompletionHandler)(MGLMapSnapshot* _Nullable snapsh
### Example

```swift
let camera = MGLMapCamera(lookingAtCenter: CLLocationCoordinate2D(latitude: 37.7184, longitude: -122.4365), fromDistance: 100, pitch: 20, heading: 0)
let camera = MGLMapCamera(lookingAtCenter: CLLocationCoordinate2D(latitude: 37.7184, longitude: -122.4365), altitude: 100, pitch: 20, heading: 0)

let options = MGLMapSnapshotOptions(styleURL: MGLStyle.satelliteStreetsStyleURL, camera: camera, size: CGSize(width: 320, height: 480))
options.zoomLevel = 10
Expand Down
2 changes: 1 addition & 1 deletion platform/darwin/test/MGLDocumentationExampleTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ class MGLDocumentationExampleTests: XCTestCase, MGLMapViewDelegate {
}

//#-example-code
let camera = MGLMapCamera(lookingAtCenter: CLLocationCoordinate2D(latitude: 37.7184, longitude: -122.4365), fromDistance: 100, pitch: 20, heading: 0)
let camera = MGLMapCamera(lookingAtCenter: CLLocationCoordinate2D(latitude: 37.7184, longitude: -122.4365), altitude: 100, pitch: 20, heading: 0)

let options = MGLMapSnapshotOptions(styleURL: MGLStyle.satelliteStreetsStyleURL, camera: camera, size: CGSize(width: 320, height: 480))
options.zoomLevel = 10
Expand Down
106 changes: 106 additions & 0 deletions platform/darwin/test/MGLMapCameraTests.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
#import <XCTest/XCTest.h>
#import <CoreLocation/CoreLocation.h>
#import <Mapbox/Mapbox.h>
#import <MapKit/MapKit.h>

@interface MGLMapCameraTests : XCTestCase

@end

@implementation MGLMapCameraTests

- (void)testEyeCoordinateInitialization {
CLLocationCoordinate2D fountainSquare = CLLocationCoordinate2DMake(39.10152215, -84.5124439696089);
CLLocationCoordinate2D unionTerminal = CLLocationCoordinate2DMake(39.10980955, -84.5352778794236);

MGLMapCamera *camera = [MGLMapCamera cameraLookingAtCenterCoordinate:fountainSquare
fromEyeCoordinate:fountainSquare
eyeAltitude:1000];
MKMapCamera *mkCamera = [MKMapCamera cameraLookingAtCenterCoordinate:fountainSquare
fromEyeCoordinate:fountainSquare
eyeAltitude:1000];
XCTAssertEqual(camera.centerCoordinate.latitude, fountainSquare.latitude);
XCTAssertEqual(camera.centerCoordinate.longitude, fountainSquare.longitude);
XCTAssertEqual(camera.centerCoordinate.latitude, mkCamera.centerCoordinate.latitude);
XCTAssertEqual(camera.centerCoordinate.longitude, mkCamera.centerCoordinate.longitude);
XCTAssertEqual(camera.altitude, 1000, @"Eye altitude should be equivalent to altitude in untilted camera.");
XCTAssertEqual(camera.altitude, mkCamera.altitude, @"Eye altitude in untilted camera should match MapKit.");
XCTAssertEqual(camera.pitch, 0, @"Camera directly over center coordinate should be untilted.");
XCTAssertEqual(camera.pitch, mkCamera.pitch, @"Camera directly over center coordinate should have same pitch as MapKit.");
XCTAssertEqual(camera.heading, 0, @"Camera directly over center coordinate should be unrotated.");
XCTAssertEqual(camera.heading, mkCamera.heading, @"Camera directly over center coordinate should have same heading as MapKit.");

camera = [MGLMapCamera cameraLookingAtCenterCoordinate:fountainSquare
fromEyeCoordinate:unionTerminal
eyeAltitude:1000];
mkCamera = [MKMapCamera cameraLookingAtCenterCoordinate:fountainSquare
fromEyeCoordinate:unionTerminal
eyeAltitude:1000];
XCTAssertEqual(camera.centerCoordinate.latitude, fountainSquare.latitude);
XCTAssertEqual(camera.centerCoordinate.longitude, fountainSquare.longitude);
XCTAssertEqual(camera.centerCoordinate.latitude, mkCamera.centerCoordinate.latitude);
XCTAssertEqual(camera.centerCoordinate.longitude, mkCamera.centerCoordinate.longitude);
XCTAssertEqual(camera.altitude, 1000);
XCTAssertEqual(camera.altitude, mkCamera.altitude, @"Eye altitude in tilted camera should match MapKit.");
XCTAssertEqualWithAccuracy(camera.pitch, 65.3469146074, 0.01);
XCTAssertEqual(camera.pitch, mkCamera.pitch);
XCTAssertEqualWithAccuracy(camera.heading, 115.066396383, 0.01);
XCTAssertEqualWithAccuracy(camera.heading, mkCamera.heading, 0.01);
}

- (void)testViewingDistanceInitialization {
CLLocationCoordinate2D fountainSquare = CLLocationCoordinate2DMake(39.10152215, -84.5124439696089);
MGLMapCamera *camera = [MGLMapCamera cameraLookingAtCenterCoordinate:fountainSquare
acrossDistance:10000
pitch:0
heading:0];
MKMapCamera *mkCamera = [MKMapCamera cameraLookingAtCenterCoordinate:fountainSquare
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

fromDistance:10000
pitch:0
heading:0];
XCTAssertEqualWithAccuracy(camera.altitude, 10000, 0.01, @"Untilted camera should use distance verbatim.");
XCTAssertEqualWithAccuracy(camera.altitude, mkCamera.altitude, 0.01, @"Untilted camera altitude should match MapKit.");

camera = [MGLMapCamera cameraLookingAtCenterCoordinate:fountainSquare
altitude:10000
pitch:0
heading:0];
XCTAssertEqual(camera.altitude, 10000, @"Untilted camera should use altitude verbatim.");

camera = [MGLMapCamera cameraLookingAtCenterCoordinate:fountainSquare
acrossDistance:10000
pitch:60
heading:0];
mkCamera = [MKMapCamera cameraLookingAtCenterCoordinate:fountainSquare
fromDistance:10000
pitch:60
heading:0];
XCTAssertEqualWithAccuracy(camera.altitude, 5000, 0.01, @"Tilted camera altitude should account for pitch.");
XCTAssertEqualWithAccuracy(camera.altitude, mkCamera.altitude, 0.01, @"Tilted camera altitude should match MapKit.");

camera = [MGLMapCamera cameraLookingAtCenterCoordinate:fountainSquare
altitude:10000
pitch:60
heading:0];
XCTAssertEqual(camera.altitude, 10000, @"Tilted camera should use altitude verbatim.");
}

- (void)testViewingDistance {
MGLMapCamera *camera = [MGLMapCamera camera];
camera.altitude = 10000;
XCTAssertEqual(camera.altitude, 10000);
XCTAssertEqual(camera.viewingDistance, 10000);
camera.viewingDistance = 10000;
XCTAssertEqual(camera.altitude, 10000);
XCTAssertEqual(camera.viewingDistance, 10000);

camera.pitch = 60;
camera.altitude = 10000;
XCTAssertEqual(camera.altitude, 10000);
XCTAssertEqualWithAccuracy(camera.viewingDistance, 20000, 0.01);
camera.viewingDistance = 10000;
XCTAssertEqualWithAccuracy(camera.altitude, 5000, 0.01);
XCTAssertEqual(camera.viewingDistance, 10000);
}

@end
3 changes: 3 additions & 0 deletions platform/ios/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT
### Other changes

* Added `MGLAltitudeForZoomLevel` and `MGLZoomLevelForAltitude` to public API for conversion between zoom levels and altitudes. ([#12986](https://github.com/mapbox/mapbox-gl-native/pull/12986))
* Deprecated the `+[MGLMapCamera cameraLookingAtCenterCoordinate:fromDistance:pitch:heading:]` method in favor of `+[MGLMapCamera cameraLookingAtCenterCoordinate:altitude:pitch:heading:]` and `+[MGLMapCamera cameraLookingAtCenterCoordinate:acrossDistance:pitch:heading:]`. ([#12966](https://github.com/mapbox/mapbox-gl-native/pull/12966))
* Fixed an issue where `+[MGLMapCamera cameraLookingAtCenterCoordinate:fromEyeCoordinate:eyeAltitude:]` created a camera looking from the wrong eye coordinate. ([#12966](https://github.com/mapbox/mapbox-gl-native/pull/12966))
* Added an `MGLMapCamera.viewingDistance` property based on the existing `MGLMapCamera.altitude` property. ([#12966](https://github.com/mapbox/mapbox-gl-native/pull/12966))
* Fixed an issue where `-[MGLMapSnapshotter startWithQueue:completionHandler:]` failed to call its completion handler in some cases. ([#12355](https://github.com/mapbox/mapbox-gl-native/pull/12355))
* Fixed bugs in coercion expression operators ("to-array" applied to empty arrays, "to-color" applied to colors, and "to-number" applied to null) [#12864](https://github.com/mapbox/mapbox-gl-native/pull/12864)
* Added the `MGLCollisionBehaviorPre4_0` Info.plist key for applications that require the collision detection behavior in version v3.7 of the SDK. ([#12941](https://github.com/mapbox/mapbox-gl-native/pull/12941))
Expand Down
Loading