Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add filter support for $id keys #2888

Merged
merged 4 commits into from
Aug 10, 2016
Merged

Add filter support for $id keys #2888

merged 4 commits into from
Aug 10, 2016

Conversation

jfirebaugh
Copy link
Contributor

@jfirebaugh jfirebaugh commented Jul 18, 2016

mapbox/mapbox-gl-style-spec#391

Before merging, we need to following upstream changes to be integrated and released:

@jfirebaugh
Copy link
Contributor Author

jfirebaugh commented Jul 18, 2016

@mcwhittemore brought to my attention the fact that once mapbox/geojson-vt#60 is integrated, we'll face an additional issue. For GeoJSON features that use string IDs, there's a GeoJSON/VT compatibility issue similar to #2434. GeoJSON allows string or numeric IDs, while VTs support only unsigned integer IDs. String values in GeoJSON feature IDs will not survive the round trip through a Vector Tile PBF which we use to transfer data from the worker to main thread.

@mourner
Copy link
Member

mourner commented Jul 18, 2016

@jfirebaugh could we replace the use of vt-pbf with geobuf? It supports the full GeoJSON spec and roundtrips exactly.

@jfirebaugh
Copy link
Contributor Author

@mourner For GeoJSON sources, yes, we could use geobuf rather than vt-pbf.

Then the question is what do we do with vector tile sources?

  1. Continue to use VT PBF format for transferring vector tile source data. Receiving end needs to deserialize in a different way depending on the source type.
  2. Transfer vector tile source data with geobuf, and figure out some way to represent the necessary layer structure. (A VT is collection of layers, each containing a collection of features. GeoJSON itself -- and thus by extension geobuf, currently -- doesn't have a way to represent this, because FeatureCollections can't be nested.)

@mourner
Copy link
Member

mourner commented Jul 19, 2016

@jfirebaugh maybe transfer a flat feature collection, but also send a JSON of the structure? Maybe like this: {offsets: [0, 235, 1243124], layers: ['foo', 'bar', baz']}

@lucaswoj
Copy link
Contributor

lucaswoj commented Aug 9, 2016

I am closing this PR because it has been inactive for a long time. The branch isn't going anywhere so please keep working on this feature and re-open the PR when it is ready!

Let me know if you have any questions.

@lucaswoj lucaswoj closed this Aug 9, 2016
@mourner mourner reopened this Aug 10, 2016
@mourner
Copy link
Member

mourner commented Aug 10, 2016

I'm picking this up tomorrow.

@mourner mourner self-assigned this Aug 10, 2016
jfirebaugh and others added 3 commits August 10, 2016 18:21
This release adds support for `$id` keys: mapbox/mapbox-gl-style-spec#391
This release adds a public "id" property to VectorTileFeature: mapbox/vector-tile-js#43
@mourner
Copy link
Member

mourner commented Aug 10, 2016

@lucaswoj good to merge?

@mourner mourner changed the title [WIP] Add filter support for $id keys Add filter support for $id keys Aug 10, 2016
@lucaswoj
Copy link
Contributor

@mourner Looks good to me!

@lucaswoj lucaswoj merged commit 28634c7 into master Aug 10, 2016
@lucaswoj lucaswoj deleted the filter-id branch August 10, 2016 17:22
@lucaswoj
Copy link
Contributor

@jfirebaugh
Copy link
Contributor Author

Did #2888 (comment) (supporting non-numeric IDs) get solved, or is there an issue for the tail work there?

@jfirebaugh
Copy link
Contributor Author

Ah, #2716 tracks the tail work.

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.

3 participants