Fix level of detail at high pitch #4750
Conversation
…le, updated unit test with large pitch angle to match new behavior
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## globe #4750 +/- ##
===========================================
- Coverage 87.98% 63.01% -24.98%
===========================================
Files 265 265
Lines 37577 37628 +51
Branches 2336 1702 -634
===========================================
- Hits 33064 23713 -9351
- Misses 3482 12966 +9484
+ Partials 1031 949 -82 ☔ View full report in Codecov by Sentry. |
|
I could use some help with the failing render and integration tests. I've looked through the render results and they look good to me, we are loading higher-resolution tiles at high pitch, which is the desired and expected behavior. Example: 
The integration tests rely on specific terrain tiles that are kept in
|
|
Check out the readme in the integration test folder. |
|
There was a concern about performance when the terrain is using a higher zoom, which will create a higher resolution terrain, which will impact performance. |
@HarelM This is how the performance risk is mitigated: 53f42b6. Terrain sources now observe the |
|
Overall, this looks like a good solution as it seems very specific to covering tiles code, which I'm happy to see is in specific files and generally contained. Thanks for all the works on this! Truly appreciated. |
Here's my understanding of the previous implementation: And here's a drawing of how it works now (in crude 3d): 
Here's a description of how it works.
Note that rather than use the true 3D distance to each tile (accounting for terrain), it is assumed that all tiles are at the same elevation as the center point. This prevents "resolution explosion" when the camera is very close to a hill, and keeps the resolution variation from map center smooth. |
|
Thanks! Can you update the initial PR comment with these drawing so that people coming to this PR in the future can get a good grasp of the change fairly fast? |
|
I've looked at some of the "x" terrain render tests and I'm not sure I understand what I see. |
|
Also, there were a lot of osm tiles added, are all of them really needed? Can't we use the "x"s instead, or less tiles? (It's like 50 or so tiles, I hope it's not for a single scene...) |
|
This is a great showcase of which tiles are used in the map! The colors you added makes it even more visible! Great! In any case, looking at the graph you added, I think the DEM is ok, but the raster is too "heavy", looking at the image at 75 degrees I think 19 tiles are not needed and also tiles after the center needs to change "faster" to lower resolution. That's my two cents, let me know what you think. |
|
CC: @louwers note that this change might cause a mismatch between native and web as this changes which zoom level tiles are loaded. If this only affect rendering when terrain is on then this shouldn't be a problem at this point as native doesn't support terrain, but it's worth remembering. |
In the images above, the center point is on a hill. This is the "worst case" for this change. Here's an example of the opposite, center point in a valley (requested zoom level 13.25, which is 14.25 for 256x256 tiles): The cause of this is that the distance to each tile is calculated assuming the tile is at the same elevation as the center point. This means areas lower than the center point will be rendered with "too high" resolution and areas above the center point will be rendered with "too low" resolution. Here's what the "no elevation" case looks like: To fully resolve #4703, the distance to each tile would be calculated using the elevation of each tile. This would complicate the code significantly, as well as making the requested tiles much "jumpier" in response to camera motion. So I decided not to attempt that in this PR. I really like the proposed tile selection scheme, it is very understandable (at least to me :)) and based on the distance from camera to each tile. I could change the relationship between distance and tile selection (to reduce resolution and top and bottom of image as you suggest), but that would require an arbitrary fudge factor that would uglify what currently has a beautiful geometric interpretation :) If I need to change the behavior, can you give me a target for max tiles to load (or ratio to current number of tiles loaded)? |
|
@HarelM How's this look? This is what happens if I don't try to maintain constant tile width on screen. Instead I try to maintain constant tile area on screen. It definitely uses less tiles, but I'm worried it won't solve the original problem.
|
|
How does this interact with styling and corresponding data changes depending on the zoom level? For example we only show small roads or buildings on high zoom levels and also include the data in vector tiles on high zoom levels. So mixing different zoom levels may lead to inconsistencies on the map. This problem already exists now as far as I can tell, but judging from the images I fear this pull requests will increase the mixing of zoomlevels especially close to the camera where the inconsistencies are more noticeable. Is this true? |
|
We were talking about a related issue at https://osmus.slack.com/archives/C01G3D28DAB/p1727331286766569 . Maybe an issue about these concerns should be opened for future reference |
The main effect of this PR is to shift tiles at high pitch angles to higher zoom levels, not to increase the mixing of zoom levels. For example, in the |
|
Maybe we can introduce some sort of parameter to define the behavior of the tiles? Somewhat similar to the exaggeration parameter...? |
|
Can you also better explain the mountain vs valley effect? |
In this PR's implementation, tile selection is independent of terrain. Here's a picture that illustration the mountain/valley effect: It is not the center elevation we need to fix this. We would need the elevation of each tile, in order to calculate how far from the camera it is so we can figure out what zoom level it is. This seems like it would get messy in a hurry (you can't figure out what tiles to request until you know their elevation, but you don't know the elevation until you have the tiles. What elevation is used for a tile? Max? min? mean? ...) |
The behavior at pitch <=60 with no terrain tiles is preserved from before. In this case, as before, all tiles are loaded at the requested zoom level of the center point.
|
Could do. That parameter would have to trickle down to
No, I don't know how to do that. |
Good to know. Also it is good to know that we can sidestep the issue by restricting pitch to 60 degree. That said in the before picture without terrain, I see consistent tiles used for the lower 2/3 of the picture where inconsistencies would be very noticeable. the first border between tiles of different zoom levels is quite far in the distance. In the after picture on the other hand the lowest row uses an additional zoom level with possible inconsistencies very close to the camera. But maybe this is not a problem in the real world if the same zoom for styling is used. after all maps tend to just add data on higher zoom levels and not remove it. so in most cases using zoom 15 tiles with a zoom 14 style will just fetch some features which are not displayed (assuming corresponding minzoom values in the style) and still show a consistent map. |
|
All the configurations options here have something that the user can understand: This is just an idea, if you can think or a different/better way to convey the idea to user that can work too. |
|
@HarelM I added a parameter with the dubious name The reasonable range for 0: As the map pitches, maintain constant tile screen x-resolution at the center. (Loads the most tiles) Values outside [0,2] can also be applied, but I can't think of any reason to do so. Of particular interest is the behavior as pitch crosses 60 degrees, with and without terrain. To match previous behavior, "below 60 degrees, no terrain" is a special case that loads all tiles with the same zoom level. |
|
Why is 60 degrees no terrain a special case? |
"60 degrees no terrain" is a special case because it was already a special case. I don't know why it was already a special case, and I didn't want to change that when I didn't know the reason behind it. I would personally love to eliminate that special case and have a smooth transition across all pitch angles.
Sure. |
|
This was also mistakenly closed due to the globe branch deletion, can you reopen it against main branch? |
Reopened here: #4779 |
This fixes #3983 and improves #4703 by calculating the desired zoom level for each tile independently. Each tile zoom is calculated by scaling the center point zoom by the ratio of (3D) distance-to-tile to distance-to-center. For Mercator transform, the old behavior is preserved when there is no terrain and pitch < 60 degrees. For Globe transform, the new behavior applies in all cases.
Before:
After:
Here's my understanding of the previous implementation:
And here's a drawing of how it works now (in crude 3d):
Here's a description of how it works.
The center point is rendered at the requested zoom level.
the 3D distance from the camera to each candidate tile is calculated and the desired zoom for that tile is the requested zoom scaled by ratio of camera-to-center distance to camera-to-tile distance. If the candidate tile is lower resolution than desired, the tile is split.
Note that rather than use the true 3D distance to each tile (accounting for terrain), it is assumed that all tiles are at the same elevation as the center point. This prevents "resolution explosion" when the camera is very close to a hill, and keeps the resolution variation from map center smooth.