Skip to content

Commit

Permalink
Addressed Comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@tallytalwar
tallytalwar committed Dec 14, 2016
1 parent 217092b commit 7353bbe
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 47 deletions.
35 changes: 23 additions & 12 deletions android/tangram/jni/jniExports.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,29 +110,21 @@ extern "C" {
delete map;
}

JNIEXPORT void JNICALL Java_com_mapzen_tangram_MapController_nativeLoadScene(JNIEnv* jniEnv, jobject obj, jlong mapPtr, jstring path) {
JNIEXPORT void JNICALL Java_com_mapzen_tangram_MapController_nativeLoadScene(JNIEnv* jniEnv, jobject obj, jlong mapPtr, jstring path, jobjectArray jcomponentPaths, jobjectArray jcomponentValues) {
assert(mapPtr > 0);
auto map = reinterpret_cast<Tangram::Map*>(mapPtr);
const char* cPath = jniEnv->GetStringUTFChars(path, NULL);
map->loadScene(resolveScenePath(cPath).c_str());
jniEnv->ReleaseStringUTFChars(path, cPath);
}

JNIEXPORT void JNICALL Java_com_mapzen_tangram_MapController_nativeLoadSceneWithUpdates(JNIEnv* jniEnv, jobject obj, jlong mapPtr, jstring path, jobjectArray jsceneUpdatePaths, jobjectArray jsceneUpdateValues) {
assert(mapPtr > 0);
size_t n_sceneUpdates = (jsceneUpdatePaths == NULL)? 0 : jniEnv->GetArrayLength(jsceneUpdatePaths);
size_t n_sceneUpdates = (jcomponentPaths == NULL)? 0 : jniEnv->GetArrayLength(jcomponentPaths);

std::vector<Tangram::SceneUpdate> sceneUpdates;
for (size_t i = 0; i < n_sceneUpdates; i++) {
jstring path = (jstring) (jniEnv->GetObjectArrayElement(jsceneUpdatePaths, i));
jstring value = (jstring) (jniEnv->GetObjectArrayElement(jsceneUpdateValues, i));
jstring path = (jstring) (jniEnv->GetObjectArrayElement(jcomponentPaths, i));
jstring value = (jstring) (jniEnv->GetObjectArrayElement(jcomponentValues, i));
sceneUpdates.emplace_back(stringFromJString(jniEnv, path), stringFromJString(jniEnv, value));
jniEnv->DeleteLocalRef(path);
jniEnv->DeleteLocalRef(value);
}

auto map = reinterpret_cast<Tangram::Map*>(mapPtr);
const char* cPath = jniEnv->GetStringUTFChars(path, NULL);
map->loadScene(resolveScenePath(cPath).c_str(), false, sceneUpdates);
jniEnv->ReleaseStringUTFChars(path, cPath);
}
Expand Down Expand Up @@ -475,6 +467,25 @@ extern "C" {
jnienv->ReleaseStringUTFChars(value, cValue);
}

JNIEXPORT void JNICALL Java_com_mapzen_tangram_MapController_nativeQueueSceneUpdates(JNIEnv* jniEnv, jobject obj, jlong mapPtr, jobjectArray jcomponentPaths, jobjectArray jcomponentValues) {
assert(mapPtr > 0);
size_t n_sceneUpdates = (jcomponentPaths == NULL)? 0 : jniEnv->GetArrayLength(jcomponentPaths);

std::vector<Tangram::SceneUpdate> sceneUpdates;
for (size_t i = 0; i < n_sceneUpdates; i++) {
jstring path = (jstring) (jniEnv->GetObjectArrayElement(jcomponentPaths, i));
jstring value = (jstring) (jniEnv->GetObjectArrayElement(jcomponentValues, i));
sceneUpdates.emplace_back(stringFromJString(jniEnv, path), stringFromJString(jniEnv, value));
jniEnv->DeleteLocalRef(path);
jniEnv->DeleteLocalRef(value);
}

if (sceneUpdates.empty()) { return; }

auto map = reinterpret_cast<Tangram::Map*>(mapPtr);
map->queueSceneUpdate(sceneUpdates);
}

JNIEXPORT void JNICALL Java_com_mapzen_tangram_MapController_nativeApplySceneUpdates(JNIEnv* jnienv, jobject obj, jlong mapPtr) {
assert(mapPtr > 0);
auto map = reinterpret_cast<Tangram::Map*>(mapPtr);
Expand Down
49 changes: 30 additions & 19 deletions android/tangram/src/com/mapzen/tangram/MapController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;

import javax.microedition.khronos.egl.EGLConfig;
Expand Down Expand Up @@ -225,27 +226,32 @@ static MapController getInstance(GLSurfaceView view) {
public void loadSceneFile(String path) {
scenePath = path;
checkPointer(mapPointer);
nativeLoadScene(mapPointer, path);
nativeLoadScene(mapPointer, path, new String[0], new String[0]);

This comment has been minimized.

Sign in to view

Copy link
@karimnaaji

karimnaaji Dec 15, 2016

Member

Nullity seems to be checked in the JNI code, what about calling it with NULL directly? Not sure what an allocated array of size 0 means in Java.
requestRender();
}

/**
* Load a new scene file
* @param path Location of the YAML scene file within the application assets
* @param sceneUpdates List of path:value pairs to be applied when loading this scene
* @param sceneUpdates List of {@code SceneUpdate}
*/
public void loadSceneFile(String path, ArrayList<SceneUpdate> sceneUpdates) {
String[] paths = new String[sceneUpdates.size()];
String[] values = new String[sceneUpdates.size()];
public void loadSceneFile(String path, List<SceneUpdate> sceneUpdates) {

if (sceneUpdates == null) {
// default to loading the scene file
loadSceneFile(path);
}
String[] componentPaths = new String[sceneUpdates.size()];
String[] componentValues = new String[sceneUpdates.size()];
int index = 0;
for (SceneUpdate sceneUpdate : sceneUpdates) {
paths[index] = sceneUpdate.path;
values[index] = sceneUpdate.value;
componentPaths[index] = sceneUpdate.path();
componentValues[index] = sceneUpdate.value();
index++;
}
scenePath = path;
checkPointer(mapPointer);
nativeLoadSceneWithUpdates(mapPointer, path, paths, values);
nativeLoadScene(mapPointer, path, componentPaths, componentValues);
requestRender();
}

Expand Down Expand Up @@ -759,24 +765,29 @@ public void setDebugFlag(DebugFlag flag, boolean on) {

/**
* Enqueue a scene component update with its corresponding YAML node value
* @param componentPath The YAML component path delimited by a '.' (example "scene.animated")
* @param value A YAML valid string (example "{ property: true }" or "true")
* @param sceneUpdate A {@code SceneUpdate}
*/
public void queueSceneUpdate(String componentPath, String value) {
public void queueSceneUpdate(SceneUpdate sceneUpdate) {
checkPointer(mapPointer);
nativeQueueSceneUpdate(mapPointer, componentPath, value);
nativeQueueSceneUpdate(mapPointer, sceneUpdate.path(), sceneUpdate.value());
}

/**
* Enqueue a scene component update with its corresponding YAML node value
* @param componentPath The YAML component path delimited by a '.' (example "scene.animated")
* @param value A YAML valid string (example "{ property: true }" or "true")
* @param sceneUpdates List of {@code SceneUpdate}
*/
public void queueSceneUpdate(ArrayList<SceneUpdate> sceneUpdates) {
public void queueSceneUpdate(List<SceneUpdate> sceneUpdates) {
checkPointer(mapPointer);
String[] componentPaths = new String[sceneUpdates.size()];
String[] componentValues = new String[sceneUpdates.size()];

int index = 0;
for(SceneUpdate sceneUpdate : sceneUpdates) {
nativeQueueSceneUpdate(mapPointer, sceneUpdate.path, sceneUpdate.value);
componentPaths[index] = sceneUpdate.path();
componentValues[index] = sceneUpdate.value();
index++;
}
nativeQueueSceneUpdates(mapPointer, componentPaths, componentValues);
}

/**
Expand Down Expand Up @@ -894,8 +905,7 @@ boolean setMarkerDrawOrder(long markerId, int drawOrder) {

private synchronized native long nativeInit(MapController instance, AssetManager assetManager);
private synchronized native void nativeDispose(long mapPtr);
private synchronized native void nativeLoadScene(long mapPtr, String path);
private synchronized native void nativeLoadSceneWithUpdates(long mapPtr, String path, String[] sceneUpdatePaths, String[] sceneUpdateValues);
private synchronized native void nativeLoadScene(long mapPtr, String path, String[] componentPaths, String[] componentValues);
private synchronized native void nativeSetupGL(long mapPtr);
private synchronized native void nativeResize(long mapPtr, int width, int height);
private synchronized native boolean nativeUpdate(long mapPtr, float dt);
Expand Down Expand Up @@ -924,7 +934,8 @@ boolean setMarkerDrawOrder(long markerId, int drawOrder) {
private synchronized native void nativeHandlePinchGesture(long mapPtr, float posX, float posY, float scale, float velocity);
private synchronized native void nativeHandleRotateGesture(long mapPtr, float posX, float posY, float rotation);
private synchronized native void nativeHandleShoveGesture(long mapPtr, float distance);
private synchronized native void nativeQueueSceneUpdate(long mapPtr, String componentPath, String value);
private synchronized native void nativeQueueSceneUpdate(long mapPtr, String componentPath, String componentValue);
private synchronized native void nativeQueueSceneUpdates(long mapPtr, String[] componentPaths, String[] componentValues);
private synchronized native void nativeApplySceneUpdates(long mapPtr);
private synchronized native void nativePickFeature(long mapPtr, float posX, float posY, FeaturePickListener listener);
private synchronized native void nativePickLabel(long mapPtr, float posX, float posY, LabelPickListener listener);
Expand Down
9 changes: 3 additions & 6 deletions android/tangram/src/com/mapzen/tangram/MapView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import android.widget.FrameLayout;

import java.util.ArrayList;
import java.util.List;

/**
* {@code MapView} is a View for displaying a Tangram map.
Expand Down Expand Up @@ -69,7 +70,7 @@ public void getMapAsync(@NonNull final OnMapReadyCallback callback,
*/
public void getMapAsync(@NonNull final OnMapReadyCallback callback,
@NonNull final String sceneFilePath,
final ArrayList<SceneUpdate> sceneUpdates) {
final List<SceneUpdate> sceneUpdates) {

disposeTask();

Expand All @@ -81,11 +82,7 @@ public void getMapAsync(@NonNull final OnMapReadyCallback callback,
@SuppressWarnings("WrongThread")
protected Boolean doInBackground(Void... params) {
mapInstance.init();
if (sceneUpdates.size() > 0) {
mapInstance.loadSceneFile(sceneFilePath, sceneUpdates);
} else {
mapInstance.loadSceneFile(sceneFilePath);
}
mapInstance.loadSceneFile(sceneFilePath, sceneUpdates);
return true;
}

Expand Down
21 changes: 11 additions & 10 deletions android/tangram/src/com/mapzen/tangram/SceneUpdate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,20 @@

public class SceneUpdate {

public String path;
public String value;

public SceneUpdate() { this("", ""); }
private String componentPath;
private String componentValue;

/**
* Add a point feature to this collection.
* @param path Series of yaml keys separated by a ".". Represents the scene path to be updated
* @param value A yaml string which will update the value at the specified path
*/
public SceneUpdate(String path, String value) {
set(path, value);
this.componentPath = path;
this.componentValue = value;
}

public SceneUpdate set(String p, String v) {
path = p;
value = v;
return this;
}
public String path() { return componentPath; }

This comment has been minimized.

Sign in to view

Copy link
@karimnaaji

karimnaaji Dec 15, 2016

Member

should be getPath and getValue()

This comment has been minimized.

Sign in to view

Copy link
@matteblair

matteblair Dec 15, 2016

Member

NB: Unlike in C++, the convention for naming getters and setters is explicit and has material impact. Java IDEs and the Kotlin language itself use the "getMember" and "setMember" notations for auto-completion, code generation, et cetera.
public String value() { return componentValue; }

}

0 comments on commit 7353bbe

Please sign in to comment.