[ios] Map view can now be optionally configured to make pitch adjustments w… #12518

d-prukop · 2018-07-31T18:40:01Z

Map view can now be optionally configured to make pitch adjustments without affecting altitude.

This addresses part of #9808

Pitch changes using MGLAltitudeForZoomLevel and MGLZoomLevelForAltitude from the MGLGeometry library were animating in a strange fashion when using the new proposed camera animation system (located here: https://github.com/mapbox/mapbox-ios-map-camera).

I added a flag to turn off this association between pitch and altitude.

I also added a function to directly update the camera with edge insets. Edge insets in this method are used to control the point on screen where the map rotates and where the center coordinate will be displayed.

Before | After
|

julianrex · 2018-07-31T20:00:51Z

@d-prukop is there any possible overlap with #12203?

d-prukop · 2018-07-31T20:42:35Z

@julianrex It does seem like it has some overlap with #12203. This approach is not specifically moving the center coord to screen point location. I'm using padding to get that effect. It shouldn't help or hinder any efforts to get a specific anchor point built. @1ec5 do you want to talk about this approach?

julianrex · 2018-07-31T20:03:13Z

platform/ios/src/MGLMapView.h

+
+ When this property is set to `YES`, pitch will work independently from altitude.
+ */
+@property(nonatomic, getter=isAltitudeUnaffectedByPitch) BOOL altitudeUnaffectedByPitch;


I think the opposite altitudeAffectedByPitch might be less confusing? Also, we should specify the default value.

I chose this naming for the purpose of defaulting to false. I'm definitely open to better naming. When you get altitude from the internal GL core zoom level, pitch is taken into account in the calculation. Same thing when you set zoom level from altitude. I want just the pure altitude and the pure pitch. Any suggestions?

julianrex · 2018-07-31T20:43:52Z

platform/ios/src/MGLMapView.h

+ @param padding The minimum padding (in screen points) that would be visible
+
+ */
+- (void)jumpToCamera:(MGLMapCamera *)camera edgePadding:(UIEdgeInsets)insets;


My understanding is that this essentially exposes core's jumpTo method. But it feels close to -flyToCamera:withDuration:completionHandler: with a 0 duration (which calls down to the internal method -_flyToCamera:edgePadding:withDuration:peakAltitude:completionHandler:.

I think this could be confusing. Thoughts?

Secondly, I think this method might need a completion handler.

I definitely need direct access to the edgePadding argument. I also don't want any animation since I'm controlling the animation outside of the iOS SDK. I looked into using one of the setCamera methods but I couldn't get it to work the way I wanted. Plus it appeared to drop frames vs this method. This could probably use a closer look.

Right know we have two direct flavors to set the camera the setCamera: XXX and flyToCamera, jumpToCamera sounds like falls into the first flavor, would it be possible to add setCamera:edgePadding: instead?

fabian-guerra

As we discussed could you please add a gif for how it looks when enabled and disabled.

fabian-guerra · 2018-07-31T21:23:40Z

platform/ios/src/MGLMapView.h

+
+ When this property is set to `YES`, pitch will work independently from altitude.
+ */
+@property(nonatomic, getter=isAltitudeUnaffectedByPitch) BOOL altitudeUnaffectedByPitch;


Is it OK if we specify that this property controls the camera updates and nothing more, something along the lines —feel free to add different wording—:

A Boolean value that determines wether updating the camera pitch will also affect the altitude.
When this property is set to YES, pitch will work independently from altitude.
The default value of this property is NO.

Also I think the documentation needs a way to clarify the scope of this change and how people may take advantage, I know you already have a repo with experimental changes, but communicating how devs may take advantage of this I think is important.

In the SDK we follow the convention of avoid using negatives as part of boolean properties. As @julianrex said isAltitudeAffectedByPitch follows that pattern. But also is it more clear the intent using something like isCameraAltitudeAffectedByPitch?

fabian-guerra · 2018-07-31T21:34:22Z

platform/ios/src/MGLMapView.h

+ @param padding The minimum padding (in screen points) that would be visible
+
+ */
+- (void)jumpToCamera:(MGLMapCamera *)camera edgePadding:(UIEdgeInsets)insets;


Right know we have two direct flavors to set the camera the setCamera: XXX and flyToCamera, jumpToCamera sounds like falls into the first flavor, would it be possible to add setCamera:edgePadding: instead?

fabian-guerra · 2018-07-31T23:01:34Z

platform/ios/src/MGLMapView.h

+ map camera without the built-in side effect that pitch influences altitude. This will make the
+ results of using outside animation more predictable and easier to control.
+ */
+@property(nonatomic, getter=isCameraAltitudeAffectedByPitch) BOOL cameraAltitudeAffectedByPitch;


nit: I think the second line is redundant, what do you think about:

A Boolean value that determines whether the updating pitch will also affect the altitude. Setting this property to `NO` will allow animation libraries outside of this SDK to control the map camera without the built-in side effect that pitch influences altitude. This will make the results of using outside animation more predictable and easier to control. The default value of this property is `YES`.

fabian-guerra · 2018-07-31T23:19:34Z

platform/ios/src/MGLMapView.h

+ @param edgePadding The minimum padding (in screen points) that would be visible
+
+ */
+- (void)setCamera:(MGLMapCamera *)camera edgePadding:(UIEdgeInsets)edgePadding;


Nit: In this case it may not be necessary to set a specific use case since this falls into the setCamera methods.

Moves the viewpoint to a different location without using a transition. @param camera The new viewpoint. @param edgePadding The minimum padding (in screen points) that would be visible.

fabian-guerra · 2018-07-31T23:38:22Z

platform/ios/src/MGLMapView.h

 map camera without the built-in side effect that pitch influences altitude. This will make the
 results of using outside animation more predictable and easier to control.
+
+ The default value of this property is YES.


We require all types be marked with ` to get rendered properly in our documentation.
Nit: YES

fabian-guerra

LGTM

julianrex · 2018-08-01T00:05:18Z

Thanks @d-prukop!

friedbunny · 2018-08-01T16:40:07Z

I’m excited about improvements to the camera system — these animations look quite nice. I have a couple questions about how we should implement these particular changes:

Should `MGLMapView.cameraAltitudeAffectedByPitch` be public API?

Or would it be acceptable to stash it in MGLMapView_Private.h and forward-declare in client implementations? As we prototype a new camera system, it feels like we’d be best served by not broadening/muddying the public API surface until we have a solid vision of what we want to support going forward.

Should pitch affect altitude?

Would it make sense to change the default behavior instead?

d-prukop · 2018-08-01T16:46:25Z

I'm OK with making this private since it will probably only be used by a new camera system. We should talk about the future of the camera!

I think the default behavior was put in there to mimic the way mapkit works. Whether we want that is a larger discussion.

1ec5

I think it would be beneficial to consider alternatives to this approach that still address the issue you’ve brought up. In addition to the feedback below, please note that all changes to the iOS public API require corresponding blurbs in the iOS changelog, and that we try to keep the iOS and macOS implementations of MGLMapView in sync as much as possible.

1ec5 · 2018-08-01T17:22:03Z

platform/ios/src/MGLMapView.mm

@@ -3374,7 +3382,8 @@ - (MGLMapCamera *)cameraForCameraOptions:(const mbgl::CameraOptions &)cameraOpti
 double zoomLevel = cameraOptions.zoom ? *cameraOptions.zoom : self.zoomLevel;
 CLLocationDirection direction = cameraOptions.angle ? mbgl::util::wrap(-MGLDegreesFromRadians(*cameraOptions.angle), 0., 360.) : self.direction;
 CGFloat pitch = cameraOptions.pitch ? MGLDegreesFromRadians(*cameraOptions.pitch) : _mbglMap->getPitch();
- CLLocationDistance altitude = MGLAltitudeForZoomLevel(zoomLevel, pitch, centerCoordinate.latitude, self.frame.size);
+ CLLocationDegrees pitchForAltitude = self.isCameraAltitudeAffectedByPitch ? pitch : 0.0;
+ CLLocationDistance altitude = MGLAltitudeForZoomLevel(zoomLevel, pitchForAltitude, centerCoordinate.latitude, self.frame.size);


Pitch is by definition affected by zoom level: when the map is tilted and the zoom level changes (whether programmatically or via a gesture), the viewing distance from the focal point and the viewpoint’s distance from the ground (the altitude) both change.

These changes affect the camera getters only. So the new property is a strange way of telling the getter to effectively lie about the camera relative to what mbgl thinks the camera is. Is there no way to derive the correct value from the existing return value, perhaps by exposing MGLAltitudeForZoomLevel publicly, as proposed in #5583?

I don't think mbgl comes in to this at all. As far as I can tell this is all happening in MGLGeometry in the iOS SDK. The pure values in mbgl are what I want.

The pure values in mbgl are what I want.

This is wrong.
The problem with exposing MGLAltitudeForZoomLevel is that, if you use setCamera, mbgl sets it's state from the result of MGLZoomLevelForAltitude. The stored zoomLevel is affected by pitch. I want to prevent the mbgl state from getting that value. I want it to store the zoomLevel without regards to pitch.

If you want to change the zoom level without changing the pitch, set a camera whose pitch is -1. This behavior is documented in MGLMapCamera but it could stand to be documented more discoverably. (If this SDK were written in Swift without regard for Obective-C compatibility, the various properties of MGLMapView would all be optional.)

@d-prukop and I chatted about this today. The pitch is considered twice when calculating the camera options, once for setting the camera’s actual pitch and once in calculating the zoom level. I would’ve thought that this is ideal, because the zoom level on a tilted map is really the viewing distance, but I suppose if the intention is to iteratively change the viewing distance by getting and setting the camera iteratively, then there could be a sort of feedback effect.

The essence of my concern here is that we’re adding a separate code path for a specific use case. If we really believe that the altitude-to-zoom-level conversion shouldn’t be affected by the pitch, then why not remove the pitch argument from MGLZoomLevelForAltitude() and MGLAltitudeForZoomLevel() and always assume a pitch of 0°? The obvious consequence would be that the MGLMapCamera returned by the camera getter would have the same altitude regardless of the pitch.

In other words, isCameraAltitudeAffectedByPitch changes the meaning of altitude from “shortest distance between the viewpoint and the ground” to “viewing distance from the focal point”, essentially a linear, physical-unit version of zoomLevel (and not what the word “altitude” means in real life). Maybe that’s what the use case above calls for: a property for explicitly setting the viewing distance of a camera instead of the altitude. We already have a zoomLevel property in MGLMapSnapshotOptions, so why not add an alternative to altitude to MGLMapCamera? One reason why is that it’s awkward for an object to sport mutually exclusive properties. Any future attempt at making the isCameraAltitudeAffectedByPitch option public or defaulting it to YES will need to consider whether we instead need a viewingDistance property on MGLMapCamera.

1ec5 · 2018-08-01T17:26:53Z

platform/ios/src/MGLMapView.h

+
+ The default value of this property is `YES`.
+ */
+@property(nonatomic, getter=isCameraAltitudeAffectedByPitch) BOOL cameraAltitudeAffectedByPitch;


MGLMapView’s public API is expressed in terms of zoom levels, not in terms of altitude. Generally speaking, MGLMapView’s direct parameters are tied to 2D concepts for simplicity, while MGLMapCamera is better suited for expressing 3D concepts because it can account for two different frames of reference (viewpoint versus focal point). Mixing the two concepts would promote one frame of reference over the other, which would call into question the very purpose of having MGLMapCamera as a separate class.

As far as MGLMapView is concerned, pitch is a detail of MGLMapCamera. You’ll note that the rest of MGLMapView avoids referring to pitch, because normally the pitch is intertwined with other camera parameters and the camera’s frame of reference.

Right now if I want to change pitch I have to use MGLMapCamera. When the MGLMapCamera is passed to cameraOptionsObjectForAnimatingToCamera, mbgl updates it's state with the zoomLevel calculated from MGLZoomLevelForAltitude.

All of the setCamera functions use cameraOptionsObjectForAnimatingToCamera.

My point is that MGLMapView’s API shouldn’t even mention pitch. If mentions pitch, it should provide the full complement of ways to interact with the pitch, which it doesn’t by design.

Think of camera as a set of parameters that get updated together. They have to be updated together in order to maintain consistency. If you want to update the pitch without updating other parameters, you set the other parameters to special values. For example, set heading to -1 or centerCoordinate to an invalid coordinate.

julianrex · 2018-08-01T17:43:26Z

Perhaps moving the property to MGLCamera is a solution here. The property name already hints that it might be in the wrong place. You'd need some additional scaffolding to pass it down appropriately though.

d-prukop · 2018-08-01T18:55:24Z

I'm not sure I can move it to MGLCamera. MGLMapView makes MGLCameras from mgbl::CameraOptions and I need to tell it how to handle those in the case where I want pitch and altitude separated.

…ithout affecting altitude

1ec5

It might be worth rewording the PR description so that the public change gets top billing and the unrelated private change (which ideally would go in a separate PR) is mentioned in passing, rather than the other way around.

1ec5 · 2018-08-03T01:22:43Z

platform/ios/src/MGLMapView.h

+ @param edgePadding The minimum padding (in screen points) that would be visible
+
+ */
+- (void)setCamera:(MGLMapCamera *)camera edgePadding:(UIEdgeInsets)edgePadding;


This method should accept an animated parameter. Just as there’s a -setVisibleCoordinateBounds:animated: and -setVisibleCoordinateBounds:edgePadding:animated:, there should be -setCamera:animated: and -setCamera:edgePadding:animated:. It seems unfair to force the developer to choose between specifying an edge padding and using the built-in animation capabilities, even if your particular use case doesn’t require those capabilities.

1ec5 · 2018-08-03T01:27:49Z

platform/ios/src/MGLMapView.mm

@@ -240,6 +240,9 @@ @interface MGLMapView () <UIGestureRecognizerDelegate,
 @property (nonatomic) MGLUserLocation *userLocation;
 @property (nonatomic) NSMutableDictionary<NSString *, NSMutableArray<MGLAnnotationView *> *> *annotationViewReuseQueueByIdentifier;

+/// A Boolean value that determines whether the updating pitch will also affect the altitude.
+@property(nonatomic, getter=isCameraAltitudeAffectedByPitch) BOOL cameraAltitudeAffectedByPitch;


A more natural name for the getter would be cameraAltitudeIsAffectedByPitch or, given #12518 (comment), cameraAltitudeIsViewingDistance.

…missing.

This is meant to (1) Make it easier for new developers to find the source (2) Make it easier to look at shader diffs when the GL JS pin changes

…ithout affecting altitude

…ative into new-camera-animations

…ithout affecting altitude

… updated" This reverts commit 39b4f62.

…ative into new-camera-animations

d-prukop · 2018-09-20T23:45:48Z

Abandoning this PR. Going to start a new one

d-prukop requested review from frederoni, bsudekum, friedbunny, 1ec5, fabian-guerra and julianrex July 31, 2018 18:40

julianrex reviewed Jul 31, 2018

View reviewed changes

fabian-guerra reviewed Jul 31, 2018

View reviewed changes

friedbunny assigned d-prukop Jul 31, 2018

friedbunny added feature iOS Mapbox Maps SDK for iOS labels Jul 31, 2018

fabian-guerra reviewed Jul 31, 2018

View reviewed changes

fabian-guerra approved these changes Jul 31, 2018

View reviewed changes

1ec5 reviewed Aug 1, 2018

View reviewed changes

d-prukop force-pushed the new-camera-animations branch from 3656a1f to c113a23 Compare August 2, 2018 20:31

d-prukop added 3 commits August 2, 2018 14:53

Map view can now be optionally configured to make pitch adjustments w…

54a7609

…ithout affecting altitude

updated to address comments

9b26024

Moved cameraAltitudeAffectedByPitch to private API. Changelog updated

0b1e7c2

d-prukop force-pushed the new-camera-animations branch from c113a23 to 0b1e7c2 Compare August 2, 2018 22:18

Update MGLMapView.h

08d8abd

1ec5 reviewed Aug 3, 2018

View reviewed changes

[android] temporarily disable CustomGeometrySource tests

3c8acb2

jfirebaugh and others added 25 commits September 19, 2018 14:54

[core] Auto-convert concat arguments to strings

89b075f

[core] For string-valued properties, do coercion rather than assertion

2e93a9d

[test] Bump gl-js pin.

ab8b6b7

[ios, macos] Fix add file content test, create Documents folder when …

9b09ec1

…missing.

[docs] Include uncompressed shader source in .cpp comments

f7a69d1

This is meant to (1) Make it easier for new developers to find the source (2) Make it easier to look at shader diffs when the GL JS pin changes

[docs] First uncompressed shader source diff

bee9a54

[ios] Update changelogs and bump podspec to 4.5.0-alpha.1

41427ff

[android] update 6.6.0-alpha.1 changelog to reflect ProGuard changes

2f7a147

Map view can now be optionally configured to make pitch adjustments w…

fc28f39

…ithout affecting altitude

updated to address comments

15c76d0

Moved cameraAltitudeAffectedByPitch to private API. Changelog updated

071b14a

Update MGLMapView.h

c1b1e29

removed setCamera: edgePadding: method

38d01ad

Merge branch 'new-camera-animations' of github.com:mapbox/mapbox-gl-n…

9d984d2

…ative into new-camera-animations

Map view can now be optionally configured to make pitch adjustments w…

b9992f5

…ithout affecting altitude

updated to address comments

55cd73d

Moved cameraAltitudeAffectedByPitch to private API. Changelog updated

82077ce

Update MGLMapView.h

ce2d2aa

Map view can now be optionally configured to make pitch adjustments w…

4ea4a94

…ithout affecting altitude

updated to address comments

adab22a

Moved cameraAltitudeAffectedByPitch to private API. Changelog updated

39b4f62

removed setCamera: edgePadding: method

666ec34

Revert "Moved cameraAltitudeAffectedByPitch to private API. Changelog…

959126f

… updated" This reverts commit 39b4f62.

Merge branch 'new-camera-animations' of github.com:mapbox/mapbox-gl-n…

a1c898f

…ative into new-camera-animations

removed setCamera: edgePadding: method

13000b3

d-prukop closed this Sep 20, 2018

d-prukop deleted the new-camera-animations branch September 20, 2018 23:46

d-prukop mentioned this pull request Sep 21, 2018

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

Closed

1ec5 mentioned this pull request Sep 25, 2018

Fix MGLMapCamera initialization and viewing distance calculations #12966

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios] Map view can now be optionally configured to make pitch adjustments w… #12518

[ios] Map view can now be optionally configured to make pitch adjustments w… #12518

d-prukop commented Jul 31, 2018 •

edited

Loading

julianrex commented Jul 31, 2018

d-prukop commented Jul 31, 2018 •

edited

Loading

julianrex Jul 31, 2018

d-prukop Jul 31, 2018

julianrex Jul 31, 2018

d-prukop Jul 31, 2018

fabian-guerra Jul 31, 2018

fabian-guerra left a comment

fabian-guerra Jul 31, 2018

fabian-guerra Jul 31, 2018

fabian-guerra Jul 31, 2018

fabian-guerra Jul 31, 2018

fabian-guerra Jul 31, 2018

fabian-guerra left a comment

julianrex commented Aug 1, 2018

friedbunny commented Aug 1, 2018 •

edited

Loading

d-prukop commented Aug 1, 2018

1ec5 left a comment

1ec5 Aug 1, 2018

d-prukop Aug 1, 2018

d-prukop Aug 1, 2018 •

edited

Loading

1ec5 Aug 1, 2018 •

edited

Loading

1ec5 Aug 3, 2018 •

edited

Loading

1ec5 Aug 1, 2018

d-prukop Aug 1, 2018

1ec5 Aug 1, 2018

julianrex commented Aug 1, 2018

d-prukop commented Aug 1, 2018

1ec5 left a comment

1ec5 Aug 3, 2018

1ec5 Aug 3, 2018

d-prukop commented Sep 20, 2018

[ios] Map view can now be optionally configured to make pitch adjustments w… #12518

[ios] Map view can now be optionally configured to make pitch adjustments w… #12518

Conversation

d-prukop commented Jul 31, 2018 • edited Loading

julianrex commented Jul 31, 2018

d-prukop commented Jul 31, 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

fabian-guerra 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

Choose a reason for hiding this comment

fabian-guerra left a comment

Choose a reason for hiding this comment

julianrex commented Aug 1, 2018

friedbunny commented Aug 1, 2018 • edited Loading

Should MGLMapView.cameraAltitudeAffectedByPitch be public API?

Should pitch affect altitude?

d-prukop commented Aug 1, 2018

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-prukop Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

1ec5 Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

1ec5 Aug 3, 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

julianrex commented Aug 1, 2018

d-prukop commented Aug 1, 2018

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-prukop commented Sep 20, 2018

d-prukop commented Jul 31, 2018 •

edited

Loading

d-prukop commented Jul 31, 2018 •

edited

Loading

friedbunny commented Aug 1, 2018 •

edited

Loading

Should `MGLMapView.cameraAltitudeAffectedByPitch` be public API?

d-prukop Aug 1, 2018 •

edited

Loading

1ec5 Aug 1, 2018 •

edited

Loading

1ec5 Aug 3, 2018 •

edited

Loading