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

Support TileJSON bounds property #10701

Merged
merged 5 commits into from
Jan 5, 2018
Merged

Support TileJSON bounds property #10701

merged 5 commits into from
Jan 5, 2018

Conversation

asheemmamoowala
Copy link
Contributor

Closes #5394.

Prevent tiles outside of TileJSON.bounds from being created in TilePyramid::update.

@asheemmamoowala asheemmamoowala added Core The cross-platform C++ core, aka mbgl GL JS parity For feature parity with Mapbox GL JS labels Dec 14, 2017
@asheemmamoowala asheemmamoowala force-pushed the 5394-tilejson-bounds branch 3 times, most recently from e349b73 to c3341bf Compare December 19, 2017 18:44
mollymerp
mollymerp approved these changes Dec 19, 2017
View reviewed changes
Copy link
Contributor

@mollymerp mollymerp left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! This looks good to me with a caveat in the comments about bounds wrapping.
Also I'm not very familiar with the tile loading logic in native so definitely a good idea to have @kkaefer or someone else take a second look!

src/mbgl/util/tile_range.hpp

bool contains(const CanonicalTileID& tileID) {
return z == tileID.z &&
(range.min.x >= range.max.x ? //For wrapped bounds
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw I don't think wrapped bounds are supported in TileJSON – I'm not sure if wrapped (i.e. > 180 || < -180) latitudes are considered to be true WSG:84 coordinates but 🤷‍♀️

    // OPTIONAL. Default: [-180, -90, 180, 90].
    // The maximum extent of available map tiles. Bounds MUST define an area
    // covered by all zoom levels. The bounds are represented in WGS:84
    // latitude and longitude values, in the order left, bottom, right, top.
    // Values may be integers or floating point numbers.
    "bounds": [ -180, -85.05112877980659, 180, 85.0511287798066 ],

https://github.com/mapbox/tilejson-spec/tree/master/2.2.0

Copy link
Contributor

Choose a reason for hiding this comment

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

in the JS implementation we also clamp the bounds, so this would introduce differing behavior between the two packages
https://github.com/mapbox/mapbox-gl-js/blob/2124161ebba0c17d764e2e7f78e2d34a283e9bae/src/source/tile_bounds.js#L22

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The input coordinates can easily be clamped, but if left> right, it will result in the unwrapped bounds in native, and invalid bounds in JS, where the contains test will always fail on longitude here.

kkaefer
kkaefer reviewed Jan 2, 2018
View reviewed changes
Copy link
Member

@kkaefer kkaefer left a comment

Choose a reason for hiding this comment

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

Looks good! Can you please also add render tests that attempt to render tiles that are on the fringe/outside of the tile bounds to verify that they aren't rendered?

src/mbgl/style/conversion/tileset.cpp Outdated
if (!validateLatitude(*bottom) || !validateLatitude(*top)){
error = { "bounds latitude values must be between -90 and 90" };
return {};
}
Copy link
Member

Choose a reason for hiding this comment

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

we should also verify that bottom <= top (and left <= right?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kkaefer, as part of fixing left <= right, should we also clamp the longitude to [-180, 180] as discussed here?

src/mbgl/util/tile_range.hpp Outdated
auto minX = std::floor(swProj.x);
auto maxX = std::ceil(neProj.x);
auto minY = std::floor(neProj.y);
auto maxY = std::ceil(swProj.y);
Copy link
Member

Choose a reason for hiding this comment

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

const all the things

src/mbgl/util/tile_range.hpp Outdated
tileID.x < range.max.x &&
tileID.x >= range.min.x &&
tileID.y < range.max.y &&
tileID.y >= range.min.y;
Copy link
Member

Choose a reason for hiding this comment

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

Does this check produce the same results as the one we're using for JS?

src/mbgl/algorithm/update_renderables.hpp Outdated
@@ -35,7 +35,9 @@ void updateRenderables(GetTileFn getTile,
auto tile = getTile(idealDataTileID);
if (!tile) {
tile = createTile(idealDataTileID);
assert(tile);
if(tile == nullptr) {
continue;
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add a comment here that explains why tile might be null?

src/mbgl/annotation/render_annotation_source.cpp Outdated
@@ -41,6 +41,7 @@ void RenderAnnotationSource::update(Immutable<style::Source::Impl> baseImpl_,
// Zoom level 16 is typically sufficient for annotations.
// See https://github.com/mapbox/mapbox-gl-native/issues/10197
{ 0, 16 },
{},
Copy link
Member

Choose a reason for hiding this comment

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

Can we change that to util::TileRange{} to make this code more readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That parameter represents an optional<LatLngBounds>. Are you suggesting to change the type of the parameter to TilePyramid::update ?

Copy link
Member

Choose a reason for hiding this comment

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

No, just writing the type here to make it explicit when reading the code.

@asheemmamoowala asheemmamoowala mentioned this pull request Jan 2, 2018
Merged
@asheemmamoowala asheemmamoowala force-pushed the 5394-tilejson-bounds branch 3 times, most recently from 3783fb2 to 7b2d2a1 Compare January 4, 2018 19:06
@asheemmamoowala asheemmamoowala merged commit 10a4405 into master Jan 5, 2018
@asheemmamoowala asheemmamoowala deleted the 5394-tilejson-bounds branch January 5, 2018 14:37
@1ec5 1ec5 mentioned this pull request Jan 26, 2018
Closed
@1ec5 1ec5 mentioned this pull request Feb 7, 2018
Merged
asheemmamoowala pushed a commit that referenced this pull request Feb 7, 2018
Support TileJSON bounds property (#10701)
fae7a14 
* [core] Parse TileJSON bounds property
* [core] Add TileRange and LatLngBounds::contains(CanonicalTileID)
Move LatLngBounds::contains impl to cpp file
* [core] Skip tile creation outside of tileset bounds
* [core] Fix TileRange for wrapped bounds and use for CustomTileLoader instead of LatLngBounds comparisons for tiles.
@asheemmamoowala asheemmamoowala mentioned this pull request Feb 7, 2018
Merged
3 tasks
asheemmamoowala pushed a commit that referenced this pull request Feb 8, 2018
Support TileJSON bounds property (#10701)
6437471 
* [core] Parse TileJSON bounds property
* [core] Add TileRange and LatLngBounds::contains(CanonicalTileID)
Move LatLngBounds::contains impl to cpp file
* [core] Skip tile creation outside of tileset bounds
* [core] Fix TileRange for wrapped bounds and use for CustomTileLoader instead of LatLngBounds comparisons for tiles.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kkaefer kkaefer kkaefer left review comments

@mollymerp mollymerp mollymerp approved these changes

Assignees
No one assigned
Labels
Core The cross-platform C++ core, aka mbgl GL JS parity For feature parity with Mapbox GL JS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@asheemmamoowala @kkaefer @mollymerp