Android - Runtime styling Api

[ivovandongen]

Now that the Runtime styling Api is taking shape in core, we can get started on the android side of things.

Major parts:

• Add/remove/manipulate layers
  • Expose layers; Background/Fill/Line/Symbol/Raster/Circle
  • Control paint properties (incl visibility)
  • Control layout properties
  • Control filtering
• Add/remove sources

It's been suggested to generate the Java bindings from the style specification. We can use the same approach as taken for the node bindings, using ejs templates or go for a more Java central approach by using a gradle plugin that transforms json into java classes, like jsonschema2pojo. Depending on integration in the main build or the Android sub-build (gradle).

Because the types (and manipulation of the types) need to go through jni, it's best to make the api flat (no 1-n relations). All manipulation/querying goes through the MapboxMap object. For some of the types we can use subclasses or type IntDef's if the types have little specialisation.

Possible api

Layer Types (generated):

• BackgroundLayer
• CircleLayer
• FillLayer
• LineLayer
• RasterLayer
• SymbolLayer

Related types:

• LayoutProperty - Each layer supports a set of layout properties
• PaintProperty - Each layer supports a set of paint properties
• Filter - Each layer can have a filter

Layer methods on MapboxMap:

• setFilter()
• addLayer()
• removeLayer()
• setPaintProperty()

Source types:

• GeoJsonSource
• RasterSource
• VectorSource

Source methods on MapboxMap

• addSource()
• removeSource()

[bleege]

/cc @mapbox/mobile

[ivovandongen]

Ok, so we have a decision to make as to the level of typing we want in this api. The core api is beautifully typed, which is great to work with. For the android side of things there are some considerations though:

• A completely typed api adds quite a bit of api surface
• Passing rich types over JNI is not what we want, performance wise. So it would be translated to simpler types before crossing the boundary, where it would again be converted to core types.

The other end of the spectrum is a non-typed api (or very basic at least). Passing mostly Strings and values around. This is how the QT api is shaping up, and it's nice and small (see for example setLayoutProperty()). As the type checking is done on the core side anyways, there is not much lost here, except for code completion / compile time checking of correct property types.

A nice compromise might be, to offer some factory methods to construct properties (as we still want to avoid Enums). This would aid in compiler assistance of type checking per property. This can easily be generated. For example:

// This file is generated. Do not edit.
package com.mapbox.mapboxsdk.style.properties;

public class LayoutPropertyFactory {

    public static LayoutProperty<Float> lineMiterLimit(Float value) {
        return new LayoutProperty<>("line-miter-limit", value);
    }
    public static LayoutProperty<Float> lineRoundLimit(Float value) {
        return new LayoutProperty<>("line-round-limit", value);
    }

 ...

    public static LayoutProperty<Boolean> textOptional(Boolean value) {
        return new LayoutProperty<>("text-optional", value);
    }

    public static final class LayoutProperty<T> {
        public final String name;
        public final T value;

        private LayoutProperty(String name, T value) {
            this.name = name;
            this.value = value;
        }
    }

}

cc @tobrun @zugaldia @bleege

[zugaldia]

Passing rich types over JNI is not what we want, performance wise.

Right. Do we foresee that the style API is something that devs would call so often that we'd notice the performance impact?

The other end of the spectrum is a non-typed api (or very basic at least). Passing mostly Strings and values around. This is how the QT api is shaping up

I personally like this approach, not only for consistency with Qt and because, as you say, checks are gonna happen anyway at the core level, but also because 1) it's easier to implement (and explain) and 2) it leaves the door open to a higher level API built on top of this one (e.g. factory methods) in the future should customers require it.

[tmpsantos]

The other end of the spectrum is a non-typed api (or very basic at least). Passing mostly Strings and values around. This is how the QT api is shaping up

Another plus of this approach is as the spec changes, you get new properties, types, etc for free.

[ivovandongen]

I've added an example of the api we could use if we type the properties (Only layout properties done for this example). See the commit for details.

So, the main entry point for the user would look like:

public void setLayoutProperty(String layerId, LayoutProperty<?> property)

Where the LayoutProperty can be retrieved from LayoutPropertyFactory. Both are generated with ejs templates.

Advantages:

• Type safe api for the user. Although not as strong as the core as it doesn't restrict the set of properties per Layer type compile time.
• Easy lookup of properties / value types

Disadvantages:

• Quite a few methods added to the api - this is only the layout properties atm

[ivovandongen]

Right. Do we foresee that the style API is something that devs would call so often that we'd notice the performance impact?

Could animating properties from Android animators be a valid use case? If so, then yes.

@zugaldia

[jfirebaugh]

Quick comments:

• I don't think we can say performance is an advantage either way unless we implement both strong and weakly typed APIs and benchmark them. Whatever JNI overhead there may be could easily be negated by the need to manually typecheck arguments.
• I think we're likely to eliminate an external distinction between paint and layout properties sometime in the future. To the runtime style API, the main implication of the distinction is that you can set a paint property for a specific paint class. But since the Android SDK is not exposing paint classes otherwise, it shouldn't provide this option for paint properties. So I see little reason to distinguish paint and layout properties in the runtime styling API.
• I strongly encourage you to implement layer and source types, and not have the entire API live on Map. I.e., assuming a weakly typed API, map.getLayer(layerId) and layer.setProperty(property). (This is likely what gl-js will switch to.)

[ivovandongen]

@jfirebaugh Thanks for the comments. I'll work that in.

About the JNI part; we've been limiting the amount of callbacks into the runtime lately (see #5103). But since we don't expose that part of the api, we can go either way depending on any performance issues.

[bleege]

For continuity, the iOS / mac OS Styling API is simultaneously being worked on in #5626.

[ivovandongen]

With the excellent guidance from @jfirebaugh I've added peer classes on the java and c++ side that keep state in sync on both sides using the high-level wrappers. This is important as we want to be able to create new layers from Java (in the addLayer() case) as well as from c++ (for the getLayer() case. Atm there is no type hierarchy yet on either side, that's the next step.

One issue atm is that the art Finalizer runs on a different thread than the thread on which we first created the c++ Layer object. This leads to exceptions when the garbage collector runs since a reference is being held to the JNIEnv to delete global references:

07-13 17:28:04.784 9244-9252/com.mapbox.mapboxsdk.testapp D/mbgl: {unknown}[JNI]: Layer destroyed
07-13 17:28:04.791 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410] JNI DETECTED ERROR IN APPLICATION: thread Thread[4,tid=9252,Native,Thread*=0xec471b00,peer=0x12c3b100,"FinalizerDaemon"] using JNIEnv* from thread Thread[4,tid=9252,Native,Thread*=0xec471b00,peer=0x12c3b100,"FinalizerDaemon"]
07-13 17:28:04.791 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]     in call to DeleteGlobalRef
07-13 17:28:04.791 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]     from void com.mapbox.mapboxsdk.style.layers.Layer.finalize()
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410] "FinalizerDaemon" daemon prio=5 tid=4 Runnable
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   | group="system" sCount=0 dsCount=0 obj=0x12c3b100 self=0xec471b00
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   | sysTid=9252 nice=0 cgrp=default sched=0/0 handle=0xf31ef930
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   | state=R schedstat=( 0 0 0 ) utm=0 stm=0 core=1 HZ=100
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   | stack=0xf30ed000-0xf30ef000 stackSize=1038KB
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   | held mutexes= "mutator lock"(shared held)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #00 pc 0058bae2  /system/lib/libart.so (art::DumpNativeStack(std::__1::basic_ostream<char, std::__1::char_traits<char> >&, int, char const*, art::ArtMethod*, void*)+226)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #01 pc 0055172e  /system/lib/libart.so (art::Thread::Dump(std::__1::basic_ostream<char, std::__1::char_traits<char> >&) const+286)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #02 pc 003a461f  /system/lib/libart.so (art::JavaVMExt::JniAbort(char const*, char const*)+1247)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #03 pc 003a5c9c  /system/lib/libart.so (art::JavaVMExt::JniAbortV(char const*, char const*, char*)+116)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #04 pc 00163950  /system/lib/libart.so (art::ScopedCheck::AbortF(char const*, ...)+62)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #05 pc 0016d84c  /system/lib/libart.so (art::ScopedCheck::CheckThread(_JNIEnv*)+1692)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #06 pc 0016e421  /system/lib/libart.so (art::ScopedCheck::Check(art::ScopedObjectAccess&, bool, char const*, art::JniValueType*) (.constprop.114)+1489)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #07 pc 00176dd5  /system/lib/libart.so (art::CheckJNI::DeleteRef(char const*, _JNIEnv*, _jobject*, art::IndirectRefKind)+645)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #08 pc 0017724d  /system/lib/libart.so (art::CheckJNI::DeleteGlobalRef(_JNIEnv*, _jobject*)+52)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #09 pc 003c9b27  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/x86/libmapbox-gl.so (_JNIEnv::DeleteGlobalRef(_jobject*)+55)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #10 pc 0040026c  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/x86/libmapbox-gl.so (jni::ObjectDeleter<mbgl::android::Layer>::operator()(jni::PointerToValue<jni::Object<mbgl::android::Layer> >) const+220)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #11 pc 003ff3fc  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/x86/libmapbox-gl.so (mbgl::android::Layer::~Layer()+476)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #12 pc 003ff66c  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/x86/libmapbox-gl.so (mbgl::android::Layer::~Layer()+44)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #13 pc 004020cf  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/x86/libmapbox-gl.so (jni::NativePeerHelper<mbgl::android::Layer, mbgl::android::Layer, std::__ndk1::unique_ptr<mbgl::android::Layer, std::__ndk1::default_delete<mbgl::android::Layer> > (_JNIEnv&)>::MakeFinalizer(jni::Field<mbgl::android::Layer, long long> const&, char const*) const::'lambda'(_JNIEnv&, jni::Object<mbgl::android::Layer>)::operator()(_JNIEnv&, jni::Object<mbgl::android::Layer>) const+783)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #14 pc 00401d97  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/x86/libmapbox-gl.so (auto jni::NativeMethodMaker<void (jni::NativePeerHelper<mbgl::android::Layer, mbgl::android::Layer, std::__ndk1::unique_ptr<mbgl::android::Layer, std::__ndk1::default_delete<mbgl::android::Layer> > (_JNIEnv&)>::MakeFinalizer(jni::Field<mbgl::android::Layer, long long> const&, char const*) const::'lambda'(_JNIEnv&, jni::Object<mbgl::android::Layer>)::*)(_JNIEnv&, jni::Object<mbgl::android::Layer>) const>::operator()<jni::NativePeerHelper<mbgl::android::Layer, mbgl::android::Layer, std::__ndk1::unique_ptr<mbgl::android::Layer, std::__ndk1::default_delete<mbgl::android::Layer> > (_JNIEnv&)>::MakeFinalizer(jni::Field<mbgl::android::Layer, long long> const&, char const*) const::'lambda'(_JNIEnv&, jni::Object<mbgl::android::Layer>)>(char const*, jni::NativePeerHelper<mbgl::android::Layer, mbgl::android::Layer, std::__ndk1::unique_ptr<mbgl::android::Layer, std::__ndk1::default_delete<mbgl::an
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #15 pc 00401d0a  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/x86/libmapbox-gl.so (auto jni::NativeMethodMaker<void (jni::NativePeerHelper<mbgl::android::Layer, mbgl::android::Layer, std::__ndk1::unique_ptr<mbgl::android::Layer, std::__ndk1::default_delete<mbgl::android::Layer> > (_JNIEnv&)>::MakeFinalizer(jni::Field<mbgl::android::Layer, long long> const&, char const*) const::'lambda'(_JNIEnv&, jni::Object<mbgl::android::Layer>)::*)(_JNIEnv&, jni::Object<mbgl::android::Layer>) const>::operator()<jni::NativePeerHelper<mbgl::android::Layer, mbgl::android::Layer, std::__ndk1::unique_ptr<mbgl::android::Layer, std::__ndk1::default_delete<mbgl::android::Layer> > (_JNIEnv&)>::MakeFinalizer(jni::Field<mbgl::android::Layer, long long> const&, char const*) const::'lambda'(_JNIEnv&, jni::Object<mbgl::android::Layer>)>(char const*, jni::NativePeerHelper<mbgl::android::Layer, mbgl::android::Layer, std::__ndk1::unique_ptr<mbgl::android::Layer, std::__ndk1::default_delete<mbgl::an
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #16 pc 0040228e  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/x86/libmapbox-gl.so (std::__ndk1::enable_if<std::is_class<jni::NativePeerHelper<mbgl::android::Layer, mbgl::android::Layer, std::__ndk1::unique_ptr<mbgl::android::Layer, std::__ndk1::default_delete<mbgl::android::Layer> > (_JNIEnv&)>::MakeFinalizer(jni::Field<mbgl::android::Layer, long long> const&, char const*) const::'lambda'(_JNIEnv&, jni::Object<mbgl::android::Layer>) const>::value, void> auto jni::MakeNativeMethod<auto jni::NativeMethodMaker<void (jni::NativePeerHelper<mbgl::android::Layer, mbgl::android::Layer, std::__ndk1::unique_ptr<mbgl::android::Layer, std::__ndk1::default_delete<mbgl::android::Layer> > (_JNIEnv&)>::MakeFinalizer(jni::Field<mbgl::android::Layer, long long> const&, char const*) const::'lambda'(_JNIEnv&, jni::Object<mbgl::android::Layer>)::*)(_JNIEnv&, jni::Object<mbgl::android::Layer>) const>::operator()<jni::NativePeerHelper<mbgl::android::Layer, mbgl::android::Layer, std::__ndk1
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #17 pc 0040222a  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/x86/libmapbox-gl.so (std::__ndk1::enable_if<std::is_class<jni::NativePeerHelper<mbgl::android::Layer, mbgl::android::Layer, std::__ndk1::unique_ptr<mbgl::android::Layer, std::__ndk1::default_delete<mbgl::android::Layer> > (_JNIEnv&)>::MakeFinalizer(jni::Field<mbgl::android::Layer, long long> const&, char const*) const::'lambda'(_JNIEnv&, jni::Object<mbgl::android::Layer>) const>::value, void> auto jni::MakeNativeMethod<auto jni::NativeMethodMaker<void (jni::NativePeerHelper<mbgl::android::Layer, mbgl::android::Layer, std::__ndk1::unique_ptr<mbgl::android::Layer, std::__ndk1::default_delete<mbgl::android::Layer> > (_JNIEnv&)>::MakeFinalizer(jni::Field<mbgl::android::Layer, long long> const&, char const*) const::'lambda'(_JNIEnv&, jni::Object<mbgl::android::Layer>)::*)(_JNIEnv&, jni::Object<mbgl::android::Layer>) const>::operator()<jni::NativePeerHelper<mbgl::android::Layer, mbgl::android::Layer, std::__ndk1
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #18 pc 004316d4  /data/app/com.mapbox.mapboxsdk.testapp-1/oat/x86/base.odex (void com.mapbox.mapboxsdk.style.layers.Layer.finalize()+104)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #19 pc 003191d8  /data/dalvik-cache/x86/system@framework@boot.oat (???)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   native: #20 pc 89d23347  ???
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   at com.mapbox.mapboxsdk.style.layers.Layer.finalize(Native method)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   at java.lang.Daemons$FinalizerDaemon.doFinalize(Daemons.java:202)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   at java.lang.Daemons$FinalizerDaemon.run(Daemons.java:185)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410]   at java.lang.Thread.run(Thread.java:818)
07-13 17:28:04.792 9244-9252/com.mapbox.mapboxsdk.testapp A/art: art/runtime/java_vm_ext.cc:410] 

[ivovandongen]

I took a few detours, but retrieving layers and setting paint properties now works.

Next up:

• layout properties (should already work, needs testing)
• remove layer
• sources
• add layer

[ivovandongen]

Layout properties work, sort-off. The style update is applied. However, for symbol-placement = point at least, the updated style is not applied to the elements already drawn it seems.

Little demo of the issue:

[ivovandongen]

-> Added removeLayer()

I ran into a little issue though when combining camera animation callbacks and removing style layers:

07-16 11:25:32.255 7133-7133/com.mapbox.mapboxsdk.testapp E/libEGL: call to OpenGL ES API with no current context (logged once per thread)
07-16 11:25:32.258 7133-7133/com.mapbox.mapboxsdk.testapp D/mbgl: {unknown}[Android]: NativeMapView::notifyMapChange()
07-16 11:25:32.259 7133-7133/com.mapbox.mapboxsdk.testapp E/mbgl: {unknown}[OpenGL]: eglSwapBuffers() returned error 12301
07-16 11:25:32.259 7133-7133/com.mapbox.mapboxsdk.testapp D/AndroidRuntime: Shutting down VM
07-16 11:25:32.260 7133-7133/com.mapbox.mapboxsdk.testapp E/AndroidRuntime: FATAL EXCEPTION: main
                                                                            Process: com.mapbox.mapboxsdk.testapp, PID: 7133
                                                                            java.lang.Error: eglSwapBuffers() failed
                                                                                at com.mapbox.mapboxsdk.maps.NativeMapView.nativeRender(Native Method)
                                                                                at com.mapbox.mapboxsdk.maps.NativeMapView.render(NativeMapView.java:131)
                                                                                at com.mapbox.mapboxsdk.maps.MapView.onDraw(MapView.java:1302)
                                                                                at android.view.View.draw(View.java:16184)
                                                                                at android.view.View.updateDisplayListIfDirty(View.java:15180)
...

That error code is EGL_BAD_SURFACE I believe.

The problem occurs when chaining removeLayer in a camera animation callback

    private void removeBuildings() {
        //Zoom to see buildings first
        mapboxMap.animateCamera(CameraUpdateFactory.zoomTo(16), new DefaultCallback() {
            @Override
            public void onFinish() {
                try {
                    mapboxMap.removeLayer("building");
                } catch (NoSuchLayerException e) {
                    Toast.makeText(RuntimeStyleActivity.this, e.getMessage(), Toast.LENGTH_SHORT).show();
                }
            }
        });
    }

Whereas this works:

    private void removeBuildings() {
        try {
            mapboxMap.removeLayer("building");
        } catch (NoSuchLayerException e) {
            Toast.makeText(RuntimeStyleActivity.this, e.getMessage(), Toast.LENGTH_SHORT).show();
        }
    }

And so does posting it on the main looper..

    private void removeBuildings() {
        //Zoom to see buildings first
        mapboxMap.animateCamera(CameraUpdateFactory.zoomTo(16), new DefaultCallback() {
            @Override
            public void onFinish() {
                new Handler(Looper.getMainLooper()).post(new Runnable() {
                    @Override
                    public void run() {
                      try {
                          mapboxMap.removeLayer("building");
                      } catch (NoSuchLayerException e) {
                          Toast.makeText(RuntimeStyleActivity.this, e.getMessage(), Toast.LENGTH_SHORT).show();
                      }
                    }
                });
            }
        });
    }

Although the callback is already being done on the main thread (I checked).

[jfirebaugh]

Map#removeLayer activates and deactivates the view/context. So the issue could be that sometimes MapView.onDraw does not properly reactivate the context if it's been deactivated by removeLayer. This comment documents the requirements/expectations for context activation.

[ivovandongen]

Little status update:

Added generated headers/classes for all the layer types. All are implemented as Peer classes. addLayer() is implemented, but needs sources to make it better testable. So sources is up next.

Todo

• Layers:
  • Get properties
  • Migrate custom layer
  • Add properties to Layer after using it with addLayer() on the Java side-> Something is crashing, should either be caught or supported. See here
• Sources

[ivovandongen]

Working on sources right now and need to make a decision around the api we will offer.

The basics are clear:

• map.addSource(Source source)
• map.addSource(String id)

Question is, do we want to offer a mutable api similar to Layer. Eg offer:

• map.getSource(String sourceId)

Mutable properties differ per Source, but for a GeoJsonSource you would be able to do:

GeoJsonSource source = map.getSource("my-previously-added-source");
source.setData(getJsonStringFromSomeWhere());

This would similar to the gl js api: getSource() through getStyle() and GeoJSONSource.setData()

[ivovandongen]

Small note. To implement the geojson source, I've added https://github.com/mapbox/geojson-vt-cpp as a dependency. Haven't looked at the impact on the size of our sdk yet, but I don't see an alternative either.

[ivovandongen]

Initial sources implementation (GeoJson source with a Fill layer):

[ivovandongen]

A little overview of the remaining items:

• [x] Filters
• [ ] Layers
  • [ ] Property getters
  • [ ] Array property conversion
  • [ ] More efficient, batched property setters
• Sources:
  • [ ] Tile sets
  • [x] RasterSource
  • [x] VectorSource

Concerning the batched property setters; last night I noticed, while making some better examples, that you easily call quite a few setters on a layer to modify it. This is not really efficient atm, as after each call the style is re-calculated and applied. I've considered adding something like a transaction model to this to let the user decide when to actually persist the style changes, but the easiest way to offer this is probably to have a general set(Property... properties); method that takes as many items as you want and only re-calculates once.

This requires a few changes; having the properties expose a type for example so it can be easily determined which update flags to send for a batch, but all in all it should be fairly quick.

[zugaldia]

Concerning the batched property setters; last night I noticed, while making some better examples, that you easily call quite a few setters on a layer to modify it.

How about a good old Builder pattern on the SDK side, possibly built on top of your proposed set(Property... properties), that gathers all the properties and has a final build() or execute()?

[ivovandongen]

How about a good old Builder pattern on the SDK side, possibly built on top of your proposed set(Property... properties), that gathers all the properties and has a final build() or execute()?

Definitely an option, actually a better name for what I was calling a transaction model. Although this does require more from the user than a simple set and a slightly larger api (The builder itself). I personally prefer a simple setter, but I will gladly defer to your better judgement.

[ivovandongen]

So, I've added an initial Filter api. Sort of a mini-dsl. This gives the user the option, besides using raw arrays, to build up a pattern in a understandable way. Something like:

Filter.Statement filter = any(
        eq("myProp", 20),
        none(
                eq("other", 20),
                in("importantProp", 2, 5, "Here")
        )
);

Or the same, but condensed:

Filter.Statement filter = any(eq("myProp", 20), none(eq("other", 20), in("importantProp", 2, 5, "Here")));

[1ec5]

So, I've added an initial Filter api. Sort of a mini-dsl. This gives the user the option, besides using raw arrays, to build up a pattern in a understandable way.

That’s great. On iOS/macOS, we’re planning to use the platform-standard NSPredicate type, which can be constructed either using a SQL-like string or in structured form.

[ivovandongen]

Added Vector and Raster sources:

[jfirebaugh]

What are your thoughts on combining setPaintProperty and setLayoutProperty into a single setProperty method, so as to leave the difference unexposed by the Android API as suggested in https://github.com/mapbox/mapbox-gl-native/issues/5610#issuecomment-231435943?

[ivovandongen]

@jfirebaugh Yes, that's actually what I was trying to describe in my comment. I want to combine both and accept a list. The only thing I'm unsure about is the need for the class parameter on paint properties, but we can pass them into the property object if needed.

So it would become (Java vararg parameter):

set( Property... properties);

This gives us the control over triggering the style re-calculation and combines the methods as to "hide" the difference between the paint/layout properties.

[ivovandongen]

Moved to the set() method as described in my previous comment.

After discussion with @cammace and @zugaldia I'm going to merge the current work into master so that @cammace can get started on creating some beautiful examples. I'll finish up the remaining work in a new PR.

Main items remaining:

• Functions (in progress)
• Layers:
  • min/max zoom
  • property getters
• Property documentation (generated from style spec)
• Sources:
  • Tile sets
  • RasterSource
  • tileSize
• Cleanup
  • JavaDoc
  • Log statements
  • Comments
  • Temporary #pragma's

[incanus]

This is some pretty old stuff now, but there is some early thinking iOS-side on functions in a style API over in https://github.com/cutting-room-floor/mapbox-gl-cocoa/pull/27/files#diff-071c00a3e1ff1660faa7f5af31ee02b3.

[ivovandongen]

Little demo of the zoom function (on fill color):

[ivovandongen]

Added min/max zoom on Layers

[bleege]

This is looking great @ivovandongen! 🚀

[incanus]

After discussion with @cammace and @zugaldia I'm going to merge the current work into master so that @cammace can get started on creating some beautiful examples.

Just a process comment here: does this in any way hold up a release or commit us to supporting runtime styling in the next release? Or can we let it sit if functions etc. take a lot more time than we anticipate for some reason?

Reason I ask is the alternative would have been to merge to a non-master branch for @cammace to work on, and keep merging master back to it to resolve issues.

Possibly a non-issue but just planning ahead.

[ivovandongen]

does this in any way hold up a release or commit

Always a risk, but we agreed it was acceptable as it is a new api and doesn't break existing functionality. I would however, also prefer another way to have examples worked on in parallel for future features that take this long to implement.

alternative would have been to merge to a non-master branch

Apparently not at the moment as the samples depend on the snapshot being built from master. It would be great to have snapshots built from other branches from gl-native that can be used on branches of the test app while in active development.

side note: functions are done btw

[incanus]

side note: functions are done btw

Looking great here.

[zugaldia]

Reason I ask is the alternative would have been to merge to a non-master branch for @cammace to work on, and keep merging master back to it to resolve issues.

@incanus We're planning a beta release soon. Considering that it's early in the game, we're planning on releasing from master instead of cutting it from a release branch.

side note: functions are done btw

💥

[ivovandongen]

Little update:

Property getters done - needed some groundwork for conversion of constants and Functions.

Next up:

• Tile json
• Custom layer

Complete list in the PR take 2

[ivovandongen]

Finished implementation and unit tests. Just up to two remaining issues:

Random crash on addLayer

08-01 18:51:24.742  1195  1195 F DEBUG   : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
08-01 18:51:24.742  1195  1195 F DEBUG   : Build fingerprint: 'Android/sdk_google_phone_x86_64/generic_x86_64:6.0/MASTER/3038907:userdebug/test-keys'
08-01 18:51:24.742  1195  1195 F DEBUG   : Revision: '0'
08-01 18:51:24.742  1195  1195 F DEBUG   : ABI: 'x86'
08-01 18:51:24.742  1195  1195 F DEBUG   : pid: 9639, tid: 9639, name: pboxsdk.testapp  >>> com.mapbox.mapboxsdk.testapp <<<
08-01 18:51:24.742  1195  1195 F DEBUG   : signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------
08-01 18:51:24.746  1195  1195 F DEBUG   : Abort message: '../../src/mbgl/style/paint_property.hpp:87: bool mbgl::style::PaintProperty<bool, PropertyEvaluator>::calculate(const mbgl::style::CalculationParameters &) [T = bool, Evaluator = PropertyEvaluator]: assertion "cascaded" failed'
08-01 18:51:24.746  1195  1195 F DEBUG   :     eax 00000000  ebx 000025a7  ecx 000025a7  edx 00000006
08-01 18:51:24.746  1195  1195 F DEBUG   :     esi f777fc50  edi 0000000b
08-01 18:51:24.746  1195  1195 F DEBUG   :     xcs 00000023  xds 0000002b  xes 0000002b  xfs 00000007  xss 0000002b
08-01 18:51:24.746  1195  1195 F DEBUG   :     eip f734a1a6  ebp 000025a7  esp fff94c20  flags 00200202
08-01 18:51:24.755  1195  1195 F DEBUG   : 
08-01 18:51:24.755  1195  1195 F DEBUG   : backtrace:
08-01 18:51:24.755  1195  1195 F DEBUG   :     #00 pc 000851a6  /system/lib/libc.so (tgkill+22)
08-01 18:51:24.755  1195  1195 F DEBUG   :     #01 pc 00082498  /system/lib/libc.so (pthread_kill+70)
08-01 18:51:24.755  1195  1195 F DEBUG   :     #02 pc 000280a5  /system/lib/libc.so (raise+36)
08-01 18:51:24.755  1195  1195 F DEBUG   :     #03 pc 0002187d  /system/lib/libc.so (abort+80)
08-01 18:51:24.755  1195  1195 F DEBUG   :     #04 pc 000255e1  /system/lib/libc.so (__libc_fatal+32)
08-01 18:51:24.755  1195  1195 F DEBUG   :     #05 pc 000219fc  /system/lib/libc.so (__assert2+60)
08-01 18:51:24.755  1195  1195 F DEBUG   :     #06 pc 0092e03a  /data/app/com.mapbox.mapboxsdk.testapp-2/lib/x86/libmapbox-gl.so (mbgl::style::PaintProperty<bool, mbgl::style::PropertyEvaluator>::calculate(mbgl::style::CalculationParameters const&)+154)
08-01 18:51:24.755  1195  1195 F DEBUG   :     #07 pc 0092de11  /data/app/com.mapbox.mapboxsdk.testapp-2/lib/x86/libmapbox-gl.so (mbgl::style::FillPaintProperties::recalculate(mbgl::style::CalculationParameters const&)+65)
08-01 18:51:24.755  1195  1195 F DEBUG   :     #08 pc 0092aed8  /data/app/com.mapbox.mapboxsdk.testapp-2/lib/x86/libmapbox-gl.so (mbgl::style::FillLayer::Impl::recalculate(mbgl::style::CalculationParameters const&)+72)
08-01 18:51:24.756  1195  1195 F DEBUG   :     #09 pc 009fcf6c  /data/app/com.mapbox.mapboxsdk.testapp-2/lib/x86/libmapbox-gl.so (mbgl::style::Style::recalculate(float, std::__ndk1::chrono::time_point<std::__ndk1::chrono::steady_clock, std::__ndk1::chrono::duration<long long, std::__ndk1::ratio<1ll, 1000000000ll> > > const&, mbgl::MapMode)+1500)
08-01 18:51:24.756  1195  1195 F DEBUG   :     #10 pc 007f2e53  /data/app/com.mapbox.mapboxsdk.testapp-2/lib/x86/libmapbox-gl.so (mbgl::Map::Impl::update()+979)
08-01 18:51:24.756  1195  1195 F DEBUG   :     #11 pc 007fe5d6  /data/app/com.mapbox.mapboxsdk.testapp-2/lib/x86/libmapbox-gl.so
08-01 18:51:24.756  1195  1195 F DEBUG   :     #12 pc 007fe507  /data/app/com.mapbox.mapboxsdk.testapp-2/lib/x86/libmapbox-gl.so
08-01 18:51:24.756  1195  1195 F DEBUG   :     #13 pc 00807b48  /data/app/com.mapbox.mapboxsdk.testapp-2/lib/x86/libmapbox-gl.so (std::__ndk1::function<void ()>::operator()() const+200)
08-01 18:51:24.756  1195  1195 F DEBUG   :     #14 pc 00b04a7a  /data/app/com.mapbox.mapboxsdk.testapp-2/lib/x86/libmapbox-gl.so (mbgl::util::AsyncTask::Impl::runTask()+202)
08-01 18:51:24.756  1195  1195 F DEBUG   :     #15 pc 00b0718e  /data/app/com.mapbox.mapboxsdk.testapp-2/lib/x86/libmapbox-gl.so (mbgl::util::RunLoop::Impl::processRunnables()+942)
08-01 18:51:24.756  1195  1195 F DEBUG   :     #16 pc 00b08e31  /data/app/com.mapbox.mapboxsdk.testapp-2/lib/x86/libmapbox-gl.so (mbgl::util::RunLoop::runOnce()+257)
08-01 18:51:24.756  1195  1195 F DEBUG   :     #17 pc 00b05ad3  /data/app/com.mapbox.mapboxsdk.testapp-2/lib/x86/libmapbox-gl.so
08-01 18:51:24.756  1195  1195 F DEBUG   :     #18 pc 0001afa7  /system/lib/libutils.so (android::Looper::pollInner(int)+439)
08-01 18:51:24.756  1195  1195 F DEBUG   :     #19 pc 0001b307  /system/lib/libutils.so (android::Looper::pollOnce(int, int*, int*, void**)+53)
08-01 18:51:24.756  1195  1195 F DEBUG   :     #20 pc 000c0efb  /system/lib/libandroid_runtime.so (android::NativeMessageQueue::pollOnce(_JNIEnv*, _jobject*, int)+77)
08-01 18:51:24.756  1195  1195 F DEBUG   :     #21 pc 000c0f6c  /system/lib/libandroid_runtime.so
08-01 18:51:24.756  1195  1195 F DEBUG   :     #22 pc 72d27704  /data/dalvik-cache/x86/system@framework@boot.oat (offset 0x1eb2000)
08-01 18:51:25.067  1195  1195 F DEBUG   : 
08-01 18:51:25.067  1195  1195 F DEBUG   : Tombstone written to: /data/tombstones/tombstone_09
08-01 18:51:25.067  1195  1195 E DEBUG   : AM write failed: Broken pipe

Crash when manipulation the style in the onMapReady() callback

08-01 18:31:48.445 18808-18808/com.mapbox.mapboxandroiddemo E/mbgl: {pboxandroiddemo}[Shader]: Vertex shader fill failed to compile: precision highp float;
                                                                    #ifdef GL_ES
                                                                    precision highp float;
                                                                    #else
                                                                    #define lowp
                                                                    #define mediump
                                                                    #define highp
                                                                    #endif

                                                                    attribute vec2 a_pos;

                                                                    uniform mat4 u_matrix;

                                                                    uniform lowp vec4 u_color;
                                                                    uniform lowp float u_opacity;

                                                                    void main() {
                                                                        lowp vec4 color = u_color;
                                                                        lowp float opacity = u_opacity;

                                                                        gl_Position = u_matrix * vec4(a_pos, 0, 1);
                                                                    }
08-01 18:31:48.525 18808-20563/com.mapbox.mapboxandroiddemo D/MapboxEventManager: response code = 204 for events 2
08-01 18:31:48.571 18808-18808/com.mapbox.mapboxandroiddemo D/AndroidRuntime: Shutting down VM

                                                                              --------- beginning of crash
08-01 18:31:48.571 18808-18808/com.mapbox.mapboxandroiddemo E/AndroidRuntime: FATAL EXCEPTION: main
                                                                              Process: com.mapbox.mapboxandroiddemo, PID: 18808
                                                                              java.lang.Error: Vertex shader fill failed to compile
                                                                                  at com.mapbox.mapboxsdk.maps.NativeMapView.nativeRender(Native Method)
                                                                                  at com.mapbox.mapboxsdk.maps.NativeMapView.render(NativeMapView.java:134)
                                                                                  at com.mapbox.mapboxsdk.maps.MapView.onDraw(MapView.java:1334)
                                                                                  at android.view.View.draw(View.java:17067)
                                                                                  at android.view.View.updateDisplayListIfDirty(View.java:16049)
                                                                                  at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:3748)
                                                                                  at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:3728)
                                                                                  at android.view.View.updateDisplayListIfDirty(View.java:16012)
                                                                                  at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:3748)
                                                                                  at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:3728)
                                                                                  at android.view.View.updateDisplayListIfDirty(View.java:16012)
                                                                                  at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:3748)
                                                                                  at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:3728)
                                                                                  at android.view.View.updateDisplayListIfDirty(View.java:16012)
                                                                                  at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:3748)
                                                                                  at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:3728)
                                                                                  at android.view.View.updateDisplayListIfDirty(View.java:16012)
                                                                                  at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:3748)
                                                                                  at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:3728)
                                                                                  at android.view.View.updateDisplayListIfDirty(View.java:16012)
                                                                                  at android.view.ViewGroup.recreateChildDisplayList(ViewGroup.java:3748)
                                                                                  at android.view.ViewGroup.dispatchGetDisplayList(ViewGroup.java:3728)
                                                                                  at android.view.View.updateDisplayListIfDirty(View.java:16012)
                                                                                  at android.view.ThreadedRenderer.updateViewTreeDisplayList(ThreadedRenderer.java:656)
                                                                                  at android.view.ThreadedRenderer.updateRootDisplayList(ThreadedRenderer.java:662)
                                                                                  at android.view.ThreadedRenderer.draw(ThreadedRenderer.java:770)
                                                                                  at android.view.ViewRootImpl.draw(ViewRootImpl.java:2796)
                                                                                  at android.view.ViewRootImpl.performDraw(ViewRootImpl.java:2604)
                                                                                  at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:2211)
                                                                                  at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:1246)
                                                                                  at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:6301)
                                                                                  at android.view.Choreographer$CallbackRecord.run(Choreographer.java:871)
                                                                                  at android.view.Choreographer.doCallbacks(Choreographer.java:683)
                                                                                  at android.view.Choreographer.doFrame(Choreographer.java:619)
                                                                                  at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:857)
                                                                                  at android.os.Handler.handleCallback(Handler.java:751)
                                                                                  at android.os.Handler.dispatchMessage(Handler.java:95)
                                                                                  at android.os.Looper.loop(Looper.java:154)
                                                                                  at android.app.ActivityThread.main(ActivityThread.java:6077)
                                                                                  at java.lang.reflect.Method.invoke(Native Method)
                                                                                  at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:865)
                                                                                  at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:755)

[ivovandongen]

First issue was resolved by rebasing master.

Second issue still remains. To be more specific, the crash happens when adding a Layer in the onMapReady callback. I've added a test case here to reproduce the issue (thanks @cammace)

08-02 10:14:44.321 4607-4607/? D/mbgl: {pboxsdk.testapp}[JNI]: nativeAddLayer
08-02 10:14:44.321 4607-4607/? D/mbgl: {pboxsdk.testapp}[Android]: NativeMapView::activate
08-02 10:14:44.321 4607-4607/? D/mbgl: {pboxsdk.testapp}[Android]: NativeMapView::deactivate
08-02 10:14:44.322 4607-4607/? D/mbgl: {pboxsdk.testapp}[JNI]: nativeLatLngForPixel
08-02 10:14:44.322 4607-4607/? D/mbgl: {pboxsdk.testapp}[JNI]: nativeLatLngForPixel
08-02 10:14:44.322 4607-4607/? D/mbgl: {pboxsdk.testapp}[JNI]: nativeLatLngForPixel
08-02 10:14:44.322 4607-4607/? D/mbgl: {pboxsdk.testapp}[JNI]: nativeLatLngForPixel
08-02 10:14:44.322 4607-4607/? D/mbgl: {pboxsdk.testapp}[JNI]: nativeGetAnnotationsInBounds
08-02 10:14:44.323 4607-4607/? D/mbgl: {pboxsdk.testapp}[Android]: NativeMapView::notifyMapChange()
08-02 10:14:44.323 4607-4607/? E/libEGL: call to OpenGL ES API with no current context (logged once per thread)
08-02 10:14:44.323 4607-4607/? E/mbgl: {pboxsdk.testapp}[Shader]: Vertex shader fill failed to compile: precision highp float;
                                       #ifdef GL_ES
                                       precision highp float;
                                       #else
                                       #define lowp
                                       #define mediump
                                       #define highp
                                       #endif

                                       attribute vec2 a_pos;

                                       uniform mat4 u_matrix;

                                       uniform lowp vec4 u_color;
                                       uniform lowp float u_opacity;

                                       void main() {
                                           lowp vec4 color = u_color;
                                           lowp float opacity = u_opacity;

                                           gl_Position = u_matrix * vec4(a_pos, 0, 1);
                                       }
08-02 10:14:44.323 4607-4607/? D/AndroidRuntime: Shutting down VM

[ivovandongen]

Merged into master

[incanus]

@ivovandongen Assuming the bug mentioned in https://github.com/mapbox/mapbox-gl-native/issues/6014 is real (in which case we'll need to ticket it separately), otherwise this feature is fully built out, right? Can we close this ticket?

[ivovandongen]

Thought it was closed already. https://github.com/mapbox/mapbox-gl-native/issues/5852 was the last missing piece.

[1ec5]

For posterity, the PRs that implemented the initial buildout were #5734 and #5830.