Add a hook to be notified when point features are added #68
Conversation
src/Leaflet.VectorGrid.js
Outdated
| @@ -5,6 +5,7 @@ L.VectorGrid = L.GridLayer.extend({ | |||
| options: { | |||
| rendererFactory: L.svg.tile, | |||
| vectorTileLayerStyles: {}, | |||
| pointFeatureHooks: [], | |||
There was a problem hiding this comment.
I would prefer this to be oneachfeature, to align better with L.GeoJSON - it's essentially the same functionality.
Also, I'd prefer to keep it simple and allow one hook - again, keep things similar to existing API to avoid surprises.
There was a problem hiding this comment.
A few reasons why I did it this way:
- the concept of feature in a vt seems less clear than in geojson to me
- vt have layers not geojson, it separates things neatly
- I see it more like vectorTileLayerStyles functions, it is external styling by the user
- It may not imitate L.GeoJSON but it imitates VectorGrid existing options
- a single callback would be executed for very dense vt layers that the user may not be interested in. This includes the user's code and the pre-processing on our side (like transforming tile coords into map coords)
src/Leaflet.VectorGrid.js
Outdated
| @@ -60,6 +63,21 @@ L.VectorGrid = L.GridLayer.extend({ | |||
| } | |||
| } | |||
|
|
|||
| // The user asked to be notified if point type features added to the current vt layer | |||
| if (pointFeatureHook && feat.type === 1) { | |||
There was a problem hiding this comment.
Do you see any particular reason this should only be done for points?
IMHO, adding this functionality for any feature would be more powerful and avoid cluttering the API with type specific hooks.
If added as oneachfeature, we could pass it the created layer as well as the original feature, more or less exactly as L.GeoJSON does. Also, I imagine doing it this way would heavily reduce the amount of code needed.
There was a problem hiding this comment.
Features other than points can be split into multiple tiles no ? So the callback will be called multiple times for the same actual object on the map right ? It seemed to me that it would not be easy to manage for the user. Points are straightforward. Butl I agree, I would love to make it more generic.
It did not feel right to pass the created layer or the vt feature as they are not standard objects and their geometries are relative to the tile they are in. I did not want to let the user make the calculations to transform coords in the tiles into coords in the map. Also if the user chooses to configure empty style on a vt layer and create a marker manually instead of letting VectorGrid render a pseudo marker, then there is no layer to be created, rendered and sent back to the user by VectorGrid.
|
Hi, and thanks for looking into this! I added a review with some suggested changes that I think would improve this functionality and make it a bit more general. Other than that, I agree with your comments - docs would be great, and I agree a comment about performance implications would be good. |
|
Hi, I just answered to your comments defending my initial commit :) But actually I see your point, a simple single callback that does not clutter the API would be nice. But it would require to reduce processing as much as possible, so no transformation of tile coords into map coords. Maybe just resolve the discrepancy of having different geometry notations from geojson-vt and from pbf. Less work in the API, a little more work on the client side. |
|
Another version in the last commit. There is a single onEachFeature callback. But it has extra parameters compared to L.GeoJSON to give all relevant info to the user. Also I wrote some little util functions to switch from tile geometry to a point or a latlng. Otherwise I feel the client code is overly complicated. Note that the code could be slightly refactored so that these functions are used internally as well. |
|
The last commit is just an idea to make user's code much more readable and straightforward. You may consider it outside the scope of VectorGrid. Actually it could be a GridLayer functionality, binding manually created layers to part of the grid and clean them implicitly seems useful. |
|
@albanm hi, sorry for dropping out here for a while. While I understand the use for the extra functions in the last commit, I think it's better to leave this out and keep VectorGrid focused. As long as we make sure to offer the right extension points, users can add this or similar functionality themselves. |
|
I'd love to see progress on this. I'll have a look myself, but if anyone with more expertise felt inclined to look again, that would be great. |
|
Having tweaked this to work with current master (or at least my fork), it seems to do what is required. Thanks, @albanm! |
|
@albanm In the demo (and when I've tried to implement this), I seem to be getting duplicate labels for each feature. How is that happening, and how could we fix it? |
|
Honestly I had totally forgotten this PR. And I don't have the time to look into it again soon. Sorry about that. |
Updated from Leaflet#68
|
Any ideas if this will be implemented? |
This solves #67
The demo is very explicit I think. The user can define a function for each vt layer to be called when a point feature is added. This function will receive the feature's properties, the tile coordinates and a point. Enough information to draw a marker on the map for example.
The demo also includes a cache to clear markers when the tiles are unloaded. It is a bit verbose. Maybe VectorGrid could implement this cache of user defined layers (the return of the hook function for example) for a smoother integration.
I didn't write the documentation yet, I wanted to get some feedback before. I suppose that it should include a warning about performance because many vector tiles layers are too dense for this feature to be a good idea.