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] Fix camera paddings reset on map view gestures performing. #188

Merged

Conversation

OlexandrStepanov
Copy link
Contributor

@OlexandrStepanov OlexandrStepanov commented Nov 28, 2021

This PR fixes the issue with inconsistent padding reset of the map view camera after performing any gesture on the map other than pan.

The logic to reset camera paddings with pinch/rotation gesture was introduced in this commit altogether with gesture unit tests. Although I'm not sure what was the original intent, it seems wrong nowadays taking into account the current logic of edge insets handling in MGLMapView.set... methods.

Just to be clear, what I mean by "current logic":

  • contentInset property of MGLMapView controls the global padding of the map view. This value persists between camera changes of the map and controls the map elements positioning, like compass, scale bar, logo view etc.
  • edgePadding value passed in the methods like setCamera:, setVisibleCoordinatesBounds:, showAnnotations: etc. is the transient value for this particular camera update.
  • The result map paddings are a combination of the map contentInset and passed edgePadding. This value is passed to the internal _mbglMap as a part of CameraOptions.
  • The result paddings persist until the next programmatic camera update or change of contentInset value.

There is another PR which is reported to be fixing the same issue, but there are some new problems introduced there.

  • Adding another way to set paddings through the camera object property overcomplicate the things. It seems that semantically camera paddings and the transient edgePadding are interchangeable, so having both of them introduces some ambiguity.
  • There is an inconsistency in the camera management: if one pass camera object to the map, and then reads it, it's expected that it returns the same object back. Padding value breaks that assumption in that PR.

More discussions on the topic:

@OlexandrStepanov
Copy link
Contributor Author

Here one can find the project with the fix demonstration. It's in the separate fix-asymmetrical-paddings-demo branch.

AsymmetricalPaddingsDemo.mov
  • Red frame demonstrates the current map view contentInset padding.
  • Blue frame demonstrates the combined cameraEdgeInsets padding.
  • Black frame is just the test region to be used in setVisibleCoordinateBounds method.
  • The grey cross demonstrates centerCoordinate position, which is efectively the camera center respecting the paddings,
  • There are two actions which demonstrates setting symmetrical and asymmetrical paddings.

@@ -2591,8 +2586,7 @@ - (MGLMapCamera *)cameraByZoomingToZoomLevel:(double)zoom aroundAnchorPoint:(CGP

- (MGLMapCamera *)cameraByRotatingToDirection:(CLLocationDirection)degrees aroundAnchorPoint:(CGPoint)anchorPoint
{
mbgl::EdgeInsets padding = MGLEdgeInsetsFromNSEdgeInsets(self.contentInset);
mbgl::CameraOptions currentCameraOptions = self.mbglMap.getCameraOptions(padding);
mbgl::CameraOptions currentCameraOptions = self.mbglMap.getCameraOptions();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here and in similar places below the padding value passed in mbglMap.getCameraOptions() just replaces the camera edge insets. Although CameraOptions is a pure model class which doesn't have any computations and side effects, it doesn't make much sense to replace its padding value.

self.mapView.contentInset = contentInset;
mbgl::EdgeInsets padding = MGLEdgeInsetsFromNSEdgeInsets(self.mapView.contentInset);
auto cameraPadding = self.mapView.mbglMap.getCameraOptions().padding;
XCTAssertEqual(padding, cameraPadding, @"MGLMapView's contentInset property should match camera's padding.");
XCTAssertTrue(UIEdgeInsetsEqualToEdgeInsets(self.mapView.contentInset, contentInset));

contentInset = UIEdgeInsetsMake(20, 20, 20, 20);
[self.mapView setCamera:self.mapView.camera withDuration:0.1 animationTimingFunction:nil edgePadding:contentInset completionHandler:nil];
[self.mapView setCamera:self.mapView.camera withDuration:0.0 animationTimingFunction:nil edgePadding:contentInset completionHandler:nil];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change must be immediate to take an effect while the test is running.

@OlexandrStepanov OlexandrStepanov marked this pull request as draft November 29, 2021 09:20
@OlexandrStepanov OlexandrStepanov marked this pull request as ready for review November 29, 2021 09:20
@petr-pokorny-1
Copy link
Contributor

This looks really good @OlexandrStepanov Thank you for the great work!

@petr-pokorny-1 petr-pokorny-1 merged commit ba1ec44 into maplibre:master Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants