kartena / Proj4Leaflet

Smooth Proj4js integration with Leaflet.
http://kartena.github.io/Proj4Leaflet/
BSD 2-Clause "Simplified" License
589 stars 173 forks source link

Previously working EPSG:27700 not working with 0.7 #52

Closed pagameba closed 10 years ago

pagameba commented 10 years ago

Hi, I have been using Proj4Leaflet for a while and just updated to Leaflet 0.7 and also updated Proj4Leaflet and proj4js to the latest recommended versions. After updating, my xyz layers are not working and I can't quite figure out what is going on.

My code looked something like this:

var crs = new L.Proj.CRS('EPSG:27700',
  '+proj=tmerc +lat_0=49 +lon_0=-2 +k=0.9996012717 +x_0=400000 +y_0=-100000 +ellps=airy +datum=OSGB36 +units=m +no_defs', {
  resolutions: [
    5078.125,
    2539.0625,
    1269.53125,
    634.765625,
    317.3828125,
    158.69140625,
    79.345703125,
    39.6728515625,
    19.8364257813,
    9.9182128906,
    4.9591064453
  ],
  transformation: new L.Transformation(1,300000,-1,1300000)
});

var map = L.map('map', {crs: crs}).setView([51.507222,-0.1275], 4);

var layer = L.tileLayer('https://my,server/{z}/{x}/{y}', {
  maxZoom: 10,
  minZoom: 0
});

   map.addLayer(layer);

This would produce a map that is centered on London, England. After updating, the same code does not load any tiles.

As far as I can tell from tracing what happens to the tiles, when it gets into _tileShouldBeLoaded(), the value of limit returned by this.getWrapTileNum() ends up being an exceedingly small number (e^-7) and thus tilePoint.x >= limit.x fails and only the top left tile (0,0 in tile coordinates) ever gets added to the DOM in any given zoom level.

When zooming out to level 0, it seems like London is centered in the map viewport so I think the location is correct - it just doesn't load tiles other than the top left one.

Any insight would be welcome.

perliedman commented 10 years ago

That's odd. There has been a change in how TileLayer's _getWrapTileNum works in 0.7 compared to earlier versions, but it should behave exactly as before unless you have overridden the method getSize in CRS (which I guess you have not).

Also, _getWrapTileNum should return a larger value the higher the zoom level, which doesn't seem to be the case from what you write.

Any chance that you can step through _getWrapTileNum and check what's going on?

Also, it seems from your code that you haven't set the continuousWorld option on the map; this isn't something that has changed, so if it worked before, it should work in 0.7 as well. Generally though, you should set it to make sure Leaflet doesn't try to wrap or cut off your tiles on Spherical Mercator's bounds.

pagameba commented 10 years ago

In the version I was using before (0.6.4 it looks like), _getWrapTileNum() looked like this:

_getWrapTileNum: function () {
  return Math.pow(2, this._getZoomForUrl());
}

in 0.7 it looks like this:

_getWrapTileNum: function () {
  var crs = this._map.options.crs,
    size = crs.getSize(this._map.getZoom());
    return size.divideBy(this.options.tileSize);
,

It seems like crs.getSize() looks up the _scale for the requested zoom in Proj4Leaflet. At level 0 in my projection definition, the resolution is 5078.125 and the equivalent scale is 0.0001969230769. The resulting value is this number divided by 256 as both x and y of an L.point, about 7.69e-7. This is the value used as limit and compared against the tile point x/y which are integer numbers like 6 and 8 in one pass through the code. All the limit values are very close to zero regardless of zoom, which is why the 0,0 tile loads and no others do.

At this point it seems like the resolution values on my side might be suspicious if it works for other projections. The resolutions I have set up come from the resolutions reported by gdal using the same proj definition and are in projection units (meters in this case) per pixel.

pagameba commented 10 years ago

An update. We got this to work by using L.Proj.CRS.TMS instead of L.Proj.CRS, but the tile source is XYZ not TMS so we set tms: false in the layer and now it works seemingly. The documentation indicates that L.Proj.CRS.TMS is only suitable for using with layers that have tms: true. Is the documentation a bit out of date or are we doing something horribly wrong that will break sooner or later?

pthorin commented 10 years ago

Hi, @perliedman opened this issue 16 hours ago: https://github.com/kartena/Proj4Leaflet/issues/53, it sounds to be related.

perliedman commented 10 years ago

@pagameba the issue is most likely caused by a change introduced in 0.7 which can cause a regression if you use TMS and/or don't define the continuousWorld option. Technically, this comes from the fact that these to conditions force _getWrapTileNum to be used, which in turn use CRS's getSize, which uses the overridden scale method, causing wrap to occur at totally different rows than in Leaflet 0.6.4.

The quick fix for you is to enable continuousWorld (which most likely is what you want anyway). Your workaround works since L.Proj.CRS.TMS overrides getSize in a proper way. Even if it works, I would not recommend it, since it is hard to say if that class will introduce more TMS specific changes in the future, as you mention.

I've submitted https://github.com/Leaflet/Leaflet/pull/2233 to address this, if you're willing to wait for that to be merged and released.

pagameba commented 10 years ago

So setting continuousWorld also fixes the problem but it it has the unintended side effect of

  1. requesting tiles outside the valid area for the projection so there are requests to tiles that don't exist and that puts pressure on the tile server as well as a bunch of 404s in the browser console.
  2. not drawing tiles left and right of the world extents because the tile coordinates are not clipped to -180 / 180

I'm curious about the difference between L.Proj.CRS and L.Proj.CRS.TMS. The only difference in the leaflet code that I can see is the inversion of the y coordinate system for tiles and I don't understand enough about projections to see why the code in L.Proj.CRS is so much different than L.Proj.CRS.TMS.

The changes to _getWrapTileNum() seem important to support projections which do not define a single tile for level 0 and I see no mechanism in L.Proj.CRS that would handle 1 x 2 or 2 x 3 level 0 tile layout? This was the major reason for me to update as I have some tilesets that don't work correctly with 0.6.4 in this case.

For now I will need to stick with L.Proj.CRS.TMS as this seems to work the best for our configuration. The only outstanding issue is some weird scale reporting in some cases, but I suspect a bug in my code and not in Leaflet/Proj4Leaflet :)

perliedman commented 10 years ago

Ok, yes, continuousWorld changes how the bounds of the world are handled, but is this applicable to your projection (EPSG:27700). From what I can tell it looks fairly local to the brittish isles (http://www.spatialreference.org/ref/epsg/27700/)? I guess many things would not work as intended when close to lat +/-90 and longitude +/-180 (which is the only limit introduced by not enabling continuousWorld), or am I missing something. Not that it's necessarily applicable to your situation, but we use continuousWorld for all our maps using local projections for Sweden (which is closer to the north pole at least!), without seeing any problems with this. You might also note that all examples in the documentation use continuousWorld, so that is what Proj4Leaflet has been tested with.

You are correct in that L.Proj.CRS.TMS is not much different from L.Proj.CRS. It was introduced as a convenience, since many users had problems figuring out how the transformation works. The main difference in 0.7 is that L.Proj.CRS.TMS takes advantage of a new feature in Leaflet 0.7, and overrides CRS's getSize method. The issue you're seeing comes from the fact that you are not using continuousWorld and L.Proj.CRS does not override getSize. That is why using L.Proj.CRS.TMS works.

The changes in _getWrapTileNum is mainly to let L.TileLayer support TMS, bounds limiting and wrapping for other projections than spherical Mercator, which it was previously hardwired against.

I'm a bit curious about the tile sets that you've had problems with. If you post them as a separate issue, I would be happy to at least try to see if I can make some sense of it. I'm not sure I understand what you mean by a 1x2 tile layout, but it sounds like it could be related to the hardwired wrapping in 0.6.4.

pagameba commented 10 years ago

Yes you are right about 27700 being local to the British Isles, our tile set starts with a single level 0 tile that covers the British Isles. Enabling continuousWorld in this case causes Leaflet to request invalid tiles which is not a serious problem but it does send out unneeded requests.

We have other projections, like EPSG:54003 which has a proj string of +proj=mill +lat_0=0 +lon_0=0 +x_0=0 +y_0=0 +R_A +ellps=WGS84 +datum=WGS84 +units=m +no_defs. Its a cylindrical projection similar to EPSG:3857 and is valid +180/-180 and about +85/-85. This needs to be set with continuousWorld: false to enable continuous panning east/west of course.

Another projection (no EPSG code) is +proj=eqc +lat_ts=39.64611114697324 +lat_0=0 +lon_0=-96 +x_0=0 +y_0=0 +datum=NAD83 +units=m +no_defs and the data we have for this doesn't conveniently fit in a square tile at level 0 and would be better suited by level 3 being 3 tiles wide and 2 tiles high.

For each of these projections, getSize seems to provide what I am looking for in that I can define the valid extent of the tiles, then Leaflet knows the tile limits of the projection and can both avoid requesting invalid tiles and wrap the tile indexes appropriately when needed.

If you are curious, I can share the tilecaches with you off list as I would prefer that they not become generally accessible and I don't have a convenient way of blocking access to them at the moment.

perliedman commented 10 years ago

I will look into making L.Proj.CRS handle the use cases you mention, I agree that getSize should be what you're looking for. It was an oversight on my part not to add it to L.Proj.CRS in version 0.7.0, since it causes some regressions and doesn't take full advantage of the new functionality in Leaflet.

pagameba commented 10 years ago

Thanks @perliedman !

perliedman commented 10 years ago

If you can, it would be great if you could try with the latest changes (45033a767e592cce199733bc4ee7df9d0260b262).

With this, you can set the option bounds (in projected coordinates) to L.Proj.CRS, which will then be used in getSize. If not set, getSize will fallback to emulating the way Leaflet 0.6.4 and earlier versions worked.

pagameba commented 10 years ago

I will try to test, it might be a day or two before I can really give it a whirl though.

perliedman commented 10 years ago

No rush! Fixing the regression is worth the change anyway, but it's of course interesting to see if the change also covers your use case.

perliedman commented 10 years ago

I'm closing this for now, please update if you still have issues.