Skip to content

Conversation

mourner
Copy link
Member

@mourner mourner commented Nov 27, 2013

This is a major refactoring of TileLayer code and everything that is related to projections.

The TileLayer part that splits it into GridLayer and TileLayer will make the code much cleaner and allow to build other tile-based implementations easier.

The long-standing projections refactoring will remove all corresponding ugly hacks across the code, consolidate everything projections-related into one place, make creating flat maps much easier, and also remove the need for ugly hacks in Proj4Leaflet plugin for different obscure projections.

GridLayer + TileLayer

  • split TileLayer into GridLayer (with all the grid / tile positioning logic) and its child TileLayer (with logic for loading image tiles from tile servers)
  • remove TileLayer.Anim.js, split animation logic between GridLayer and TileLayer
  • remove all Earth hardcode in Grid/TileLayer implementations, make everything work depending on CRS
  • remove TileLayer.Canvas in favor of a much more flexible and powerful GridLayer
  • fix bounding and wrapping in GridLayer to allow negative coordinate space
  • cover GridLayer with some tests

CRS & Projections

  • add Projection bounds property which determines projected coordinates bounds
  • add CRS properties: wrapLng, wrapLat (determining whether the world wraps and how) and infinite (if true, the layer will not be bounded, with negative tiles etc.)
  • add CRS getProjectedBounds that returns projected world bounds for zoom and corresponding Map getPixelWorldBounds, derived from Projection bounds, used for wrapping/bounding tiles
  • fix EPSG:4326 to have 2x1 tiles at zoom 0, EPSG 4326 Support Broken for TileLayers #1207
  • fix Projection.SphericalMercator to project to meter units
  • add CRS unproject
  • remove LatLng wrap and add CRS and Map wrapLatLng that depends on CRS, fix tiles' bounds are not wrapped before being checked against layer's bounds #1618 again
  • remove MapDrag worldCopyJump hardcoded logic to work for other projections
  • update Proj4Leaflet to reflect all projection changes (remove lots of ugly hacks)
  • cover CRS with more tests

Misc. changes

  • improve L.bind to prepend real arguments to bound ones, and use native bind where available (was needed for a part of the code)

Before merge

@mourner
Copy link
Member Author

mourner commented Nov 27, 2013

Needs some history cleanup with interactive rebase, will do...

@danzel
Copy link
Member

danzel commented Nov 27, 2013

Have taken a read through. Seems good so far!

@mourner
Copy link
Member Author

mourner commented Nov 27, 2013

@perliedman had to do some modifications to EPSG:4326 to make the WMS Marble example work and also fix #1207. Could you verify that please?

@mourner
Copy link
Member Author

mourner commented Nov 28, 2013

@perliedman awesome. Could you look at what would be required for Proj4Leaflet to reflect those changes?

@mourner
Copy link
Member Author

mourner commented Nov 28, 2013

@tmcw @jfirebaugh this is pretty much ready for review, only minor stuff left to do.

@perliedman
Copy link
Member

@mourner I will try to find time in the next couple of days. Mostly removing stuff I guess.

@mourner
Copy link
Member Author

mourner commented Nov 28, 2013

@perliedman awesome!

@mourner
Copy link
Member Author

mourner commented Nov 28, 2013

Copying what I said in IRC:

Final issue for the projections refactoring is how LatLng wrapping should be done. We need to move this from LatLng wrap to CRS that determines how to wrap, but how to implement it there? Should we use CRS getSize and convert the given LatLng to pixels, wrap, and then convert back? Or should we convert Projection bounds to LatLngBounds in CRS and use that for wrapping?

@perliedman
Copy link
Member

The idea to move wrapping is to handle the flat/other world's scenario, right?

I think it depends on just how weird those other world's might be.

  1. If we assume they will be mostly like LatLng, but with other bounds, putting it in CRS and convert pixels would be sufficient
  2. If we think more esoteric coordinate systems, where wrapping around x/lng would affect y/lat, it should be done in Projection, which knows what wrapping means in this particular projection

I'm leaning against the first, in CRS. I don't know any actual use case that needs the second method.

@mourner
Copy link
Member Author

mourner commented Nov 28, 2013

@perliedman yeah, I agree that esoteric CRS which would have trouble with wrapping don't need it anyway, so I'm inclined to go with a simpler method. The only issue is that wrapping by unprojecting, wrapping and projecting again seems a bit complex, so I thought we could simplify that by wrapping LatLng directly using LatLng values derived from Projection bounds.

@perliedman
Copy link
Member

Oh. Yes, that makes sense.

@mourner
Copy link
Member Author

mourner commented Nov 28, 2013

OK, now the refactoring looks pretty good and feature-complete. The only thing left before merge is to cover all that goodness with more tests.

@mourner
Copy link
Member Author

mourner commented Nov 28, 2013

Cleaned up and rebased, can be cleanly merged into master now.

@mourner
Copy link
Member Author

mourner commented Nov 28, 2013

@sheppard @pthorin @leplatrem @calvinmetcalf @turban @moonlite guys, you're welcome to review this as well — let me know what you think.

@mourner
Copy link
Member Author

mourner commented Nov 29, 2013

All the projections stuff in Leaflet FINALLY makes complete sense to me after years of "I'll figure this out later" thinking. Big thanks to @perliedman (Proj4Leaflet maintainer) for all the help.

@mourner mourner mentioned this pull request Nov 29, 2013
@perliedman
Copy link
Member

This will be a great improvement for anyone not using the already builtin projections. Also lots less to maintain in Proj4Leaflet :)

Happy that I could help!

@mattiasb
Copy link
Contributor

This looks great!
Had a quick look on the combined diff and I didn't find anything to comment on. Will hopefully have time for a longer review tomorrow (since you asked :)).

Great work!

@leplatrem
Copy link
Contributor

I feel honored you asked me :)
Looks great! It will help greatly the integration of tiled vector data!

And I guess there is a chance it fixes an inexplicable bug we had on Proj4Leaflet kartena/Proj4Leaflet#37

@calvinmetcalf
Copy link
Contributor

This seems fantastic, still looking through the code, but so far, ❤️

@turban
Copy link
Contributor

turban commented Nov 30, 2013

Looks promising! I would like to use Leaflet with a Lambert Azimuthal Equal Area Projection (image), but I'm having trouble when I use the fitBounds method for an area around the pole. Leaflet only accept bounds in LatLngs and not in projected coordinates. This still seems to be the case with this refactoring. Any plans to support projected coordinates for the fitBounds method, L.Rectangle etc.?

polar

@pthorin
Copy link

pthorin commented Dec 1, 2013

This looks really nice and really helpful for Proj4Leaflet. Well done!

@mourner
Copy link
Member Author

mourner commented Dec 2, 2013

Thanks for the feedback guys! @turban yeah, it's not currently possible to pass projected coordinates to fitBounds and L.rectangle, and L.Circle (radius) but I agree that should be addressed next. With this PR in, I think we're in a much better position to do further improvements in this direction.

@mourner mourner merged commit 9cb36d0 into master Dec 2, 2013
@mourner mourner deleted the gridlayer2 branch December 2, 2013 18:06
@mourner
Copy link
Member Author

mourner commented Dec 2, 2013

Merged, yay! 🔨

@aparshin
Copy link
Contributor

I have few comments about TileLayer refactoring (sorry for delay):

  1. Great work! Looks much better then previous version! :)
  2. _tileReady function now can be called for tiles, that are already removed (for example, because of redraw method). As a result events loading and load are fired incorrectly.
  3. tileload/tileunload events are fired with tile as the only parameter. How can user find out what coordinates has loaded tile? Maybe it can be useful to add tile coordinates to event parameters?
  4. _addTile relays on number of formal parameter of createTile function (to distinguish synchronous/asynchronous tile loading). IMHO not very obvious behavior...

Have you considered possibility not to create DOM elements for some tiles (for example, allow createTile to return null)? Real live example is tile-based vector layers. It often happens that there are no data in some tiles. The possibility to remove these empty tiles from the DOM can significantly improve performance of map movements (at least in case of 100+ layers).

@mourner
Copy link
Member Author

mourner commented Dec 20, 2013

  1. Thanks for the feedback!
  2. Can you clarify this point? Not sure how that can happen.
  3. Yep, will probably add that.
  4. Maybe not obvious, but it makes the user-facing API simpler as you don't need to specify if the tile layer is async or not when creating it.
  5. Yeah, will consider this.

@aparshin
Copy link
Contributor

Here is a test case for "2": http://jsfiddle.net/dh76y/
If you zoom-in twice fast enough, _tilesToLoad will become negative. load and loading events are fired based on this variable... (By the way, can I find somewhere builded version of Leaflet.js from the master branch?)

@dobrych
Copy link

dobrych commented Feb 20, 2014

@turban did you get your "Lambert Azimuthal Equal Area Projection" map working after all latest changes? starting same today, just curious about known issues…

@turban
Copy link
Contributor

turban commented Feb 20, 2014

@dobrych Using a polar projection with Leaflet is still difficult I think, because lack of support for projected coordinates. You're better off with D3.js or OpenLayers.

@dobrych
Copy link

dobrych commented Feb 20, 2014

@turban thanks for confirmation, you saved me some time :-) already started D3 version in parallel, will see if tile layer of d3 will work properly with polar projection

@perliedman
Copy link
Member

I'm interested in what issues you're seeing, feel free to report and problems to Proj4Leaflet (which I would assume you would want to use in this case).

There is a branch to be used together with the new projection/CRS changes in Leaflet master: https://github.com/kartena/Proj4Leaflet/tree/leaflet-proj-refactor

@turban
Copy link
Contributor

turban commented Feb 21, 2014

@perliedman Proj4Leaflet works good with cylindrical (pseudo)cylindrical projections, I'm using it for various maps in UTM and Mollweide projections. The problem araise with a projection where you need to operate in projected coordinates, not only geographical (latitude and longitude). If you have a polar projection like the one shown one the screenshot above, and want to use fitBounds to set the map view with the North Pole in the middle - it's not possible with the current fitBounds method. There are also issues with L.rectangle and L.Circle. Would be nice if you add a fix for this in Proj4Leaflet :-)

@perliedman
Copy link
Member

Sorry that I didn't read your previous comment.

If I understand correctly, the problem is not showing the tiles and not that there are actual bugs related to polar projections, but rather that the API isn't very useful in this use case, since for example bounds in the form of latitudes and longitudes does not make much sense, right? I can definitely see that it would be a problem.

I'll try to think of if there's anything we can do in Proj4Leaflet to help this!

@turban
Copy link
Contributor

turban commented Feb 22, 2014

True, the problem is not showing the tiles, but setting the view and drawing vectors on top when latitude and longitudes don't make sense.

@atombender
Copy link

What is the intended replacement for TileLayer.Canvas, which has been removed entirely? That class provided very useful functionality. I don't see any other class that fits the same purpose, and I'd rather not implement my own GridLayer descendant.

@mourner
Copy link
Member Author

mourner commented May 6, 2014

@atombender check out the debug/map/canvas.html example. It's almost the same code-wise but uses GridLayer instead of TileLayer.Canvas. It's much more flexible and as easy to use.

@atombender
Copy link

@mourner That's perfect, thank you.

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.

tiles' bounds are not wrapped before being checked against layer's bounds