enforce TileJSON bounds #4556
enforce TileJSON bounds #4556
Conversation
mollymerp
force-pushed
the
enforce-bounds
branch
from
e49d03e to
af31877
Compare
src/source/tile_bounds.js
Outdated
| const level = [ | ||
| [Math.floor(this.lngX(this.bounds.getWest(), coord.z)), Math.floor(this.latY(this.bounds.getNorth(), coord.z))], | ||
| [Math.ceil(this.lngX(this.bounds.getEast(), coord.z)), Math.ceil(this.latY(this.bounds.getSouth(), coord.z))] | ||
| ]; |
There was a problem hiding this comment.
nit: for readability, instead of an array, could this be four individual consts minX, minY, etc.?
| t.true(source.hasTile({z: 8, x:95, y: 132}), 'returns true for tiles inside bounds'); | ||
| t.end(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Let's also include tests:
- for the case where
boundsis included directly on the TileJSON (i.e. where the source is created with{url: ...} - equivalent ones for raster source
- in
source_cache.test.js, confirming that SourceCache useshasTile()to filter which tiles it loads.
|
Wow. This is awesome, thanks @mollymerp. Can't believe I never got back to this after Thanksgiving (!). |
|
@mollymerp looks like we now have some code duplication between the new |
|
@mourner we'd need to extract the common calculation into |
|
I'm also not convinced that avoiding one duplication is critical... |
|
@anandthakker I mean, not directly instantiating |
Hm, along the lines of #3350, I would be hesitant to increase any |
|
Any reason this bounds parameter isn't in the tile spec Sources documentation? I was wondering if just this feature was supported but I had to dig through the tickets here. |
|
@underbluewaters good call. The source class is a combination of TileJSON (which does have this property), and a few custom properties. Ticketed in #5124 |
wraps up #3703 -- thank you @jlaxson for your contribution!
closes #1775
cc @vincentsarago @anandthakker
Launch Checklist