Annotating Null Island also annotates its antipode

[brunoabinader]

If I add an annotation at the coordinates 0°, 0°, an annotation also appears at the coordinates 0°, 180°, but only at z1.

[1ec5]

Actually, more spurious annotations appear at other zoom levels, but on different tiles. Here’s what the same annotation produces at z2:

[friedbunny]

As one would expect, this reproduces on iOS:

[friedbunny]

If you really must draw an annotation at null island, here’s a relatively silly but effective work around:

- (MGLPointAnnotation *)validateCoordinateForAnnotation:(MGLPointAnnotation *)annotation {
    if (annotation.coordinate.latitude == 0 && annotation.coordinate.longitude == 0) {
        annotation.coordinate = CLLocationCoordinate2DMake(0.0000001, 0.0000001);
    }

    return annotation;
}

[brunoabinader]

This happens because the symbol annotation geographic coordinate intersects with all four surrounding tiles, causing them all to call for SymbolAnnotationImpl::updateLayer, which calculates the projected point based on modulo operations that eventually puts the features into position { 0, 0 } of each respective tile. I've fixed that in #6553 by adding an additional clause to the symbol tree query in AnnotationManager::getTileData that prevents tiles in which the projection calculation would be invalid.

[mention-bot]

@brunoabinader, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jfirebaugh, @boundsj and @1ec5 to be potential reviewers.

[1ec5]

Please avoid commandeering an issue opened by someone else by converting it into a PR. The resulting page is confusing for anyone who hasn’t been following the conversation, moreso if for whatever reason the PR gets rejected.

[jfirebaugh]

I'm not sure this is the right fix. Per #5419 and #1673, we actually want the annotation on the border of a tile to appear in all tiles that share that border. So isn't the issue rather that the subsequent projection math places the feature at the wrong tile coordinate for some of the tiles?

[brunoabinader]

Apologies @1ec5 - this was part of a PR creation batch and I should've paid more attention to this not being a recently created issue of mine.

[brunoabinader]

You are correct @jfirebaugh - we were actually doing the wrong coordinate system conversions in SymbolAnnotationImpl::updateLayer - so in fact #5419 is just a different manifestation of this issue 🎉 🎉 🎉

Going from Point<double> in SymbolAnnotation to GeometryCoordinate involves:

1. Convert Point<double> (aka. LonLat) to TileCoordinate via TileCoordinate::fromLatLng
2. Convert Point<double> from TileCoordinate.p to GeometryCoordinate (aka. TilePoint) via TileCoordinate::toGeometryCoordinate

For 2), we require #6626 - thus I'm rebasing this PR against that. We can either merge #6626 first and rebase this against that or just review and merge this and close #6626, I'm fine with either option.

[brunoabinader]

Fixes #5419.

[brunoabinader]

Because geometry tile features are now properly duplicated when they intersect with other tile edges, queryRenderedFeatures would return wrong values. With cbd5a0a we now filter these results by removing features with the same identifier.

[brunoabinader]

Filtering out duplicated annotations affects the results of some query render tests e.g. edge-cases/null-island now outputs 1 feature instead of 4. I believe we shouldn't output duplicated features and thus, update the expectations for these.

[brunoabinader]

Another side-effect: opaque annotations on tile edges are overdrawn:

[brunoabinader]

One solution to the opaque feature overdraw is to always enable symbol bucket clipping:

This potentially fixes #1673 and #5055.

[brunoabinader]

This potentially fixes #1673 and #5055.

Not true for #1673. Features that are not exactly on the tile edges, but close enough are still clipped as you can see below (notice the cross pattern):

[brunoabinader]

i've had an insightful talk with @kkaefer about the possibility to get rid of stencil clipping for symbols. In sum, these are the options we have so far:

Option 1: Get rid of stencil clipping for symbols

• Filter symbol features in which geometries intersects more than one tile, so only one feature gets inside symbol layer
  • Implies disabling stencil clip for symbols so we don't clip features close to tile edges
• Pros:
  • Fixes collision tile rendering
  • Fixes overdrawn features on tile edges
• Cons:
  • Because symbol-avoid-edges depends on the stencil clip, it stops working
  • 5 failing render-tests:
    • symbol-avoid-edges/default
    • symbol-avoid-edges/rotated-false
    • symbol-spacing/line-close
    • symbol-spacing/line-far
    • text-tile-edge-clipping/default

Option 2: Always clip symbol buckets

• This is the default implementation for MapMode::Still
• Pros:
  • All tests pass
  • Fixes collision tile rendering
• Cons:
  • Clipped features on tile edges
  • queryRenderedFeatures returns duplicated features - this is desired (?)
    • See e.g. https://github.com/mapbox/mapbox-gl-test-suite/blob/master/query-tests/edge-cases/null-island/expected.json
  • Depends on stencil clip - which @kkaefer suggested to get rid of in the future

[brunoabinader]

Pinging @jfirebaugh for evaluating the options.

[jfirebaugh]

Pretty sure that for now we want to go with Option 2: Always clip symbol buckets. Getting rid of stencil clipping for symbols is a good long-term goal but there are a number of tricky issues that we will need to solve for that to be viable, and we should fix this bug in the meantime.

[brunoabinader]

One way of fixing the clipped features close to tile edges is increasing the bounds when querying for renderable features within tile bounds - downside is that we'll eventually insert features to render tiles that won't be visible at all due to stencil clipping.

[brunoabinader]

Per conversion with @jfirebaugh, I've decided to split the fixes previously contained in this PR in separate ones:

• [core] Skip duplicated IDs when querying point annotations - [core] Skip duplicated IDs when querying point annotations #6655
• [core] Cleanup SymbolLayout - [core] Cleanup SymbolLayout #6666
• [core] Disable stencil test when rendering collision boxes - [core] Disable stencil test when rendering collision boxes #6669

The commit left for review on this PR fixes the immediate cause for the issue initially mentioned on this issue (now PR).

In order to fix rendering of opaque symbols near tile edges, I'm moving this discussion to a new issue: #6670

[jfirebaugh]

This introduces a TransformState to a bunch of functions in order to satisfy a composition of functions -- essentially TileCoordinate::toGeometryCoordinate(TileCoordinate::fromLatLng(latLng)) -- in which TransformState should "cancel out" and be irrelevant to the result.

Can we instead write a method such as GeometryCoordinate CanonicalTileID::project(const LatLng&) which does the geographic coordinate ⇢ tile coordinate projection directly? (It doesn't necessarily have to be a member of CanonicalTileID -- a standalone function would also be fine.)

[brunoabinader]

Can we instead write a method such as GeometryCoordinate CanonicalTileID::project(const LatLng&) which does the geographic coordinate ⇢ tile coordinate projection directly?

Totally @jfirebaugh - this actually ended up in moving {un,}project() into Projection class - cleaning up a lot of code in between and moving us one step closer into switchable projection types in the future 😉

[jfirebaugh]

Unused.

[1ec5]

This would be nice to get on the new release-ios-v3.4.0 branch before it diverges.