[android] Refactor mapview + associated classes to high-level JNI bindings #8083

ivovandongen · 2017-02-16T14:39:32Z

Main classes:

NativeMapView
FileSource
Offline

mention-bot · 2017-02-16T14:39:36Z

@ivovandongen, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @zugaldia and @1ec5 to be potential reviewers.

tobrun · 2017-02-17T09:20:06Z

Looking great so far!

tobrun · 2017-02-23T15:46:38Z

I'm able to build arm-v7 but hitting build issues for arm-v8:

tvn@tvn:~/Mapbox/mapbox-gl-native$ make android-lib-arm-v8
. build/android-arm-v8-21/Debug/env.sh && ${CMAKE} --build build/android-arm-v8-21/Debug -- -j8 mapbox-gl example-custom-layer
[1/6] Building CXX object CMakeFiles/mbgl-core.dir/platform/android/src/offline/offline_region.cpp.o
FAILED: /home/tvn/Android/Sdk/ndk-bundle/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++ -target aarch64-none-linux-android -gcc-toolchain /home/tvn/Android/Sdk/ndk-bundle/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64 --sysroot=/home/tvn/Android/Sdk/ndk-bundle/platforms/android-21/arch-arm64 -DMBGL_USE_GLES2=1 -DRAPIDJSON_HAS_STDSTRING=1 -DUCHAR_TYPE=char16_t -I../../../include -I../../../src -I../../../mason_packages/headers/geometry/0.9.0/include -I../../../mason_packages/headers/variant/1.1.4/include -I../../../mason_packages/headers/unique_resource/cba309e/include -I../../../mason_packages/headers/rapidjson/1.1.0/include -I../../../mason_packages/headers/boost/1.62.0/include -I../../../mason_packages/headers/geojson/0.4.0/include -I../../../mason_packages/headers/geojsonvt/6.2.0/include -I../../../mason_packages/headers/supercluster/0.2.0-1/include -I../../../mason_packages/headers/kdbush/0.1.1-1/include -I../../../mason_packages/headers/earcut/0.12.1/include -I../../../mason_packages/headers/protozero/1.4.2/include -I../../../mason_packages/headers/polylabel/1.0.2/include -I../../../platform/default -I../../../mason_packages/android-arm-v8-21/sqlite/3.14.2/include -I../../../mason_packages/android-arm-v8-21/nunicode/1.7.1/include -I../../../mason_packages/android-arm-v8-21/libzip/1.1.3/include -I../../../mason_packages/android-arm-v8-21/libzip/1.1.3/lib/libzip/include -I../../../mason_packages/headers/jni.hpp/3.0.0/include -I../../../mason_packages/android-arm-v8-21/icu/58.1/include -isystem /home/tvn/Android/Sdk/ndk-bundle/sources/cxx-stl/llvm-libc++/include -isystem /home/tvn/Android/Sdk/ndk-bundle/sources/android/support/include -isystem /home/tvn/Android/Sdk/ndk-bundle/sources/cxx-stl/llvm-libc++abi/include -g -DANDROID -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -Wa,--noexecstack -Wformat -Werror=format-security -fno-exceptions -fno-rtti -std=c++11 -frtti -fexceptions -std=c++14 -ftemplate-depth=1024 -Wall -Wextra -Wshadow -Werror -Wno-variadic-macros -Wno-unknown-pragmas -O0 -fno-limit-debug-info -fPIC -fPIC -fvisibility-inlines-hidden -fvisibility=hidden -ffunction-sections -fdata-sections -Os -MD -MT CMakeFiles/mbgl-core.dir/platform/android/src/offline/offline_region.cpp.o -MF CMakeFiles/mbgl-core.dir/platform/android/src/offline/offline_region.cpp.o.d -o CMakeFiles/mbgl-core.dir/platform/android/src/offline/offline_region.cpp.o -c /home/tvn/Mapbox/mapbox-gl-native/platform/android/src/offline/offline_region.cpp
/home/tvn/Mapbox/mapbox-gl-native/platform/android/src/offline/offline_region.cpp:169:45: error: no matching member function for call to 'New'
auto jregion = OfflineRegion::javaClass.New(env, constructor,
~~~~~~~~~~~~~~~~~~~~~~~~~^~~
../../../mason_packages/headers/jni.hpp/3.0.0/include/jni/class.hpp:51:29: note: candidate template ignored: deduced conflicting types for parameter 'Args' (<long long, jni::Objectmbgl::android::FileSource, long long, jni::Objectmbgl::android::OfflineRegionDefinition, jni::Array<signed char, void>> vs. <long long, jni::Objectmbgl::android::FileSource, long, jni::Objectmbgl::android::OfflineRegionDefinition, jni::Array<signed char, void>>)
Object New(JNIEnv& env, const Constructor<TagType, Args...>& method, const Args&... args) const
^
1 error generated.
ninja: build stopped: subcommand failed.
Makefile:597: recipe for target 'android-lib-arm-v8' failed
make: *** [android-lib-arm-v8] Error 1

ivovandongen · 2017-02-23T15:52:25Z

@tobrun Could you clean your build first and try again? Just did a fresh build without problems:

17:24 $ make android-lib-arm-v8
platform/android/scripts/ndk.sh arm-v8 arm64-v8a 21 > build/android-arm-v8-21/Debug/env.sh.tmp && mv build/android-arm-v8-21/Debug/env.sh.tmp build/android-arm-v8-21/Debug/env.sh
Using system-provided Android NDK at /Users/ivo/Library/Android/sdk/ndk-bundle
# Invoke CMake twice to fix issues from double inclusion of toolchain.cmake on the first run.
. build/android-arm-v8-21/Debug/env.sh && ([ -f build/android-arm-v8-21/Debug/build.ninja ] || ${CMAKE} -H. -B"build/android-arm-v8-21/Debug" -G"${CMAKE_GENERATOR}" ${CMAKE_ARGS} -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DMBGL_PLATFORM=android -DMASON_PLATFORM=android -DMASON_PLATFORM_VERSION=arm-v8-21) && ${CMAKE} -H. -B"build/android-arm-v8-21/Debug"
-- Check for working CXX compiler: /Users/ivo/Library/Android/sdk/ndk-bundle/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang++
-- Check for working CXX compiler: /Users/ivo/Library/Android/sdk/ndk-bundle/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Check for working C compiler: /Users/ivo/Library/Android/sdk/ndk-bundle/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang
-- Check for working C compiler: /Users/ivo/Library/Android/sdk/ndk-bundle/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Updating submodules...
[Mason] Downloading package https://mason-binaries.s3.amazonaws.com/android-arm-v8-21/gtest/1.8.0.tar.gz...
[Mason] Unpacking package to mason_packages/android-arm-v8-21/gtest/1.8.0...
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/ivo/git/mapbox-gl-native/build/android-arm-v8-21/Debug
-- Updating submodules...
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/ivo/git/mapbox-gl-native/build/android-arm-v8-21/Debug
. build/android-arm-v8-21/Debug/env.sh && ${CMAKE} --build build/android-arm-v8-21/Debug --  -j8 mapbox-gl example-custom-layer
[230/230] Linking CXX shared library libexample-custom-layer.so

kkaefer · 2017-02-17T11:00:38Z

platform/android/src/geometry/feature.cpp

+
+void Feature::registerNative(jni::JNIEnv& env) {
+    // Lookup the class
+    Feature::javaClass = *jni::Class<Feature>::Find(env).NewGlobalRef(env).release();


No need to specify Feature:: in these static methods.

Thanks. Saw your comments too late. Will fix this up in a follow-up pr

kkaefer · 2017-02-23T15:57:55Z

platform/android/src/annotation/marker.cpp

+jni::Class<Marker> Marker::javaClass;
+
+mbgl::Point<double> Marker::getPosition(jni::JNIEnv& env, jni::Object<Marker> marker) {
+    static auto positionField = Marker::javaClass.GetField<jni::Object<LatLng>>(env, "position");


Are these fields guaranteed to be the same on all threads? Can these methods be invoked on other threads?

There is one class definition for all threads as far as I know. The passed in env is assumed to be already attached.

ivovandongen added Android Mapbox Maps SDK for Android ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Feb 16, 2017

ivovandongen added this to the android-v5.0.0 milestone Feb 16, 2017

ivovandongen self-assigned this Feb 16, 2017

ivovandongen force-pushed the ivovandongen-jni-refactor branch 7 times, most recently from a654051 to 677ba1d Compare February 23, 2017 14:39

ivovandongen requested review from zugaldia and tobrun February 23, 2017 14:39

ivovandongen force-pushed the ivovandongen-jni-refactor branch from 677ba1d to b8e11e2 Compare February 23, 2017 15:00

ivovandongen force-pushed the ivovandongen-jni-refactor branch 2 times, most recently from 082630d to 40e96c4 Compare February 24, 2017 11:55

[android] jni high level binding refactor

bb123db

ivovandongen force-pushed the ivovandongen-jni-refactor branch from 40e96c4 to bb123db Compare February 24, 2017 15:27

ivovandongen removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Feb 24, 2017

tobrun approved these changes Feb 24, 2017

View reviewed changes

kkaefer reviewed Feb 24, 2017

View reviewed changes

ivovandongen merged commit 465c949 into master Feb 24, 2017

ivovandongen deleted the ivovandongen-jni-refactor branch February 24, 2017 15:55

tobrun mentioned this pull request Mar 1, 2017

OfflineManager and MapView should share a singleton DefaultFileSource #4386

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[android] Refactor mapview + associated classes to high-level JNI bindings #8083

[android] Refactor mapview + associated classes to high-level JNI bindings #8083

ivovandongen commented Feb 16, 2017 •

edited

Loading

mention-bot commented Feb 16, 2017

tobrun commented Feb 17, 2017

tobrun commented Feb 23, 2017 •

edited

Loading

ivovandongen commented Feb 23, 2017

kkaefer Feb 17, 2017

ivovandongen Feb 24, 2017

kkaefer Feb 23, 2017

ivovandongen Feb 24, 2017

[android] Refactor mapview + associated classes to high-level JNI bindings #8083

[android] Refactor mapview + associated classes to high-level JNI bindings #8083

Conversation

ivovandongen commented Feb 16, 2017 • edited Loading

mention-bot commented Feb 16, 2017

tobrun commented Feb 17, 2017

tobrun commented Feb 23, 2017 • edited Loading

ivovandongen commented Feb 23, 2017

kkaefer Feb 17, 2017

Choose a reason for hiding this comment

ivovandongen Feb 24, 2017

Choose a reason for hiding this comment

kkaefer Feb 23, 2017

Choose a reason for hiding this comment

ivovandongen Feb 24, 2017

Choose a reason for hiding this comment

ivovandongen commented Feb 16, 2017 •

edited

Loading

tobrun commented Feb 23, 2017 •

edited

Loading