mapbox / mapbox-gl-native

Interactive, thoroughly customizable maps in native Android, iOS, macOS, Node.js, and Qt applications, powered by vector tiles and OpenGL
https://mapbox.com/mobile
Other
4.37k stars 1.33k forks source link

Initializers for MGLStyle #6386

Open 1ec5 opened 8 years ago

1ec5 commented 8 years ago

Now that MGLStyle behaves similar to a model object (actually a window into MGLMapView), we should add initializers to MGLStyle and replace MGLMapView's styleURL property with a style property.

We'd want some subset of the following initializers:

This enhancement would shift some of the complexity from MGLMapView to MGLStyle and reunite the default style API with the style URL code path. MGLOfflineRegion would continue to use style URLs. The existing "Style URL" IB inspectable will remain, because IB3 plugins still haven't made a comeback. :wink:

The upshot is that styles will be fully object oriented, instead of the current situation where the developer initially deals with an NSURL only to switch to MGLStyle as soon as they want to make modifications. I'm told this approach is even a bit more Reactive, although that isn't a design goal.

This has the potential to be a fairly disruptive change, so we may want to deprecate the existing NSURL-based API instead of removing it outright.

/cf mapbox/mapbox-gl-js#1989 /cc @jfirebaugh @incanus @bsudekum @frederoni

1ec5 commented 8 years ago

-initWithJSONData: would restore the functionality I took out as part of an MGLMapView spring cleaning last year. I remain convinced that there are usually cleaner, more performant, less circuitous ways to load a style, including referring to a local file URL, but I've included the JSON initializer in this proposal for completeness' sake. I think the added complexity in MGLStyle would be manageable.

Previous discussion: #900, https://github.com/mapbox/mapbox-gl-native/issues/1147#issuecomment-88171503, #1299, #1367, #1997, #4769.

1ec5 commented 8 years ago

Combined with #6383, we could also expose an -[MGLMapView setStyle:completionHandler:] in addition to the MGLMapView.style property envisioned by this proposal. I think every developer would prefer such an API over the delegate method proposed in #6412.

1ec5 commented 8 years ago

@incanus points out in https://github.com/mapbox/mapbox-gl-native/pull/6636#pullrequestreview-3370778 that, once we expose any API that makes use of setStyleJSON(), we’ll need to make any style URL API nullable.

1ec5 commented 7 years ago

Putting on the v3.4.1 milestone for the initializers themselves. The completion block–based API proposed in https://github.com/mapbox/mapbox-gl-native/issues/6386#issuecomment-249033669 would be a somewhat longer-term goal.

1ec5 commented 7 years ago

7563 would add a method to get the current style JSON, which would complement the -initWithJSONData: initializer proposed here.

1ec5 commented 7 years ago

One common task that currently requires some hand-editing of JSON is creating a raster style, whether for compatibility with TileMill or to display naturally rasterized tiles like aerial imagery. We can add a convenience initializer, -initWithTileSetIdentifier:, similar to what MapboxStatic.swift currently provides.

1ec5 commented 7 years ago

With a full implementation of this feature, the developer may expect to be able to use a single MGLStyle object with multiple instances of MGLMapView, either concurrently or in series. For example, one view controller with a map view may need to push another view controller onto the stack that has a similar-looking map view. Or perhaps an iPad application implements a split-screen view that must synchronize the two maps across runtime styling changes.

I can see how sharing a style across multiple map views could become a rabbit hole. For one thing, mbgl would need to support sharing a single mbgl::style::Style among multiple mbgl::Maps, so that we could remove the backpointer from MGLStyle to MGLMapView. Various parts of the SDK’s runtime styling code depend on this backpointer, including -[MGLVectorSource featuresInSourceLayersWithIdentifiers:predicate:].

MGLOpenGLStyleLayer’s public methods such as -drawInMapView:withContext: may also complicate matters. In these methods, the map view parameter is used for disambiguating between multiple map views and potentially for obtaining state that isn’t part of MGLStyleLayerDrawingContext. Maybe we can pass in an MGLStyle instead of an MGLMapView for disambiguation purposes.

Beyond that, I haven’t thought very deeply about the implementations of style sharing. It would be an improvement over the basic feature proposed here, which would already make the API a lot more pleasant to work with.

stale[bot] commented 5 years ago

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

1ec5 commented 5 years ago

Reopening per #14137.

1ec5 commented 5 years ago

9280 introduced a Style constructor as part of mbgl’s public API. It takes a Scheduler, FileSource, and pixel ratio, all of which can be obtained from +[MGLRendererConfiguration currentConfiguration] and mbgl::sharedThreadPool(). getURL() and getJSON() would correspond to the proposed -initWithURL: and -initWithJSONData: methods.

So here’s a sketch of what the proposed implementation would look like in MGLStyle:

- (instancetype)initWithURL:(NSURL *)url {
    if (self = [super init]) {
        [self commonInit];
        _rawStyle->loadURL(url.absoluteString.UTF8String);
    }
}

- (instancetype)initWithData:(NSData *)data encoding:(NSStringEncoding)encoding error:(NSError * _Nullable *)outError {
    if (self = [super init]) {
        [self commonInit];
        NSString *string = [[NSString alloc] initWithData:data encoding:encoding];
        if (!string) {
            if (outError) {
                *outError = [NSError errorWithDomain:MGLErrorDomain code:MGLErrorCodeUnknown userInfo:nil];
            }
            return nil;
        }

        _rawStyle->loadJSON(string.UTF8String);
    }
}

- (void)commonInit {
    MGLRendererConfiguration *config = [MGLRendererConfiguration currentConfiguration];
    _rawStyle = std::make_unique<mbgl::style::Style>(mbgl::sharedThreadPool(), config.fileSource, config.scaleFactor);
}

and in MGLMapView:

- (void)setStyle:(MGLStyle *)style {
    _mbglMap->setStyle(style);
}
1ec5 commented 5 years ago

In the meantime, if all you have is the JSON and no file on disk, a workaround for the lack of -[MGLStyle initWithData:encoding:error:] would be to save the JSON to a temporary file and set the style URL to that file’s URL.

/cc @tobrun

captainbarbosa commented 5 years ago

👋 For those following along, I wanted to circle back and note where the team is currently at regarding this issue. For iOS, we're currently considering making changes to the following APIs:

MGLStyle ✨ Introduce -[MGLStyle initWithURL:] ✨ Introduce -[MGLStyle initWithJSONData:encoding:error:]

MGLMapView ✨ Introduce -[MGLMapView initWithFrame:style] ✂️ Deprecate -[MGLMapView initWithFrame:styleURL:

Support for completion handlers, offline APIs, and additional methods such as -[MGLStyle initWithSources:layers:] may be scoped into later work.