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

Fix fit to bounds #1783

Merged
merged 4 commits into from
Jun 26, 2015
Merged

Fix fit to bounds #1783

merged 4 commits into from
Jun 26, 2015

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Jun 23, 2015

This PR reimplements “fit to bounds” functionality to take advantage of MGLCoordinateBounds and additionally exposes the method publicly. I started by re-porting Camera.prototype.fitBounds() in mapbox/mapbox-gl-js to ensure correct baseline behavior. Then I wrote some logic for edge insets and rotation. Along for the ride, I added a bunch of public utility functions related to MGLCoordinateBounds.

@friedbunny @incanus, can you try out this implementation and see if there are any other edge cases that MapKit and Google Maps handle differently?

@1ec5 1ec5 added bug GL JS parity For feature parity with Mapbox GL JS iOS Mapbox Maps SDK for iOS refactor ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Jun 23, 2015
@1ec5 1ec5 self-assigned this Jun 23, 2015
@1ec5 1ec5 added this to the iOS Beta 3 milestone Jun 23, 2015
@1ec5
Copy link
Contributor Author

1ec5 commented Jun 23, 2015

Adjust the viewport to account for top and bottom layout guides

In light of #1781, maybe it doesn’t make sense for MGLMapView to handle this itself. What does MKMapView do with top and bottom bars?

@jfirebaugh
Copy link
Contributor

I'd like to move the core native API towards gl-js's Camera. How hard would it be to push the calculations down into GL core via Map::fitBounds()?

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 24, 2015

How hard would it be to push the calculations down into GL core via Map::fitBounds()?

Done in ed99904. That was pretty easy, actually, because I was relying on mbgl::Map instead of the view for all the calculations. That change also neatly avoids an erroneous call to -setCenterCoordinate:animated: instead of -setCenterCoordinate:animated:preservingTracking:, so that fixes #1256 (and obviates part of #1257).

@friedbunny
Copy link
Contributor

This works well so far. I'll test again once the layout guides adjustments are in, that's the biggest difference between MapKit/Google and us right now.

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 24, 2015

Note that fitting to bounds forcefully resets the direction back to north, without animation.

Also, in 31d2918, I adapted the fit to bounds method to account for top and bottom bars, but it appears that MapKit does not. So I’m probably going to roll back that change. It’s easy enough to handle manually in client code.

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 25, 2015

If you try to fit an MKMapView to any empty rect, MKMapView zooms way into the top-left corner of the map (in the Arctic Sea, above the Bering Strait). With this PR, however, MGLMapView centers on the empty coordinate bounds (which may originate anywhere) – I think that’s better behavior.

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 26, 2015

The fit to bounds method now respects the current rotation. That turned out to require a lot less trig than I feared. 😃

@1ec5 1ec5 removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jun 26, 2015
@1ec5 1ec5 changed the title [WIP] Fix fit to bounds Fix fit to bounds Jun 26, 2015
1ec5 added 4 commits June 25, 2015 21:54
The new implementation is now public and takes advantage of MGLCoordinateBounds. It is re-ported from `Camera.prototype.fitBounds()` in mapbox/mapbox-gl-js to ensure correct behavior. A new function, MGLCoordinateBoundsMake(), makes it easier to create an MGLCoordinateBounds for use with this method.
Each side of the bounding box is specified independently, allowing more flexibility than the offset + padding construct supported in mapbox/mapbox-gl-js’ Camera.
Added a bunch of functions to work with MGLCoordinateBounds in a separate header analogous to MKGeometry.h. Added resolution-independent tests for common fit to bounds scenarios.
Also fit to the rotated bounds. A little more verbose than necessary due to <http://stackoverflow.com/a/2357688/4585461>.

ref mapbox/mapbox-gl-js#1338
@1ec5 1ec5 merged commit bcbb56c into master Jun 26, 2015
@1ec5 1ec5 removed the in progress label Jun 26, 2015
@1ec5 1ec5 deleted the 1ec5-zoom-bounds-1092 branch June 26, 2015 05:10
1ec5 added a commit that referenced this pull request Jun 26, 2015
Followup to #1783: Implemented the non-animated version of fit to bounds and makes it a KVO-compliant property.
@friedbunny
Copy link
Contributor

4435752_1431360735 7466_updates

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.