fix antimeridian flicker #6438

ansis · 2018-04-02T15:54:41Z

This fixes several intertwined bugs. The first two commits fix adding and removing tiles by their wrapped tileIDs. This let's us reuse cached tiles for tiles with any wrap value.

The last commit fixes two issues:

First, after a user pans across the antimeridian and the longitude gets wrapped (175, pan to 183 which wraps as -177), the same area of the screen gets covered by a tile with a different wrap value. The commit updates the CrossTileSymbolIndex state so that it uses the symbol values for the nearest version of the tile (what visually you would think is "the same tile").

Second, previously it wasn't possible to reuse a wrapped version of a tile in the next frame (without adding it to the cache). For example, if you had z0/x0/y0/w0 and you set the center to 360 it would load a new tile for z0/x0/y0/w1 instead of reusing it. The right fix here would be to make deep changes to sourcecache so that it can use wrapped tiles from retain. But this gets complicated, especially if you want to prioritize exact matches. I wasn't confident that I could write a bug-free implementation of that for this release (progress on that attempt: 6fc31ec, 7bc39da), so I went with a slightly easier fix: I do the same thing I do for symbols and just update all the tileIDs so that the data matches the nearest tile from the previous frame.

I still need to fix some tests, but otherwise this is ready for review.

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
[n/a] document any changes to public APIs
post benchmark scores
manually test the debug page

ChrisLoer · 2018-04-02T21:02:42Z

ChrisLoer

I've been looking at the new lru_cache behavior and I don't think I understand how the changes are supposed to work. It doesn't seem like the behavior matches the comments. As far as I can tell SourceCache is the only consumer so we may be able to get away with tweaking the behavior arbitrarily -- but if we're calling it LRUCache it should have pretty generic LRU-like behavior.

How does LRUCache#get work -- it pops data but doesn't affect the order? How is the order supposed to be managed in general now? It seems like order could grow indefinitely if each add isn't matched with a remove/getAndRemove (w/o any get calls)?

ChrisLoer · 2018-04-02T21:52:19Z

Umm sorry ignore #6438 (comment), I'm not seeing the changes any more after a full reload, not sure what I had cached or if rollup just hadn't finished rebuilding when I loaded the page... 😳

Looks great! (well there's still the symbol flicker but overall it's a great improvement)

ChrisLoer · 2018-04-02T22:42:01Z

The handleWrapJump approach is starting to make sense to me. I still haven't figured out exactly what to expect the CrossTileSymbolIndex to be able to pick up. It seems like after I change zoom levels, there will be symbol flicker the next time I cross the antimeridian (and then no more after that), but I haven't got my head around what's happening yet:

ChrisLoer · 2018-04-03T00:25:13Z

I think the sign is wrong here: index.tileID = index.tileID.unwrapTo(index.tileID.wrap - wrapDelta);

From logging it looks like what happens is we wrap in the wrong direction the first time after changing zoom levels, ending up with two tiles, and after that it doesn't matter which direction you change the wrap until that part of the symbol index gets cleared out:

place-city-md-n: Shifting tile 4/0/4 wrap 0 by 1 <flicker happens>
place-city-md-n: Shifting tile 4/0/4 wrap -1 by -1
place-city-md-n: Shifting tile 4/0/4 wrap 1 by -1
place-city-md-n: Shifting tile 4/0/4 wrap 0 by 1
place-city-md-n: Shifting tile 4/0/4 wrap 2 by 1

Changing to (index.tileID.wrap + wrapDelta) fixes the problem as far as I can tell. And although it looked totally normal to me at first, adding the delta makes more sense and is the same thing we do in SourceCache, right?

anandthakker · 2018-04-03T23:44:28Z

src/source/source_cache.js

@@ -43,6 +43,7 @@ class SourceCache extends Evented {
    _sourceLoaded: boolean;
    _sourceErrored: boolean;
    _tiles: {[any]: Tile};
+    _prevLng: number;


number | void

anandthakker · 2018-04-04T00:40:38Z

src/source/source_cache.js

+        // calculates this difference in wrap values so that we can match tiles
+        // across frames where the longitude gets wrapped.
+        const prevLng = this._prevLng === undefined ? lng : this._prevLng;
+        const wrapDelta = Math.round((lng - prevLng) / 360);


Why is wrapDelta = Math.round((lng - prevLng) / 360)) correct? This is one of those formulas that I find superficially indistinguishable from, e.g., Math.round((lng - prevLng) / 180)), Math.floor((lng - prevLng) / 360)), Math.floor(lng - prevLng) % 360, etc., at least until I get out some scrap paper and do a little scribbling. Is there any way to make this a bit more self-documenting?

Would this be clearer?

const lngDifference = lng - prevLng; const worldDifference = lngDifference / 360; const wrapDelta = Math.round(worldDifference);

👍 worldDifference definitely helps, but it's still not obvious to me why wrapDelta is 1 for 0.5 <= worldDifference < 1.5 rather than for, say, 1 <= worldDifference < 2.

Btw, I recently learned that in JS, Math.round(-1.5) is -1, not -2 as you might expect from other languages.

wrapDelta is for 0.5 <= worldDifference < 1.5 because it is matching with the nearest frame and the sign of the distance to the nearest frame doesn't matter. If the map goes from lng: 270 to lng: 10 then the former z/x/y/1 tile is closer to the current area of the z/x/y/0 tile than the current z/x/y/1 tile.

Any ideas on how I could clarify that well with a code or comment change?

Now that I'm seeing the expanded comment you added, together with these var names, I think it's pretty clear 🙇

anandthakker · 2018-04-04T00:46:36Z

src/source/source_cache.js

@@ -392,6 +393,27 @@ class SourceCache extends Evented {
        this._cache.setMaxSize(maxSize);
    }

+    handleWrapJump(lng: number) {
+        // Wrapping longitude values can cause a jump in `wrap` values. Tiles that
+        // cover a similar area of the screen can have different wrap values. This


Tiles that cover a similar area of the screen can have different wrap values

Is this about the different wrap values of adjacent tiles, like (1,0,1) wrap=-1 and (0,0,1) wrap=0, or about the different wrap values of the same tile in two successive frames, like (0,0,1) wrap=0 => (0,0,1) wrap=-1? The latter seems like the relevant thing here, but the sentence sounds more like it's referring to the former.

anandthakker · 2018-04-04T01:06:13Z

src/source/source_cache.js

        if (tile) {
+            // set the tileID because the cached tile could have had a different wrap value


Attach comment to the line where tile.tileID is being reassigned.

anandthakker · 2018-04-04T01:07:07Z

src/source/source_cache.js

+            const tile = this._tiles[id];
+            this._setTileReloadTimer(id, tile);
+        }
+    }


I'd be inclined to inline this since it's only used in handleWrapJump

ansis · 2018-04-13T16:05:28Z

@anandthakker 132bfd7 fixes things based on your review. Can you check if the comment/calculation changes are clearer now?

@ChrisLoer thanks for catching the wrapDelta bug. Fixed in 913f891

How does LRUCache#get work -- it pops data but doesn't affect the order? How is the order supposed to be managed in general now? It seems like order could grow indefinitely if each add isn't matched with a remove/getAndRemove (w/o any get calls)?

6ffb4d8 fixes the bug of get popping the data. After a quick look it seems like it might be possible to eliminate get completely with a bit of SourceCache changes but that might be best left for a separate pr.

Are there any other changes I should make?

ChrisLoer

I think this is looking pretty good, and we probably want to get it in soon. I still have two things on my wish list:

Rename LRUCache and change comments to make it clearly a subordinate of SourceCache's use case.
Add unit tests for CrossTileSymbolIndex and SourceCache that exercise handleWrapJump

anandthakker

👍 concur w/ @ChrisLoer

ansis · 2018-04-16T19:52:06Z

@ChrisLoer ee32a0b turns LRUCache into TileCache. A careful review of this would be great.

4297d51 adds tests for handleWrapJump

ChrisLoer · 2018-04-16T20:37:10Z

@ChrisLoer ee32a0b turns LRUCache into TileCache. A careful review of this would be great.

Changes look good! A few questions:

Is there any reason for TileCache to still have its value type templated? Since we know it's going to hold Tiles, we might as well hardwire that in and make it a little easier to read.
TileCache#keys doesn't seem to have any callers? And it has a surprising implementation since it can return the same key multiple times. We can probably just delete it, right?
Should we care that cache size is limited by the length of this._order which isn't technically the number of tiles held by the cache? The way we use TileCache from SourceCache should guarantee that there's at most one entry per unwrapped tile ID, which I think mostly mitigates this concern. And then the handleWrapJump behavior should I think keep us from having too many unwrapped versions of the same wrapped tile ID. The thought experiment I'm trying to play through in my head is: what happens if you zoom out to z0 and pan aggressively through several wraps -- is it possible you'd clear the cache of all but a couple tiles?

ansis · 2018-04-16T20:58:03Z

Is there any reason for TileCache to still have its value type templated?

Nope. ab2e59c

TileCache#keys doesn't seem to have any callers? And it has a surprising implementation since it can return the same key multiple times. We can probably just delete it, right?

Deleted in ab2e59c. It used to be used by the unit tests but I had changed them to just use order directly.

Should we care that cache size is limited by the length of this._order which isn't technically the number of tiles held by the cache?

It is number of tiles held by the cache. It's just not the number of unique tiles held by the cache. The number of unique tiles would be lower. This is what you were saying, right? I think this seems fine.

The thought experiment I'm trying to play through in my head is: what happens if you zoom out to z0 and pan aggressively through several wraps -- is it possible you'd clear the cache of all but a couple tiles?

I think you're right that the way we use SourceCache makes this ok. I don't think you'd clear the cache of all but a couple of tiles, because:

a new tile gets loaded only if the cache contains no tile with the same wrapped id
the maximum number of tiles with the same wrapped id across both the cache and _tiles is the same as the maximum number of tiles with the same wrapped id that have been visible in the viewport at the same time
if all the tiles were visible at the same time, then caching them all is a reasonable thing to do

If you have a really wide viewport it would be possible to fill the entire cache with just 0/0/0/w tiles, but I think it's an unlikely situation and valid behaviour for that situation.

ChrisLoer · 2018-04-16T21:43:17Z

If you have a really wide viewport it would be possible to fill the entire cache with just 0/0/0/w tiles, but I think it's an unlikely situation and valid behaviour for that situation.

Yup, I'm sold, thanks for helping me walk through that reasoning.

🚀 This all looks good to me.

anandthakker · 2018-04-18T15:00:13Z

Note that this should be re-targeted at master

When panning cross the antimeridian, the longitude value gets wrapped. This results in tileIDs getting assigned a different `wrap` value for tiles that cover roughly the same area on the screen. This pr calculates what this change in wrap values is and updates the state of both `SourceCache` and `CrossTileSymbolIndex` so that areas use the same tile and symbol state for the same screen areas even if they have a different `wrap` value. I think this is the long term fix for the CrossTileSymbolIndex. For SourceCache, it may be better to rework how tiles are retained so that you can actually use versions of tiles with a different wrap as tiles in the next frame.

LRUCache was only being used for tiles and recently we started adding some non-standard behaviour including storing multiple values for a single key. This pr: - replaces `number` key with `OverscaledTileID` so that we can enforce the type and enforce tile id wrapping - moves expiry logic into cache and gets rid of `cacheTimers` in `SourceCache`

zhsisusie · 2018-11-12T06:51:10Z

smartWrap this function in mapbox-gl source code ,can solve the problem, when renderCopies is true .
`function smartWrap(lngLat, priorPos, transform) {
lngLat = new LngLat(lngLat.lng, lngLat.lat);

// First, try shifting one world in either direction, and see if either is closer to the
// prior position. This preserves object constancy when the map center is auto-wrapped
// during animations.
if (priorPos) {
var left = new LngLat(lngLat.lng - 360, lngLat.lat);
var right = new LngLat(lngLat.lng + 360, lngLat.lat);
var delta = transform.locationPoint(lngLat).distSqr(priorPos);
if (transform.locationPoint(left).distSqr(priorPos) < delta) {
lngLat = left;
} else if (transform.locationPoint(right).distSqr(priorPos) < delta) {
lngLat = right;
}
}

// Second, wrap toward the center until the new position is on screen, or we can't get
// any closer.
while (Math.abs(lngLat.lng - transform.center.lng) > 180) {
var pos = transform.locationPoint(lngLat);
if (
pos.x >= 0 &&
pos.y >= 0 &&
pos.x <= transform.width &&
pos.y <= transform.height
) {
break;
}
if (lngLat.lng > transform.center.lng) {
lngLat.lng -= 360;
} else {
lngLat.lng += 360;
}
}

return lngLat;
}`

ansis requested review from ChrisLoer and anandthakker April 2, 2018 15:54

ansis changed the base branch from master to release-0.45 April 2, 2018 16:41

ansis force-pushed the fix-antimeridian-flicker branch from 75adcce to ff393e1 Compare April 2, 2018 17:25

ChrisLoer suggested changes Apr 2, 2018

View reviewed changes

anandthakker reviewed Apr 4, 2018

View reviewed changes

mollymerp added the release blocker ⛔ label Apr 9, 2018

ChrisLoer approved these changes Apr 13, 2018

View reviewed changes

anandthakker approved these changes Apr 13, 2018

View reviewed changes

ansis added 4 commits April 18, 2018 11:34

correctly set tileID wrap when adding a tile from cache

db872f7

lru_cache: support multiple values with the same key

f0b122b

ansis force-pushed the fix-antimeridian-flicker branch from ab2e59c to 5882bfe Compare April 18, 2018 15:34

ansis changed the base branch from release-0.45 to master April 18, 2018 15:34

ansis merged commit a4eed2f into master Apr 18, 2018

ansis deleted the fix-antimeridian-flicker branch April 18, 2018 15:46

ChrisLoer mentioned this pull request May 10, 2018

Symbols flicker on crossing antimeridian mapbox/mapbox-gl-native#11878

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix antimeridian flicker #6438

fix antimeridian flicker #6438

ansis commented Apr 2, 2018

ChrisLoer commented Apr 2, 2018 •

edited

Loading

ChrisLoer left a comment

ChrisLoer commented Apr 2, 2018

ChrisLoer commented Apr 2, 2018

ChrisLoer commented Apr 3, 2018

anandthakker Apr 3, 2018

anandthakker Apr 4, 2018

ansis Apr 13, 2018

anandthakker Apr 13, 2018

anandthakker Apr 13, 2018

ansis Apr 13, 2018

anandthakker Apr 13, 2018

anandthakker Apr 4, 2018

anandthakker Apr 4, 2018

anandthakker Apr 4, 2018

ansis commented Apr 13, 2018

ChrisLoer left a comment

anandthakker left a comment

ansis commented Apr 16, 2018

ChrisLoer commented Apr 16, 2018

ansis commented Apr 16, 2018

ChrisLoer commented Apr 16, 2018

anandthakker commented Apr 18, 2018

zhsisusie commented Nov 12, 2018

		if (tile) {
		// set the tileID because the cached tile could have had a different wrap value

fix antimeridian flicker #6438

fix antimeridian flicker #6438

Conversation

ansis commented Apr 2, 2018

Launch Checklist

ChrisLoer commented Apr 2, 2018 • edited Loading

ChrisLoer left a comment

Choose a reason for hiding this comment

ChrisLoer commented Apr 2, 2018

ChrisLoer commented Apr 2, 2018

ChrisLoer commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansis commented Apr 13, 2018

ChrisLoer left a comment

Choose a reason for hiding this comment

anandthakker left a comment

Choose a reason for hiding this comment

ansis commented Apr 16, 2018

ChrisLoer commented Apr 16, 2018

ansis commented Apr 16, 2018

ChrisLoer commented Apr 16, 2018

anandthakker commented Apr 18, 2018

zhsisusie commented Nov 12, 2018

ChrisLoer commented Apr 2, 2018 •

edited

Loading