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

Optionally persist layers and sources across styles #6180

Open
1ec5 opened this issue Aug 26, 2016 · 21 comments
Open

Optionally persist layers and sources across styles #6180

1ec5 opened this issue Aug 26, 2016 · 21 comments
Labels
annotations Annotations on iOS and macOS or markers on Android feature gl-ios iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling

Comments

@1ec5
Copy link
Contributor

1ec5 commented Aug 26, 2016

Annotations added to the map persist even after switching styles. This can be especially handy for an application that allows the user to switch back and forth between Streets and Satellite: any dropped pins, drawn shapes, or route lines remain on the map the whole time. The developer isn’t responsible for rebuilding the data on each style change.

Per #5626 (comment), for the runtime styling API to have parity with the annotation API, there would need to be some way for certain layers and sources to optionally persist after switching styles. I’m not sure whether the SDK would have to take full responsibility for persisting this data, or whether mbgl can keep track of some state.

/cc @jfirebaugh @incanus @frederoni

@1ec5 1ec5 added feature iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS annotations Annotations on iOS and macOS or markers on Android runtime styling labels Aug 26, 2016
@jfirebaugh
Copy link
Contributor

This is a well-recognized problem, but there appear to be few designs that don't introduce significant additional complexity of their own.

mapbox/DEPRECATED-mapbox-gl#25 has a proposal for one possible approach.

@boundsj
Copy link
Contributor

boundsj commented Feb 28, 2017

For reference, here is a post that illustrates the kind of workaround that platform developers must do to manage runtime styling sources/layers on top of changing styles.

@nitrag
Copy link
Contributor

nitrag commented Feb 28, 2017

@1ec5 , Would it be much work for the SDK to remove the source+layers from the mapview right before a style change, keep the references, and add them directly back when the style is ready?

I think asking the developers to do this isn't efficient and it's not clear in the docs that this WILL happen on style changes.

I would need to maintain a class level variable for MGLShapeSource to retain the shapes, right?

@boundsj
Copy link
Contributor

boundsj commented Feb 28, 2017

@nitrag Would it be much work for the SDK to remove the source+layers from the mapview right before a style change, keep the references, and add them directly back when the style is ready?

Such an approach might be blocked by #7437

@nitrag
Copy link
Contributor

nitrag commented Mar 7, 2017

I discovered a bug with the workaround in #7124 (comment). If you use a class level variable to hold a MGLShapeSource and even though you properly remove the source from the map before changing the style and adding it back to the new style you will get the following crash. Reproduced with aggressive zoom behavior and standard annotations on the map.

libc++abi.dylib: terminating with uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument

updateUserLocationAnnotationView
image

I've resorted to wiping all the layers clean and starting fresh each time the style changes. Not perfect solution.
Also, this prevents the use case of being able to record users tracks on a map and being able to change the map style when doing so.

Please consider this a friendly bump in priority for this feature.

@1ec5
Copy link
Contributor Author

1ec5 commented Mar 7, 2017

If I’m not mistaken, yanking a source out from under a layer that uses it could result in undefined behavior. So to be safe, you’d have to remove the layers before removing the source that they use, the change the style, then add the sources and layers back, in that order. I recognize that isn’t a far cry from starting afresh.

@anandthakker
Copy link
Contributor

Here's a broader tracking ticket for designing a style-spec level solution to this problem: mapbox/mapbox-gl-js#4225

@1ec5
Copy link
Contributor Author

1ec5 commented Mar 7, 2017

mapbox/mapbox-gl-js#4225 would only be a complete solution inasmuch as we plan to remove the ability to switch styles altogether, although it would eliminate one use case for switching styles.

@nitrag
Copy link
Contributor

nitrag commented Mar 7, 2017

@1ec5, you're right, I was removing source before layer. I've optimized my code but still getting errors

var recordGeoJSON = MGLShapeSource(identifier: "record-source", shape: nil, options: nil)
...
...
func pseudoChangeMapStyle(){
    if let recordS = self.mapView.style?.layer(withIdentifier: "record-style"){
        self.mapView.style?.removeLayer(recordS)
    }
    self.mapView.style?.removeSource(self.recordGeoJSON)
    //also tried:
    /**
    if let recordS = self.mapView.style?.source(withIdentifier: "record-source"){
        self.mapView.style?.removeSource(recordS)
    }
    **/

    ...
    self.mapView.styleURL = newStyle
}

func mapViewDidFinishLoading() {
    ....
    style.addSource(self.recordGeoJSON)
    let recordStyle = MGLLineStyleLayer(identifier: "record-style", source: self.recordGeoJSON)
    ...
    style.addLayer(recordStyle)
    ....
}
func updateRecordingLine(_ geojson: JSON){
    if let geojsonData = try? geojson.rawData() {
        self.recordGeoJSON.shape = try? MGLShape(data: geojsonData, encoding: String.Encoding.utf8.rawValue)
    }
    //also tried
    /**
    if let geojsonData = try? geojson.rawData() {
        if let source = self.mapView.style?.source(withIdentifier: "record-source") as? MGLShapeSource{
            source.shape = try? MGLShape(data: geojsonData, encoding: String.Encoding.utf8.rawValue)
        }   
    }
    **/
}
func removeRecordingLine(){
    if let source = self.mapView.style?.source(withIdentifier: "record-source") as? MGLShapeSource{
        source.shape = nil
    }
}

I'll either get this error:

Location update received
offroad2(25727,0x114ad83c0) malloc: *** mach_vm_map(size=11448890837103263744) failed (error code=3)
*** error: can't allocate region
*** set a breakpoint in malloc_error_break to debug
libc++abi.dylib: terminating with uncaught exception of type std::bad_alloc: std::bad_alloc

or this one:

libc++abi.dylib: terminating with uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument

or a crash in CF_RUN_LOOP.

I think this is because my location delegate is trying to update self.recordGeoJSON's when it's in transition of ownership phase?

As you can see above, I've tried self.recordGeoJSON and let source = self.mapView.style?.source(withIdentifier: "record-source") as? MGLShapeSource and they don't show any impact/difference.

Either way, what a mess.

@nitrag
Copy link
Contributor

nitrag commented Mar 8, 2017

I've since removed class level style layers and sources variables and am reloading data on every style change in mapViewDidFinishLoadingMap. While inconvenient, at least it's stable this way. Sorry for the noise. Looking forward to this feature.

☀️

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 4, 2017

Per nitaliano/react-native-mapbox-gl#812 (comment), React Native Mapbox GL automatically removes and readds layers and sources when the style URL changes, essentially working around this issue for the time being. Unfortunately, I think it foils any transitions between styles.

@captainbarbosa
Copy link
Contributor

Related: #12167

@anandthakker
Copy link
Contributor

mapbox/mapbox-gl-js#4225 would only be a complete solution inasmuch as we plan to remove the ability to switch styles altogether, although it would eliminate one use case for switching styles.

@1ec5 what are use cases for switching-style-but-persisting-some-sources/layers that couldn't (or shouldn't) be covered by composable style components?

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 28, 2018

It’s difficult to answer concretely based on the current style component or nested style proposals, but a layer may be used to visualize data that remains relevant despite a wholesale change to the style. For example, it’s typical for an iOS application to display annotations on the map and expect the annotations to remain visible when switching between the Outdoors and Satellite styles.

The iOS navigation SDK automatically switches between its day and night styles (for example, when entering or exiting a tunnel); the route line and any symbol layers representing destinations have to be reapplied manually. That’s workable for the navigation SDK, because the route line can be derived from the stored route data on demand. But an application that puts its own data on the map can’t use the map as a reliable storage mechanism if the style URL ever changes.

I filed this issue as part of #6181, as a prerequisite for reimplementing the annotation API atop the runtime styling API. Developers treat MGLMapView as a data store when they add point and shape annotations to the map. They don’t expect the data to be manipulated (#6178, #7376) or lost as a result of ostensibly visual changes (this issue).

@anandthakker
Copy link
Contributor

It’s difficult to answer concretely based on the current style component or nested style proposals, but a layer may be used to visualize data that remains relevant despite a wholesale change to the style. For example, it’s typical for an iOS application to display annotations on the map and expect the annotations to remain visible when switching between the Outdoors and Satellite styles.

Ah -- yeah, it may not be clear in the existing proposals in, but I think of this type of use case as one of the core requirements that any style components design should address.

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 29, 2018

Perhaps “style components” is a misnomer in that case, because I think developers might still expect this behavior when switching between two wholly different styles, not necessarily a single style that composes or nests two substyles. For example, consider this workflow in MapKit or the Google Maps SDK:

  1. Set the style to the built-in Standard style.
  2. Add some pins to the map.
  3. Set the style to the built-in Satellite style.
  4. Observe that the same (object-equal) pins are still there, ready to be interacted with.

This workflow doesn’t require the developer to create or customize a style to accommodate the pins or the switch from Standard to Satellite. It just works. This workflow is currently implemented with the annotation API. If the style components feature is to address this workflow, then I think it would subsume the concept of a style, bringing us back to square one.

@stale stale bot added the archived Archived because of inactivity label Dec 26, 2018
@stale
Copy link

stale bot commented Dec 27, 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 Dec 27, 2018
@1ec5
Copy link
Contributor Author

1ec5 commented Mar 8, 2019

Still needed as a prerequisite for #6181.

@songyuyang0918
Copy link

songyuyang0918 commented Jul 12, 2019

@1ec5 In mapbox-ios: I wonder if modifying styleURL to preserve layers and sources is supported?
Is it implemented as a plug-in?
I've looked at all your discussions and I can't find a solution.

@1ec5
Copy link
Contributor Author

1ec5 commented Jul 26, 2019

In mapbox-ios: I wonder if modifying styleURL to preserve layers and sources is supported?
Is it implemented as a plug-in?
I've looked at all your discussions and I can't find a solution.

No, there is no general solution to the problem at the moment. Your options are:

  • Manually redo any runtime styling changes.
  • Avoid switching styles and instead manipulate the current style to look like a different style.
  • Use annotations for pins and overlays. However, note that the plan in Reimplement MGLAnnotation atop MGLSource and MGLStyleLayer #6181 is to reimplement annotations as style layers. Unfortunately, it’s likely that that’ll happen before this issue is fixed, so annotations will no longer persist across styles and you’ll need to use one of the other options.

/cc @chloekraw

@stale
Copy link

stale bot commented May 22, 2020

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
annotations Annotations on iOS and macOS or markers on Android feature gl-ios iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling
Projects
None yet
Development

No branches or pull requests

7 participants