Address review feedback and fix broken CI buildsupstream/ahm-custom-layer-api-change

7 files changed, 68 insertions, 81 deletions

diff --git a/include/mbgl/renderer/renderer_backend.hpp b/include/mbgl/renderer/renderer_backend.hpp
index b83c128169..1d5f4e8f4e 100644
--- a/include/mbgl/renderer/renderer_backend.hpp
+++ b/include/mbgl/renderer/renderer_backend.hpp
@@ -47,10 +47,10 @@ protected:
     // as a matched pair, exclusively through BackendScope, in two situations:
     //
     //   1. When releasing GL resources during Renderer destruction
-    //      (Including calling CustomLayerDeinitializeFunction during RenderCustomLayer destruction)
+    //      (Including calling CustomLayerHost::deinitialize during RenderCustomLayer destruction)
     //   2. When renderering through Renderer::render()
-    //      (Including calling CustomLayerDeinitializeFunction for newly added custom layers and
-    //       CustomLayerDeinitializeFunction on layer removal)
+    //      (Including calling CustomLayerHost::initialize for newly added custom layers and
+    //       CustomLayerHost::deinitialize on layer removal)
     virtual void activate() = 0;
     virtual void deactivate() = 0;
 
diff --git a/include/mbgl/style/layers/custom_layer.hpp b/include/mbgl/style/layers/custom_layer.hpp
index 5aa80a3afe..9911df9a1e 100644
--- a/include/mbgl/style/layers/custom_layer.hpp
+++ b/include/mbgl/style/layers/custom_layer.hpp
@@ -2,13 +2,11 @@
 
 #include <mbgl/style/layer.hpp>
 
-#include <functional>
-
 namespace mbgl {
 namespace style {
 
 /**
- * Parameters that define the current camera position for a CustomLayerRenderFunction.
+ * Parameters that define the current camera position for a `CustomLayerHost::render()` function.
  */
 struct CustomLayerRenderParameters {
     double width;
@@ -23,41 +21,41 @@ struct CustomLayerRenderParameters {
 
 class CustomLayerHost {
 public:
-virtual ~CustomLayerHost() = default;
-/**
- * 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.
- */
-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
- * make changes to the state, and is not required to reset values such as the depth mask, stencil
- * mask, and corresponding test flags to their original values.
- * 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.
- */
-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
- * `CustomLayerInitializeFunction` will be called instead to prepare for a new render.
- *
- */
-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.
- */
-virtual void deinitialize() = 0;
+    virtual ~CustomLayerHost() = default;
+    /**
+     * 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 `deinitialize` function.
+     */
+    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
+     * make changes to the state, and is not required to reset values such as the depth mask, stencil
+     * mask, and corresponding test flags to their original values.
+     * 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.
+     */
+    virtual void render(const CustomLayerRenderParameters&) = 0;
+
+    /**
+     * Called when the system has destroyed the underlying GL context. The
+     * `deinitialize` function will not be called in this case, however
+     * `initialize` will be called instead to prepare for a new render.
+     *
+     */
+    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 `initialize` function has not been called.
+     */
+    virtual void deinitialize() = 0;
 };
 
 class CustomLayer : public Layer {
diff --git a/platform/android/src/example_custom_layer.cpp b/platform/android/src/example_custom_layer.cpp
index 88a3c89eeb..e805532541 100644
--- a/platform/android/src/example_custom_layer.cpp
+++ b/platform/android/src/example_custom_layer.cpp
@@ -115,15 +115,6 @@ static const GLchar * fragmentShaderSource = "uniform highp vec4 fill_color; voi
 class ExampleCustomLayer: mbgl::style::CustomLayerHost {
 public:
     ~ExampleCustomLayer() {
-        __android_log_write(ANDROID_LOG_INFO, LOG_TAG, "~ExampleCustomLayer");
-        if (program) {
-            glDeleteBuffers(1, &buffer);
-            glDetachShader(program, vertexShader);
-            glDetachShader(program, fragmentShader);
-            glDeleteShader(vertexShader);
-            glDeleteShader(fragmentShader);
-            glDeleteProgram(program);
-        }
     }
 
     void initialize() {
@@ -159,7 +150,7 @@ public:
     }
 
     void render(const mbgl::style::CustomLayerRenderParameters&) {
-        mbgl::Log::Info(mbgl::Event::General, "Render");
+        __android_log_write(ANDROID_LOG_INFO, LOG_TAG, "Render");
         glUseProgram(program);
         glBindBuffer(GL_ARRAY_BUFFER, buffer);
         glEnableVertexAttribArray(a_pos);
@@ -180,10 +171,20 @@ public:
     }
 
     void contextLost() {
+        __android_log_write(ANDROID_LOG_INFO, LOG_TAG, "ContextLost");
+        program = 0;
     }
 
     void deinitialize() {
-
+        __android_log_write(ANDROID_LOG_INFO, LOG_TAG, "DeInitialize");
+        if (program) {
+            glDeleteBuffers(1, &buffer);
+            glDetachShader(program, vertexShader);
+            glDetachShader(program, fragmentShader);
+            glDeleteShader(vertexShader);
+            glDeleteShader(fragmentShader);
+            glDeleteProgram(program);
+        }
     }
 
     GLuint program = 0;
diff --git a/platform/darwin/src/MGLOpenGLStyleLayer.mm b/platform/darwin/src/MGLOpenGLStyleLayer.mm
index d9aa4f2575..bbd7422c74 100644
--- a/platform/darwin/src/MGLOpenGLStyleLayer.mm
+++ b/platform/darwin/src/MGLOpenGLStyleLayer.mm
@@ -1,4 +1,4 @@
- #import "MGLOpenGLStyleLayer.h"
+#import "MGLOpenGLStyleLayer.h"
 
 #import "MGLMapView_Private.h"
 #import "MGLStyle_Private.h"
@@ -13,9 +13,6 @@ public:
         layerRef = styleLayer;
         layer = nil;
     }
-    ~MGLOpenGLLayerHost () {
-        layer = nil;
-    }
 
     void initialize() {
         if (layerRef == nil) return;
@@ -25,7 +22,7 @@ public:
     }
 
     void render(const mbgl::style::CustomLayerRenderParameters &params) {
-        assert(layerRef);
+        if(!layer) return;
 
         MGLStyleLayerDrawingContext drawingContext = {
             .size = CGSizeMake(params.width, params.height),
@@ -108,24 +105,15 @@ private:
 }
 
 #pragma mark - Adding to and removing from a map view
-
-- (void)setStyle:(MGLStyle *)style {
-    if (_style && style) {
-        [NSException raise:@"MGLLayerReuseException"
-                    format:@"%@ cannot be added to more than one MGLStyle at a time.", self];
-    }
-    _style.openGLLayers[self.identifier] = nil;
-    _style = style;
-    _style.openGLLayers[self.identifier] = self;
-}
-
 - (void)addToStyle:(MGLStyle *)style belowLayer:(MGLStyleLayer *)otherLayer {
     self.style = style;
+    self.style.openGLLayers[self.identifier] = self;
     [super addToStyle:style belowLayer:otherLayer];
 }
 
 - (void)removeFromStyle:(MGLStyle *)style {
     [super removeFromStyle:style];
+    self.style.openGLLayers[self.identifier] = nil;
     self.style = nil;
 }
 
diff --git a/src/mbgl/renderer/layers/render_custom_layer.cpp b/src/mbgl/renderer/layers/render_custom_layer.cpp
index cc9eede463..85ca5b074e 100644
--- a/src/mbgl/renderer/layers/render_custom_layer.cpp
+++ b/src/mbgl/renderer/layers/render_custom_layer.cpp
@@ -13,6 +13,7 @@ using namespace style;
 
 RenderCustomLayer::RenderCustomLayer(Immutable<style::CustomLayer::Impl> _impl)
     : RenderLayer(LayerType::Custom, _impl), host(_impl->host) {
+    assert(BackendScope::exists());
     host->initialize();
 }
 
@@ -20,7 +21,7 @@ RenderCustomLayer::~RenderCustomLayer() {
     assert(BackendScope::exists());
     if (contextDestroyed) {
         host->contextLost();
-    } else if (!contextDestroyed) {
+    } else {
         host->deinitialize();
     }
 }
diff --git a/src/mbgl/style/layers/custom_layer.cpp b/src/mbgl/style/layers/custom_layer.cpp
index f6749275ee..0e51a70e50 100644
--- a/src/mbgl/style/layers/custom_layer.cpp
+++ b/src/mbgl/style/layers/custom_layer.cpp
@@ -26,6 +26,7 @@ std::unique_ptr<Layer> CustomLayer::cloneRef(const std::string&) const {
 }
 
 // Visibility
+
 void CustomLayer::setVisibility(VisibilityType value) {
     if (value == getVisibility())
         return;
diff --git a/test/api/custom_layer.test.cpp b/test/api/custom_layer.test.cpp
index b95e4117ae..6cb148a349 100644
--- a/test/api/custom_layer.test.cpp
+++ b/test/api/custom_layer.test.cpp
@@ -36,17 +36,6 @@ void main() {
 
 class TestLayer : public mbgl::style::CustomLayerHost {
 public:
-    ~TestLayer() {
-        if (program) {
-            MBGL_CHECK_ERROR(glDeleteBuffers(1, &buffer));
-            MBGL_CHECK_ERROR(glDetachShader(program, vertexShader));
-            MBGL_CHECK_ERROR(glDetachShader(program, fragmentShader));
-            MBGL_CHECK_ERROR(glDeleteShader(vertexShader));
-            MBGL_CHECK_ERROR(glDeleteShader(fragmentShader));
-            MBGL_CHECK_ERROR(glDeleteProgram(program));
-        }
-    }
-
     void initialize() {
         program = MBGL_CHECK_ERROR(glCreateProgram());
         vertexShader = MBGL_CHECK_ERROR(glCreateShader(GL_VERTEX_SHADER));
@@ -77,7 +66,16 @@ public:
 
     void contextLost() {}
 
-    void deinitialize() {}
+    void deinitialize() {
+         if (program) {
+                MBGL_CHECK_ERROR(glDeleteBuffers(1, &buffer));
+                MBGL_CHECK_ERROR(glDetachShader(program, vertexShader));
+                MBGL_CHECK_ERROR(glDetachShader(program, fragmentShader));
+                MBGL_CHECK_ERROR(glDeleteShader(vertexShader));
+                MBGL_CHECK_ERROR(glDeleteShader(fragmentShader));
+                MBGL_CHECK_ERROR(glDeleteProgram(program));
+            }
+    }
 
     GLuint program = 0;
     GLuint vertexShader = 0;