maplibre / maplibre-gl-js

MapLibre GL JS - Interactive vector tile maps in the browser
https://maplibre.org/maplibre-gl-js/docs/
Other
6.3k stars 689 forks source link

Raster tiles are much lower detail when terrain is enabled #3983

Open olsen232 opened 5 months ago

olsen232 commented 5 months ago

(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

Screenshot 2024-04-12 at 2 19 50 PM

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

Screenshot 2024-04-12 at 2 20 01 PM

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

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 commented 5 months ago

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

without terrain: grafik

with terrain: grafik

UberMouse commented 4 months ago

Any updates on this @prozessor13 ?

notnotzero commented 4 months ago

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

HarelM commented 4 months ago

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 commented 4 months ago

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: https://github.com/maplibre/maplibre-gl-js/blob/db7e027e5aefedde6e7c614a2e52ae51a3fc8796/src/source/source_cache.ts#L600-L635

olsen232 commented 4 months ago

If I can save some time - the PR I linked https://github.com/maplibre/maplibre-gl-js/pull/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 commented 4 months ago

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:

https://github.com/maplibre/maplibre-gl-js/blob/db7e027e5aefedde6e7c614a2e52ae51a3fc8796/src/source/source_cache.ts#L600-L635

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 commented 4 months ago

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.

IMG_3948

HarelM commented 4 months ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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.