mapbox / mapbox-maps-ios

Interactive, thoroughly customizable maps for iOS powered by vector tiles and Metal
https://www.mapbox.com/mapbox-mobile-sdk
Other
462 stars 151 forks source link

MapboxMap.coordinateBounds gives wrong answer when rotation != 0 #605

Open akirmse opened 3 years ago

akirmse commented 3 years ago

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.

macdrevx commented 3 years ago

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

tobrun commented 3 years ago

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.

macdrevx commented 3 years ago

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 commented 3 years ago

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

akirmse commented 3 years ago

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.

jmkiley commented 2 years ago

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.

endanke commented 2 years ago

@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 commented 2 years ago

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

Screenshot 2021-11-04 at 14 30 54

endanke commented 2 years ago

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

galinelle commented 2 years ago

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)

endanke commented 2 years ago

@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 commented 2 years ago

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?

akirmse commented 2 years ago

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.

endanke commented 2 years ago

@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.

akirmse commented 2 years ago

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 commented 2 years ago

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).

macdrevx commented 2 years ago

@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)

akirmse commented 2 years ago

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 commented 2 years ago

Thanks, this appears to be a pretty good workaround.