[core] Rework attribute binding (again)

These changes are necessary for programs whose set of active attributes is not fixed at compile time by a template parameter pack, but rather varies based on the generated shader text at runtime. In such cases, the attribute location of a given named attribute may vary between instances of the same Program.

Previously, attribute bindings were implicitly associated with a location based on template parameter order, and -1 was used to indicate an inactive attribute. This left us unable to disable the appropriate attribute when it went from active to inactive. Now, the state tracker for bindings explicitly associates locations and state, and an empty optional is used to indicate an inactive attribute.

In addition, a gl::VertexArray class is now exposed, allowing more flexibility in the relationship between Programs, Segments, and attribute bindings. In this commit, that relationship does not change, but the subsequent commit adjusts it to match gl-js, reduce rebinds, and work around buggy VAO implementations.

VertexArray uses a pimpl idiom in order to support implementations that lack the VAO extension. In that case, all VertexArrays share global binding state, reflecting the platform reality in the absence of VAOs, while still providing a uniform API.

6 files changed, 109 insertions, 36 deletions

diff --git a/src/mbgl/programs/binary_program.cpp b/src/mbgl/programs/binary_program.cpp
index 3b37cfa442..1cad0a2693 100644
--- a/src/mbgl/programs/binary_program.cpp
+++ b/src/mbgl/programs/binary_program.cpp
@@ -96,7 +96,7 @@ std::string BinaryProgram::serialize() const {
     return data;
 }
 
-gl::AttributeLocation BinaryProgram::attributeLocation(const std::string& name) const {
+optional<gl::AttributeLocation> BinaryProgram::attributeLocation(const std::string& name) const {
     for (const auto& pair : attributes) {
         if (pair.first == name) {
             return pair.second;
@@ -111,7 +111,7 @@ gl::UniformLocation BinaryProgram::uniformLocation(const std::string& name) cons
             return pair.second;
         }
     }
-    return {};
+    return -1;
 }
 
 } // namespace mbgl
diff --git a/src/mbgl/programs/binary_program.hpp b/src/mbgl/programs/binary_program.hpp
index 8ff3863dc1..2806f4fb3e 100644
--- a/src/mbgl/programs/binary_program.hpp
+++ b/src/mbgl/programs/binary_program.hpp
@@ -1,6 +1,7 @@
 #pragma once
 
 #include <mbgl/gl/types.hpp>
+#include <mbgl/util/optional.hpp>
 
 #include <string>
 #include <vector>
@@ -29,7 +30,8 @@ public:
     const std::string& identifier() const {
         return binaryIdentifier;
     }
-    gl::AttributeLocation attributeLocation(const std::string& name) const;
+
+    optional<gl::AttributeLocation> attributeLocation(const std::string& name) const;
     gl::UniformLocation uniformLocation(const std::string& name) const;
 
 private:
diff --git a/src/mbgl/programs/program.hpp b/src/mbgl/programs/program.hpp
index 65f17e5bdc..3ff6e59c6b 100644
--- a/src/mbgl/programs/program.hpp
+++ b/src/mbgl/programs/program.hpp
@@ -2,6 +2,7 @@
 
 #include <mbgl/gl/program.hpp>
 #include <mbgl/gl/features.hpp>
+#include <mbgl/programs/segment.hpp>
 #include <mbgl/programs/binary_program.hpp>
 #include <mbgl/programs/attributes.hpp>
 #include <mbgl/programs/program_parameters.hpp>
@@ -51,26 +52,37 @@ public:
               gl::DepthMode depthMode,
               gl::StencilMode stencilMode,
               gl::ColorMode colorMode,
-              UniformValues&& uniformValues,
+              const UniformValues& uniformValues,
               const gl::VertexBuffer<LayoutVertex>& layoutVertexBuffer,
               const gl::IndexBuffer<DrawMode>& indexBuffer,
-              const gl::SegmentVector<Attributes>& segments,
+              const SegmentVector<Attributes>& segments,
               const PaintPropertyBinders& paintPropertyBinders,
               const typename PaintProperties::Evaluated& currentProperties,
               float currentZoom) {
-        program.draw(
-            context,
-            std::move(drawMode),
-            std::move(depthMode),
-            std::move(stencilMode),
-            std::move(colorMode),
-            uniformValues
-                .concat(paintPropertyBinders.uniformValues(currentZoom, currentProperties)),
-            LayoutAttributes::bindings(layoutVertexBuffer)
-                .concat(paintPropertyBinders.attributeBindings(currentProperties)),
-            indexBuffer,
-            segments
-        );
+        typename AllUniforms::Values allUniformValues = uniformValues
+            .concat(paintPropertyBinders.uniformValues(currentZoom, currentProperties));
+
+        typename Attributes::Bindings allAttributeBindings = LayoutAttributes::bindings(layoutVertexBuffer)
+            .concat(paintPropertyBinders.attributeBindings(currentProperties));
+
+        for (auto& segment : segments) {
+            if (!segment.vertexArray) {
+                segment.vertexArray = context.createVertexArray();
+            }
+
+            program.draw(
+                context,
+                std::move(drawMode),
+                std::move(depthMode),
+                std::move(stencilMode),
+                std::move(colorMode),
+                allUniformValues,
+                *segment.vertexArray,
+                Attributes::offsetBindings(allAttributeBindings, segment.vertexOffset),
+                indexBuffer,
+                segment.indexOffset,
+                segment.indexLength);
+        }
     }
 };
 
diff --git a/src/mbgl/programs/segment.cpp b/src/mbgl/programs/segment.cpp
new file mode 100644
index 0000000000..bb09843e21
--- /dev/null
+++ b/src/mbgl/programs/segment.cpp
@@ -0,0 +1,7 @@
+#include <mbgl/programs/segment.hpp>
+
+namespace mbgl {
+namespace gl {
+
+} // namespace gl
+} // namespace mbgl
diff --git a/src/mbgl/programs/segment.hpp b/src/mbgl/programs/segment.hpp
new file mode 100644
index 0000000000..d8cc9679d7
--- /dev/null
+++ b/src/mbgl/programs/segment.hpp
@@ -0,0 +1,40 @@
+#pragma once
+
+#include <mbgl/gl/context.hpp>
+#include <mbgl/gl/vertex_array.hpp>
+#include <mbgl/util/optional.hpp>
+
+#include <cstddef>
+#include <vector>
+#include <map>
+
+namespace mbgl {
+
+template <class Attributes>
+class Segment {
+public:
+    Segment(std::size_t vertexOffset_,
+            std::size_t indexOffset_,
+            std::size_t vertexLength_ = 0,
+            std::size_t indexLength_ = 0)
+        : vertexOffset(vertexOffset_),
+          indexOffset(indexOffset_),
+          vertexLength(vertexLength_),
+          indexLength(indexLength_) {}
+
+    const std::size_t vertexOffset;
+    const std::size_t indexOffset;
+
+    std::size_t vertexLength;
+    std::size_t indexLength;
+
+    mutable optional<gl::VertexArray> vertexArray;
+};
+
+template <class Attributes>
+class SegmentVector : public std::vector<Segment<Attributes>> {
+public:
+    SegmentVector() = default;
+};
+
+} // namespace mbgl
diff --git a/src/mbgl/programs/symbol_program.hpp b/src/mbgl/programs/symbol_program.hpp
index 47d92f912c..2fddebde09 100644
--- a/src/mbgl/programs/symbol_program.hpp
+++ b/src/mbgl/programs/symbol_program.hpp
@@ -7,6 +7,7 @@
 
 #include <mbgl/programs/attributes.hpp>
 #include <mbgl/programs/uniforms.hpp>
+#include <mbgl/programs/segment.hpp>
 #include <mbgl/shaders/symbol_icon.hpp>
 #include <mbgl/shaders/symbol_sdf.hpp>
 #include <mbgl/util/geometry.hpp>
@@ -156,7 +157,7 @@ public:
     }
     
     SymbolSizeAttributes::Bindings attributeBindings() const override {
-        return SymbolSizeAttributes::Bindings { gl::DisabledAttribute() };
+        return SymbolSizeAttributes::Bindings { {} };
     }
 
     void upload(gl::Context&) override {}
@@ -352,29 +353,40 @@ public:
               gl::DepthMode depthMode,
               gl::StencilMode stencilMode,
               gl::ColorMode colorMode,
-              UniformValues&& uniformValues,
+              const UniformValues& uniformValues,
               const gl::VertexBuffer<LayoutVertex>& layoutVertexBuffer,
               const SymbolSizeBinder& symbolSizeBinder,
               const gl::IndexBuffer<DrawMode>& indexBuffer,
-              const gl::SegmentVector<Attributes>& segments,
+              const SegmentVector<Attributes>& segments,
               const PaintPropertyBinders& paintPropertyBinders,
               const typename PaintProperties::Evaluated& currentProperties,
               float currentZoom) {
-        program.draw(
-            context,
-            std::move(drawMode),
-            std::move(depthMode),
-            std::move(stencilMode),
-            std::move(colorMode),
-            uniformValues
-                .concat(symbolSizeBinder.uniformValues(currentZoom))
-                .concat(paintPropertyBinders.uniformValues(currentZoom, currentProperties)),
-            LayoutAttributes::bindings(layoutVertexBuffer)
-                .concat(symbolSizeBinder.attributeBindings())
-                .concat(paintPropertyBinders.attributeBindings(currentProperties)),
-            indexBuffer,
-            segments
-        );
+        typename AllUniforms::Values allUniformValues = uniformValues
+            .concat(symbolSizeBinder.uniformValues(currentZoom))
+            .concat(paintPropertyBinders.uniformValues(currentZoom, currentProperties));
+
+        typename Attributes::Bindings allAttributeBindings = LayoutAttributes::bindings(layoutVertexBuffer)
+            .concat(symbolSizeBinder.attributeBindings())
+            .concat(paintPropertyBinders.attributeBindings(currentProperties));
+
+        for (auto& segment : segments) {
+            if (!segment.vertexArray) {
+                segment.vertexArray = context.createVertexArray();
+            }
+
+            program.draw(
+                context,
+                std::move(drawMode),
+                std::move(depthMode),
+                std::move(stencilMode),
+                std::move(colorMode),
+                allUniformValues,
+                *segment.vertexArray,
+                Attributes::offsetBindings(allAttributeBindings, segment.vertexOffset),
+                indexBuffer,
+                segment.indexOffset,
+                segment.indexLength);
+        }
     }
 };