fix recursive dem tiles loading for 404s

[stepankuzmin]

Launch Checklist

This PR fixes recursive dem tiles loading according to #11161

• briefly describe the changes in this PR
• write tests for all new functionality
• manually test the debug page
• apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
• add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>fix recursive dem tiles loading for 404s</changelog>

Before

before-dem-fix.mov

After

after-dem-fix.mov

[stepankuzmin]

Tests are failing because sourceCache doesn't contain some tiles that are available at the terrain lookup path in _findTileCoveringTileID see https://github.com/mapbox/mapbox-gl-js/blob/main/src/terrain/terrain.js#L1339

also, the terrain is wiggling

terrain-wiggling.mov

[ansis]

Other than failing tests, this looks good.

Is there anything we can do further to make this less brittle? If we wanted to refactor and remove the updateForTerrain argument, what would that look like? Or maybe we could just make the argument not optional.

[astojilj]

It is useful to evaluate what happens (what tile requests are made) when displaying sea view. In such case, 404s are used for tiles away from land.

[astojilj]

It is good to evaluate what happens when when raster dem source is not used for terrain, but only for hillshade. userForTerrain property gets evaluated to true here.

Otherwise, when used for terrain, we need to cache tile size for this source supplied (it is scaled up in code below) and if in consecutive update here we end up requesting tile cover with different tile size. This is apparently the issue that existed before but now with sparse dem tile set the behavior is different.

mapbox-gl-js/src/terrain/terrain.js

Line 281 in cd46ea1

this.sourceCache.update(transform, demScale * proxyTileSize, true);

[stepankuzmin]

The test is initiating tiles requests on the 14th zoom level and blocks tiles requests for zoom levels below 12 at the same time.

mapbox-gl-js/test/unit/source/source_cache.test.js

Lines 1784 to 1791 in 5657f9d

	loadTile (tile, callback) {
	if (tile.tileID.canonical.z > 12) {
	setTimeout(() => callback({status: 404}), 0);
	} else {
	tile.state = 'loaded';
	callback(null);
	}
	}

Without the fix, sourceCache.getRenderableIds() doesn't contain any tiles for raster-dem sources that is used for terrain and sourceCache.getIds() only contains tiles for the requested zoom level.

t.deepEqual(sourceCache.getRenderableIds(), []);

t.deepEqual(sourceCache.getIds(), [
    new OverscaledTileID(14, 0, 14, 8192, 8192).key,
    new OverscaledTileID(14, 0, 14, 8191, 8192).key,
    new OverscaledTileID(14, 0, 14, 8192, 8191).key,
    new OverscaledTileID(14, 0, 14, 8191, 8191).key,
]);

After the fix, raster-dem sources used for terrain behave the same way as the other sources, i.e., recursively load parent tiles if current tiles are not found.

t.deepEqual(sourceCache.getRenderableIds(), [
    new OverscaledTileID(12, 0, 12, 2048, 2048).key,
    new OverscaledTileID(12, 0, 12, 2047, 2048).key,
    new OverscaledTileID(12, 0, 12, 2048, 2047).key,
    new OverscaledTileID(12, 0, 12, 2047, 2047).key,
]);

t.deepEqual(sourceCache.getIds(), [
    new OverscaledTileID(12, 0, 12, 2048, 2048).key,
    new OverscaledTileID(12, 0, 12, 2047, 2048).key,
    new OverscaledTileID(12, 0, 12, 2048, 2047).key,
    new OverscaledTileID(12, 0, 12, 2047, 2047).key,
    new OverscaledTileID(13, 0, 13, 4096, 4096).key,
    new OverscaledTileID(13, 0, 13, 4095, 4096).key,
    new OverscaledTileID(13, 0, 13, 4096, 4095).key,
    new OverscaledTileID(13, 0, 13, 4095, 4095).key,
    new OverscaledTileID(14, 0, 14, 8192, 8192).key,
    new OverscaledTileID(14, 0, 14, 8191, 8192).key,
    new OverscaledTileID(14, 0, 14, 8192, 8191).key,
    new OverscaledTileID(14, 0, 14, 8191, 8191).key,
]);

/cc @ansis @astojilj

[stepankuzmin]

There are some failing tests. One is render-tests/free-camera/terrain; it times out after 5000ms on CI but passes on with watch-render. UPDATE: seems that this was a flaky test

The other one is render-tests/debug/terrain/camera-under-terrain. It fails because of the transparent background (sky?) on the first render.

Here is the video of the camera-under-terrain/style.json before and after the fix was applied. I'm not sure if it's significant.

Before

cut-before.mov

After

cut-after.mov

I've updated the expected.png for this test

[astojilj]

undefined for tileSize is a bit problematic as it doesn't verify real case:

mapbox-gl-js/src/terrain/terrain.js

Lines 282 to 283 in 941ba0f

	// As a result of update, we get new set of tiles: reset lookup cache.
	this._findCoveringTileCache[this.sourceCache.id] = {};

I'd suggest to use transform.zoom = 16, tileSize of 4 * 512 and DEM source maxZoom of 10.

[stepankuzmin]

I've created a separate test for the terrain

mapbox-gl-js/test/unit/terrain/terrain.test.js

Lines 1547 to 1605 in 7565093

	test('terrain recursively loads parent tiles on 404', (t) => {
	t.plan(2);
	const style = createStyle();
	const map = createMap(t, {style, center: [0, 0], zoom: 16});
	map.once('style.load', () => {
	map.addSource('mapbox-dem', {
	'type': 'raster-dem',
	'tiles': ['http://example.com/{z}/{x}/{y}.png'],
	'tileSize': TILE_SIZE,
	'maxzoom': 14
	});
	const cache = map.style._getSourceCache('mapbox-dem');
	cache.used = cache._sourceLoaded = true;
	cache._loadTile = (tile, callback) => {
	if (tile.tileID.canonical.z > 10) {
	setTimeout(() => callback({status: 404}), 0);
	} else {
	tile.state = 'loaded';
	callback(null);
	}
	};
	map.setTerrain({'source': 'mapbox-dem'});
	map.once('idle', () => {
	t.deepEqual(cache.getRenderableIds(), [
	new OverscaledTileID(10, 0, 10, 512, 512).key,
	new OverscaledTileID(10, 0, 10, 511, 512).key,
	new OverscaledTileID(10, 0, 10, 512, 511).key,
	new OverscaledTileID(10, 0, 10, 511, 511).key,
	], 'contains first renderable tiles');
	t.deepEqual(cache.getIds(), [
	new OverscaledTileID(10, 0, 10, 512, 512).key,
	new OverscaledTileID(10, 0, 10, 511, 512).key,
	new OverscaledTileID(10, 0, 10, 512, 511).key,
	new OverscaledTileID(10, 0, 10, 511, 511).key,
	new OverscaledTileID(11, 0, 11, 1024, 1024).key,
	new OverscaledTileID(11, 0, 11, 1023, 1024).key,
	new OverscaledTileID(11, 0, 11, 1024, 1023).key,
	new OverscaledTileID(11, 0, 11, 1023, 1023).key,
	new OverscaledTileID(12, 0, 12, 2048, 2048).key,
	new OverscaledTileID(12, 0, 12, 2047, 2048).key,
	new OverscaledTileID(12, 0, 12, 2048, 2047).key,
	new OverscaledTileID(12, 0, 12, 2047, 2047).key,
	new OverscaledTileID(13, 0, 13, 4096, 4096).key,
	new OverscaledTileID(13, 0, 13, 4095, 4096).key,
	new OverscaledTileID(13, 0, 13, 4096, 4095).key,
	new OverscaledTileID(13, 0, 13, 4095, 4095).key,
	new OverscaledTileID(14, 0, 14, 8192, 8192).key,
	new OverscaledTileID(14, 0, 14, 8191, 8192).key,
	new OverscaledTileID(14, 0, 14, 8192, 8191).key,
	new OverscaledTileID(14, 0, 14, 8191, 8191).key,
	], 'recursively loads parent tiles if current tiles not found');
	});
	});

[karimnaaji]

Looks good to me.