Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Limit pitch based on edge insets. Fix max Z calculation in getProjMatrix. #15195

Merged
merged 2 commits into from
Aug 1, 2019

Conversation

astojilj
Copy link
Contributor

@astojilj astojilj commented Jul 23, 2019

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:

  1. 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.
  2. 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()));

IMG_0254

Mapping diagram to code:
fD: furthestDistance,
c: cameraToCenterDistance,
small phi: getPitch()

Addresses: #15163

@friedbunny friedbunny added Core The cross-platform C++ core, aka mbgl rendering labels Jul 23, 2019
@friedbunny friedbunny added this to the release-queso milestone Jul 23, 2019
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the failsafe introduced here introduces a change in behavior that’ll be noticeable in the navigation SDK and navigation-related applications. It’s a sound move, but it does put more pressure on us to implement adaptive zoom levels (aka level of detail): #9037.

src/mbgl/map/transform.cpp Show resolved Hide resolved
@astojilj
Copy link
Contributor Author

astojilj commented Jul 23, 2019

Note that the failsafe introduced here introduces a change in behavior that’ll be noticeable in the navigation SDK and navigation-related applications. It’s a sound move, but it does put more pressure on us to implement adaptive zoom levels (aka level of detail): #9037.

@1ec5,

Edited the comment:

The only change in behavior I see if moving center of perspective towards the bottom, to 84% of screen height. Then app wouldn't try to load e.g. 20000+ tiles but e.g. 500. It would still be very slow but prevents crash and is improvement compared to current state.

About change in behavior:

The change here is done this way to prevent current visual change (it is still 60 degrees for smaller padding, like it was), and allow the largest possible pitch that doesn't show area above horizon.

Agree, there is a pressure to work on #9037 immediately, with or without changes in this PR. I to work on #9037

Did I understand the "change in behavior" correctly or there is something additional?

@astojilj astojilj force-pushed the astojilj-near-horizon branch from e5d6e7e to 89fc482 Compare July 24, 2019 13:52
@astojilj astojilj changed the title Limit pitch based on edge insets. Fix max Z calculation in getProjMat… [core] Limit pitch based on edge insets. Optimize tileCover algorithm. Jul 24, 2019
@astojilj astojilj changed the title [core] Limit pitch based on edge insets. Optimize tileCover algorithm. [core] Limit pitch based on edge insets. Fix max Z calculation in getProjMatrix. Jul 24, 2019
@astojilj astojilj force-pushed the astojilj-near-horizon branch 2 times, most recently from 84f6ac6 to 5955610 Compare July 24, 2019 14:53
@astojilj astojilj marked this pull request as ready for review July 24, 2019 15:36
@astojilj
Copy link
Contributor Author

@1ec5 , @alexshalamov
PTAL.
As mentioned, this is first part of fixing #15195.

@alexshalamov I'll split the tile cover optimization to other patch - here, contributing unit tests for verifying the upcoming patch against current state.

src/mbgl/map/transform.cpp Outdated Show resolved Hide resolved
src/mbgl/map/transform.cpp Show resolved Hide resolved
src/mbgl/map/transform.cpp Show resolved Hide resolved
@astojilj astojilj force-pushed the astojilj-near-horizon branch from 5955610 to b21b82c Compare July 26, 2019 13:34
@astojilj astojilj force-pushed the astojilj-near-horizon branch from b21b82c to bd3f137 Compare July 27, 2019 10:44
@astojilj astojilj requested review from a team and 1ec5 July 27, 2019 10:44
@astojilj
Copy link
Contributor Author

@1ec5 ,
Before merging this, I'd like to verify that I've correctly understood and addressed your concerns here.

Work on this, and making the max pitch larger, continues in ##15230. But, I'd like to push this patch first as it is failsafe, preventing out of memory and block but not altering currently max allowed/possible pitch.

@1ec5
Copy link
Contributor

1ec5 commented Jul 29, 2019

Work on this, and making the max pitch larger, continues in ##15230. But, I'd like to push this patch first as it is failsafe, preventing out of memory and block but not altering currently max allowed/possible pitch.

This is a good plan, thank you!

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested changelog entry:

  • Fixed an issue where it was possible to set the map’s content insets then tilt the map enough to see the horizon, causing performance issues.

…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
@astojilj astojilj force-pushed the astojilj-near-horizon branch from bd3f137 to f37d81d Compare August 1, 2019 05:14
@astojilj
Copy link
Contributor Author

astojilj commented Aug 1, 2019

Suggested changelog entry:

  • Fixed an issue where it was possible to set the map’s content insets then tilt the map enough to see the horizon, causing performance issues.

Thanks, added.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runaway tile downloads, symbol placement when map is tilted with top padding
5 participants