[core] Make ElementGroup safer

Template on shader types, rather than count. This allows the compiler to enforce using the correct VAO for the shader and PaintMode. This fixes OverdrawMode with circle layers.

While here, avoid using unique_ptrs for groups. Instead, ensure ElementGroup is movable.

cmake/core-files.cmake

@@ -61,7 +61,6 @@ set(MBGL_CORE_FILES
     src/mbgl/gl/context.hpp
     src/mbgl/gl/debugging.cpp
     src/mbgl/gl/debugging.hpp
-    src/mbgl/gl/element_group.hpp
     src/mbgl/gl/extension.cpp
     src/mbgl/gl/extension.hpp
     src/mbgl/gl/gl.cpp
@@ -143,6 +142,7 @@ set(MBGL_CORE_FILES
     src/mbgl/renderer/circle_bucket.hpp
     src/mbgl/renderer/debug_bucket.cpp
     src/mbgl/renderer/debug_bucket.hpp
+    src/mbgl/renderer/element_group.hpp
     src/mbgl/renderer/fill_bucket.cpp
     src/mbgl/renderer/fill_bucket.hpp
     src/mbgl/renderer/frame_history.cpp

src/mbgl/gl/vao.cpp

@@ -7,11 +7,6 @@
 namespace mbgl {
 namespace gl {
 
-VertexArrayObject::VertexArrayObject() {
-}
-
-VertexArrayObject::~VertexArrayObject() = default;
-
 void VertexArrayObject::bindVertexArrayObject(Context& context) {
     if (!GenVertexArrays || !BindVertexArray) {
         static bool reported = false;

src/mbgl/gl/vao.hpp

@@ -3,19 +3,15 @@
 #include <mbgl/gl/shader.hpp>
 #include <mbgl/gl/context.hpp>
 #include <mbgl/gl/vertex_buffer.hpp>
-#include <mbgl/util/noncopyable.hpp>
 #include <mbgl/util/optional.hpp>
 
 #include <stdexcept>
 
 namespace mbgl {
 namespace gl {
 
-class VertexArrayObject : public util::noncopyable {
+class VertexArrayObject {
 public:
-    VertexArrayObject();
-    ~VertexArrayObject();
-
     template <typename Shader, typename T>
     void bind(Shader& shader,
               const VertexBuffer<T>& vertexBuffer,

src/mbgl/layout/symbol_layout.cpp

@@ -391,7 +391,7 @@ std::unique_ptr<SymbolBucket> SymbolLayout::place(CollisionTile& collisionTile)
         if (hasText) {
             collisionTile.insertFeature(symbolInstance.textCollisionFeature, glyphScale, layout.textIgnorePlacement);
             if (glyphScale < collisionTile.maxScale) {
-                addSymbols<SymbolBucket::TextBuffer, SymbolBucket::TextElementGroup>(
+                addSymbols(
                     bucket->text, symbolInstance.glyphQuads, glyphScale,
                     layout.textKeepUpright, textPlacement, collisionTile.config.angle);
             }
@@ -400,7 +400,7 @@ std::unique_ptr<SymbolBucket> SymbolLayout::place(CollisionTile& collisionTile)
         if (hasIcon) {
             collisionTile.insertFeature(symbolInstance.iconCollisionFeature, iconScale, layout.iconIgnorePlacement);
             if (iconScale < collisionTile.maxScale) {
-                addSymbols<SymbolBucket::IconBuffer, SymbolBucket::IconElementGroup>(
+                addSymbols(
                     bucket->icon, symbolInstance.iconQuads, iconScale,
                     layout.iconKeepUpright, iconPlacement, collisionTile.config.angle);
             }
@@ -414,7 +414,7 @@ std::unique_ptr<SymbolBucket> SymbolLayout::place(CollisionTile& collisionTile)
     return bucket;
 }
 
-template <typename Buffer, typename GroupType>
+template <typename Buffer>
 void SymbolLayout::addSymbols(Buffer &buffer, const SymbolQuads &symbols, float scale, const bool keepUpright, const style::SymbolPlacementType placement, const float placementAngle) {
 
     const float placementZoom = ::fmax(std::log(scale) / std::log(2) + zoom, 0);
@@ -449,16 +449,15 @@ void SymbolLayout::addSymbols(Buffer &buffer, const SymbolQuads &symbols, float
 
         const int glyph_vertex_length = 4;
 
-        if (buffer.groups.empty() || (buffer.groups.back()->vertex_length + glyph_vertex_length > 65535)) {
+        if (buffer.groups.empty() || buffer.groups.back().vertexLength + glyph_vertex_length > 65535) {
             // Move to a new group because the old one can't hold the geometry.
-            buffer.groups.emplace_back(std::make_unique<GroupType>());
+            buffer.groups.emplace_back();
         }
 
         // We're generating triangle fans, so we always start with the first
         // coordinate in this polygon.
-        assert(buffer.groups.back());
-        auto &triangleGroup = *buffer.groups.back();
-        size_t triangleIndex = triangleGroup.vertex_length;
+        auto& group = buffer.groups.back();
+        size_t index = group.vertexLength;
 
         // Encode angle of glyph
         uint8_t glyphAngle = std::round((symbol.glyphAngle / (M_PI * 2)) * 256);
@@ -474,15 +473,15 @@ void SymbolLayout::addSymbols(Buffer &buffer, const SymbolQuads &symbols, float
                             minZoom, maxZoom, placementZoom, glyphAngle);
 
         // add the two triangles, referencing the four coordinates we just inserted.
-        buffer.triangles.emplace_back(static_cast<uint16_t>(triangleIndex + 0),
-                                      static_cast<uint16_t>(triangleIndex + 1),
-                                      static_cast<uint16_t>(triangleIndex + 2));
-        buffer.triangles.emplace_back(static_cast<uint16_t>(triangleIndex + 1),
-                                      static_cast<uint16_t>(triangleIndex + 2),
-                                      static_cast<uint16_t>(triangleIndex + 3));
-
-        triangleGroup.vertex_length += glyph_vertex_length;
-        triangleGroup.elements_length += 2;
+        buffer.triangles.emplace_back(static_cast<uint16_t>(index + 0),
+                                      static_cast<uint16_t>(index + 1),
+                                      static_cast<uint16_t>(index + 2));
+        buffer.triangles.emplace_back(static_cast<uint16_t>(index + 1),
+                                      static_cast<uint16_t>(index + 2),
+                                      static_cast<uint16_t>(index + 3));
+
+        group.vertexLength += glyph_vertex_length;
+        group.indexLength += 2;
     }
 }
 
@@ -518,7 +517,7 @@ void SymbolLayout::addToDebugBuffers(CollisionTile& collisionTile, SymbolBucket&
                 auto& collisionBox = bucket.collisionBox;
                 if (collisionBox.groups.empty()) {
                     // Move to a new group because the old one can't hold the geometry.
-                    collisionBox.groups.emplace_back(std::make_unique<SymbolBucket::CollisionBoxElementGroup>());
+                    collisionBox.groups.emplace_back();
                 }
 
                 collisionBox.vertices.emplace_back(anchor.x, anchor.y, tl.x, tl.y, maxZoom, placementZoom);
@@ -530,8 +529,8 @@ void SymbolLayout::addToDebugBuffers(CollisionTile& collisionTile, SymbolBucket&
                 collisionBox.vertices.emplace_back(anchor.x, anchor.y, bl.x, bl.y, maxZoom, placementZoom);
                 collisionBox.vertices.emplace_back(anchor.x, anchor.y, tl.x, tl.y, maxZoom, placementZoom);
 
-                auto &group= *collisionBox.groups.back();
-                group.vertex_length += 8;
+                auto& group= collisionBox.groups.back();
+                group.vertexLength += 8;
             }
         }
     }

src/mbgl/layout/symbol_layout.hpp

@@ -70,7 +70,7 @@ class SymbolLayout {
     void addToDebugBuffers(CollisionTile&, SymbolBucket&);
 
     // Adds placed items to the buffer.
-    template <typename Buffer, typename GroupType>
+    template <typename Buffer>
     void addSymbols(Buffer&, const SymbolQuads&, float scale,
                     const bool keepUpright, const style::SymbolPlacementType, const float placementAngle);
 

src/mbgl/renderer/circle_bucket.cpp

@@ -31,7 +31,7 @@ void CircleBucket::render(Painter& painter,
 }
 
 bool CircleBucket::hasData() const {
-    return !triangleGroups.empty();
+    return !groups.empty();
 }
 
 bool CircleBucket::needsClipping() const {
@@ -64,13 +64,13 @@ void CircleBucket::addGeometry(const GeometryCollection& geometryCollection) {
             vertices.emplace_back(x, y, 1, 1); // 3
             vertices.emplace_back(x, y, -1, 1); // 4
 
-            if (!triangleGroups.size() || (triangleGroups.back()->vertex_length + 4 > 65535)) {
+            if (!groups.size() || groups.back().vertexLength + 4 > 65535) {
                 // Move to a new group because the old one can't hold the geometry.
-                triangleGroups.emplace_back(std::make_unique<TriangleGroup>());
+                groups.emplace_back();
             }
 
-            TriangleGroup& group = *triangleGroups.back();
-            uint16_t index = group.vertex_length;
+            auto& group = groups.back();
+            uint16_t index = group.vertexLength;
 
             // 1, 2, 3
             // 1, 4, 3
@@ -81,27 +81,25 @@ void CircleBucket::addGeometry(const GeometryCollection& geometryCollection) {
                                    static_cast<uint16_t>(index + 3),
                                    static_cast<uint16_t>(index + 2));
 
-            group.vertex_length += 4;
-            group.elements_length += 2;
+            group.vertexLength += 4;
+            group.indexLength += 2;
         }
     }
 }
 
-void CircleBucket::drawCircles(CircleShader& shader, gl::Context& context) {
+void CircleBucket::drawCircles(CircleShader& shader, gl::Context& context, PaintMode paintMode) {
     GLbyte* vertexIndex = BUFFER_OFFSET(0);
     GLbyte* elementsIndex = BUFFER_OFFSET(0);
 
-    for (auto& group : triangleGroups) {
-        assert(group);
+    for (auto& group : groups) {
+        if (!group.indexLength) continue;
 
-        if (!group->elements_length) continue;
+        group.getVAO(shader, paintMode).bind(shader, *vertexBuffer, *indexBuffer, vertexIndex, context);
 
-        group->array[0].bind(shader, *vertexBuffer, *indexBuffer, vertexIndex, context);
+        MBGL_CHECK_ERROR(glDrawElements(GL_TRIANGLES, static_cast<GLsizei>(group.indexLength * 3), GL_UNSIGNED_SHORT, elementsIndex));
 
-        MBGL_CHECK_ERROR(glDrawElements(GL_TRIANGLES, group->elements_length * 3, GL_UNSIGNED_SHORT, elementsIndex));
-
-        vertexIndex += group->vertex_length * vertexBuffer->vertexSize;
-        elementsIndex += group->elements_length * indexBuffer->primitiveSize;
+        vertexIndex += group.vertexLength * vertexBuffer->vertexSize;
+        elementsIndex += group.indexLength * indexBuffer->primitiveSize;
     }
 }
 

src/mbgl/renderer/circle_bucket.hpp

@@ -1,11 +1,11 @@
 #pragma once
 
 #include <mbgl/renderer/bucket.hpp>
+#include <mbgl/renderer/element_group.hpp>
 #include <mbgl/map/mode.hpp>
 #include <mbgl/tile/geometry_tile_data.hpp>
 #include <mbgl/gl/vertex_buffer.hpp>
 #include <mbgl/gl/index_buffer.hpp>
-#include <mbgl/gl/element_group.hpp>
 #include <mbgl/shader/circle_vertex.hpp>
 
 namespace mbgl {
@@ -24,14 +24,13 @@ class CircleBucket : public Bucket {
     bool needsClipping() const override;
     void addGeometry(const GeometryCollection&);
 
-    void drawCircles(CircleShader&, gl::Context&);
+    void drawCircles(CircleShader&, gl::Context&, PaintMode);
 
 private:
     std::vector<CircleVertex> vertices;
     std::vector<gl::Triangle> triangles;
 
-    using TriangleGroup = gl::ElementGroup<3>;
-    std::vector<std::unique_ptr<TriangleGroup>> triangleGroups;
+    std::vector<ElementGroup<CircleShader>> groups;
 
     optional<gl::VertexBuffer<CircleVertex>> vertexBuffer;
     optional<gl::IndexBuffer<gl::Triangle>> indexBuffer;

src/mbgl/renderer/element_group.hpp

@@ -0,0 +1,28 @@
+#pragma once
+
+#include <mbgl/gl/vao.hpp>
+#include <mbgl/renderer/render_pass.hpp>
+
+namespace mbgl {
+
+template <class... Shaders>
+struct ElementGroup {
+    template <class Shader>
+    struct VAOs {
+        gl::VertexArrayObject normalVAO;
+        gl::VertexArrayObject overdrawVAO;
+    };
+
+    std::tuple<VAOs<Shaders>...> vaos;
+
+    template <class Shader>
+    gl::VertexArrayObject& getVAO(const Shader&, PaintMode paintMode) {
+        auto& vao = std::get<VAOs<Shader>>(vaos);
+        return paintMode == PaintMode::Overdraw ? vao.overdrawVAO : vao.normalVAO;
+    }
+
+    std::size_t vertexLength = 0;
+    std::size_t indexLength = 0;
+};
+
+} // namespace mbgl