Promote Style to public API #9280
Conversation
There was a problem hiding this comment.
Thanks, this gets us a lot further towards #6386.
do we need
Map::setStyle(std::unique_ptr<Style>)
Yes, especially for implementing -[MGLStyle initWithURL:] and -[MGLStyle initWithJSON:] (see below), but also by analogy with the existing -[MGLMapView setStyleURL:]. The style sharing proposed in #6386 (comment) would probably complicate things, but it doesn’t strictly need to be part of the current effort.
Does Style need to be constructible without the Scheduler&, FileSource&, float pixelRatio arguments?
The file source and pixel ratio can be obtained via singletons, so the MGLStyle initializer could pass them in internally, just as -[MGLMapView commonInit] does today. (The ability to pass in a pixel ratio would recall #7819, in any case.) I’m less sure about Scheduler – it would be great if we could hide that parameter.
include/mbgl/map/map.hpp
Outdated
| style::TransitionOptions getTransitionOptions() const; | ||
| void setTransitionOptions(const style::TransitionOptions&); | ||
|
|
||
| // Style | ||
| void setStyleURL(const std::string&); |
There was a problem hiding this comment.
With this method, the workflow is to create the map, then set its style to a URL. #6386 also proposes exposing an -[MGLStyle initWithURL:]. I think this would require a way to initialize an mbgl::style::Style with nothing but a URL, so that later on mbgl::Map::setStyle() could be called, passing in that style. Do you see any complications with that approach? -[MGLStyle initWithJSONData:] (analogous to setStyleJSON()) is also a highly requested feature.
| CGFloat pitch = _mbglMap->getStyle().getDefaultPitch(); | ||
| CLLocationDirection heading = mbgl::util::wrap(_mbglMap->getStyle().getDefaultBearing(), 0., 360.); | ||
| CLLocationDistance distance = MGLAltitudeForZoomLevel(_mbglMap->getStyle().getDefaultZoom(), pitch, 0, self.frame.size); | ||
| self.camera = [MGLMapCamera cameraLookingAtCenterCoordinate:MGLLocationCoordinate2DFromLatLng(_mbglMap->getStyle().getDefaultLatLng()) |
There was a problem hiding this comment.
Ultimately, we’d add more internal properties to MGLStyle so that calls to _mbglMap->getStyle() become largely unnecessary in MGLMapView’s implementation.
jfirebaugh
force-pushed
the
public-style
branch
3 times, most recently
from
b1074fd
to
dd314a1
Compare
|
Cool -- I moved the JSON and URL loading methods from
Now that the loading methods are on |
| @@ -166,14 +153,12 @@ Map::Impl::Impl(Map& map_, | |||
| } | |||
| }) { | |||
| style = std::make_unique<Style>(scheduler, fileSource, pixelRatio); | |||
There was a problem hiding this comment.
Not part of this pr. But why aren't we initialising this in the member initialisation list?
There was a problem hiding this comment.
It probably should be.
| void setTransitionOptions(const TransitionOptions&); | ||
|
|
||
| // Light | ||
| Light* getLight(); |
There was a problem hiding this comment.
Should we use indentation here that is more common? Not sure if this helps with readability, but it prevents using standard formatting tools.
There was a problem hiding this comment.
It make clear the parallelism with the const version of the accessor -- you can immediately see where the consts are added, that they are in the right place, and that there aren't any other differences in the signatures. I'm opposed to using formatting tools that can't handle nuances like this.
src/mbgl/style/style_impl.cpp
Outdated
| loaded = false; | ||
| url = url_; | ||
|
|
||
| styleRequest = nullptr; |
There was a problem hiding this comment.
Is this necessary? The next line would already destruct any previous style before assignment right?
There was a problem hiding this comment.
It's not necessary, I'll remove it.
| } | ||
|
|
||
| Layer* Style::Impl::addLayer(std::unique_ptr<Layer> layer, optional<std::string> before) { | ||
| // TODO: verify source |
There was a problem hiding this comment.
Great you got rid of BackendScope here!
Move the responsibility for initialization/deinitialization/rendering to RenderCustomLayer. This eliminates special case code from Map and Style.
Make
style.hppa public header, provideStyle& Map::getStyle(), and move runtime-styling APIs that previously lived onMaptoStyle. This is a refactor made relatively easy by the async rendering changes. It reduces the responsibilities of theMapobject itself and is a prerequisite for #6386. @1ec5, can you review what else is needed from core to support #6386? In particular, do we needMap::setStyle(std::unique_ptr<Style>)? DoesStyleneed to be constructible without theScheduler&, FileSource&, float pixelRatioarguments? I think we can hide those if need be.