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

Use a host interface for CustomLayer instead of function pointers #11553

Merged
merged 6 commits into from
Apr 2, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 19 additions & 23 deletions include/mbgl/style/layers/custom_layer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,11 @@

#include <mbgl/style/layer.hpp>

#include <functional>
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary?


namespace mbgl {
namespace style {

/**
* Initialize any GL state needed by the custom layer. This method is called once, from the
* main thread, at a point when the GL context is active but before rendering for the first
* time.
*
* Resources that are acquired in this method must be released in the UninitializeFunction.
*/
using CustomLayerInitializeFunction = void (*)(void* context);

/**
* Parameters that define the current camera position for a CustomLayerRenderFunction.
*/
Expand All @@ -28,6 +21,18 @@ struct CustomLayerRenderParameters {
double fieldOfView;
};

class CustomLayerHost {
public:
virtual ~CustomLayerHost() = default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: indentation.

/**
* Initialize any GL state needed by the custom layer. This method is called once, from the
* main thread, at a point when the GL context is active but before rendering for the first
* time.
*
* Resources that are acquired in this method must be released in the UninitializeFunction.
Copy link
Contributor

Choose a reason for hiding this comment

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

"in the UninitializeFunction" → "in deinitialize"

*/
virtual void initialize() = 0;

/**
* Render the layer. This method is called once per frame. The implementation should not make
* any assumptions about the GL state (other than that the correct context is active). It may
Expand All @@ -36,38 +41,29 @@ struct CustomLayerRenderParameters {
* Make sure that you are drawing your fragments with a z value of 1 to take advantage of the
* opaque fragment culling in case there are opaque layers above your custom layer.
*/
using CustomLayerRenderFunction = void (*)(void* context, const CustomLayerRenderParameters&);
virtual void render(const CustomLayerRenderParameters&) = 0;

/**
* Called when the system has destroyed the underlying GL context. The
* `CustomLayerDeinitializeFunction` will not be called in this case, however
Copy link
Contributor

Choose a reason for hiding this comment

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

Update docs.

* `CustomLayerInitializeFunction` will be called instead to prepare for a new render.
*
*/
using CustomLayerContextLostFunction = void (*)(void* context);
virtual void contextLost() = 0;

/**
* Destroy any GL state needed by the custom layer, and deallocate context, if necessary. This
* method is called once, from the main thread, at a point when the GL context is active.
*
* Note that it may be called even when the InitializeFunction has not been called.
Copy link
Contributor

Choose a reason for hiding this comment

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

Update docs.

*/
using CustomLayerDeinitializeFunction = void (*)(void* context);
virtual void deinitialize() = 0;
};

class CustomLayer : public Layer {
public:
CustomLayer(const std::string& id,
CustomLayerInitializeFunction,
CustomLayerRenderFunction,
CustomLayerContextLostFunction,
CustomLayerDeinitializeFunction,
void* context);

CustomLayer(const std::string& id,
CustomLayerInitializeFunction,
CustomLayerRenderFunction,
CustomLayerDeinitializeFunction,
void* context);
std::unique_ptr<CustomLayerHost> host);

~CustomLayer() final;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,8 @@
public class CustomLayer extends Layer {

public CustomLayer(String id,
long context,
long initializeFunction,
long renderFunction,
long deinitializeFunction) {
this(id, context, initializeFunction, renderFunction, 0L, deinitializeFunction);
}

public CustomLayer(String id,
long context,
long initializeFunction,
long renderFunction,
long contextLostFunction,
long deinitializeFunction) {
initialize(id, initializeFunction, renderFunction, contextLostFunction, deinitializeFunction, context);
long host) {
initialize(id, host);
}

public CustomLayer(long nativePtr) {
Expand All @@ -33,9 +21,7 @@ public void update() {
nativeUpdate();
}

protected native void initialize(String id, long initializeFunction, long renderFunction,
long contextLostFunction, long deinitializeFunction,
long context);
protected native void initialize(String id, long host);

protected native void nativeUpdate();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,7 @@ private void swapCustomLayer() {
fab.setImageResource(R.drawable.ic_layers);
} else {
customLayer = new CustomLayer("custom",
ExampleCustomLayer.createContext(),
ExampleCustomLayer.InitializeFunction,
ExampleCustomLayer.RenderFunction,
ExampleCustomLayer.ContextLostFunction, // Optional
ExampleCustomLayer.DeinitializeFunction);
ExampleCustomLayer.createContext());
mapboxMap.addLayerBelow(customLayer, "building");
fab.setImageResource(R.drawable.ic_layers_clear);
}
Expand Down
58 changes: 19 additions & 39 deletions platform/android/src/example_custom_layer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ void checkCompileStatus(GLuint shader) {
static const GLchar * vertexShaderSource = "attribute vec2 a_pos; void main() { gl_Position = vec4(a_pos, 0, 1); }";
static const GLchar * fragmentShaderSource = "uniform highp vec4 fill_color; void main() { gl_FragColor = fill_color; }";

class ExampleCustomLayer {
class ExampleCustomLayer: mbgl::style::CustomLayerHost {
public:
~ExampleCustomLayer() {
__android_log_write(ANDROID_LOG_INFO, LOG_TAG, "~ExampleCustomLayer");
Copy link
Contributor

Choose a reason for hiding this comment

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

The code below needs to move to deinitialize. (The host must not assume that the context is active when the destructor is called.)

Copy link
Contributor Author

@asheemmamoowala asheemmamoowala Mar 30, 2018

Choose a reason for hiding this comment

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

For Android, it looks like deinitialize and contextLost need to cleanup

Expand Down Expand Up @@ -158,8 +158,15 @@ class ExampleCustomLayer {
GL_CHECK_ERROR(glBufferData(GL_ARRAY_BUFFER, 8 * sizeof(GLfloat), background, GL_STATIC_DRAW));
}

void render() {
__android_log_write(ANDROID_LOG_INFO, LOG_TAG, "Render");
void render(const mbgl::style::CustomLayerRenderParameters&) {
mbgl::Log::Info(mbgl::Event::General, "Render");
glUseProgram(program);
glBindBuffer(GL_ARRAY_BUFFER, buffer);
glEnableVertexAttribArray(a_pos);
glVertexAttribPointer(a_pos, 2, GL_FLOAT, GL_FALSE, 0, NULL);
glDisable(GL_STENCIL_TEST);
glDisable(GL_DEPTH_TEST);
glUniform4fv(fill_color, 1, color);

GL_CHECK_ERROR(glUseProgram(program));
GL_CHECK_ERROR(glBindBuffer(GL_ARRAY_BUFFER, buffer));
Expand All @@ -172,6 +179,13 @@ class ExampleCustomLayer {

}

void contextLost() {
}

void deinitialize() {

Copy link
Member

Choose a reason for hiding this comment

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

nit additional return vs contextLost()

}

GLuint program = 0;
GLuint vertexShader = 0;
GLuint fragmentShader = 0;
Expand All @@ -186,7 +200,8 @@ GLfloat ExampleCustomLayer::color[] = { 0.0f, 1.0f, 0.0f, 1.0f };

jlong JNICALL nativeCreateContext(JNIEnv*, jobject) {
__android_log_write(ANDROID_LOG_INFO, LOG_TAG, "nativeCreateContext");
return reinterpret_cast<jlong>(new ExampleCustomLayer());
auto exampleCustomLayer = std::make_unique<ExampleCustomLayer>();
return reinterpret_cast<jlong>(exampleCustomLayer.release());
}

void JNICALL nativeSetColor(JNIEnv*, jobject, jfloat red, jfloat green, jfloat blue, jfloat alpha) {
Expand All @@ -197,25 +212,6 @@ void JNICALL nativeSetColor(JNIEnv*, jobject, jfloat red, jfloat green, jfloat b
ExampleCustomLayer::color[3] = alpha;
}

void nativeInitialize(void *context) {
__android_log_write(ANDROID_LOG_INFO, LOG_TAG, "nativeInitialize");
reinterpret_cast<ExampleCustomLayer*>(context)->initialize();
}

void nativeRender(void *context, const mbgl::style::CustomLayerRenderParameters& /*parameters*/) {
__android_log_write(ANDROID_LOG_INFO, LOG_TAG, "nativeRender");
reinterpret_cast<ExampleCustomLayer*>(context)->render();
}

void nativeContextLost(void */*context*/) {
__android_log_write(ANDROID_LOG_INFO, LOG_TAG, "nativeContextLost");
}

void nativeDeinitialize(void *context) {
__android_log_write(ANDROID_LOG_INFO, LOG_TAG, "nativeDeinitialize");
delete reinterpret_cast<ExampleCustomLayer*>(context);
}

extern "C" JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM *vm, void *) {
__android_log_write(ANDROID_LOG_INFO, LOG_TAG, "OnLoad");

Expand All @@ -234,22 +230,6 @@ extern "C" JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM *vm, void *) {
return JNI_ERR;
}

env->SetStaticLongField(customLayerClass,
env->GetStaticFieldID(customLayerClass, "InitializeFunction", "J"),
reinterpret_cast<jlong>(nativeInitialize));

env->SetStaticLongField(customLayerClass,
env->GetStaticFieldID(customLayerClass, "RenderFunction", "J"),
reinterpret_cast<jlong>(nativeRender));

env->SetStaticLongField(customLayerClass,
env->GetStaticFieldID(customLayerClass, "ContextLostFunction", "J"),
reinterpret_cast<jlong>(nativeContextLost));

env->SetStaticLongField(customLayerClass,
env->GetStaticFieldID(customLayerClass, "DeinitializeFunction", "J"),
reinterpret_cast<jlong>(nativeDeinitialize));

return JNI_VERSION_1_6;
}

Expand Down
10 changes: 3 additions & 7 deletions platform/android/src/style/layers/custom_layer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,10 @@
namespace mbgl {
namespace android {

CustomLayer::CustomLayer(jni::JNIEnv& env, jni::String layerId, jni::jlong initializeFunction, jni::jlong renderFunction, jni::jlong contextLostFunction, jni::jlong deinitializeFunction, jni::jlong context)
CustomLayer::CustomLayer(jni::JNIEnv& env, jni::String layerId, jni::jlong host)
: Layer(env, std::make_unique<mbgl::style::CustomLayer>(
jni::Make<std::string>(env, layerId),
reinterpret_cast<mbgl::style::CustomLayerInitializeFunction>(initializeFunction),
reinterpret_cast<mbgl::style::CustomLayerRenderFunction>(renderFunction),
reinterpret_cast<mbgl::style::CustomLayerContextLostFunction>(contextLostFunction),
reinterpret_cast<mbgl::style::CustomLayerDeinitializeFunction>(deinitializeFunction),
reinterpret_cast<void*>(context))
std::unique_ptr<mbgl::style::CustomLayerHost>(reinterpret_cast<mbgl::style::CustomLayerHost*>(host)))
) {
}

Expand Down Expand Up @@ -53,7 +49,7 @@ namespace android {
// Register the peer
jni::RegisterNativePeer<CustomLayer>(
env, CustomLayer::javaClass, "nativePtr",
std::make_unique<CustomLayer, JNIEnv&, jni::String, jni::jlong, jni::jlong, jni::jlong, jni::jlong, jni::jlong>,
std::make_unique<CustomLayer, JNIEnv&, jni::String, jni::jlong>,
"initialize",
"finalize",
METHOD(&CustomLayer::update, "nativeUpdate"));
Expand Down
2 changes: 1 addition & 1 deletion platform/android/src/style/layers/custom_layer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class CustomLayer : public Layer {

static void registerNative(jni::JNIEnv&);

CustomLayer(jni::JNIEnv&, jni::String, jni::jlong, jni::jlong, jni::jlong, jni::jlong, jni::jlong);
CustomLayer(jni::JNIEnv&, jni::String, jni::jlong);

CustomLayer(mbgl::Map&, mbgl::style::CustomLayer&);

Expand Down
86 changes: 43 additions & 43 deletions platform/darwin/src/MGLOpenGLStyleLayer.mm
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#import "MGLOpenGLStyleLayer.h"
#import "MGLOpenGLStyleLayer.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: whitespace.


#import "MGLMapView_Private.h"
#import "MGLStyle_Private.h"
Expand All @@ -7,49 +7,50 @@
#include <mbgl/style/layers/custom_layer.hpp>
#include <mbgl/math/wrap.hpp>

/**
Runs the preparation handler block contained in the given context, which is
implicitly an instance of `MGLOpenGLStyleLayer`.
class MGLOpenGLLayerHost : public mbgl::style::CustomLayerHost {
public:
MGLOpenGLLayerHost(MGLOpenGLStyleLayer *styleLayer) {
layerRef = styleLayer;
layer = nil;
}
~MGLOpenGLLayerHost () {
layer = nil;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary to nil member pointers in a destructor.

}

@param context An `MGLOpenGLStyleLayer` instance that was provided as context
when creating an OpenGL style layer.
*/
void MGLPrepareCustomStyleLayer(void *context) {
MGLOpenGLStyleLayer *layer = (__bridge MGLOpenGLStyleLayer *)context;
[layer didMoveToMapView:layer.style.mapView];
}
void initialize() {
if (layerRef == nil) return;
else if (layer == nil) layer = layerRef;

/**
Runs the drawing handler block contained in the given context, which is
implicitly an instance of `MGLOpenGLStyleLayer`.
[layer didMoveToMapView:layer.style.mapView];
}

@param context An `MGLOpenGLStyleLayer` instance that was provided as context
when creating an OpenGL style layer.
*/
void MGLDrawCustomStyleLayer(void *context, const mbgl::style::CustomLayerRenderParameters &params) {
MGLOpenGLStyleLayer *layer = (__bridge MGLOpenGLStyleLayer *)context;
MGLStyleLayerDrawingContext drawingContext = {
.size = CGSizeMake(params.width, params.height),
.centerCoordinate = CLLocationCoordinate2DMake(params.latitude, params.longitude),
.zoomLevel = params.zoom,
.direction = mbgl::util::wrap(params.bearing, 0., 360.),
.pitch = static_cast<CGFloat>(params.pitch),
.fieldOfView = static_cast<CGFloat>(params.fieldOfView),
};
[layer drawInMapView:layer.style.mapView withContext:drawingContext];
}
void render(const mbgl::style::CustomLayerRenderParameters &params) {
assert(layerRef);

MGLStyleLayerDrawingContext drawingContext = {
.size = CGSizeMake(params.width, params.height),
.centerCoordinate = CLLocationCoordinate2DMake(params.latitude, params.longitude),
.zoomLevel = params.zoom,
.direction = mbgl::util::wrap(params.bearing, 0., 360.),
.pitch = static_cast<CGFloat>(params.pitch),
.fieldOfView = static_cast<CGFloat>(params.fieldOfView),
};
[layer drawInMapView:layer.style.mapView withContext:drawingContext];
}

/**
Runs the completion handler block contained in the given context, which is
implicitly an instance of `MGLOpenGLStyleLayer`.
void contextLost() {}

@param context An `MGLOpenGLStyleLayer` instance that was provided as context
when creating an OpenGL style layer.
*/
void MGLFinishCustomStyleLayer(void *context) {
MGLOpenGLStyleLayer *layer = (__bridge_transfer MGLOpenGLStyleLayer *)context;
[layer willMoveFromMapView:layer.style.mapView];
}
void deinitialize() {
if (layer == nil) return;

[layer willMoveFromMapView:layer.style.mapView];
layerRef = layer;
layer = nil;
}
private:
__weak MGLOpenGLStyleLayer * layerRef;
MGLOpenGLStyleLayer * layer = nil;
};

/**
An `MGLOpenGLStyleLayer` is a style layer that is rendered by OpenGL code that
Expand Down Expand Up @@ -98,10 +99,7 @@ @implementation MGLOpenGLStyleLayer
*/
- (instancetype)initWithIdentifier:(NSString *)identifier {
auto layer = std::make_unique<mbgl::style::CustomLayer>(identifier.UTF8String,
MGLPrepareCustomStyleLayer,
MGLDrawCustomStyleLayer,
MGLFinishCustomStyleLayer,
(__bridge_retained void *)self);
std::make_unique<MGLOpenGLLayerHost>(self));
return self = [super initWithPendingLayer:std::move(layer)];
}

Expand All @@ -116,7 +114,9 @@ - (void)setStyle:(MGLStyle *)style {
[NSException raise:@"MGLLayerReuseException"
format:@"%@ cannot be added to more than one MGLStyle at a time.", self];
}
_style.openGLLayers[self.identifier] = nil;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clears the entry from the previous style when calling [styleLayer removeFromStyle:]. I had removed this in #10765, but I think it needs to be returned. In the Obj-C SDK, the style and the CustomLayerHost-implementation share ownership of the peer layer.

_style = style;
_style.openGLLayers[self.identifier] = self;
}

- (void)addToStyle:(MGLStyle *)style belowLayer:(MGLStyleLayer *)otherLayer {
Expand Down
2 changes: 2 additions & 0 deletions platform/darwin/src/MGLStyle.mm
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ @interface MGLStyle()
@property (nonatomic, readonly, weak) MGLMapView *mapView;
@property (nonatomic, readonly) mbgl::style::Style *rawStyle;
@property (readonly, copy, nullable) NSURL *URL;
@property (nonatomic, readwrite, strong) NS_MUTABLE_DICTIONARY_OF(NSString *, MGLOpenGLStyleLayer *) *openGLLayers;
@property (nonatomic) NS_MUTABLE_DICTIONARY_OF(NSString *, NS_DICTIONARY_OF(NSObject *, MGLTextLanguage *) *) *localizedLayersByIdentifier;

@end
Expand Down Expand Up @@ -125,6 +126,7 @@ - (instancetype)initWithRawStyle:(mbgl::style::Style *)rawStyle mapView:(MGLMapV
if (self = [super init]) {
_mapView = mapView;
_rawStyle = rawStyle;
_openGLLayers = [NSMutableDictionary dictionary];
_localizedLayersByIdentifier = [NSMutableDictionary dictionary];
}
return self;
Expand Down
Loading