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

Add pitch argument to cameraThatFits functions #12213

Merged
merged 18 commits into from
Jul 2, 2018
Merged

Add pitch argument to cameraThatFits functions #12213

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
cf43870
Add pitch argument to cameraThatFits functions
Jun 25, 2018
073e497
check if nil
Jun 25, 2018
74a166a
Change around
Jun 26, 2018
6de4b0a
fix copy pasta
Jun 27, 2018
8e04e6b
Merge branch 'master' into cameraThatFitsWithPitch
Jun 27, 2018
d72d77b
Round
Jun 27, 2018
3c273bc
use rad
Jun 27, 2018
f5f7397
sp
Jun 27, 2018
3946841
retest
Jun 27, 2018
92dac01
Update
Jun 28, 2018
0dca9d8
fix
Jun 28, 2018
2dab59c
add camera fitting bounds, add macos
Jun 29, 2018
aa4dc6a
Merge branch 'master' into cameraThatFitsWithPitch
Jun 29, 2018
28d2cd7
Changelog
Jun 29, 2018
cd5828c
add link
Jul 2, 2018
71720d0
Dont allow for negative values
Jul 2, 2018
d862574
do everywhere
Jul 2, 2018
110c70b
Use current camera
Jul 2, 2018
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
6 changes: 3 additions & 3 deletions include/mbgl/map/map.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ class Map : private util::noncopyable {
void jumpTo(const CameraOptions&);
void easeTo(const CameraOptions&, const AnimationOptions&);
void flyTo(const CameraOptions&, const AnimationOptions&);
CameraOptions cameraForLatLngBounds(const LatLngBounds&, const EdgeInsets&, optional<double> bearing = {}) const;
CameraOptions cameraForLatLngs(const std::vector<LatLng>&, const EdgeInsets&, optional<double> bearing = {}) const;
CameraOptions cameraForGeometry(const Geometry<double>&, const EdgeInsets&, optional<double> bearing = {}) const;
CameraOptions cameraForLatLngBounds(const LatLngBounds&, const EdgeInsets&, optional<double> bearing = {}, optional<double> pitch = {}) const;
CameraOptions cameraForLatLngs(const std::vector<LatLng>&, const EdgeInsets&, optional<double> bearing = {}, optional<double> pitch = {}) const;
CameraOptions cameraForGeometry(const Geometry<double>&, const EdgeInsets&, optional<double> bearing = {}, optional<double> pitch = {}) const;
LatLngBounds latLngBoundsForCamera(const CameraOptions&) const;

// Position
Expand Down
33 changes: 33 additions & 0 deletions platform/ios/src/MGLMapView.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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


@param bounds The coordinate bounds to fit to the receiver’s viewport.
@param direction The direction of the viewport, measured in degrees clockwise from true north.
@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 insets The minimum padding (in screen points) that would be visible
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.
*/
- (MGLMapCamera *)cameraThatFitsCoordinateBounds:(MGLCoordinateBounds)bounds direction:(CLLocationDirection)direction pitch:(CGFloat)pitch edgePadding:(UIEdgeInsets)insets;

/**
Returns the camera that best fits the given shape, with the specified direction,
optionally with some additional padding on each side.
Expand All @@ -940,6 +957,22 @@ MGL_EXPORT IB_DESIGNABLE
*/
- (MGLMapCamera *)cameraThatFitsShape:(MGLShape *)shape direction:(CLLocationDirection)direction edgePadding:(UIEdgeInsets)insets;

/**
Returns the camera that best fits the given shape, with the specified direction and pitch,
optionally with some additional padding on each side.

@param shape The shape to fit to the receiver’s viewport.
@param direction The direction of the viewport, measured in degrees clockwise from true north.
@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 insets The minimum padding (in screen points) that would be visible
around the returned camera object if it were set as the receiver’s camera.
@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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


/**
Returns the point in this view’s coordinate system on which to "anchor" in
response to a user-initiated gesture.
Expand Down
20 changes: 18 additions & 2 deletions platform/ios/src/MGLMapView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3348,14 +3348,30 @@ - (MGLMapCamera *)cameraThatFitsCoordinateBounds:(MGLCoordinateBounds)bounds edg
return [self cameraForCameraOptions:cameraOptions];
}

- (MGLMapCamera *)cameraThatFitsShape:(MGLShape *)shape direction:(CLLocationDirection)direction edgePadding:(UIEdgeInsets)insets {
- (MGLMapCamera *)cameraThatFitsCoordinateBounds:(MGLCoordinateBounds)bounds direction:(CLLocationDirection)direction pitch:(CGFloat)pitch edgePadding:(UIEdgeInsets)insets
{
mbgl::EdgeInsets padding = MGLEdgeInsetsFromNSEdgeInsets(insets);
padding += MGLEdgeInsetsFromNSEdgeInsets(self.contentInset);
mbgl::CameraOptions cameraOptions = _mbglMap->cameraForLatLngBounds(MGLLatLngBoundsFromCoordinateBounds(bounds), padding, direction, pitch);
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return [self cameraForCameraOptions:cameraOptions];
}

- (MGLMapCamera *)cameraThatFitsShape:(MGLShape *)shape direction:(CLLocationDirection)direction edgePadding:(UIEdgeInsets)insets {
mbgl::EdgeInsets padding = MGLEdgeInsetsFromNSEdgeInsets(insets);
padding += MGLEdgeInsetsFromNSEdgeInsets(self.contentInset);

mbgl::CameraOptions cameraOptions = _mbglMap->cameraForGeometry([shape geometryObject], padding, direction);

return [self cameraForCameraOptions:cameraOptions];
}

- (MGLMapCamera *)cameraThatFitsShape:(MGLShape *)shape direction:(CLLocationDirection)direction pitch:(CGFloat)pitch edgePadding:(UIEdgeInsets)insets {
mbgl::EdgeInsets padding = MGLEdgeInsetsFromNSEdgeInsets(insets);
padding += MGLEdgeInsetsFromNSEdgeInsets(self.contentInset);

mbgl::CameraOptions cameraOptions = _mbglMap->cameraForGeometry([shape geometryObject], padding, direction, pitch);

return [self cameraForCameraOptions:cameraOptions];
}

- (MGLMapCamera *)cameraForCameraOptions:(const mbgl::CameraOptions &)cameraOptions
Expand Down
41 changes: 28 additions & 13 deletions src/mbgl/map/map.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,13 +364,13 @@ void Map::setLatLngZoom(const LatLng& latLng, double zoom, const EdgeInsets& pad
impl->onUpdate();
}

CameraOptions Map::cameraForLatLngBounds(const LatLngBounds& bounds, const EdgeInsets& padding, optional<double> bearing) const {
CameraOptions Map::cameraForLatLngBounds(const LatLngBounds& bounds, const EdgeInsets& padding, optional<double> bearing, optional<double> pitch) const {
return cameraForLatLngs({
bounds.northwest(),
bounds.southwest(),
bounds.southeast(),
bounds.northeast(),
}, padding, bearing);
}, padding, bearing, pitch);
}

CameraOptions cameraForLatLngs(const std::vector<LatLng>& latLngs, const Transform& transform, const EdgeInsets& padding) {
Expand Down Expand Up @@ -426,26 +426,41 @@ CameraOptions cameraForLatLngs(const std::vector<LatLng>& latLngs, const Transfo
return options;
}

CameraOptions Map::cameraForLatLngs(const std::vector<LatLng>& latLngs, const EdgeInsets& padding, optional<double> bearing) const {
if(bearing) {
double angle = -*bearing * util::DEG2RAD; // Convert to radians
Transform transform(impl->transform.getState());
transform.setAngle(angle);
CameraOptions options = mbgl::cameraForLatLngs(latLngs, transform, padding);
options.angle = angle;
return options;
} else {
CameraOptions Map::cameraForLatLngs(const std::vector<LatLng>& latLngs, const EdgeInsets& padding, optional<double> bearing, optional<double> pitch) const {

if (!bearing && !pitch) {
return mbgl::cameraForLatLngs(latLngs, impl->transform, padding);
}

Transform transform(impl->transform.getState());

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

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

if (bearing) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

options.angle = transform.getAngle();
}
if (pitch) {
options.angle = transform.getAngle();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should set pitch here.

}

return options;
}

CameraOptions Map::cameraForGeometry(const Geometry<double>& geometry, const EdgeInsets& padding, optional<double> bearing) const {
CameraOptions Map::cameraForGeometry(const Geometry<double>& geometry, const EdgeInsets& padding, optional<double> bearing, optional<double> pitch) const {

std::vector<LatLng> latLngs;
forEachPoint(geometry, [&](const Point<double>& pt) {
latLngs.push_back({ pt.y, pt.x });
});
return cameraForLatLngs(latLngs, padding, bearing);
return cameraForLatLngs(latLngs, padding, bearing, pitch);
}

LatLngBounds Map::latLngBoundsForCamera(const CameraOptions& camera) const {
Expand Down