[core] Fix shape annotations disappearing after maxZoom-1

[brunoabinader]

Fixes #5138.

👀 @kkaefer @jfirebaugh @1ec5

[zugaldia]

Adding to 4.1 milestone as a potential cherrypick before release.

[1ec5]

👍

[mb12]

@brunoabinader Is it possible to explain why it fixes the problem? 1 <<max zoom is equivalent to pow (2, maxzoom)? Was it 32 bit overflow? Is there a compiler option that could have caught this. On java side Android studio shows a warning for code like this.

[1ec5]

It was indeed overflow. Clang's static analyzer should catch issues like this. Unfortunately, we're unable to run the analyzer in CI due to the time it takes, so we have to manually run it periodically to catch these issues.

[brunoabinader]

@mb12 you are right - there is a certain amount of flakiness when testing zooming in between zoom 19 and 20 that I'm still getting: until z19 it works fine, between z19 and z20 it disappears and exactly on z20 it works again.

[1ec5]

Yes, that same flakiness is in master too.

[jfirebaugh]

Here's a similar case. Can this overflow?

[brunoabinader]

@jfirebaugh yes - see the example below:

#include <cstdio>
#include <cstdint>

int main() {
    int32_t x = 0;
    x = 1 << 32;
    printf("%d\n", x);
    return 0;
}

In this case, GCC was able to catch the overflow:

test.cpp: In function ‘int main()’:
test.cpp:6:14: warning: left shift count >= width of type [-Wshift-count-overflow]
     x = 1 << 32;
              ^

Interestingly, if we switch that value to 31, we get -2147483648 - meaning that the number overflowed - and the compiler did not warned us about it. This happens even on unsigned uint32_t type - because it stores up to (2^32 - 1).

However, concerning our code I believe 1) we shouldn't be using signed types for values like this and 2) uint32_t should be more than enough, as our maximum zoom is usually around 20, 22 tops.

[zugaldia]

As discussed with @tobrun, this won't make it to android 4.1.0, removing milestone tag.

[mattgraham1]

Any idea what release this fix will be publish to?

[kkaefer]

we could just use 1ull << maxZoom

[brunoabinader]

I realized that the issue disappears once AnnotationSource uses the same zoom range to calculate the idealZoom as VectorSource e.g. 0..16 (current is 0..22). This changed in 755cc80 - @jfirebaugh we could solve this by having Style to calculate its own aggregate zoom range when adding sources and passing that information when updating sources in Source::update - does that looks feasible?

[jfirebaugh]

755cc80 did not change the maximum zoom behavior for AnnotationSource. Prior to that commit, it used the default value from Tileset, which is also 22.

That said, I'm not aware of any reason why the maximum zoom needs to be 22, though I seem to recall some discussion of the appropriate maximum zoom for GeoJSON sources, which are closely related. Currently they use the default value of 18 from geojsonvt, which has changed in the past. @mourner do you remember the considerations there?

[tmpsantos]

Neat and thanks for adding more tests.

[tmpsantos]

Why is this not clamped at util::MAX_ZOOM? (I know it wasn't before, just curious)

[brunoabinader]

See my comment below and also #4272.

[brunoabinader]

Given that GL JS also uses 20 as default max zoom, and that ​_breaks_​ line-width/very-overscaled render test (not actually wrong because the test assumed that the max zoom would be clamped to 0..20 range), I'm going to postpone setting the map max zoom to 22.

[mourner]

Currently they use the default value of 18 from geojsonvt, which has changed in the past. @mourner do you remember the considerations there?

It was initially 14 and this was too low. 18 is probably okayish for most cases but I don't see any reason this couldn't go even further — it doesn't affect performance much, but increases detalization.

[brunoabinader]

Tested on both iOS and Android test applications @ master.

[mb12]

@brunoabinader Wouldn't this result in inaccurate placement/positioning of marker at zoom level > 18?

The following will not work.

• Add a marker using double precision latitude, longitude (precision high enough for zoom 22).
• With zoom level 18, wouldn't there be an error in placement of the marker at zoom level 22.

Since the error increases by a power of 2 in both x and y with every zoom level increase, wouldn't this error be unacceptably high at zoom 22?

[brunoabinader]

@mb12 yes, that requires further investigation - we're going to address this in #5505.

[1ec5]

This caused a regression in world wrapping: #5648.

[brunoabinader]

This caused a regression in world wrapping: #5648.

Fixed in #5648.

[1ec5]

This was cherry-picked to the release-ios-v3.3.0 branch in 2a0e296 and noted in the changelog in 223e3b3.

[mourner]

Since the error increases by a power of 2 in both x and y with every zoom level increase, wouldn't this error be unacceptably high at zoom 22?

Each tile has a 8192 extent; if a 512px z18 tile is zoomed to z22, this gives exactly 8192 / (2^4) = 512 pixel precision. So there will be no precision loss for marker position.

Also note that maximum possible error due to rounding of a z18 tile to a 8192 grid is (40075160 / (Math.pow(2, 18) * 8192)) / 2 = 0.009, or roughly 1cm. I think this is enough for most mapping needs.