Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

astojilj
Copy link
Contributor

@astojilj astojilj commented Jul 24, 2019

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:

                            master  |   patch
                          ---------------------
TileCoverPitchedViewport    72000ns |  50300ns
TileCoverBounds              1620ns |   1400ns

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:

    Keeping the clockwise winding order (abcd), we rotated convex quadrilateral
    angles in such way that angle a (bounds[0]) is on top):
              a
            /   \
           /     b
          /      |
         /       c
        /  ....
       / ..
      d
    This is an example: we handle also cases where d.y < c.y, d.y < b.y etc.
    Split the scan to tree steps:
              a
            /   \     (1)
           /     b
     -----------------
          /      |    (2)
         /       c
     -----------------
        /  ....
       / ..           (3)
      d

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.

…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 astojilj self-assigned this Jul 24, 2019
@astojilj astojilj added Core The cross-platform C++ core, aka mbgl performance Speed, stability, CPU usage, memory usage, or power usage rendering labels Jul 24, 2019
@astojilj astojilj changed the title util::tileCover optimization: three scans and no duplicates handling. [core] util::tileCover optimization: three scans and no duplicates handling. Jul 24, 2019
/ ....
/ .. (3)
d
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice diagram. 👍

@friedbunny friedbunny added this to the release-queso milestone Jul 24, 2019
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

Nice optimization! Just a few questions, but it works great in macosapp. It feels like there is a slowdown with raster tiles (satellite streets style) on pitched and rotated maps, but I did not verify performance or runtime.

// 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 });
Copy link
Contributor

Choose a reason for hiding this comment

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

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 astojilj force-pushed the astojilj-near-horizon branch 2 times, most recently from b21b82c to bd3f137 Compare July 27, 2019 10:44
@chloekraw chloekraw added the needs changelog Indicates PR needs a changelog entry prior to merging. label Jul 30, 2019
@astojilj
Copy link
Contributor Author

astojilj commented Dec 3, 2019

Closing in favor of mapbox/mapbox-gl-js#8975. @asheemmamoowala, thanks for review.

@astojilj astojilj closed this Dec 3, 2019
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 needs changelog Indicates PR needs a changelog entry prior to merging. performance Speed, stability, CPU usage, memory usage, or power usage rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants