getPointAnnotationsInBounds with a rotated map

[tobrun]

This is a follow up issue on #5051. In that issue we implemented that camera animations and projection should take MapView padding into account. While testing my changes for projection, I started to notice that getAnnotationsInBounds was not returning the actual annotations for the projection.

After some debugging and playing around with some values. I started to notice that having tilt = 0 and direction = 0 returns the correct annotations in bounds, When the map is rotated we aren't receiving the correct values anymore.

Example

In below gif I'm seeing the top left marker as an annotations in bounds even though it's not a part of the current projection. When I make the map face north again it becomes a part of the projection.

[1ec5]

Are you sure you’re passing in a suitable bounding box? When the map is rotated, a bounding box obtained by converting the four corners of the view to coordinates may circumscribe or be circumscribed by the actual viewport. (See mbgl::Map::cameraForLatLngs() for an attempt at compensating for rotation.) Additionally, when the map is tilted, the viewport covers a trapezoidal region rather than a rectangular region. To find all the visible annotations using getPointAnnotationsInBounds(), you need to specify a bounding box that somehow circumscribes the visible region.

We ran into the same issue on iOS: #4801 (comment). In fb759e8, @boundsj worked around this issue by removing the call to annotationTagsInRect(), instead iterating over an index of annotations in the SDK code to find the annotations within bounds. (This is also explains the behavior in #5138 (comment) .)

[tobrun]

Thanks for 👀 @1ec5 , I guess I was doing the same as what iOS was doing. Will look into adding the same fix in next days. Now that it's clear it isn't a core bug, adding android label.

[1ec5]

I still think it would be worthwhile at some point to go back to getPointAnnotationsInBounds() (or perhaps queryRenderedFeatures()) on iOS, because then we can take advantage of the indexing that AnnotationManager performs, to avoid any penalty for annotations that are nowhere near the viewport.

[jfirebaugh]

Is it possible for the SDKs to switch over to queryRenderedFeatures so we can remove getPointAnnotationsInBounds entirely?

[1ec5]

Yes, I’m looking into this now. All that should be required is for the AnnotationTileFeatures to know their IDs.

[1ec5]

As explained in #5165 (comment), I think we’ll have to punt on fixing getPointAnnotationsInBounds() until querySourceFeatures() is implemented as part of #352.

[tobrun]

Closing as inittial observation was not correct and we are planning to resolve this with querySourceFeatures(). See #5165 for more information. Closing for now.