Raster tiles are much lower detail when terrain is enabled

[olsen232]

(This may not be a bug - if that's the case, I am just looking for info about how to control this behaviour).

Adding terrain to a map viewer causes it to load raster tiles at lower zoom, that is, load the lower detail rasters with a lower "z" value. Also, the zoom drops away much quicker when terrain is enabled, so that one end of the screen is noticeably lower detail than the other even for a reasonably small camera pitch, and worse for extreme pitches. Maps without terrain, by contrast, do a good job of maintaining the illusion that all tiles change zoom at once.

maplibre-gl-js version: 4.1.2

browser: Brave

Steps to Trigger Behavior

1. Set up a map with raster tiles. Turn on map.showTileBoundaries
2. Compare it with the same map but also with 3d terrain enabled.
3. Observe different tile zoom levels in each case.

Link to Demonstration

Map with terrain disabled:
https://jsbin.com/mesowukici/1/edit?html,output

Identical map with terrain enabled:
https://jsbin.com/rigisawuze/1/edit?html,output

Expected Behavior

Adding terrain shouldn't affect raster tile zoom - maybe?
This may be by design for performance reasons. If so, do you know if it can be tweaked?

Actual Behavior

Adding terrain has a large and obvious impact on raster tile zoom.

[HarelM]

Yes, this was by design, I don't remember exactly why, probably related to render to texture performance.
I'm not familiar with a way to tweak it.
Cc: @prozessor13

[olsen232]

FYI I found the most visible difference between the two is in this line in transform.ts:
const refPoint = options.terrain ? cameraPoint : centerPoint;

When that's switched to always be centerPoint, as in this draft PR, the raster tiles are much higher detail.

I'm suspicious of whether this is a performance change - there's no comments, so I can only guess, but it doesn't look anything like "if terrain is on then reduce detail by x%" - instead it is "if terrain is on then use a different center-point for the following calculation". That could still be for performance reasons but it's not obvious to me that it is

[HarelM]

I do see this change as part of the terrain 3D change here:

• https://github.com/maplibre/maplibre-gl-js/pull/1022/files#diff-f94e1d5f322cb54e1bcbdbce239d76dbb71a467f7389eb966684fdf652f97935

From what I remember in the discussions around the terrain, this change is towards performance, to allow loading less details for the terrain mesh, making the terrain smoother.

I don't know why center point is the reference point.
I hope @prozessor13 remembers.

[wiesehahn]

I have the same problem. It makes the vizualization with terrain unusable in my case.

without terrain:

with terrain:

[UberMouse]

Any updates on this @prozessor13 ?

[notnotzero]

I have also encountered this issue; we are unable to use high-resolution tiles for our applications.

[HarelM]

I think @msbarry is looking into improving that by splitting the logic of which tiles to fetch according to the source cache and underlying source as part of the contour source implementation.

[msbarry]

I am looking into some weird behavior in deciding which tiles to load - but it should only affect crosstalk when a raster-dem source is shared between hillshading and terrain. This issue seems to be on any source when terrain is enabled. This issue might be related to this code block which is the only thing in source_cache that affects other sources when terrain is present:

maplibre-gl-js/src/source/source_cache.ts

Lines 600 to 635 in db7e027

	if (terrain) {
	const idealRasterTileIDs: {[_: string]: OverscaledTileID} = {};
	const missingTileIDs: {[_: string]: OverscaledTileID} = {};
	for (const tileID of idealTileIDs) {
	if (this._tiles[tileID.key].hasData())
	idealRasterTileIDs[tileID.key] = tileID;
	else
	missingTileIDs[tileID.key] = tileID;
	}
	// search for a complete set of children for each missing tile
	for (const key in missingTileIDs) {
	const children = missingTileIDs[key].children(this._source.maxzoom);
	if (this._tiles[children[0].key] && this._tiles[children[1].key] && this._tiles[children[2].key] && this._tiles[children[3].key]) {
	idealRasterTileIDs[children[0].key] = retain[children[0].key] = children[0];
	idealRasterTileIDs[children[1].key] = retain[children[1].key] = children[1];
	idealRasterTileIDs[children[2].key] = retain[children[2].key] = children[2];
	idealRasterTileIDs[children[3].key] = retain[children[3].key] = children[3];
	delete missingTileIDs[key];
	}
	}
	// search for parent for each missing tile
	for (const key in missingTileIDs) {
	const parent = this.findLoadedParent(missingTileIDs[key], this._source.minzoom);
	if (parent) {
	idealRasterTileIDs[parent.tileID.key] = retain[parent.tileID.key] = parent.tileID;
	// remove idealTiles which would be rendered twice
	for (const key in idealRasterTileIDs) {
	if (idealRasterTileIDs[key].isChildOf(parent.tileID)) delete idealRasterTileIDs[key];
	}
	}
	}
	// cover all tiles which are not needed
	for (const key in this._tiles) {
	if (!idealRasterTileIDs[key]) this._coveredTiles[key] = true;
	}
	}

[olsen232]

If I can save some time - the PR I linked #3994 fixes this behaviour, by making the tiles loading logic when terrain is enabled more similar to when the logic when it is disabled. Since the logic is currently switched from one behaviour to the other depending on whether terrain is enabled or not, and switching the logic back fixes it, I don't think we need to investigate any crosstalk between layers etc (or if so, that's a separate issue).

Anyway, all this to say that there is already a fix. Of course, if that entire piece of code is being rewritten anyway, that's fine too.

This is basically blocked on anyone (@prozessor13?) being able to explain what the purpose for the two different logics is (possibly performance?), or alternatively waiting until someone signs off on switching it so that it always uses the apparently superior logic, even if we don't understand what the purpose was previously.

[prozessor13]

I am looking into some weird behavior in deciding which tiles to load - but it should only affect crosstalk when a raster-dem source is shared between hillshading and terrain. This issue seems to be on any source when terrain is enabled. This issue might be related to this code block which is the only thing in source_cache that affects other sources when terrain is present:

maplibre-gl-js/src/source/source_cache.ts

Lines 600 to 635 in db7e027

	if (terrain) {
	const idealRasterTileIDs: {[_: string]: OverscaledTileID} = {};
	const missingTileIDs: {[_: string]: OverscaledTileID} = {};
	for (const tileID of idealTileIDs) {
	if (this._tiles[tileID.key].hasData())
	idealRasterTileIDs[tileID.key] = tileID;
	else
	missingTileIDs[tileID.key] = tileID;
	}
	// search for a complete set of children for each missing tile
	for (const key in missingTileIDs) {
	const children = missingTileIDs[key].children(this._source.maxzoom);
	if (this._tiles[children[0].key] && this._tiles[children[1].key] && this._tiles[children[2].key] && this._tiles[children[3].key]) {
	idealRasterTileIDs[children[0].key] = retain[children[0].key] = children[0];
	idealRasterTileIDs[children[1].key] = retain[children[1].key] = children[1];
	idealRasterTileIDs[children[2].key] = retain[children[2].key] = children[2];
	idealRasterTileIDs[children[3].key] = retain[children[3].key] = children[3];
	delete missingTileIDs[key];
	}
	}
	// search for parent for each missing tile
	for (const key in missingTileIDs) {
	const parent = this.findLoadedParent(missingTileIDs[key], this._source.minzoom);
	if (parent) {
	idealRasterTileIDs[parent.tileID.key] = retain[parent.tileID.key] = parent.tileID;
	// remove idealTiles which would be rendered twice
	for (const key in idealRasterTileIDs) {
	if (idealRasterTileIDs[key].isChildOf(parent.tileID)) delete idealRasterTileIDs[key];
	}
	}
	}
	// cover all tiles which are not needed
	for (const key in this._tiles) {
	if (!idealRasterTileIDs[key]) this._coveredTiles[key] = true;
	}
	}

Because tiles in terrain renders to a texture, and because the textures are cached, this logic is needed to not render raster tiles of different zoomlevels (because of race-conditions during tile-loading) into on render-to-texture tile.

[prozessor13]

Yes, it was some kind of performance decision, but either it is too agressive, or there is a bug for rastertiles. The Problem is the following scenario:

You are in heavy terrain, and you tilt the camera from Pos A to Pos B than you are behind a mountain. The distance to the centerPoint decreases radically and you have to render tiles in very high zoomlevel, but you still have a very wide view behind the mountain, because of camera altitude. So because there is no visibility check of the tiles behind the mountain a ton of tiles will be loaded. I cannot remember on all the details, but i think changing this is an issue to test very well.

[HarelM]

@prozessor13 thanks for the info!
I think the best way forward would be to create a test that recreates this scenario and see which tiles are loaded in each case.
It might be that improving the logic of which tiles to fetch can solve this issue as well, IDK...

[UberMouse]

Anything that can get this moving forward? I would love to drop our fork of maplibre which exists solely to change this one line of code. Even just adding an option to change this would be fine.

[HarelM]

As far as I understand, this is not the right solution when it comes to terrain performance.
Introducing a flag will force us to do a breaking change to remove it when this is fixed properly.
I believe the solution should be tile loading and caching based on a strategy related to the source type. This is obviously not a one line of code fix.
You can help push this forward by implementing this tile loading strategy.