Add id feature property to tiles #60
Conversation
|
@jfirebaugh like this? |
|
Yes, thanks! This looks good to me, but I'll wait for @mourner to merge and release. |
|
@mourner would love to see this merged and potentially make it into the next release of mapbox-gl-js so that |
| }; | ||
| if (typeof feature.id !== 'undefined') { | ||
| clippedFeature.id = feature.id; | ||
| } |
There was a problem hiding this comment.
This pattern will probably negatively affect performance in V8. I think it'll be fine to just do id: feature.id === undefined ? null : feature.id in place.
There was a problem hiding this comment.
The problem is it increases the size of the tiles with lots of id: undefined keys, when the original geojson has no id. I can just set id: feature.id and then delete undefined ids in the last step in tile creation?
There was a problem hiding this comment.
Are you serializing the tiles on disk, or are you concerned about memory footprint? The overhead shouldn't be significant. delete is not an option because it harms performance significantly. Also, I'd prefer id: null for non-existing ids rather than id: undefined.
There was a problem hiding this comment.
I'd prefer !('id' in feature) for non-existing ids. A low-level library like geojson-vt should be very precise about the difference between !in, undefined, and null, not assume that it won't make a difference to downstream consumers.
There was a problem hiding this comment.
@jfirebaugh it's important for properties, but I don't think that's the case for feature ids. Also, if we add the id property after defining the object without it, like in the code above, V8 has to switch a hidden class for the object every time, slowing things down, so it's a performance antipattern. It may not be significant but we should measure and make a call.
There was a problem hiding this comment.
In the context of mapbox/mapbox-gl-js#2874 what if a feature has no id provided, or any properties for that matter, it would be good to still be able to do feature highlights on hover in this case. Would this require geojson-vt to generate ids where the don't exist so that GL JS can use them in a hover filter?
|
Planning to do some benchmarking and then merge early next week. |
|
That would be great @mourner, thanks. |
|
I wanted to take a slightly different angle to implementation of this, so opened a #70 instead (using ideas from your PR). Thanks again for the PR! |
Replaces #59
fixes mapbox/mapbox-gl-js#2716