search

kartena / Proj4Leaflet

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

Using crs.scale with zoom level bigger than maxZoom does not throw errors #144

Open theashyster opened 7 years ago

theashyster commented 7 years ago

When using Leaflet and at some point crs.scale function is called with a zoom level bigger than maxZoom that executes this part of the code https://github.com/kartena/Proj4Leaflet/blob/master/src/proj4leaflet.js#L111 from this line https://github.com/kartena/Proj4Leaflet/blob/master/src/proj4leaflet.js#L118 we will have undefined returned if the resolutions does not have this zoom level defined.

Then Leaflet will work with the undefined and at some point [NaN, NaN] will be passed into this function https://github.com/kartena/Proj4Leaflet/blob/master/src/proj4leaflet.js#L38 for proj4js to handle with inverse function. Since proj4js does no handle [NaN, NaN] input consistently (does not always return [NaN, NaN], could return completely normal result as [0, 90]) this will result in different issues.

One of the issues will be cause by this line https://github.com/kartena/Proj4Leaflet/blob/master/src/proj4leaflet.js#L40 to throw the Invalid LatLng object message.

This is in part a problem with proj4js and I have filed an issue there https://github.com/proj4js/proj4js/issues/250, but also maybe proj4leaflet could show a warning or throw an error if https://github.com/kartena/Proj4Leaflet/blob/master/src/proj4leaflet.js#L118 returns undefined?

perliedman commented 7 years ago

Hi and thanks for this detailed writeup!

It looks pretty clear what the problem is, but it's less clear what scale should return for values outside the defined range. Maybe just returning the maximum zoom's scale is enough.

It sounds like this is a duplicate of #82, so solving this could close both.

theashyster commented 7 years ago

Hi, :)

Yep, not sure about that too. For me the main issue was that I did not specify the resolution for the zoom level I was trying to zoom to when I was already on the max zoom level (using double click from leaflet). So I just added a resolution that is half of the smallest resolution I had defined. That resulted into the scale being there for this zoom level and that was twice as big as the previous one. I am not sure if just returning the maximum zoom scale is enough or it should be doubled.

The problems this causes sound quite the same, not sure if the actual cause is the same though.

P.S. Just did a check on this in getZoomScale on leaflet and if toZoom > this.getMaxZoom() and the result we return is 2, the map works as it should, if the result is 1 the map does not move when double clicking in the max zoom level, if the result is 0.5 the map moves in the different direction when double clicking somewhere in the corners at max zoom level.

perliedman commented 6 years ago

Hi again and sorry for a very late reply!

Some thoughts on this: the issue, like you said initially, is that Leaflet will sometimes need resolution/scale for zoom levels outside the [minZoom, maxZoom] interval - which is no problem for Leaflet's default projection, since it has a formula to calculate scale, rather than static values for each zoom level.

A workaround is definitely to define resolution of extra zoom levels outside the intervals, like you did.

Ideally, we'd want a way to fix this more properly without having to understand why they would need a workaround like this. I don't have a real plan here, I'm typing this out more like a brain dump.

For most maps, the way we explicitly specify resolutions or scales is tedious and unnecessary, since there is a fixed factor between each zoom level, typically 2, just like Leaflet's default projection. We could do away with the array of fixed resolutions all together and just specify the resolution/scale for one zoom level together with the factor. This way we could calculate the scale for any zoom level - this would mean simpler configuration and solving this issue.

We could also use this as a fallback for maps which specify the scales/resolutions like today as well: use the specified resolutions inside the defined zoom levels, but fall back to calculating using a default factor when outside the array.

While not entirely fool proof, I think this would at least work better than what we have now.