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

mbgl::style::GeoJSONSource should have GeoJSON getter #7376

Closed
1ec5 opened this issue Dec 11, 2016 · 8 comments
Closed

mbgl::style::GeoJSONSource should have GeoJSON getter #7376

1ec5 opened this issue Dec 11, 2016 · 8 comments
Labels
archived Archived because of inactivity Core The cross-platform C++ core, aka mbgl runtime styling

Comments

@1ec5
Copy link
Contributor

1ec5 commented Dec 11, 2016

mbgl::style::GeoJSONSource should have a getGeoJSON() method, which would be required for fixing #7375. Currently, setGeoJSON() tiles up the passed-in GeoJSON object, but it doesn’t hang onto that object, nor does there appear to be a way to losslessly convert a GeoJSONTile back into the original GeoJSON. If it wouldn’t increase memory consumption unreasonably, perhaps GeoJSONSource would hang onto the GeoJSON object alongside the tiled data.

/cc @jfirebaugh @ivovandongen

@jfirebaugh
Copy link
Contributor

This would increase memory consumption, probably unreasonably so in some cases that the core API needs to support, such as embedded or otherwise memory constrained devices.

I've filed the inverse behavior as a bug in GL JS, and I would similarly argue that the native SDKs should choose the memory efficient route, and leave it up to the developer to keep the input data around in the case that they need it and are willing to accept the memory usage. But if this is a critical feature for iOS, and you are sure that the convenience of having the is worth the increased memory consumption in all cases, then you could implement it at the SDK level.

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 12, 2016

It is already implemented at the SDK level, but that inevitably leads to #7375. The only way I can imagine getting around both problems is for MGLStyle to have a canonical, one-to-one mapping of MGLSource objects to mbgl::style::Source objects, like it does for MGLOpenGLStyleLayers and mbgl::style::CustomLayers and like MGLMapView does for MGLAnnotations.

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 12, 2016

leave it up to the developer to keep the input data around in the case that they need it and are willing to accept the memory usage

I wanted to do this in #7377 to address #7375, but I held off because MGLShapeSource.shape’s getter and setter already appear to be the most popular part of the runtime styling API on iOS, and because it makes little sense for a property to be write-only: #7375 (comment).

@nitrag
Copy link
Contributor

nitrag commented Feb 22, 2017

Peanut gallery here: I still think that if you had append functionality there would be less memory concern because I wouldn't need to use getting/setting of the shape/geojson source. If you are going to get 50 shapes, add 1, then set 51 shapes...that's inefficient in itself.

@1ec5
Copy link
Contributor Author

1ec5 commented Feb 22, 2017

True, that’s one of the nice things about the annotation API: you can add and remove without having to regenerate the whole layer (as far as I can tell); see #6177.

@stale
Copy link

stale bot commented Nov 25, 2018

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this as completed Nov 25, 2018
@1ec5
Copy link
Contributor Author

1ec5 commented Mar 8, 2019

Still needed as a prerequisite for #6181.

@1ec5 1ec5 reopened this Mar 8, 2019
@stale stale bot removed the archived Archived because of inactivity label Mar 8, 2019
@stale stale bot added the archived Archived because of inactivity label Sep 4, 2019
@stale
Copy link

stale bot commented Sep 4, 2019

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this as completed Sep 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived Archived because of inactivity Core The cross-platform C++ core, aka mbgl runtime styling
Projects
None yet
Development

No branches or pull requests

3 participants