Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[iOS] add-camera-padding #1348

Closed
wants to merge 5 commits into from
Closed

[iOS] add-camera-padding #1348

wants to merge 5 commits into from

Conversation

JulianBissekkou
Copy link
Collaborator

@JulianBissekkou JulianBissekkou commented Jul 12, 2023

Why is this change needed?

We are maintaining the Flutter plugin for maplibre and we are currently implementing maplibre/flutter-maplibre-gl#258 which brings the padding feature to the sdk.
On android we are already able to create camera updates with a padding value, but this is missing on iOS.

This PR exposes the padding value of the MLNMapCamera so that we are able to implement this feature.

This introduces the changes from an unmerged PR that we have seen in the old mapbox repository.
(mapbox-gl-native-ios/pull/323/files)

Comment on lines +4390 to +4392
if (!MGLEdgeInsetsEqual(camera.padding, MGLEdgeInsetsZero)) {
options.padding = MLNEdgeInsetsFromNSEdgeInsets(camera.padding);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moving the discussion of the old PR over to this PR:

Not sure if this is correct, as there is edgePadding as argument as well as in padding in MGLMapCamera. The padding in MGLMapCamera overwrites the param here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me do some more testing before we merge this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did my testing. However, since this is my first PR in maplibre native I would like to have feedback from someone that knows this part of the code well enough :)

Copy link
Contributor

@1ec5 1ec5 Jul 14, 2023

Choose a reason for hiding this comment

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

Not sure if this is correct, as there is edgePadding as argument as well as in padding in MGLMapCamera. The padding in MGLMapCamera overwrites the param here.

The library currently conflates two different concepts as “padding”:

  • MLNMapView.contentInset is supposed to be persistent, something the developer sets according to UI that takes up space within the map view, such as a translucent sidebar or a camera “notch” on iOS. The developer should only have to think about this inset when sizing the map view or embedding a subview, not when manipulating or animating the camera. The resulting virtual rectangle is often called a “safe area” on Apple platforms.
  • Various camera-manipulating methods like -[MLNMapView setCamera:withDuration:animationTimingFunction:edgePadding:completionHandler:] take an edgePadding parameter for more precisely targeting the camera transition. In particular, the navigation SDK (and navigation applications in general) need to anchor the user location indicator somewhere other than the center of the safe area, typically in the lower third of the map view.

In the original design in mapbox/mapbox-gl-native#3583, the mbgl::CameraOptions::padding was the sum of the (persistent) content insets and the (temporary) edge padding. mbgl::CameraOptions::padding affected the frame of reference for mbgl::CameraOptions::center, but mbgl::Map::transform never persisted the padding because it had no context about how much of that padding was persistent and how much was temporary.

Unfortunately, a change was made to mbgl that conflated these two concepts, creating quite a headache for iOS/macOS developers: mapbox/mapbox-gl-native#15232 mapbox/mapbox-gl-native#15233. I have recommended that mbgl::Transform decouple the concepts of “padding” and “insets” and even replace “padding” with a more explicit way to anchor the center coordinate within screen space: mapbox/mapbox-gl-native#15233 (comment).

None of this was taken into account on Android; this mbgl functionality was exposed more literally without much regard for the practical use cases. Unfortunately, this means cross-platform libraries like the Flutter library will need to reconcile the two approaches.

@JulianBissekkou JulianBissekkou changed the title Draft: [iOS] add-camera-padding [iOS] add-camera-padding Jul 12, 2023
@louwers louwers added iOS enhancement New feature or request labels Jul 12, 2023
@louwers
Copy link
Collaborator

louwers commented Jul 12, 2023

This introduces the changes from an unmerged PR that we have seen in the old mapbox repository.
(mapbox-gl-native-ios/pull/323/files)

Which to this date (and at the time the PR was merged) is permissively licensed.

Comment on lines +1680 to +1697
- (void)centerSelectedAnnotationWithPadding {
id<MLNAnnotation> annotation = self.mapView.selectedAnnotations.firstObject;

if (!annotation)
return;

CGPoint point = [self.mapView convertCoordinate:annotation.coordinate toPointToView:self.mapView];

// Animate, so that point becomes the the center
CLLocationCoordinate2D center = [self.mapView convertPoint:point toCoordinateFromView:self.mapView];
MLNMapCamera *camera = self.mapView.camera.copy;
camera.centerCoordinate = center;
camera.padding = UIEdgeInsetsMake(200, 50, 10, 4);

[self.mapView setCamera:camera animated:YES];
}

- (void)centerSelectedAnnotationWithoutPadding {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know that this adjustment on the example app is not perfect.
It's been a while since I wrote some objective-c so that's why I quickly adjusted an existing entry in the feature list instead of creating a new one.

If someone has a better idea and can maybe help me out then let me know 👍🏽

Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

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

Could you add an entry to the iOS changelog?

@JulianBissekkou JulianBissekkou requested a review from louwers July 12, 2023 06:24
@louwers
Copy link
Collaborator

louwers commented Jul 12, 2023

Sorry for the confusion, this is the iOS changelog: https://github.com/maplibre/maplibre-native/blob/main/platform/ios/platform/ios/CHANGELOG.md

@louwers
Copy link
Collaborator

louwers commented Jul 13, 2023

These tests fail.

image

E.g.

testHandleTwoFingerTapGesture: ((padding) not equal to (cameraPadding)) failed: ("{length = 32, bytes = 0x00000000 0000f03f 00000000 0000f03f ... 00000000 0000f03f }") is equal to ("{length = 40, bytes = 0x00000000 0000f03f 00000000 0000f03f ... 01000000 00000000 }")

MLNMapProjectionTests also fails:

testHandleDoubleTapGestureContentInset: ((padding) not equal to (cameraPadding)) failed: ("{length = 32, bytes = 0x00000000 0000f03f 00000000 0000f03f ... 00000000 0000f03f }") is equal to ("{length = 40, bytes = 0x00000000 0000f03f 00000000 0000f03f ... 01000000 00000000 }")

@JulianBissekkou
Copy link
Collaborator Author

Closing this because its not required anymore.

@JulianBissekkou JulianBissekkou deleted the add-camera-padding branch August 3, 2023 06:29
@1ec5 1ec5 mentioned this pull request Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants