mapbox-maps-ios icon indicating copy to clipboard operation
mapbox-maps-ios copied to clipboard

MapboxMap.coordinateBounds gives wrong answer when rotation != 0

Open akirmse opened this issue 4 years ago • 19 comments

The function MapboxMap.coordinateBounds is presumably supposed to return an axis-aligned rectangle that encompasses the entire camera view: https://docs.mapbox.com/ios/maps/api/10.0.0-rc.6/Classes/MapboxMap.html#/Camera%20Fitting

But the implementation assumes that the camera's rotation is 0. When it is not, a rather meaningless rectangle is returned.

This implementation does the right thing:

        let p1 = self.map.coordinate(for: CGPoint(x: rect.maxX, y: rect.minY))
        let p2 = self.map.coordinate(for: CGPoint(x: rect.minX, y: rect.minY))
        let p3 = self.map.coordinate(for: CGPoint(x: rect.minX, y: rect.maxY))
        let p4 = self.map.coordinate(for: CGPoint(x: rect.maxX, y: rect.maxY))
        let minLat = min(p2.latitude, p1.latitude, p3.latitude, p4.latitude)
        let maxLat = max(p2.latitude, p1.latitude, p3.latitude, p4.latitude)
        let minLng = min(p2.longitude, p1.longitude, p3.longitude, p4.longitude)
        let maxLng = max(p2.longitude, p1.longitude, p3.longitude, p4.longitude)

        let southwest = CLLocationCoordinate2D(latitude: minLat, longitude: minLng)
        let northeast = CLLocationCoordinate2D(latitude: maxLat, longitude: maxLng)
        return CoordinateBounds(southwest: southwest, northeast: northeast)

The same bug exists in the Mapbox Android SDK. The Google SDK's implementation is correct.

akirmse avatar Aug 18 '21 06:08 akirmse

I've verified the behavior you describe. Here's a sample view controller that visualizes the issue:

final class DebugViewController: UIViewController {

    var mapView: MapView!
    var annotationManager: PolylineAnnotationManager!

    override public func viewDidLoad() {
        super.viewDidLoad()
        mapView = MapView(frame: view.bounds)
        mapView.autoresizingMask = [.flexibleWidth, .flexibleHeight]
        view.addSubview(mapView)
        mapView.mapboxMap.onNext(.mapLoaded) { _ in
            let tapGestureRecognizer = UITapGestureRecognizer(target: self, action: #selector(self.showBounds))
            tapGestureRecognizer.delegate = self
            self.mapView.addGestureRecognizer(tapGestureRecognizer)

            self.annotationManager = self.mapView.annotations.makePolylineAnnotationManager()
        }
    }

    @objc func showBounds() {
        let bounds = mapView.mapboxMap.coordinateBounds(for: mapView.bounds.insetBy(dx: 20, dy: 20))
        let lineString = LineString([bounds.northeast, bounds.northwest, bounds.southwest, bounds.southeast, bounds.northeast])
        var annotation = PolylineAnnotation(lineString: lineString)
        annotation.lineColor = ColorRepresentable(color: .red)
        annotationManager.annotations = [annotation]
    }
}

extension DebugViewController: UIGestureRecognizerDelegate {
    func gestureRecognizer(_ gestureRecognizer: UIGestureRecognizer, shouldRecognizeSimultaneouslyWith otherGestureRecognizer: UIGestureRecognizer) -> Bool {
        return true
    }
}

Simulator Screen Shot - iPhone 12 - 2021-09-02 at 12 58 49

cc: @tobrun

macdrevx avatar Sep 02 '21 17:09 macdrevx

This is a bug in the C++ implementation, you can workaround this with APIs from CameraManagerProtocol that explicitly let you add bearing/pitch etc. Will ticket upstream.

tobrun avatar Sep 02 '21 18:09 tobrun

Note that there are 2 methods with the prefix coordinateBounds( on MapboxMap:

My example above tested only the latter.

I'm guessing that @akirmse is referring to the latter since his suggested implementation starts by converting the four corners of an input rectangle to coordinates. In that case, I think the logic error may well be in mapbox-maps-ios itself.

macdrevx avatar Sep 02 '21 20:09 macdrevx

@akirmse can you clarify which methods you're referring to for both iOS and Android?

macdrevx avatar Sep 02 '21 20:09 macdrevx

That's correct, I was using the second one.

BTW my replacement code still appears to give bad answers when the camera's pitch is large (or maybe just non-zero; haven't tested it extensively). MapboxMap.coordinate(for: CGPoint) may have another problem there.

akirmse avatar Sep 02 '21 20:09 akirmse

Tested out getting the coordinate bounds with two methods, both of which seem to return the wrong value when the map is tilted or rotated.

MapboxMap.coordinateBounds(for: CGRect) does seem related to https://github.com/mapbox/mapbox-maps-ios/issues/788, since it uses MapboxMap.coordinate(for: CGPoint) to call [MBMCameraManager coordinateForPixel:].

MapboxMaps.coordinateBounds(for: CameraOptions uses [MBMCameraManager coordinateBoundsForCamera:]. @endanke - does a similar limitation apply to this method?

Noting that these two methods do result in different CoordinateBounds.

jmkiley avatar Nov 03 '21 19:11 jmkiley

@jmkiley unfortunately I think the linked issue is not the same as this one. For that problem the precondition was to have a pitch value which is larger than zero. As I understand this issue happens also for zero pitch, just based on the rotation value, correct? In any case we will need further investigation on the gl-native side.

endanke avatar Nov 04 '21 09:11 endanke

I've investigate this further and I think we're talking about two separate issues here:

  • Issue one: If the pitch is zero, but rotation is not zero: It is an issue with CoordinateBounds as Tobrun mentioned. The coordinate(for: CGPoint) works correctly for the zero pitch case, you can observe on the image that the top right and bottom left coordinates are correctly positioned, and we construct the bounds from those two points. We would need to update the implementation for CoordinateBounds to take in consideration the camera rotation.

Screenshot 2021-11-04 at 14 30 54

  • Issue two: ~~If the pitch is large coordinate(for: CGPoint) might not work as expected. When I tested it I noticed that it happens when the horizon is visible, and the top right point gets clamped. (For some reason it fully disappears but most likely it's still caused by the clamping.)~~ Edit: I need to revisit this because I realized that the point is not visible as the rectangle is rendered on the map, where the illustrated points are not placed outside of the map's bounds as expected.

endanke avatar Nov 04 '21 12:11 endanke

Also just for demonstration, if I modify this example to use the four corner points instead of the CoordinateBounds, it places the rectangle correctly: https://github.com/mapbox/mapbox-maps-ios/issues/605#issuecomment-911927577

let topRight = mapView.mapboxMap.coordinate(for: CGPoint(x: rect.maxX, y: rect.minY))
let bottomLeft = mapView.mapboxMap.coordinate(for: CGPoint(x: rect.minX, y: rect.maxY))
let topLeft = mapView.mapboxMap.coordinate(for: CGPoint(x: rect.minX, y: rect.minY))
let bottomRight = mapView.mapboxMap.coordinate(for: CGPoint(x: rect.maxX, y: rect.maxY))
let lineString = LineString([topRight, topLeft, bottomLeft, bottomRight, topRight])

(Note: this example is not using wrapping)

simulator_screenshot_ACAC6F46-FD2E-48DE-A530-AE708C9C987C

endanke avatar Nov 04 '21 13:11 endanke

CoordinateBounds is an axis aligned bound, and as such can't represent something rotated (containing inside only 2 coordinates: northeast and southwest).

If you want the geometry I'm afraid you need to extract it from map corners as we do not currently offer a geometryForCamera method (although this approach, extracting from corners, is used internally in at least one place)

galinelle avatar Nov 04 '21 21:11 galinelle

@galinelle would it make sense to add a new constructor to CoordinateBounds to allow the creation from the 4 corner coordinates? The bounds would be still axis aligned but when we query those corners using coordinate(for: CGPoint) they would be created based on the camera's state, so it would produce the expected results.

Edit: I've checked the implementation and unfortunately it looks like it would require quite a lot of changes to use four points in CoordinateBounds since all the underlying calculations are using directly the existing two corner points. So this is most likely not a good idea.

endanke avatar Nov 05 '21 07:11 endanke

Another alternative would be to just define a new bounds class which can be constructed using 4 points, but then it would need to reimplement most of the methods.

@akirmse could you tell us a bit more about your use-case?

endanke avatar Nov 05 '21 07:11 endanke

In my use case, I have thousands of markers and polylines, too many to load them all into memory at the same time (especially the polylines). So what I do is when the camera becomes idle, I get the viewport, load any markers/polylines that are newly visible, and unload any that are now outside the viewport. However, since the viewport from this function is incorrect, what users actually see is that markers/polylines spontaneously go missing as the camera is rotated or tilted. It is an extremely broken experience, though it's mitigated by the fact that it works in the Google Maps SDK, and the user can always switch back.

Ideally I'd be able to simply get the correct viewport. Axis-aligned (i.e. potentially overly large) is OK for my purposes. It would be ideal if I could specify a max distance (far plane) for when the view is tilted. At high tilt, lots of the planet is visible, and I can't afford to include all of the objects that are extremely far away.

akirmse avatar Nov 06 '21 00:11 akirmse

@akirmse thanks for the details!

From our side modifying the behaviour of CoordinateBounds wouldn't be ideal, but we could add another helper class that is created from the four corner points, and it could have a contains function for at least points and lines. This could be even created outside of the SDK, but we would need to add a version of coordinate(for: CGPoint) which returns the wrapped version of the coordinate.

Considering the pitch is a bit tricky, but using the corners we could also calculate the distance between the bottom and top coordinates on the screen, and some of the features could be filtered if the distance is too large.

endanke avatar Nov 08 '21 07:11 endanke

Ideally there would be a way to quickly determine which of a set of points are in the view frustum, even when pitched, given a far plane distance. Clearly many implementations are possible. I've done this in the past for video games.

akirmse avatar Nov 10 '21 22:11 akirmse

Is there any news on this? I still see the problem in 10.3.0. It's not a great look to have all of the markers on the map disappear as the camera is rotated (because they're calculated to be outside the field of view).

akirmse avatar Feb 14 '22 21:02 akirmse

@akirmse would something like this work for your use case?

mapView.mapboxMap.coordinateBoundsUnwrapped(for: CameraOptions(cameraState: mapView.cameraState))

(not to say that coordinateBounds(for rect: CGRect) doesn't have a bug — just offering a potential alternative until we fix it)

macdrevx avatar Apr 21 '22 20:04 macdrevx

It looks like coordinateBoundsUnwrapped is still in beta, and I generally don't touch beta versions of libraries. I'll try it after it's released.

On Thu, Apr 21, 2022 at 1:11 PM Andrew Hershberger @.***> wrote:

@akirmse https://github.com/akirmse would something like this work for your use case?

mapView.mapboxMap.coordinateBoundsUnwrapped(for: CameraOptions(cameraState: mapView.cameraState))

(not to say that coordinateBounds(for rect: CGRect) doesn't have a bug — just offering a potential alternative until we fix it)

— Reply to this email directly, view it on GitHub https://github.com/mapbox/mapbox-maps-ios/issues/605#issuecomment-1105710399, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARJJ4QBB7TNBOTRLJ4ATY3VGGY6JANCNFSM5CLJ4ZLQ . You are receiving this because you were mentioned.Message ID: @.***>

akirmse avatar Apr 21 '22 21:04 akirmse

Thanks, this appears to be a pretty good workaround.

akirmse avatar May 07 '22 18:05 akirmse