Add "geoJSONToTile" method - #38
Conversation
…ojson-to-tile # Conflicts: # src/index.js
| features = clip(left || [], pow2, (y - buffer), (y + 1 + buffer), 1, -1, 2, options); | ||
| } | ||
|
|
||
| return features |
There was a problem hiding this comment.
I think I would expect an empty array instead of null, can features really be null?
There was a problem hiding this comment.
Yes, clip can return null https://github.com/maplibre/geojson-vt/blob/main/src/clip.ts#L15
There was a problem hiding this comment.
an empty array as a return value is a better API I believe.
There was a problem hiding this comment.
This function returns a tile, not an array. I have just pushed 57ecfc5 which returns an empty tile instead of null.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #38 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 11 +1
Lines 752 764 +12
Branches 193 199 +6
=========================================
+ Hits 752 764 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for taking the time to open this PR! Looking at the cod, the tests and the coverage report, I'm guessing there's some cases missing in terms of testing... [Optional] I would also consider moving this method to a different file as it is not really part of the geojsonvt class... |
|
I spent quite a bit of time looking into this and thinking it through. While this approach may be beneficial in some cases, it likely won’t help with larger datasets. The performance benefit of geojson-vt comes from ascending the tile pyramid until it reaches geometry that has already been processed, and then only handling a small subset at deeper zoom levels. Its clipping is also incremental - it occurs after traversing upward to the deepest clipped tile and then reclipping back down. In contrast, geoJSONToTile processes the entire geometry on every tile request, which removes most of those performance advantages. I’m fine with adding this to the package, but I'd strongly prefer that the function and its options live in a separate file and be exported as a completely independent class. |
6cacff8 to
c96b8a3
Compare
Done in c96b8a3 |
0fc0a56 to
bb7048f
Compare
I just added a couple more test cases. |
I agree with you that this won't lead to any maplibre-gl-js perf improvements. This function serves other users. |
|
Added a few last comments, looks good otherwise. |
|
@lucaswoj It would be interesting to know which cases this is actually more performant. If it applies to many cases, or especially animation cases, then it may be worth adding an option into MapLibre to allow for the use of this method. |
|
@wayofthefuture We use this function extensively for creating vector tiles dynamically from a vector provider that doesn't support vector tile generation on the server. IE the user provides a vector tile source and then we provide a traditional ZXY endpoint that dynamically creates tiles from the provided data source. In this case the server only needs a single tile and not the entire pyramid. |
|
|
@ingalls, @lucaswoj Not sure if this helps I'm sure my benchmarks need a little adjustment... see the PR here: #40 |
|
How is it possible that noon indexed is faster than to tile? Caching? |
Because the test is requesting 4 tiles to simulate 1 map view load, meaning that it re-parses the full geometry on each tile. In reality it would likely be closer to 16 tiles, or: |
|
Thanks all for adding this function. It looks like this PR was merged, but the code was never pushed to NPM. Is there any more work that I can help with so you can release this to NPM? This would help with a use case I currently have where I am attempting to wrap an external JSON API as a MVT API. It would help reduce having to build multiple tiles, and allow me to do this: async mvt(z: number, x: number, y: number, options: PlacesSearchOptions): Promise<Uint8Array> {
// Get 100 points in this tile
const places = await this.placesInTile(z, x, y, options)
// Shortcut if no places were found
if (!places.length <= 0) {
return new Uint8Array()
}
// Convert the raw API response to GeoJSON
const yelpGeojson = this.pointsToGeojson(yelpPlaces)
// Convert the GeoJSON to specifically the tile requested
const mvt = geoJSONToTile(yelpGeojson, z, x, y, { extent: 4096 })
// Convert the tile to a PBF byte array
return vtpbf.fromGeojsonVt({ [term]: mvt })
} |
|
We want to include: And then release a version that will be used in maplibre-gl-js to facilitate for geojson updates. |
|
@jgimbel version 6 should include this, let me know if you have any issues with it. |
|
Works great for me. I am not in a rush, I was confused because this was merged into main, but not put on npm. It made me wonder if this was forgotten, dead, or on hold for a second. I understand what is happening now. I can obviously get around this for now by using the old method. It is fast enough for the ~100 geojson I convert. const places = await this.placesInTile(z, x, y, term)
if (!places || places.length === 0) {
return new Uint8Array()
}
const placesGeojson = this.placesToGeojson(places)
const geojsonVectorStore = global.geojsonvt(yelpGeojson, {
maxZoom: z,
indexMaxZoom: z,
tolerance: 0,
updateable: false,
})
const mvt = geojsonVectorStore.getTile(z, x, y)
return vtpbf.fromGeojsonVt({ [term]: mvt as unknown as GeoJSONVT }) |
|
Version 6 was released, you should be able to use it now. |
Just noticed this change is now available in the MapLibre fork. As such, I will be migrating our project to use it. Our use case for this function is to translate WFS server responses (to requests made at tile boundaries) into vector tiles on the fly. This made adding WFS support to our existing vector tile data pipeline relatively simple. |
|
@neodescis Great! That is the intended use case of this feature. |
This feature was added to
geojson-vt-cppat mapbox/geojson-vt-cpp#65It was ported to
geojson-vtby @montzkie18 at mapbox/geojson-vt#152 (related to mapbox/geojson-vt#92 ) but has not been merged by the project maintainers there.The demand for this feature seems to be reasonably high and the cost of adding it very low. So I'm humbly proposing that we merge it into this
maplibre/geojson-vtfork.Original PR comments: