Open 1ec5 opened 5 years ago
1ec5
commented
5 years ago There are two potential fixes for this issue:
astojilj
commented
5 years ago There are two potential fixes for this issue:
- Adaptive tile loading (#9037)
- Varying the maximum pitch – but by the viewable distance rather than the zoom level, as proposed in #6908
As it is about high memory consumption, adding reference to mapbox/mapbox-gl-js#1898 and https://github.com/mapbox/mapbox-gl-native/issues/15022 - there we have memory issues with no pitch and it is expected to have the same with pitch.
chloekraw
commented
5 years ago 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.
1ec5
commented
5 years ago 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.
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.
astojilj
commented
5 years ago 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.
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.
astojilj
commented
5 years ago There are two potential fixes for this issue:
- Adaptive tile loading (#9037)
- Varying the maximum pitch – but by the viewable distance rather than the zoom level, as proposed in #6908
@1ec5 , to further clarify what I'm working on in https://github.com/mapbox/mapbox-gl-native/pull/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.
astojilj
commented
5 years ago This is not fixed yet - work is ongoing in https://github.com/mapbox/mapbox-gl-native/pull/15230
JustinGanzer
commented
5 years ago 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: With a top padding, as is the case in our app, it used to feel like the camera was simply moved up in terms of latitude before this change. Now it feels like the camera is moved up AND a lower angle of view is applied effectively no longer truly representing the camera pitch we had envisioned or am I wrong?
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:
As 1ec5 has mentioned, this hammers away at the battery life during navigation. I feel like the only solution at the moment is to change from translucent to opaque overlays and resizing the MapView or play around with CameraOptions.padding?
astojilj
commented
5 years ago @JustinGanzer Thanks. Let me take another look at it. We are working on tile LOD to show higher pitch values but that would only address Issue 2.
Now the new behaviour is very pleasing, albeit different and not something I'd expect personally when reading the documentation.
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?
JustinGanzer
commented
5 years ago @astojilj
Here are comparative images and code regarding Issue 1. I'd like to have a pitch of 60 degrees and a topInset of 70% as to have the center at the bottom 15% of the mapView. Here you'll see that without insets they're both identical, but with its as though the pitch changed, or the curvature of the camera lens at the top drastically increased.
And 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 :
JustinGanzer
commented
5 years ago @astojilj
And the docs here, read:
For instance, if the only the top edge is inset, the map center is effectively shifted downward
With that in mind, my assumption would be that using setCenterCoordinate:animated: of MGLMapView to apply the very same location to two MapViews would look almost the same.
The only difference would be the offset of the real camera center due to the contentInset, thus having the latitude & longitude of said real center slightly moved. Pitch, heading etc would stay the same.
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.
astojilj
commented
5 years ago @JustinGanzer Thanks, a lot, for help on this. Let's continue using your screenshots to discuss what's happening. Red rectangle is an approximation of area that becomes content area (defined by content insets):
Images with Mapbox 4.10
Images with Mapbox 5.5
With that in mind, my assumption would be that using setCenterCoordinate:animated: of MGLMapView to apply the very same location to two MapViews would look almost the same
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.
Now it feels like the camera is moved up AND a lower angle of view is applied effectively no longer truly representing the camera pitch we had envisioned or am I wrong?
https://github.com/mapbox/mapbox-gl-native/pull/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 lower angle of view?
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.
JustinGanzer
commented
5 years ago @astojilj glad it's of any use, and thank you for the insightful description of what's going on.
Yes I did refer to the increasing field of view as lower higher angle of view. Sorry about the lower/higher mistake here.
Ahh I did not see that in 4.10 the zoom was incorrectly affected by the use of contentInset. Good eye!
Just out of curiosity, why does the field of view increase in version 5.5? Is it because the camera is not moved on the latitude/longitude while at the same time trying to maintain zoom and pitch for the specified camera center? Would it still be compliant with the API if the camera was moved a bit instead?
astojilj
commented
5 years ago @JustinGanzer Please ignore my analysis from above. I have double-checked the screenshots again and it seems that there is an optical illusion :) The pitch in both cases is rendered the same on screen. Different level of detail, thin lines at the top and the position of screenshot (one on the right looks like having larger pitch value) makes it look that the pitch is different. This is going to be fixed soon, after porting https://github.com/mapbox/mapbox-gl-js/pull/8975 to gl-native.
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 https://github.com/mapbox/mapbox-gl-js/pull/8975 progresses, let me provide more details/screenshots, with the same center (52.510361, 13.389938) to back up this claim.
Just out of curiosity, why does the field of view increase in version 5.5? Is it because the camera is not moved on the latitude/longitude while at the same time trying to maintain zoom and pitch for the specified camera center? Would it still be compliant with the API if the camera was moved a bit instead?
Let's consider no-inset center of view field of view (fov on the image) and inset center of view (new fovon the image). Center is at the same distance from camera, pitch is the same but camera shows much larger area of the map: map area (2) is with inset, map area (1) is with no insets. I've scaled map to the screen center - it probably looks weird that it is in front of the screen but hopefully it helps explaining.
Not sure if I have covered the question, though.
JustinGanzer
commented
5 years ago @astojilj You have fully answered my question with that insightful drawing of yours. Thanks, that really helped me understand exactly what is happening.
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?
This may sounds strange at first but here is why I found the wrong behaviour useful:
I have found myself wanting to center a point of the map in the top/bottom half of the screen -> but give the camera a pitch and distance relative to the device screen center, not the MapView/ViewPort center. For example inside a navigation. A Mapbox user might want the location of the user at the bottom, but have the camera at a 60 degree pitch at the center with a given distance. That is where I would make use of the wrong edgePadding or contentInset back in MapBox 4.10 and lower.
astojilj
commented
5 years ago @JustinGanzer
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.
JustinGanzer
commented
5 years ago @astojilj Thanks for you help. With it, it was easy enough to create a setCamera method that mimics the old behaviour. 👍
astojilj
commented
4 years ago We have LOD integrated but it is not yet used in tile calculation (with edge insets, limit should be lower than 60 at https://github.com/mapbox/mapbox-gl-native/blob/30b4e57828acc1e2304373f9c8e4caecc39a3803/src/mbgl/util/tile_cover.cpp#L166).
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