This repository has been archived by the owner on Aug 8, 2023. It is now read-only.
[core] util::tileCover optimization: three scans and no duplicates handling. #15206
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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
astojilj
added
Core
astojilj
changed the title
friedbunny
reviewed
Jul 24, 2019
| / .... | ||
| / .. (3) | ||
| d | ||
| */ |
| // Scan (3) - the triangle at the bottom. | ||
| if (ad.y1 < bc.y1) { std::swap(ad, bc); } | ||
| ymax = std::ceil(ad.y1); | ||
| bc = edge({ bc.x1, bc.y1 }, { ad.x1, ad.y1 }); |
There was a problem hiding this comment.
nit: I know it would include an additional edge creation, but for clarity it would be nice to rename this to cd.
| double m0 = e0.dx / e0.dy; | ||
| double m1 = e1.dx / e1.dy; | ||
| double ySort = e0.y0 == e1.y0 ? std::min(e0.y1, e1.y1) : std::max(e0.y0, e1.y0); | ||
| if (e0.x0 - (e0.y0 - ySort) * m0 < e1.x0 - (e1.y0 - ySort) * m1) { |
There was a problem hiding this comment.
Is there double swapping going on here and then x coords are swapped again in scanLine?
Maybe i'm not grokking this fully
tileCover is swapping edges so that they have y0 < y1. In which case, wouldn't it be more straightforward if the check here ensured that e0 has the lower x co-ordinate at the non-floored equivalent of ymin, or if e0.x0 == e1.x0, then e0 has the lower x co-ordinate at the non-ceilinged equivalent of ymax.
Then the call to scanLine can also swap the order of the arguments.
astojilj
force-pushed
the
astojilj-near-horizon
branch
2 times, most recently
from
b21b82c
to
bd3f137
Compare
chloekraw
added
the
needs changelog
astojilj
force-pushed
the
astojilj-near-horizon
branch
from
bd3f137
to
f37d81d
Compare
|
Closing in favor of mapbox/mapbox-gl-js#8975. @asheemmamoowala, thanks for review. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 src/mbgl/util/tile_cover.cpp from master and from this patch:
TileCoverPitchedViewport modified to have pitch capped by large 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 - tests are contributed as part of #15195 (this patch is rebased onto it).
Snipped copied from code comments:
Related to: #15163 - speeding up would help performance or larger result set processing as there is a need to further enhance returned tileset in adaptive tile coverage (#9037) implementation.