Tile coverage updates #12310

brunoabinader · 2018-07-04T18:01:42Z

This PR:

Limits the amount of covered tiles for TileCoordinate-based coverage: we are now clamping the coverage to up to 4 (value is arbitrary) neighbor tiles in each direction
Treats pan (prefetch) tiles as ideal, so they can be used to render the "horizon" in higher pitch values
Increases the maximum pitch value to 67.5 degrees

From the example above, the topmost tile is a higher zoom tile (configurable via Map.prefetchZoomDelta() that covers the remaining visible area.

Using pan tiles to cover the horizon reduces the amount of rendered tiles, and because they are barely visible, the lower fidelity doesn't account much. This PR is an ongoing effort to eventually support modifying the projection matrix eye according to the edge insets (#12107) - modifying the eye position can also cause more tiles to be visible.

Most rendering failures in render tests are caused by setting a pitch value higher than maximum pitch, which causes different results between GL native (max pitch: 67.5) and GL JS (max pitch: 60.0).

/cc @asheemmamoowala @kkaefer @tmpsantos

mb12 · 2018-07-05T16:31:49Z

@brunoabinader Can you please kindly clarify the following?

With this change does it mean that for pitch > maxpitch irrespective of the width and height of the viewport, only 64 tiles at the most will be loaded?
Can you please clarify why do tl, tr etc have to be rotated for the specific case of >maxpitch but not otherwise? Also why does the transformation include only the angle of rotation but it does not include the pitch? Also why is the rotation w.r.t. to (0, 0) and not the center of the map?

1ec5

The new maximum pitch would be a good change to add to the Android, iOS, and macOS changelogs. It improves not only the developer experience but also potentially the end user experience.

brunoabinader · 2018-07-06T08:27:54Z

With this change does it mean that for pitch > maxpitch irrespective of the width and height of the viewport, only 64 tiles at the most will be loaded?

We start limit (clamping the range of) tile coverage when pitch trespasses maxPitchForUnlimitedCoverage. When that happens, we clamp the amount of covered tiles to the range [CenterX-4...CenterX+4][CenterY-4...CenterY+4], which can give up to 9x9=81 tiles.

Notice, however, that in continuous rendering mode, we also calculate pan tiles, which throughputs bigger tiles that should cover the remaining part of the visible map - more about this in #7741.

Can you please clarify why do tl, tr etc have to be rotated for the specific case of >maxpitch but not otherwise?

We only need the rotated values to calculate clamping when limiting (and we rotate back after). Otherwise, we proceed with the original tile coverage algorithm.

Also why does the transformation include only the angle of rotation but it does not include the pitch?

We don't need pitch because it has been already taken in consideration when calculating the TileCoordinate points received by tileCover(). We rotate these coordinates because we want a viewport axis-aligned TileCoordinate bbox to apply proper clamping - see screenshots below for the difference between rotated and non-rotated clamping:

viewport axis-aligned	non-rotated

Also why is the rotation w.r.t. to (0, 0) and not the center of the map?

Not sure what you mean here - in tileCover(), c is the center tile coordinate of the viewport center - see https://github.com/mapbox/mapbox-gl-native/blob/master/src/mbgl/util/tile_cover.cpp#L157 and https://github.com/mapbox/mapbox-gl-native/blob/master/src/mbgl/util/tile_cover.cpp#L171.

brunoabinader · 2018-07-06T08:38:28Z

Worth mentioning that using an arbitrary fixed limiting factor of 4 is a simpler solution to a somewhat complex problem. When implementing this, I've also considered the following:

Calculate different limiting factors for x and y, taking width and height ratio in consideration.
Take pitch in consideration when calculating the limiting factor (example below)

double rangeX = (topRight.x - topLeft.x) * std::cos(pitch);

For future consideration, we can also think of:

Instead of two fixed tile layers (ideal and pan tileS), apply instead a mipmap-like approach that dynamically decreases the tile zoom as it is more distant from the center.

asheemmamoowala

@brunoabinader In playing with this branch I see that the order in which tiles are fetched and rendered is prioritizing the panTiles over the idealTiles. This works great for satellite imagery. With vector tiles, Im sometimes seeing that low zoom tiles at the top edge of the screen are being loaded before tiles in the center or bottom of the screen. It's rare, but wondering if there's something to be done about it ?
Is there a reason for enabling the clamping only past 60 degrees pitch ? It seems like it would be useful starting ~ 45 degrees.

brunoabinader · 2018-07-10T07:39:53Z

@brunoabinader In playing with this branch I see that the order in which tiles are fetched and rendered is prioritizing the panTiles over the idealTiles. This works great for satellite imagery. With vector tiles, Im sometimes seeing that low zoom tiles at the top edge of the screen are being loaded before tiles in the center or bottom of the screen. It's rare, but wondering if there's something to be done about it ?

I believe that's intended - my understanding of pan tiles is that they prevent empty areas when panning - so we want to start showing off these faster than the higher zoomed tiles. However, we could add some logic to prevent this prioritization when not gesturing (TransformState knows about that). Let's try out this logic and observe behavior.

Is there a reason for enabling the clamping only past 60 degrees pitch ? It seems like it would be useful starting ~ 45 degrees.

We can modify that 👍 However, clamping past 60 degrees preserves the current behavior (because we've been limiting pitch to 60 degrees until now).

Also, the amount of visible tiles also depends whether your zoom level is close to its integer value (e.g. zoom 10.1 = less tiles), or closer its ceiling value (zoom 10.95 = more tiles).

brunoabinader · 2018-07-10T14:35:03Z

Render tests regressions/mapbox-gl-js#5911 and regressions/mapbox-gl-js#5911a are now failing when setting the tilt threshold for limiting the tile coverage to 45 instead of 60 degrees - this is actually one of the reasons why I selected 60 degrees as minimum threshold beforehand 😅

asheemmamoowala

This looks good to me. Any chance you can add a render test too?

brunoabinader · 2018-07-10T17:55:18Z

This looks good to me. Any chance you can add a render test too

Good idea, I'll add one 👍

stale

brunoabinader · 2018-08-08T12:57:48Z

@asheemmamoowala could you please re-review?

asheemmamoowala

@brunoabinader Im not sure I understand why Symbol layers are skipped on the pan tiles, and how that relates to this PR.

asheemmamoowala · 2018-08-14T17:27:13Z

test/util/tile_cover.test.cpp

@@ -43,6 +45,22 @@ TEST(TileCover, Pitch) {
 { 2, 1, 2 }, { 2, 1, 1 }, { 2, 2, 2 }, { 2, 2, 1 }, { 2, 3, 2 }
 }),
 util::tileCover(transform.getState(), 2));
+
+ // 2 tiles on the left, 1 center tiler, then 2 tiles on the right.


Break this into a separate test

asheemmamoowala · 2018-08-14T17:27:47Z

test/util/tile_cover.test.cpp

+ uint8_t integerZoom = std::floor(zoom);
+ transform.setZoom(zoom);
+ auto fullTiles = util::tileCover(transform.getState(), integerZoom, util::TileCoverMode::Full);
+ auto limitedTiles = util::tileCover(transform.getState(), integerZoom, util::TileCoverMode::Limited5x5);


It would be good to also check the correctness of the returned tiles as part of the test.

asheemmamoowala · 2018-08-14T17:28:44Z

src/mbgl/util/tile_cover.cpp

+ point = util::rotate(rotated, -bearing);
+ };
+
+ // Limit tile coverage in continuous mode.


Is this comment still appropriate? How is continuous mode being checked here?

asheemmamoowala · 2018-08-14T17:43:01Z

src/mbgl/util/tile_cover.cpp

- const Point<double>& tr,
- const Point<double>& br,
- const Point<double>& bl,
+std::vector<UnwrappedTileID> tileCover(Point<double> tl,


nit: non-uniform input arguments.

Would it be better to keep these as const references, and conditionally return a clamped point from limitTileCoordinate?

asheemmamoowala · 2018-08-14T18:01:52Z

src/mbgl/util/tile_cover.cpp

 auto scanLine = [&](int32_t x0, int32_t x1, int32_t y) {
- int32_t x;
- if (y >= 0 && y <= tiles) {


This condition is no longer necessary? I think it is necessary at the poles.

stale · 2018-10-24T07:21:17Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ChrisLoer · 2018-10-24T17:24:24Z

This would be great to resurrect! Related discussion is still going on the JS side: mapbox/mapbox-gl-js#3731

stale · 2018-12-24T01:58:53Z

This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

brunoabinader added feature Core The cross-platform C++ core, aka mbgl Google Maps parity For feature parity with the Google Maps SDK for Android or iOS labels Jul 4, 2018

brunoabinader requested review from kkaefer and asheemmamoowala July 4, 2018 18:01

brunoabinader mentioned this pull request Jul 5, 2018

Limit pitch to 60 degrees in render tests mapbox/mapbox-gl-js#6917

Merged

brunoabinader force-pushed the pitch-67.5-degrees branch 5 times, most recently from 0c503c5 to ed972b2 Compare July 5, 2018 12:41

brunoabinader requested a review from tmpsantos July 5, 2018 12:44

brunoabinader force-pushed the pitch-67.5-degrees branch from ed972b2 to 11969d0 Compare July 5, 2018 13:16

1ec5 reviewed Jul 5, 2018

View reviewed changes

This was referenced Jul 6, 2018

Harden projection #12338

Merged

Tile coverage updates #9284

Closed

brunoabinader force-pushed the pitch-67.5-degrees branch from 11969d0 to 5145a76 Compare July 9, 2018 07:16

asheemmamoowala previously requested changes Jul 9, 2018

View reviewed changes

brunoabinader force-pushed the pitch-67.5-degrees branch from 5145a76 to faa16d9 Compare July 10, 2018 07:29

brunoabinader force-pushed the pitch-67.5-degrees branch from faa16d9 to 691bf55 Compare July 10, 2018 13:14

brunoabinader force-pushed the pitch-67.5-degrees branch from 691bf55 to e5491d8 Compare July 10, 2018 14:41

asheemmamoowala reviewed Jul 10, 2018

View reviewed changes

brunoabinader mentioned this pull request Jul 11, 2018

[core] coveredByChildren is false if at least one child is uncovered #12371

Merged

brunoabinader force-pushed the pitch-67.5-degrees branch from e5491d8 to 8400fd7 Compare July 16, 2018 14:52

brunoabinader force-pushed the pitch-67.5-degrees branch from 8400fd7 to 3e5c7a2 Compare July 17, 2018 08:24

This was referenced Jul 18, 2018

Maximum pitch should vary by zoom level #6908

Open

Support max pitch > 60 mapbox/mapbox-gl-js#3731

Closed

friedbunny added the needs changelog Indicates PR needs a changelog entry prior to merging. label Jul 18, 2018

friedbunny mentioned this pull request Jul 18, 2018

Add elastic bounce-back if tilt gesture hits maximum angle #2191

Open

brunoabinader added 6 commits August 8, 2018 15:52

[core] tileCover zoom is uint8_t

b218d7d

[core] Remove unused 'rendered' set

a9cd952

[core] Limit TileCoordinate-based coverage

160c7b4

[core] Increase pitch to 67.5 degrees

9899518

[core] Remove redundant check in scanLine

12b8482

[core] Do not render symbol layers for pan tiles

1b795ea

brunoabinader force-pushed the pitch-67.5-degrees branch from 3e5c7a2 to 1b795ea Compare August 8, 2018 12:53

asheemmamoowala suggested changes Aug 14, 2018

View reviewed changes

stale bot added the archived Archived because of inactivity label Oct 24, 2018

stale bot removed the archived Archived because of inactivity label Oct 24, 2018

stale bot added the archived Archived because of inactivity label Dec 23, 2018

stale bot closed this Dec 24, 2018

brunoabinader deleted the pitch-67.5-degrees branch March 3, 2019 12:14

friedbunny removed needs changelog Indicates PR needs a changelog entry prior to merging. labels Jun 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tile coverage updates #12310

Tile coverage updates #12310

brunoabinader commented Jul 4, 2018

mb12 commented Jul 5, 2018

1ec5 left a comment

brunoabinader commented Jul 6, 2018

brunoabinader commented Jul 6, 2018

asheemmamoowala left a comment •

edited

Loading

brunoabinader commented Jul 10, 2018

brunoabinader commented Jul 10, 2018

asheemmamoowala left a comment

brunoabinader commented Jul 10, 2018

brunoabinader commented Aug 8, 2018

asheemmamoowala left a comment

asheemmamoowala Aug 14, 2018

asheemmamoowala Aug 14, 2018

asheemmamoowala Aug 14, 2018

asheemmamoowala Aug 14, 2018

asheemmamoowala Aug 14, 2018

stale bot commented Oct 24, 2018

ChrisLoer commented Oct 24, 2018

stale bot commented Dec 24, 2018

Tile coverage updates #12310

Tile coverage updates #12310

Conversation

brunoabinader commented Jul 4, 2018

mb12 commented Jul 5, 2018

1ec5 left a comment

Choose a reason for hiding this comment

brunoabinader commented Jul 6, 2018

brunoabinader commented Jul 6, 2018

asheemmamoowala left a comment • edited Loading

Choose a reason for hiding this comment

brunoabinader commented Jul 10, 2018

brunoabinader commented Jul 10, 2018

asheemmamoowala left a comment

Choose a reason for hiding this comment

brunoabinader commented Jul 10, 2018

brunoabinader commented Aug 8, 2018

asheemmamoowala left a comment

Choose a reason for hiding this comment

asheemmamoowala Aug 14, 2018

Choose a reason for hiding this comment

asheemmamoowala Aug 14, 2018

Choose a reason for hiding this comment

asheemmamoowala Aug 14, 2018

Choose a reason for hiding this comment

asheemmamoowala Aug 14, 2018

Choose a reason for hiding this comment

asheemmamoowala Aug 14, 2018

Choose a reason for hiding this comment

stale bot commented Oct 24, 2018

ChrisLoer commented Oct 24, 2018

stale bot commented Dec 24, 2018

asheemmamoowala left a comment •

edited

Loading