addMarkers in JNI

[tobrun]

Add markers to JNI

Original post:
List missing features:
- add Z index
- #1726
- #1882

[tobrun]

Jake Wharton gave a nice presentation about eliminating code overhead in Android applications. He has some nice pointers that will improve scalability.

[ljbade]

@tobrun Big improvment will be to use arrays/lists to pass markers to JNI in bulk rather than having to loop in Java.

[tobrun]

@ljbade
I tried adding addMarkers function in jni.cpp, could you validate the code?
I have added some inline comments with questions.

Also while compiling I'm getting:

../../android/cpp/jni.cpp:824:20: error: unused function 'nativeAddMarkers' [-Werror,-Wunused-function] jlongArray JNICALL nativeAddMarkers(JNIEnv *env, jobject obj, jlong nativeMapViewPtr, jobject jlist) {

jni.cpp:

jlongArray JNICALL nativeAddMarkers(JNIEnv *env, jobject obj, jlong nativeMapViewPtr, jobject jlist) {
    mbgl::Log::Debug(mbgl::Event::JNI, "nativeAddMarkers");
    assert(nativeMapViewPtr != 0);
    NativeMapView *nativeMapView = reinterpret_cast<NativeMapView *>(nativeMapViewPtr);

    std::vector<mbgl::PointAnnotation> markers;

    if (jlist == nullptr) {
        if (env->ThrowNew(nullPointerExceptionClass, "List cannot be null.") < 0) {
            env->ExceptionDescribe();
            return nullptr;
        }
        return nullptr;
    }

    jobjectArray jarray =
        reinterpret_cast<jobjectArray>(env->CallObjectMethod(jlist, listToArrayId));
    if (env->ExceptionCheck() || (jarray == nullptr)) {
        env->ExceptionDescribe();
        return nullptr;
    }

    jsize len = env->GetArrayLength(jarray);
    if (len < 0) {
        env->ExceptionDescribe();
        return nullptr;
    }

    markers.reserve(len);

    for (jsize i = 0; i < len; i++) {
        /* Could you validate that this loop is correct? */
        jobject marker = reinterpret_cast<jobject>(env->GetObjectArrayElement(jarray, i));

        jobject position = env->GetObjectField(marker, markerPositionId);
        if (env->ExceptionCheck()) {
            env->ExceptionDescribe();
            return nullptr;
        }

        jstring jsprite = reinterpret_cast<jstring>(env->GetObjectField(marker, markerSpriteId));
        std::string sprite = std_string_from_jstring(env, jsprite);

        jdouble latitude = env->GetDoubleField(position, latLngLatitudeId);
        if (env->ExceptionCheck()) {
            env->ExceptionDescribe();
            return nullptr;
        }

        jdouble longitude = env->GetDoubleField(position, latLngLongitudeId);
        if (env->ExceptionCheck()) {
            env->ExceptionDescribe();
            return nullptr;
        }

        markers.emplace_back(mbgl::PointAnnotation(mbgl::LatLng(latitude, longitude), sprite));

        /* Do I need to delete other LocalRefs? */
        env->DeleteLocalRef(marker);
     }

    env->DeleteLocalRef(jarray);

    std::vector<uint32_t> pointAnnotationIDs = nativeMapView->getMap().addPointAnnotations(markers);
    return std_vector_uint_to_jobject(env, pointAnnotationIDs);
}

NativeMapView

    public long[] addMarkers(List<Marker> markers) {
        return nativeAddMarkers(mNativeMapViewPtr, markers);
    }

    private native long[] nativeAddMarkers(long nativeMapViewPtr, List<Marker> markers);

[ljbade]

@tobrun That error is because you need to add an entry to https://github.com/mapbox/mapbox-gl-native/blob/master/android/cpp/jni.cpp#L1614

Loop looks good.

You should be fine with the local refs. JNI automatically releases any local references on return from the C++ function (that is why you see NewGlobalRef for long lived objects). However DeleteLocalRef does allow the JVM to reduce how much memory is used during the function call, particularly inside a loop where you might get 1000s of live objects.

[incanus]

Z-index, great circles, and even circle primitives are non-essential right now. Let's step back and consider more carefully what we focus on in Android — full parity with the raster SDK isn't necessarily the goal.

/cc @bleege

[bleege]

@incanus Thanks for the direction. That said Z-Index I believe is coming from the Google Maps API for Annotations.

[incanus]

Refs #991

[bleege]

Ah... thanks for clarifying @incanus. 👍

[ljbade]

@tobrun Can we close this now PR landed?