Fix for WMS on Leaflet when using Polar stereographic #5618
Conversation
There was a problem hiding this comment.
Hi and thanks for this!
I read this quickly, and it looks like you have identified the same problems as in kartena/Proj4Leaflet#149 - it also looks like this fix indeed addresses this problem in a sensible way.
However, this pull request currently breaks the tests, and from the looks of it the code as it is now contains some syntax errors. Check the details here: https://travis-ci.org/Leaflet/Leaflet/builds/251868759?utm_source=github_status&utm_medium=notification
|
Bother, shouldn't have edited in github. I will look at this when I get to work tomorrow.
|
|
Boy that system is fussy about indents. All of them done. Now that I have figured out how this system works, it is rather nice. Is there way to run the integration checks on my fork before making a pull request? |
|
Was rather hoping for a way to use github travis. Our build/test environment is very different so a lot of unfamiliar tools to install. (starting with npm). Anyway, seems to have got past the checks. ESRI fix has real issues (as opposed to style) so looking more carefully at that now. |
There was a problem hiding this comment.
Thanks, this is looking good! I know the linting can be a bit painful when starting to work with Leaflet, but it's there for good reason so we ensure a consistent style throughout the whole code base, something I think has served us well in the long run.
I made another remark which I think needs to be addressed before merging, to make the code a bit cleaner and easier to read (even though you added good comments!), hope this seems right to you!
src/layer/tile/TileLayer.WMS.js
Outdated
| nw = this._crs.project(tileBounds[0]), | ||
| se = this._crs.project(tileBounds[1]); | ||
| // in polar projections NS lat/lon box might reversed (S 'above' N) so check and swap if need be | ||
| if (se.y > nw.y) { |
There was a problem hiding this comment.
It will look cleaner if you just turn nw and se into a Bounds instance, that will automatically handle swapping around coordinates. You can then either access them through min and max properties or its various methods for getting each of the bound's four corners.
There was a problem hiding this comment.
Okay. I can see how this would work okay with projected coordinates - using a bounds object with the lat/long was part of the original trouble.
Part of fix for Leaflet#5617 for polar WMS systems. Added to bypass using an unsafe bounds object.
Part of fix for Leaflet#5617 to fetch proper bbox coordinate on polar stereographic projections.
Cut/paste error and buch of style stuff.
This is the patch proposed in #5617