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

Cannot select annotations that lie on a tile boundary #12472

Closed
jadar opened this issue Jul 24, 2018 · 9 comments
Closed

Cannot select annotations that lie on a tile boundary #12472

jadar opened this issue Jul 24, 2018 · 9 comments
Labels
bug Core The cross-platform C++ core, aka mbgl high priority

Comments

@jadar
Copy link

jadar commented Jul 24, 2018

We have a bug where we're not able to select/tap some of our annotations on the map in our iOS app. After some head scratching, we found out that the annotation just happened to lie on a tile boundary! Stepping through the Mapbox code, it appears that annotationTagsInRect:, (called in annotationTagAtPoint:persistingResults:,) returns a std::vector of size 0 when there is clearly an annotation within the rect, except the annotation lies between a few tiles.

Steps to reproduce

  1. Create and add/display a UIView-backed annotation on an MGLMapView
  2. Place the annotation between a couple tiles. The lat/lon we can reproduce the issue with are (36.38149043210595, -119.59605216979982).
  3. Try to tap on it.

Expected behavior

The annotation is detected and the relevant delegate methods are called.

Actual behavior

No delegate methods are called.

simulator screen shot - iphone 8 - 2018-07-24 at 13 55 54
While the screenshot says "THIS WILL WORK", it actually means it won't work.

Configuration

Mapbox SDK versions: 4.2.0
iOS/macOS versions: iOS 11.4
Device/simulator models: iPhone 8 and iPhone X
Xcode version: Xcode 9.4.1

@ChrisLoer
Copy link
Contributor

Hi @jadar thanks for the report! Is there any chance you could post some example code reproducing the problem? What you're describing is a class of bug we've had before, and we believe we've fixed. There may be some subtle difference in how you're exercising the functionality that still causes the problem to manifest.

@jadar
Copy link
Author

jadar commented Jul 24, 2018

@ChrisLoer Here is a bare-bones implementation that reproduces the bug. If you zoom in to where the scale shows ~100ft, it won't show a callout. If you zoom out to where the scale shows ~600 ft, the callout shows.
https://gist.github.com/jadar/7a8f8454b78a4168f5724c5c61de6b6b

@ChrisLoer
Copy link
Contributor

Hi @jadar I'm sorry but I'm still not seeing the problem. Here's what I see when I tap on the annotation created with your code pasted into my example app (I moved the location to null island i.e. [0,0] just to be sure it was on a tile boundary, but I couldn't reproduce with your original coordinates either):

screenshot 2018-07-24 15 14 23

@jadar
Copy link
Author

jadar commented Jul 24, 2018

Hmm, I can't seem to reproduce it at [0,0] either. When I use the coordinates I gave, it works up until zoom level 16. 15 and below work correctly.

I don't know if it changes anything, but I'm using this style, and the 4.2.0 version of Mapbox.
mapbox://styles/mapbox/satellite-streets-v9
https://github.com/mapbox/mapbox-gl-native/releases/download/ios-v4.2.0/mapbox-ios-sdk-4.2.0-dynamic.zip

@jadar
Copy link
Author

jadar commented Jul 24, 2018

It also seems that with [0.000000000000001, 0.00000000000001], it doesn't work at any zoom level.

@ChrisLoer
Copy link
Contributor

Got it, thanks! I can reproduce now at [0.000000000000001, 0.00000000000001]. I'll take a closer look tomorrow and hopefully get to the bottom of it.

@fabian-guerra fabian-guerra added bug Core The cross-platform C++ core, aka mbgl labels Jul 25, 2018
@ChrisLoer
Copy link
Contributor

This is a good bug! 😅

The problem is that when the AnnotationManager decides which tile the annotation at [0.000000000000001, 0.00000000000001] should go in, it correctly chooses the "northeast" tile, based on LatLng coordinates:

LatLngBounds tileBounds(tileID);
symbolTree.query(boost::geometry::index::intersects(tileBounds),
boost::make_function_output_iterator([&](const auto& val){
val->updateLayer(tileID, *pointLayer);
}));

However, it then translates into tile coordinates:

LatLng latLng { annotation.geometry.y, annotation.geometry.x };
TileCoordinate coordinate = TileCoordinate::fromLatLng(0, latLng);
GeometryCoordinate tilePoint = TileCoordinate::toGeometryCoordinate(UnwrappedTileID(0, tileID), coordinate.p);
layer.addFeature(id, FeatureType::Point, GeometryCollection {{ {{ tilePoint }} }}, featureProperties);

TileCoordinate::fromLatLng(0, latLng) returns [0.5, 0.5] (using a coordinate system that represents the whole world in web mercator projection with positions from 0 to 1). You can see the low bits have already been lost (it should really be something like [.5000000000001, .4999999999999]. TileCoordinate::toGeometryCoordinate then returns [0,8192] -- that is, the point that's just off the bottom edge of the tile (the tile goes from [0,0] to [8191,8191].

I haven't actually traced the problem from here all the way back to the annotationsInRect issue, but I'm fairly confident this is the root issue.

@mourner, do you have any ideas about how we should fix this? The quick and dirty fix would be to just build a little tolerance into getTileData() and allow duplicates along the tile edges, but it seems like it would be more consistent to transform to some sort of integer-based coordinate system at the point we insert into the SymbolAnnotationTree, so that we don't have to worry about precision issues after that?

@mourner
Copy link
Member

mourner commented Jul 26, 2018

@ChrisLoer yeah, it might make sense to use a global integer coordinates system based on web mercator. Uint32 only allows for values up to z19 (32 - log2(8192)), so this would be need to be Uint64.

ChrisLoer added a commit that referenced this issue Aug 7, 2018
Fixes issue #12472.
This commit doesn't address the underlying issues that come from symbolAnnotationTree using a slightly lower precision coordinate system than the annotations themselves.
Instead, it just puts a small padding around each tile when it queries for tile data, so that symbols right at the tile boundary will be included in both tiles.
The rendering/querying code will take care of only displaying one instance.
The padding is in global coordinates, so at higher zoom the padding will be larger in tile units -- this is consistent with precision loss also being greater at higher zoom.
ChrisLoer added a commit that referenced this issue Aug 8, 2018
Fixes issue #12472.
This commit doesn't address the underlying issues that come from symbolAnnotationTree using a slightly lower precision coordinate system than the annotations themselves.
Instead, it just puts a small padding around each tile when it queries for tile data, so that symbols right at the tile boundary will be included in both tiles.
The rendering/querying code will take care of only displaying one instance.
The padding is in global coordinates, so at higher zoom the padding will be larger in tile units -- this is consistent with precision loss also being greater at higher zoom.
ChrisLoer added a commit that referenced this issue Aug 14, 2018
Fixes issue #12472.
This commit doesn't address the underlying issues that come from symbolAnnotationTree using a slightly lower precision coordinate system than the annotations themselves.
Instead, it just puts a small padding around each tile when it queries for tile data, so that symbols right at the tile boundary will be included in both tiles.
The rendering/querying code will take care of only displaying one instance.
The padding is in global coordinates, so at higher zoom the padding will be larger in tile units -- this is consistent with precision loss also being greater at higher zoom.
ChrisLoer added a commit that referenced this issue Aug 14, 2018
Fixes issue #12472.
This commit doesn't address the underlying issues that come from symbolAnnotationTree using a slightly lower precision coordinate system than the annotations themselves.
Instead, it just puts a small padding around each tile when it queries for tile data, so that symbols right at the tile boundary will be included in both tiles.
The rendering/querying code will take care of only displaying one instance.
The padding is in global coordinates, so at higher zoom the padding will be larger in tile units -- this is consistent with precision loss also being greater at higher zoom.
@ChrisLoer
Copy link
Contributor

Fixed in #12570.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Core The cross-platform C++ core, aka mbgl high priority
Projects
None yet
Development

No branches or pull requests

5 participants