Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Support GeoJSON sources #2161

Merged
merged 7 commits into from
Dec 12, 2015
Merged

Support GeoJSON sources #2161

merged 7 commits into from
Dec 12, 2015

Conversation

kkaefer
Copy link
Member

@kkaefer kkaefer commented Dec 10, 2015

GeoJSON sources are a documented part of the Mapbox GL style specification. mapbox-gl-native should have full and direct support for them, regardless of whatever other annotation/external data APIs it supports.

@jfirebaugh jfirebaugh mentioned this pull request Aug 24, 2015
Closed
@incanus incanus added feature GL JS parity For feature parity with Mapbox GL JS labels Aug 24, 2015
@tmcw
Copy link
Contributor

tmcw commented Sep 13, 2015

Reiterating that this is an important feature: GeoJSON is not just a web thing. It is a part of the core Mapbox GL Style Spec that we advertise as cross-platform compatible. This needs to be implemented on the mapbox-gl-native level and available on all platforms, not just via the node bindings.

@bsudekum
Copy link

bsudekum commented Nov 4, 2015

Bumping again to put on peoples radar; this feature would be huge.

@friedbunny friedbunny mentioned this pull request Nov 19, 2015
Closed
@mikemorris mikemorris mentioned this pull request Nov 20, 2015
Merged
@bsudekum
Copy link

Bi-monthly bump, flagging this is important:

GeoJSON is not just a web thing. It is a part of the core Mapbox GL Style Spec that we advertise as cross-platform compatible.

@incanus
Copy link
Contributor

incanus commented Nov 24, 2015

Great, let's /cc @twbell on how this fits into overall product priorities. Note the P0, P1, and P2 labels already existing on this project with nearly 30 issues between them. How does this fit in?

@jfirebaugh
Copy link
Contributor Author

This is P1 in my mind after the styles API.

@tmcw
Copy link
Contributor

tmcw commented Nov 24, 2015

Is P0 higher or lower than P1

@jfirebaugh
Copy link
Contributor Author

Higher, P0s are "working on it now".

This was referenced Nov 30, 2015
Open
Closed
@kkaefer kkaefer self-assigned this Dec 10, 2015
@kkaefer
Copy link
Member

kkaefer commented Dec 10, 2015

@kkaefer kkaefer force-pushed the 2161-geojson-sources branch 2 times, most recently from e1b56b7 to 6320b4c Compare December 11, 2015 01:46
@kkaefer
Copy link
Member

kkaefer commented Dec 11, 2015

@jfirebaugh 👓

jfirebaugh
jfirebaugh reviewed Dec 11, 2015
View reviewed changes
src/mbgl/tile/geojson_tile.cpp

// A monitor can have its GeoJSONVT object swapped out (e.g. when loading a new GeoJSON file).
// In that case, we're sending new notifications to all observers.
void GeoJSONTileMonitor::setGeoJSONVT(mapbox::geojsonvt::GeoJSONVT* vt) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually used?

@jfirebaugh
Copy link
Contributor Author

Looks good. I can see that we're going to want to refactor to a Source base + subclasses for the styles API, but that can be longer term too.

Align Tile/Geometry/Tag classes of GeoJSONVT and Mapbox GL so we can avoid a copy

How big a lift is this?

@kkaefer kkaefer force-pushed the 2161-geojson-sources branch from 6320b4c to cde1ae1 Compare December 12, 2015 00:11
@kkaefer kkaefer force-pushed the 2161-geojson-sources branch from cde1ae1 to 8d711e9 Compare December 12, 2015 00:23
@kkaefer
Copy link
Member

kkaefer commented Dec 12, 2015

Align Tile/Geometry/Tag classes of GeoJSONVT and Mapbox GL so we can avoid a copy

How big a lift is this?

If we do this, we should probably move to a shared header-only module that defines c++ geometry types and is used across our gl native codebase

@kkaefer kkaefer merged commit 8d711e9 into master Dec 12, 2015
@incanus
Copy link
Contributor

incanus commented Dec 12, 2015

🎉

@kkaefer kkaefer deleted the 2161-geojson-sources branch December 12, 2015 01:11
@jfirebaugh jfirebaugh mentioned this pull request Dec 15, 2015
Closed
@tobrun tobrun mentioned this pull request Jan 20, 2016
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@kkaefer kkaefer

Labels
feature GL JS parity For feature parity with Mapbox GL JS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kkaefer @tmcw @bsudekum @incanus @jfirebaugh