From c3cafe7c6c2835c161e1c7d6ad9637fef447101d Mon Sep 17 00:00:00 2001 From: Asheem Mamoowala Date: Fri, 30 Mar 2018 15:23:51 -0700 Subject: Address review feedback and fix broken CI builds --- include/mbgl/renderer/renderer_backend.hpp | 6 +- include/mbgl/style/layers/custom_layer.hpp | 74 ++++++++++++------------ platform/android/src/example_custom_layer.cpp | 23 ++++---- platform/darwin/src/MGLOpenGLStyleLayer.mm | 20 ++----- src/mbgl/renderer/layers/render_custom_layer.cpp | 3 +- src/mbgl/style/layers/custom_layer.cpp | 1 + test/api/custom_layer.test.cpp | 22 ++++--- 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 -#include - 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 ¶ms) { - 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 _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 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; -- cgit v1.2.1