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

[ios] Fix toCamera.centerCoordinate in shouldChangeFromCamera #10433

Merged
merged 4 commits into from Nov 14, 2017

Conversation

fabian-guerra
Copy link
Contributor

Fixes #10391

@fabian-guerra fabian-guerra self-assigned this Nov 9, 2017
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

This change only works when the zoom gesture is centered over the map view. It doesn’t work when attempting to zoom with an uncentered anchor.

The good news is that we no longer need to implement a change-and-revert dance or write manual conversion math here, because mbgl::Map::latLngBoundsForCamera() can give us the answer more directly as of #8510.

@@ -1827,6 +1827,10 @@ - (MGLMapCamera *)cameraByZoomingToZoomLevel:(double)zoom aroundAnchorPoint:(CG
currentCameraOptions.anchor = anchor;
camera = [self cameraForCameraOptions:currentCameraOptions];

CLLocationCoordinate2D centerCoordinate = [self convertPoint:anchorPoint toCoordinateFromView:view];

camera.centerCoordinate = centerCoordinate;
Copy link
Contributor

Choose a reason for hiding this comment

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

This change conflates the anchor point of a zoom with the map view’s center coordinate: #10391 (comment).

@1ec5 1ec5 added bug iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS labels Nov 10, 2017
@1ec5
Copy link
Contributor

1ec5 commented Nov 10, 2017

The good news is that we no longer need to implement a change-and-revert dance or write manual conversion math here, because mbgl::Map::latLngBoundsForCamera() can give us the answer more directly as of #8510.

To expand on this, the anchor when zooming apparently affects the CameraOptionspadding:

if (anchor) {
camera.padding = EdgeInsets(anchor->y, anchor->x, state.size.height - anchor->y, state.size.width - anchor->x);
}

So if we set a similar padding on the CameraOptions we pass into mbgl::Map::latLngBoundsForCamera(), then feed the resulting LatLngBounds into -[MGLMapView cameraThatFitsCoordinateBounds:edgePadding:], that should get us the camera we’re looking for. It won’t necessarily be accurate on a tilted map, but I don’t think our existing conversions are accurate in that case, either.

@1ec5
Copy link
Contributor

1ec5 commented Nov 10, 2017

Also, please make corresponding changes to the macOS implementation of MGLMapView, if applicable. Thanks!

@1ec5 1ec5 removed the macOS Mapbox Maps SDK for macOS label Nov 10, 2017
@fabian-guerra
Copy link
Contributor Author

fabian-guerra commented Nov 10, 2017

@1ec5 looks like we didn't implement the camera projection on mac I will port cameraByZoomingToZoomLevel, cameraByRotatingToDirection, cameraByTiltingToPitch, cameraByPanningWithTranslation

MGLCoordinateBounds bounds = MGLCoordinateBoundsFromLatLngBounds(_mbglMap->latLngBoundsForCamera(currentCameraOptions));

MGLMapCamera *camera;
camera = [self cameraThatFitsCoordinateBounds:bounds];
Copy link
Contributor

Choose a reason for hiding this comment

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

Return this expression directly instead of creating a local variable for it.

@fabian-guerra fabian-guerra added this to the ios-v3.7.0 milestone Nov 13, 2017
@@ -37,6 +37,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT

* Users of VoiceOver can now swipe left and right to navigate among visible places, points of interest, and roads. ([#9950](https://github.com/mapbox/mapbox-gl-native/pull/9950))
* Increased the default maximum zoom level from 20 to 22. ([#9835](https://github.com/mapbox/mapbox-gl-native/pull/9835))
* Fixed an issue that causes `mapView:shouldChangeFromCamera:toCamera:` delegate pass the wrong `centerCoordinate` in `toCamera` parameter. ([#10433](https://github.com/mapbox/mapbox-gl-native/pull/10433))
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the fully qualified syntax so that jazzy will autolink these references in the front page of the API reference:

Fixed an issue where the same value was passed in as the oldCamera and newCamera parameters to the -[MGLMapViewDelegate mapView:shouldChangeFromCamera:toCamera:] method.

@1ec5
Copy link
Contributor

1ec5 commented Nov 13, 2017

looks like we didn't implement the camera projection on mac I will port cameraByZoomingToZoomLevel, cameraByRotatingToDirection, cameraByTiltingToPitch, cameraByPanningWithTranslation

It may not be strictly necessary to implement any of these methods on macOS: since AppKit handles drift panning for us, we can do the set-and-revert dance without any real consequences. But keeping the implementations as similar as possible could make it easier to factor out the camera implementation into a platform-agnostic class in the future, so perhaps we can implement that functionality as tail work.

@fabian-guerra fabian-guerra merged commit addefe5 into release-agua Nov 14, 2017
@fabian-guerra fabian-guerra deleted the fabian-restrict-camera-10391 branch November 14, 2017 18:37
mappy-mobile pushed a commit to Mappy/mapbox-gl-native that referenced this pull request Nov 29, 2017
* release-android-v5.2.0: (788 commits)
  release android v5.2.0
  release v5.2.0-beta.5 (mapbox#10464)
  [ios] Update puck arrow stroke color when tint changes
  Monkey crashes (mapbox#10440) (mapbox#10472)
  Deploy macosapp as part of releases (mapbox#10191)
  [ios] Update podspecs to v3.7.0-rc.1
  [ios] Updated Spanish, Vietnamese translations
  [ios] Fix toCamera.centerCoordinate in shouldChangeFromCamera  (mapbox#10433)
  MapSnapshot attribution (mapbox#10362)
  Downgrade min sdk to 14 (mapbox#10355)
  Update MGLMapSnapshotter docs (mapbox#10438)
  [android] - harden deselection mechanism for markers (mapbox#10403)
  [android] Cherry picks to agua (mapbox#10442)
  [ios] Silence smart invert warnings (mapbox#10425)
  [ios] Doc fixes for "Adding Points to a Map" guide
  [ios, macos] Cleanup duplicated snapshotter frame code.
  [android] release 5.2.0-beta.4 (mapbox#10384)
  [android] - add config file for excluding generated tests, refactor generation script output
  [ios] Bump podspec to beta.4
  [ios, macos] Add attribution to snapshots.
  ...

# Conflicts:
#	.gitignore
#	cmake/core-files.cmake
#	include/mbgl/annotation/annotation.hpp
#	platform/android/CHANGELOG.md
#	platform/android/MapboxGLAndroidSDK/build.gradle
#	platform/android/MapboxGLAndroidSDK/gradle.properties
#	platform/android/MapboxGLAndroidSDK/src/main/AndroidManifest.xml
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/Mapbox.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationSource.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AnnotationManager.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AttributionDialogManager.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapGestureDetector.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Transform.java
#	platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/camera/LatLngBoundsActivity.java
#	platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/maplayout/SimpleMapActivity.java
#	platform/android/build.gradle
#	platform/android/config.cmake
#	platform/android/dependencies.gradle
#	platform/android/settings.gradle
#	platform/android/src/map/camera_position.cpp
#	platform/android/src/native_map_view.cpp
#	platform/android/src/native_map_view.hpp
#	src/mbgl/algorithm/generate_clip_ids_impl.hpp
#	src/mbgl/annotation/line_annotation_impl.cpp
#	src/mbgl/renderer/layers/render_line_layer.hpp
#	src/mbgl/renderer/painter.cpp
#	src/mbgl/renderer/painter_line.cpp
#	src/mbgl/renderer/render_item.hpp
#	src/mbgl/renderer/render_line_layer.cpp
#	src/mbgl/sprite/sprite_atlas.cpp
#	src/mbgl/sprite/sprite_atlas.hpp
#	src/mbgl/sprite/sprite_parser.cpp
#	src/mbgl/sprite/sprite_parser.hpp
#	src/mbgl/storage/resource.cpp
#	src/mbgl/style/layers/line_layer.cpp
#	src/mbgl/style/layers/line_layer_impl.hpp
#	src/mbgl/style/style.cpp
#	src/mbgl/style/style.hpp
#	src/mbgl/tile/tile_loader_impl.hpp
#	test/algorithm/generate_clip_ids.test.cpp
#	test/sprite/sprite_atlas.test.cpp
#	test/storage/resource.test.cpp
mappy-mobile pushed a commit to Mappy/mapbox-gl-native that referenced this pull request Dec 12, 2017
* mapbox_release_5.2: (788 commits)
  release android v5.2.0
  release v5.2.0-beta.5 (mapbox#10464)
  [ios] Update puck arrow stroke color when tint changes
  Monkey crashes (mapbox#10440) (mapbox#10472)
  Deploy macosapp as part of releases (mapbox#10191)
  [ios] Update podspecs to v3.7.0-rc.1
  [ios] Updated Spanish, Vietnamese translations
  [ios] Fix toCamera.centerCoordinate in shouldChangeFromCamera  (mapbox#10433)
  MapSnapshot attribution (mapbox#10362)
  Downgrade min sdk to 14 (mapbox#10355)
  Update MGLMapSnapshotter docs (mapbox#10438)
  [android] - harden deselection mechanism for markers (mapbox#10403)
  [android] Cherry picks to agua (mapbox#10442)
  [ios] Silence smart invert warnings (mapbox#10425)
  [ios] Doc fixes for "Adding Points to a Map" guide
  [ios, macos] Cleanup duplicated snapshotter frame code.
  [android] release 5.2.0-beta.4 (mapbox#10384)
  [android] - add config file for excluding generated tests, refactor generation script output
  [ios] Bump podspec to beta.4
  [ios, macos] Add attribution to snapshots.
  ...

# Conflicts:
#	.gitignore
#	.gitmodules
#	cmake/core-files.cmake
#	include/mbgl/annotation/annotation.hpp
#	platform/android/CHANGELOG.md
#	platform/android/MapboxGLAndroidSDK/build.gradle
#	platform/android/MapboxGLAndroidSDK/gradle.properties
#	platform/android/MapboxGLAndroidSDK/src/main/AndroidManifest.xml
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/Mapbox.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationSource.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AnnotationManager.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AttributionDialogManager.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapGestureDetector.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Transform.java
#	platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/camera/LatLngBoundsActivity.java
#	platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/maplayout/SimpleMapActivity.java
#	platform/android/build.gradle
#	platform/android/config.cmake
#	platform/android/dependencies.gradle
#	platform/android/settings.gradle
#	platform/android/src/map/camera_position.cpp
#	platform/android/src/native_map_view.cpp
#	platform/android/src/native_map_view.hpp
#	src/mbgl/algorithm/generate_clip_ids_impl.hpp
#	src/mbgl/annotation/line_annotation_impl.cpp
#	src/mbgl/renderer/layers/render_line_layer.hpp
#	src/mbgl/renderer/painter.cpp
#	src/mbgl/renderer/painter_line.cpp
#	src/mbgl/renderer/render_item.hpp
#	src/mbgl/renderer/render_line_layer.cpp
#	src/mbgl/sprite/sprite_atlas.cpp
#	src/mbgl/sprite/sprite_atlas.hpp
#	src/mbgl/sprite/sprite_parser.cpp
#	src/mbgl/sprite/sprite_parser.hpp
#	src/mbgl/storage/resource.cpp
#	src/mbgl/style/layers/line_layer.cpp
#	src/mbgl/style/layers/line_layer_impl.hpp
#	src/mbgl/style/style.cpp
#	src/mbgl/style/style.hpp
#	src/mbgl/tile/tile_loader_impl.hpp
#	test/algorithm/generate_clip_ids.test.cpp
#	test/sprite/sprite_atlas.test.cpp
#	test/storage/resource.test.cpp
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants