Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Runtime styling API #416

Closed
tsemerad opened this issue Sep 19, 2016 · 18 comments
Closed

Runtime styling API #416

tsemerad opened this issue Sep 19, 2016 · 18 comments

Comments

@tsemerad
Copy link
Contributor

I've been following the progress on implementing the runtime styling API on both the iOS and Android SDKs. They both appear to be available in the latest Android v4.2.0 beta and the iOS v3.4.0 alphas. I want to start a conversation on how this should be implemented in react-native-mapbox-gl.

My first thought is to be able to pass in the style JSON as an immutable JS object, and the component itself determines what individual changes should be made. That is exactly what happens in react-map-gl, as well as what appears to be built into Mapbox GL JS now as diffStyles.

Do we think this is the right approach? I'd be interested to hear if there are plans to implement a diffStyles equivalent in the iOS and Android SDKs, or if this is something that should just be made in react-native-mapbox-gl.

The other option is to expose the necessary methods to imperatively modify the current style. This is less ideal for my situation, as I'm already managing my style JSON in a Redux store. And it also seems to be less in line with the React way of doing things, as pointed out in the above diffStyles article.

@1ec5
Copy link
Contributor

1ec5 commented Sep 20, 2016

We don't have a timetable for implementing diffStyles in the Android and iOS SDKs (see mapbox/mapbox-gl-native#5665). It isn't clear how such a feature would fit into the existing APIs. The closest thing I can fathom as part of the SDK would be turning MGLStyle into a proper model object that you can create from JSON: mapbox/mapbox-gl-native#6386.

However, diffStyle itself would be a prime candidate for an external library that might one day be folded into the SDK, if it makes sense. Perhaps the existing diffStyles function can serve as a template for an implementation specific to RNMBGL.

More broadly, the purely declarative model in React Native is not a great fit for the largely imperative APIs that the Android and iOS SDKs expose, from annotations to location updates to offline packs. (This also limits the level of integration we can provide with Interface Builder.) There will always be impedance mismatches, but we welcome pull requests in this project that help to paper them over.

@tsemerad
Copy link
Contributor Author

Thanks for the quick response!

Turning MGLStyle into a model object, complete with initWithJSONData, would be a great fit for passing in a style JSON that is managed in a Redux store (or any other Flux implementation). I'm guessing it probably wouldn't be as efficient as individually updating style properties as the JSON changes, but maybe it would be efficient enough?

One of my use-cases is to toggle the visibility of a layer. So if I pass in a style with the layer visible, then pass in the exact same JSON, except that the layer isn't visible, would it just update that layer without flickering the rest? Even if it did flicker the rest, being able to pass in a style JSON object and having the map update to it would be a big win for RNMBGL.

Either way, it sounds like implementing a diffStyles algorithm in react-native-mapbox-gl is a good path to go down. I like the idea of mimicking diffStyles from Mapbox GL JS, and also having MGLStyle with initWithJSONData to fall back on (and to initialize the style from).

@tsemerad
Copy link
Contributor Author

I'm going to try my hand at using the Runtime Styling API on iOS. Are there any examples using it, or documentation of it anywhere? I know it's still in alpha. I've found the Mapbox Android test app, but no iOS test app.

@1ec5
Copy link
Contributor

1ec5 commented Sep 20, 2016

The iosapp test application is located in the mapbox-gl-native repository. It’s considerably less polished than the Android equivalent. See this readme for instructions on getting the application running on a device or simulator.

We’ll publish examples once v3.4.0 is released; until then, you can use GL JS’s wide range of examples as a model for what you’d do on iOS.

API documentation based on documentation comments is available online (also in Xcode and Dash). Note that various runtime styling–related APIs are missing from the documentation: mapbox/mapbox-gl-native#5959 mapbox/mapbox-gl-native#5960 mapbox/mapbox-gl-native#5953. The style specification documentation may help you fill in those gaps. However, note that it often talks about types in JavaScript terms that don’t necessarily line up with the iOS SDK. Refer to the SDK documentation for correct type information, since we’ve tried to reuse Foundation and UIKit types like NSPredicate, UIEdgeInsets, and UIColor as much as possible.

Finally, note that I’m currently working on a revamp of the type system for MGLStyleLayer classes (mapbox/mapbox-gl-native#5970) that should make the properties easier to use but will be incompatible with the current code.

Hope this helps! Please feel free to file issues in the mapbox/mapbox-gl-native repository whenever you spot problems in the SDKs.

@xrayman
Copy link

xrayman commented Oct 6, 2016

@tsemerad Hi. Have you managed to implement layers visibility toggling on iOS? I'm curious if it's possible using iOS SDK v3.4.0

@tsemerad
Copy link
Contributor Author

tsemerad commented Oct 6, 2016

It certainly looks like it's possible using the latest iOS SDK v3.4.0 alpha, but I haven't had time to implement it yet. Implementing feature querying turned out to be a higher priority. For now I'm switching between 2 separate styles, one with the layer and one without, but this is just a temporary hack.

@esamelson
Copy link
Contributor

esamelson commented Dec 21, 2016

I'm currently working on exposing the addLayer and addSource methods to the React Native component. Currently, when I pass in a JSON layer object to addLayer, I'm just running it through giant (generated) Objective C/Java files which painstakingly check for every possible layout/paint property, as per the Mapbox Style Spec, and create corresponding MGLStyleLayer/Layer objects to pass into the Mapbox component. Is there any more efficient way to do this??

@1ec5
Copy link
Contributor

1ec5 commented Dec 21, 2016

This library in general is a reverse engineering of the Android and iOS SDKs to feel more like GL JS, so it isn’t surprising that you’ve had to create essentially the reverse of the scripts that the iOS SDK uses to turn style specification JSON into something that’s idiomatic in Objective-C and Swift (but not at all idiomatic in JavaScript).

mapbox/mapbox-gl-native#6386 would add a way to initialize an MGLStyle from a JSON object. This feature could conceivably be extended to allow you to initialize an individual MGLStyleLayer or MGLSource from a JSON object as well. This feature isn’t going to make it into iOS SDK v3.4.0, but it wouldn’t be technically challenging – the core support code has been there all along.

As far as changes after style loading, mapbox/mapbox-gl-native#2445 would make it possible for the core renderer to accept a JSON object, diff it against the existing style, and efficiently apply only the relevant changes. This feature is much more complex and wouldn’t likely happen in the near future.

In the meantime, I think you’ve actually got the right idea with those giant files, at least until those JSON-based APIs are implemented. Hopefully the scripts above will help you make progress on your reverse transformation.

@esamelson
Copy link
Contributor

esamelson commented Dec 21, 2016

Great, thanks for the feedback. Good to hear this is the right avenue for now. I'll keep going with this and will look forward to the future features you mentioned.

Once I finish up these changes, is this an enhancement you guys would look at a PR for? I've written js scripts similar to the ones you linked, so my giant files can be kept up to date with the style spec.

@1ec5
Copy link
Contributor

1ec5 commented Jan 22, 2017

Hi @esamelson, sorry for the delay. We just published the final release of iOS SDK v3.4.0, so the runtime styling API is no longer the moving target you may’ve found it to be while working on support for it. If you open a PR with your implementation, I’ll take a look and try to offer pointers; hopefully those more well-versed in React Native can help out with the other parts.

@bayyar
Copy link

bayyar commented Jan 22, 2017

We really miss runtime styling now that we've switched from Swift to RN. @esamelson - please let me know how I can be helpful on this, too...

@esamelson
Copy link
Contributor

@1ec5 No problem at all with the delay, and thanks for all your hard work getting the iOS runtime styling SDK in solid working order - we're really excited to be using it. I am happy to post a PR with what I have but want to make sure that it's the correct route for this library before doing so.

With the current SDKs, I see at least two different options for exposing runtime styling:

  1. Expose methods for addLayer, removeLayer, addSource, and removeSource to JS, which take JSON layer/source objects, and then have the native code convert them to the proper native layer/source objects. This is what I have working currently. Upside to this is that it is simpler to manage if you just need to add some extra dynamic layers to an existing base style - no need to touch the existing styleJSON. Downside is that it doesn't expose full control over the style -- for example, to modify the properties of an existing layer from JS, one must simply remove and re-add the layer. (This is still quite performant for my use case.)
  2. Have the RN component accept the full style as a JSON object, perform diffing in JS a la https://github.com/mapbox/mapbox-gl-style-spec/blob/mb-pages/lib/diff.js or Tool for generating iOS/macOS runtime styling code from style diff mapbox/mapbox-gl-style-spec#642 and pass over an array of commands to be performed by the native code. Upside is that this would expose full control to the JS side of things. Downside is that it would probably require the JS programmer to do more management of the full style object. Also, as a relatively new Obj-C programmer, I don't know how this compares to option 1 in terms of performance/danger level.

Do we have a sense of which option is a better fit for the majority of this library's users? I'm happy to take on option 2 as well, but it would be an entirely different PR from what I have so it would probably take me another week or so. Or are there other options I'm missing? Any feedback would be great.

@wafisher
Copy link
Contributor

I vote #2 as it's significantly more React-like. If you were to implement #1, all the clients of the code would have to think more in terms of state transitions rather than state itself, which is precisely the paradigm that React is trying to get rid of. The second option is definitely more work but I think the library deserves this. Let me know if there's something we can help with!

@1ec5
Copy link
Contributor

1ec5 commented Jan 24, 2017

The second approach does sound more React-friendly; however, note that it'll require both mapbox/mapbox-gl-native#6386 and mapbox/mapbox-gl-native#2445, which will likely come to v3.5.0 but not v3.4.x. It would also require a port of the diff-and-patch (aka smart setStyle) mechanism.

/cc @anandthakker

@anandthakker
Copy link

It would also require a port of the diff-and-patch (aka smart setStyle) mechanism.

Yep - this is something I'm starting to work on in mapbox-gl-native in the near future, but I don't have a concrete timeline for it yet as I'm just starting initial research. I'll post there or here (or both) once I know a bit more.

@vicapow
Copy link

vicapow commented Sep 6, 2017

Can I ask why this issue was closed?

@nitaliano
Copy link
Owner

nitaliano commented Sep 6, 2017

I'm clearing out all the old issues since we're completely rewriting this library. Runtime styling is going to be part of the rewrite

@nitaliano
Copy link
Owner

@vicapow #643

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants