Runaway tile downloads, symbol placement when map is tilted with top padding #15163
Comments
1ec5
added
performance
|
There are two potential fixes for this issue:
|
As it is about high memory consumption, adding reference to mapbox/mapbox-gl-js#1898 and #15022 - there we have memory issues with no pitch and it is expected to have the same with pitch. |
Ah okay, sounds good. Let's try to address those issues first then, in the next release. |
That’s certainly part of it, but the issue is really about wasteful tile loading, which affects not only memory usage but also time-to-render, energy usage, and bandwidth consumption. The memory-related fixes will only go so far to mitigate the underlying issue, which is that the developer can implicitly control the camera’s elevation (mapbox/mapbox-gl-js#3552) without the limits that we’d impose if we implemented that feature explicitly. |
Yes. Working on that first. |
…rix. As we don't want to show horizon, there is a need to limit max pitch based on edge insets. The limit shouldn't be that bad, as to e.g. have 60 deg pitch, top - bottom insets should be less than 0.732 of screen height. TransformState::getProjMatrix calculation of farZ was an aproximation. Replacing it with simpler, but precise calculation. Related to: #15163
TileCoverPitchedViewport modified to have pitch capped by targe top inset, returning 28098 tiles in zoom level 8. TileCover.PitchOverAllowedByContentInsets test verifies pitch capping by large top inset. Expectation was calculated using previous tileCover algorithm implementation. Related to: #15163
@1ec5 , to further clarify what I'm working on in #15195, both are required: even with adaptive tile loading, maximum pitch for given edge insets shouldn't allow displaying area above horizon. Max pith values will likely be higher than the current 60 deg, but then it depends on edge insets. |
TileCover: Replaced 4 scanSpans by 3. As the split lines (triangle, trapezoid, triangle) is horizontal, there is no need to handle duplicates.
Benchmarks (release build) on MacBookPro11,2 (Mid 2014) with Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz compared against src/mbgl/util/tile_cover.cpp from master and from the patch:
```
master | patch
---------------------
TileCoverPitchedViewport 72000ns | 50300ns
TileCoverBounds 1620ns | 1400ns
```
TileCoverPitchedViewport modified to have pitch capped by targe top inset, returning 1124 tiles at zoom level 8.
TileCover.PitchOverAllowedByContentInsets test verifies pitch capping by large top inset. Expectation was calculated using previous tileCover algorithm implementation.
Related to: #15163
…ProjMatrix. Patch partly fixes #15163 in a way that it doesn't allow loading tens of thousands of tiles and attempt to show area above horizon: Limit pitch based on edge insets. It is not too bad - current limit of 60 degrees stays active until center of perspective is moved towards the bottom, to 84% of screen height. The plan is to split removal of 60 degrees limit to follow up patch. Fix max Z calculation in getProjMatrix. TransformState::getProjMatrix calculation of farZ was complex with possibility to lead to negative z values. Replacing it with simpler, precise calculation: furthestDistance = cameraToCenterDistance / (1 - tanFovAboveCenter * std::tan(getPitch())); TransformState::getProjMatrix calculation of farZ was an aproximation. Replacing it with simpler, but precise calculation. Related to: #15163
TileCover: Replaced 4 scanSpans by 3. As the split lines (triangle, trapezoid, triangle) is horizontal, there is no need to handle duplicates.
Benchmarks (release build) on MacBookPro11,2 (Mid 2014) with Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz compared against src/mbgl/util/tile_cover.cpp from master and from the patch:
```
master | patch
---------------------
TileCoverPitchedViewport 72000ns | 50300ns
TileCoverBounds 1620ns | 1400ns
```
TileCoverPitchedViewport modified to have pitch capped by targe top inset, returning 1124 tiles at zoom level 8.
TileCover.PitchOverAllowedByContentInsets test verifies pitch capping by large top inset. Expectation was calculated using previous tileCover algorithm implementation.
Related to: #15163
…ProjMatrix. Patch partly fixes #15163 in a way that it doesn't allow loading tens of thousands of tiles and attempt to show area above horizon: Limit pitch based on edge insets. It is not too bad - current limit of 60 degrees stays active until center of perspective is moved towards the bottom, to 84% of screen height. The plan is to split removal of 60 degrees limit to follow up patch. Fix max Z calculation in getProjMatrix. TransformState::getProjMatrix calculation of farZ was complex with possibility to lead to negative z values. Replacing it with simpler, precise calculation: furthestDistance = cameraToCenterDistance / (1 - tanFovAboveCenter * std::tan(getPitch())); TransformState::getProjMatrix calculation of farZ was an aproximation. Replacing it with simpler, but precise calculation. Related to: #15163
…ProjMatrix. Patch partly fixes #15163 in a way that it doesn't allow loading tens of thousands of tiles and attempt to show area above horizon: Limit pitch based on edge insets. It is not too bad - current limit of 60 degrees stays active until center of perspective is moved towards the bottom, to 84% of screen height. The plan is to split removal of 60 degrees limit to follow up patch. Fix max Z calculation in getProjMatrix. TransformState::getProjMatrix calculation of farZ was complex with possibility to lead to negative z values. Replacing it with simpler, precise calculation: furthestDistance = cameraToCenterDistance / (1 - tanFovAboveCenter * std::tan(getPitch())); TransformState::getProjMatrix calculation of farZ was an aproximation. Replacing it with simpler, but precise calculation. Related to: #15163
…ProjMatrix. Patch partly fixes #15163 in a way that it doesn't allow loading tens of thousands of tiles and attempt to show area above horizon: Limit pitch based on edge insets. It is not too bad - current limit of 60 degrees stays active until center of perspective is moved towards the bottom, to 84% of screen height. The plan is to split removal of 60 degrees limit to follow up patch. Fix max Z calculation in getProjMatrix. TransformState::getProjMatrix calculation of farZ was complex with possibility to lead to negative z values. Replacing it with simpler, precise calculation: furthestDistance = cameraToCenterDistance / (1 - tanFovAboveCenter * std::tan(getPitch())); TransformState::getProjMatrix calculation of farZ was an aproximation. Replacing it with simpler, but precise calculation. Related to: #15163
…ProjMatrix. Patch partly fixes #15163 in a way that it doesn't allow loading tens of thousands of tiles and attempt to show area above horizon: Limit pitch based on edge insets. It is not too bad - current limit of 60 degrees stays active until center of perspective is moved towards the bottom, to 84% of screen height. The plan is to split removal of 60 degrees limit to follow up patch. Fix max Z calculation in getProjMatrix. TransformState::getProjMatrix calculation of farZ was complex with possibility to lead to negative z values. Replacing it with simpler, precise calculation: furthestDistance = cameraToCenterDistance / (1 - tanFovAboveCenter * std::tan(getPitch())); TransformState::getProjMatrix calculation of farZ was an aproximation. Replacing it with simpler, but precise calculation. Related to: #15163
…ProjMatrix. Patch partly fixes #15163 in a way that it doesn't allow loading tens of thousands of tiles and attempt to show area above horizon: Limit pitch based on edge insets. It is not too bad - current limit of 60 degrees stays active until center of perspective is moved towards the bottom, to 84% of screen height. The plan is to split removal of 60 degrees limit to follow up patch. Fix max Z calculation in getProjMatrix. TransformState::getProjMatrix calculation of farZ was complex with possibility to lead to negative z values. Replacing it with simpler, precise calculation: furthestDistance = cameraToCenterDistance / (1 - tanFovAboveCenter * std::tan(getPitch())); TransformState::getProjMatrix calculation of farZ was an aproximation. Replacing it with simpler, but precise calculation. Related to: #15163
|
This is not fixed yet - work is ongoing in #15230 |
|
This is a very real issue for us with a large overlay at the top. We would use the MGLMapView's contentInset to translate the maps center location which seemed to be it's purpose back in #3583. Now the new behaviour is very pleasing, albeit different and not something I'd expect personally when reading the documentation. Issue 1: Reading this conversion gives me reason to believe that this new behaviour is intended. However how would one apply an exact pitch of 60 degrees now while contentInsets are in place? I feel like we'd need another way of translating MGLMapViews center. Issue 2: |
|
@JustinGanzer
Could you please share the APIs or even better, come of the code with inset values you use to set content inset... and the part of documentation about the non expected behavior? |
|
Here are comparative images and code regarding Issue 1. Images with Mapbox 4.10Without Inset: With Inset: Images with Mapbox 5.5Without Inset: With Inset: Code used to test behaviourAnd here are some performance changes when navigating across the map, without insets they are almost identical with 5.5 a bit better actually. But with insets : Performance in Mapbox 4.10 vs Mapbox 5.5 (Insets)4.10 with insets 5.5 with insets |
|
And the docs here, read:
With that in mind, my assumption would be that using Edit: I with topInsets at about 70% of the mapViews height, the camera at a pitch of 50 looks near identical to a camera with no insets with a pitch at 60. Perhaps I'm interpreting in wrong, but I hope this makes it kind of understandable as to why this change had me a bit confused. |
|
@JustinGanzer Thanks, a lot, for help on this. Images with Mapbox 4.10
Images with Mapbox 5.5
It should look the same. 4.10: Effectively, the zoom in center (that is now offset to 15% of the hight, measured from the bottom) is not what is specified by the API: zoom is larger. 5.5: The zoom in center is according to specified, but the pitch is not.
#15195 limits the pitch in order to prevent horizon to be displayed. But, it doesn't cover the issue you are experiencing - it doesn't do it so that pitch takes into account effect of asymmetric viewport (wider field of view) to pitch. Even the pitch in this case is ~60, it looks on the screen as if the pitch is having much larger value. Did you mean that with I'll work on fixing this. It would likely not require tile LOD to improve performance: number of tiles processed should be the same as with no content inset. |
|
@astojilj glad it's of any use, and thank you for the insightful description of what's going on. Ahh I did not see that in 4.10 the zoom was incorrectly affected by the use of Just out of curiosity, why does the field of view increase in version 5.5? |
|
@JustinGanzer So, the "experiment" showing that the pitch is actually correct:I have cut center + upper part of no-inset screen (1 on the picture), moved it down where it should be rendered as expected by content inset, and combined it with upper part(2) of inset screenshot. They fit perfectly (3rd image from left, if zooming you'll be able to see icons at 30% hight of the top, part of cut (1)).
Screenshot with large top Inset now doesn't look good and has severe performance issue because upper part is too busy: tiles at the top are showing way too much details compared to central or bottom area. As work on mapbox/mapbox-gl-js#8975 progresses, let me provide more details/screenshots, with the same center (52.510361, 13.389938) to back up this claim.
Let's consider no-inset center of view field of view (
Not sure if I have covered the question, though. |
|
@astojilj Again you have proven quite the keen eye for detail. With the experiment-screenshots I now also understand that the behaviour in Mapbox 4.10 was wrong and less desire-able. Disregard my remarks on the documentation along with issue 1 :). I'm positive that tile LOD will be the curator of issue 2 (performance). With that I have nothing more to add, but perhaps an idea for a convenience function in the future that works much like the old wrong behaviour in version 4.10? |
|
4.10 implementation was internally using API under MGLMapView's convertPoint to calculate latlng of the new center (offset from current screen center) and then to set it. I think this could be done on your end using existing API. Should you have issues with it, feel free to contact. |
|
@astojilj |
|
We have LOD integrated but it is not yet used in tile calculation (with edge insets, limit should be lower than 60 at mapbox-gl-native/src/mbgl/util/tile_cover.cpp Line 166 in 30b4e57 |
As of #14664, increasing the camera’s top padding can cause the camera’s focal point to lie lower than the center of the map. This significantly improves the perspective effect, but it also causes excessive detail to be shown toward the top of the map. Adaptive tile coverage (#9037) is still unimplemented, so tiles for the same zoom level get loaded throughout the map and displayed in smaller and smaller cells as you go toward the top of the map.
Here’s a map view displayed full-screen on an iPhone 8 simulator, with the top content inset set to 40% of the view’s height and the left content inset set to half the view’s height. The camera is rotated 258° counterclockwise from north (approximately west-southwest), tilted 60°, and raised about 200 meters above the ground. Tile boundaries are enabled. The top of the map is covered by an inordinate number of tiles:
Sometimes, with slightly different rotation and zooming, mbgl hangs because of the sheer amount of symbol placement that needs to happen – many times the amount of symbol placement that would happen with a normal viewport:
Increasing the top content inset to fully half the view’s height causes mbgl to not even render the top half of the map:
This issue could potentially affect a variety of iOS applications. The iOS and macOS map SDKs automatically increase the top content inset by default to accommodate any translucent top bar or toolbar in the same view controller. The issue isn’t acute in iosapp because the top bar there is relatively thin, but navigation applications in particular tend to have larger elements on top, such as a search bar or visual guidance instructions.
/cc @mapbox/gl-core @mapbox/maps-ios @mapbox/navigation-ios
The text was updated successfully, but these errors were encountered: