glenrobertson / leaflet-tilelayer-geojson

Leaflet TileLayer for GeoJSON tiles
Other
250 stars 79 forks source link

Direction of leaflet-tilelayer-geojson / alternative approaches #1

Closed lxbarth closed 10 years ago

lxbarth commented 12 years ago

@glenrobertson - we started to use leaflet-tilelayer-geojson over on http://github.com/mapbox/owlviewer. I wound up rewriting very similar functionality. I'd like to share my thinking here and figure out where it could make sense to join forces.

I needed jsonp support and I found an issue where the _map pointer in the layer object (I believe) wound up being null under certain circumstances.

Partly to understand the problem space better I pared down your approach to this:

https://gist.github.com/4019660#file_geojson_tiles.js

Demo:

http://bl.ocks.org/4019660

  1. You'll see these are just first steps, but I've tried to keep the code to a minimum and aimed for minimal duplication with Leaflet's functionality.
  2. Looking at the resulting data, the main difference in the approach is that geojson-tiles.js creates a single GeoJSON layer from multiple tiles, while leaflet-tilelayer-geojson creates a layer per tile. Further down the line, I'm hoping to be able to assemble geometries that span across tiles. I'm curious as to why you went the layer-per-tile route and how you're thinking of creating intact geometries across layers.
  3. Due to (2) you'll see that my demo under certain cirumstances does not load all data. That's because if one data tile fails due to invalid data, Leaflet stops building the GeoJSON layer. Not sure how to solve this without overwriting geometryToLayer() and addData()
  4. I've tried to reuse as much functionality of L.TileLayer as possible. You'll see that this approach requires slightly more responsibilities from the API user, but I feel it's preferable over trying to create a cross between L.TileLayer and L.GeoJSON - nothing in Leaflet helps you to do so so you'll wind up forking a lot of code.
  5. I'm not implementing hover interactivity in this demo, but you see how it would be simple to add.
  6. Am I missing any important features of tile rendering you'd be looking for?

Looking forward to your thoughts.

/cc @ppawel

ppawel commented 12 years ago

Important features from my point of view:

Also I would think about a layer of abstraction above TileLayer that would allow reusing some of the functionality (e.g. request management) with different AJAX-based tile layers, e.g. with JSON tiles.

glenrobertson commented 12 years ago

Hey @lxbarth / @ppawel

Thanks for the help so far! It's good to have some help with this... @lxbarth's version is a lot more concise!

I pretty much agree with everything @lxbarth said - However I'm not sure I understand what you mean about Leaflet stopping rebuilding the Layer on an invalid data. e.g. why can't it keep building the layer for subsequent requests?

@ppawel I also agree with all of your suggestions, especially the need for a 'TileLayer.Data' type layer to load arbitrary JSON tiles.. I had written something similar that loaded the data tiles and trigger tileloaded events, which I listen to, to retrieve the JSON objects..

The most important feature that I have been trying to figure out is how best to merge 'feature parts' (geometries that have been trimmed to the tile boundary). I have been exploring two potential methods:

  1. Union the geometries in adjacent tiles (http://stackoverflow.com/a/981442/315117 ?)
  2. Build a Multi Geometry for each unique feature across the tile-set, and hide the tile outlines (where geometries intersect tile boundaries).. Not sure if it's possible to do the equivalent of setting { outline: false } on just the part of a path that intersects a tile boundary.

Regarding @ppawel's suggestion around arbitrary feature properties - maybe we can just pass some kind of function into the Layer options to identify if a 'feature part' belongs with another 'feature part'. Something like:

options: {
    .... ,
    featureMergeKey: function (feature) {
        return feature.id.toString();  // or some other set of properties concatenated...
    }
}

Have you heard of any plans in regard to AJAX request handling in Leaflet? Would be good to remove the JQuery dependency at some point.

Will be good to hear what you guys think!

lxbarth commented 12 years ago

Hey glen -

geometry merging could be left up to the api user. I am thinking that it will be very use case and data specific. Look at my latest iteration here http://github.com/mapbox/owlviewer and how there is now a base class L.TileLayer.Data that does not even have any assumptions about what tupe of data it handles.

No idea on async request handling in leaflet. I just know jsonp is hacky and i'm starting to see us running into problems when we fire off too many requests at the same time. E. g. when i create and load 2 data tile layers at once. Wonder how that's going to work on retina :p

http://twitter.com/lxbarth

On Nov 7, 2012, at 5:47 PM, Glen Robertson notifications@github.com wrote:

Hey @lxbarth / @ppawel

Thanks for the help so far! It's good to have some help with this... @lxbarth's version is a lot more concise!

I pretty much agree with everything @lxbarth said - However I'm not sure I understand what you mean about Leaflet stopping rebuilding the Layer on an invalid data. e.g. why can't it keep building the layer for subsequent requests?

@ppawel I also agree with all of your suggestions, especially the need for a "TileLayer.Data ?" type layer to load arbitrary JSON tiles.. I had written something similar that loaded the data tiles and trigger tileloaded events, which I listen to, to retrieve the JSON objects..

The most important feature (excuse pun?) that I have been trying to figure out is how best to merge 'feature parts' (geometries that have been trimmed to the tile boundary). I have been exploring two potential methods:

Union the geometries in adjacent tiles Build a Multi Geometry for each unique feature across the tile-set, and somehow hiding the tile outlines where the geometries are trimmed.. Not sure if it's possible to do the equivalent of setting { outline: false } on just the part of a path that intersects a tile boundary. I am not sure about @ppawel's suggestion around arbitrary feature properties? Maybe we can just pass some kind of function into the Layer options to identify if a 'feature part' belongs with another 'feature part'. Something like:

options: { .... , featureMerge: function (feature, otherFeature) { feature.properties.id === otherFeature.properties.id } } Have you heard of any plans in regard to AJAX request handling in Leaflet? Would be good to remove the JQuery dependency at some point.

Anyway that's my thoughts.. Will be good to hear what you guys think!

— Reply to this email directly or view it on GitHub.

glenrobertson commented 11 years ago

Have updated the master to be based pretty much on the way @lxbarth's gist works, but split into TileLayer.Ajax, and TileLayer.GeoJSON a one-liner to set up (passing the L.GeoJSON to the constructor).

Also found an issue where if you zoom twice in quick succession, it appears that _tilesToLoad in the TileLayer class can go into negative, which causes a few tiles to be missing when the user has finished zooming..

_update: function() {
    if (this._map._panTransition && this._map._panTransition._inProgress) { return; }
    if (this._tilesToLoad < 0) this._tilesToLoad = 0;  // <--- need to add this line
    L.TileLayer.prototype._update.apply(this, arguments);
}