Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[android] Add jni binding for styleable snapshotter #16286

Merged
merged 18 commits into from
Mar 24, 2020

Conversation

Chaoba
Copy link
Contributor

@Chaoba Chaoba commented Mar 9, 2020

This pr add jni binding for styleable napshotter.
The related android pr: mapbox/mapbox-gl-native-android#268

Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to expose MapSnapshotterObserver and MapSnapshotter::style() to the Java side, so that the client can add / remove layers and sources, as well as subscribe to MapSnapshotterObserver events.

@Chaoba
Copy link
Contributor Author

Chaoba commented Mar 10, 2020

@alexshalamov This is the first idea come into my mind, but Style instance is created with NativeMapView in android sdk. @tobrun do you think it is possible to create Style from MapSnapshotter?

@tobrun
Copy link
Member

tobrun commented Mar 10, 2020

@Chaoba current Style.java is tied-to and optimized-for the map implementation and will not match the requirements for this feature. It's not a peer object that has 1-on-1 relation with the c++ equivalent.

For this iteration of the SDK, I feel we should support the full style API as addLayer(Layer layer)/removeSource()/etc. on the java side that will eventually call into snapshotter->getStyle() in jni code. This way we don't need to marshall the style object.

@Chaoba Chaoba changed the title [WIP][android] Add jni binding for styleable snapshotter [android] Add jni binding for styleable snapshotter Mar 17, 2020
@Chaoba Chaoba requested a review from tobrun March 17, 2020 08:42
@alexshalamov
Copy link
Contributor

@pozdnyakov I could not figure out why scheduled lambda goes out of scope in caller thread before it is pushed to the target scheduler queue. Both 1 and 2 were not reliable. 3rd approach doesn't look nice, but works.

  1. First approach eeea3af
  2. Custom deleter cd91d5e
  3. Deleting lambda d0d560e

@pozdnyakov
Copy link
Contributor

@alexshalamov here I guess it shall be weakScheduler->schedule([ptr = std::shared_ptr<mbgl::MapSnapshotter>(p)] {});

Copy link
Contributor

@pozdnyakov pozdnyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few nits

platform/android/src/snapshotter/map_snapshotter.cpp Outdated Show resolved Hide resolved
platform/android/src/snapshotter/map_snapshotter.cpp Outdated Show resolved Hide resolved
platform/android/src/snapshotter/map_snapshotter.cpp Outdated Show resolved Hide resolved
platform/android/src/snapshotter/map_snapshotter.cpp Outdated Show resolved Hide resolved
platform/android/src/snapshotter/map_snapshotter.cpp Outdated Show resolved Hide resolved
platform/android/src/snapshotter/map_snapshotter.cpp Outdated Show resolved Hide resolved
platform/android/src/snapshotter/map_snapshotter.cpp Outdated Show resolved Hide resolved
platform/android/src/snapshotter/map_snapshotter.hpp Outdated Show resolved Hide resolved
@alexshalamov alexshalamov requested review from alexshalamov and removed request for alexshalamov March 20, 2020 23:16
@Chaoba Chaoba merged commit 3f45b0a into master Mar 24, 2020
@Chaoba Chaoba deleted the kl-styleable-snapshotter branch March 24, 2020 06:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants