Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stop returning invalid x tile values in pointToTile #27

Closed
wants to merge 1 commit into from

Conversation

tcql
Copy link

@tcql tcql commented Dec 25, 2015

Currently pointToTile on coordinates with +180 longitude will result in invalid X tile coordinates. IE:

tilebelt.pointToTile(180, 0, 0);

yields [1, 0, 0] instead of [0, 0, 0]

cc @mourner

@@ -156,6 +156,8 @@ function pointToTileFraction(lon, lat, z) {
z2 = Math.pow(2, z),
x = z2 * (lon / 360 + 0.5),
y = z2 * (0.5 - 0.25 * Math.log((1 + sin) / (1 - sin)) / Math.PI);
if (x >= z2) x = z2 - 1;
if (y >= z2) x = z2 - 1;
Copy link
Member

Choose a reason for hiding this comment

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

You probably meant y = on this line.

@mourner
Copy link
Member

mourner commented Jan 4, 2016

I think we'd want to go with a more sophisticated approach here:

  • wrap x instead of clamping (so that 190 longitude is equivalent to -170, not -180)
  • clamp negative y as well (if (y < 0) y = 0)

@mourner
Copy link
Member

mourner commented Dec 14, 2016

Any updates here?

@DenisCarriere
Copy link
Contributor

coordinates with +180 longitude will result in invalid X tile coordinates.

Might be useful as reference material, global-mercator doesn't have the -180 or 180 longitude issue using lngLatToTile([x, y], zoom).

const mercator = require('global-mercator')
mercator.lngLatToTile([-180, 0], 0)
//=[ 0, 0, 0 ]

wrap x instead of clamping (so that 190 longitude is equivalent to -170, not -180)

Reversing longitudes over the dateline isn't overly complicated, however if you need another reference, I built bbox-dateline for that exact purpose.

CC: @tcql @mourner

@DenisCarriere
Copy link
Contributor

DenisCarriere commented Jul 19, 2017

I've made a wrapTile method that can easily be added:

/**
 * Wrap Tile -- Handles tiles which crosses the 180th meridian or 90th parallel
 *
 * @param {[number, number, number]} tile Tile
 * @param {number} zoom Zoom Level
 * @returns {[number, number, number]} Wrapped Tile
 * @example
 * wrapTile([0, 3, 2])
 * //= [0, 3, 2] -- Valid Tile X
 * wrapTile([4, 2, 2])
 * //= [0, 2, 2] -- Tile X 4 does not exist, wrap around to TileX=0
 */
function wrapTile (tile) {
  var tx = tile[0]
  var ty = tile[1]
  var zoom = tile[2]

  // Maximum tile allowed
  // zoom 0 => 1
  // zoom 1 => 2
  // zoom 2 => 4
  // zoom 3 => 8
  var maxTile = Math.pow(2, zoom)

  // Handle Tile X
  tx = tx % maxTile
  if (tx < 0) tx = tx + maxTile

  return [tx, ty, zoom]
}

You could also include these test cases:

test('tilebelt.pointToTile -- cross meridian', t => {
  // X axis
  t.deepEqual(tilebelt.pointToTile(-180, 85, 2), [0, 0, 2], '[-180, 85] zoom 2')
  t.deepEqual(tilebelt.pointToTile(180, 85, 2), [0, 0, 2], '[+180, 85] zoom 2')
  t.deepEqual(tilebelt.pointToTile(-185, 85, 2), [3, 0, 2], '[-185, 85] zoom 2')
  t.deepEqual(tilebelt.pointToTile(185, 85, 2), [0, 0, 2], '[+185, 85] zoom 2')

  // Y axis
  t.deepEqual(tilebelt.pointToTile(-175, -97, 2), [0, 0, 2], '[-175, -95] zoom 2')
  t.deepEqual(tilebelt.pointToTile(-175, 95, 2), [0, 3, 2], '[-175, +95] zoom 2')
  t.end()
})

http://www.maptiler.org/google-maps-coordinates-tile-bounds-projection/
image

Benchmark results

wrapTile x 30,352,603 ops/sec ±2.05% (87 runs sampled)

CC: @tcql @mourner

@tcql
Copy link
Author

tcql commented Jul 20, 2017

@DenisCarriere the X wrapping looks good, but Y should definitely clip, not wrap. -90 and 90 latitude don't touch (whereas -180 and 180 longitude do, so wrapping makes sense)

@DenisCarriere
Copy link
Contributor

DenisCarriere commented Jul 20, 2017

👍 You're correct about the Y clip, I added it there last minute.. but it was mostly the X wrapping that was the main one.

@tcql Do you want me to make a PR for this? It's already good to copy-paste the example I posted (minus the Y clip).

@mourner
Copy link
Member

mourner commented Jul 27, 2017

Superseded by #32

@mourner mourner closed this Jul 27, 2017
@tcql tcql deleted the fix-180-longitude branch July 27, 2017 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants