Shaders refactor

[jfirebaugh]

Followup to #6468, specifically #6468 (comment).

• Fix overdraw debug mode

[mention-bot]

@jfirebaugh, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kkaefer and @brunoabinader to be potential reviewers.

[brunoabinader]

Potentially fixes the issues mentioned in #6799 (comment) (needs verification).

[jfirebaugh]

Ready for review!

[jfirebaugh]

@kkaefer The test sources now need the path for generated shader .hpp's in their include paths. Where in the cmake files do I change that?

[kkaefer]

You can add ${MBGL_GENERATED}/include to the include dir list. The actual code that generates the shaders is in cmake/shaders.cmake.

[kkaefer]

I'd propose adding this to enforce initialization of the primitive types with a value:

diff --git a/src/mbgl/gl/draw_mode.hpp b/src/mbgl/gl/draw_mode.hpp
index 8f7db0a..b099a5f 100644
--- a/src/mbgl/gl/draw_mode.hpp
+++ b/src/mbgl/gl/draw_mode.hpp
@@ -11,6 +11,7 @@ public:
     using Primitive = Point;
     static constexpr std::size_t bufferGroupSize = 1;

+    Points(float pointSize_) : pointSize(pointSize_) {}
     float pointSize;
 };

@@ -19,6 +20,9 @@ public:
     using Primitive = Line;
     static constexpr std::size_t bufferGroupSize = 2;

+    Lines(float lineWidth_) : lineWidth(lineWidth_) {
+        assert(lineWidth > 0);
+    }
     float lineWidth;
 };

@@ -29,6 +33,9 @@ public:
     using Primitive = Line;
     static constexpr std::size_t bufferGroupSize = 1;

+    LineStrip(float lineWidth_) : lineWidth(lineWidth_) {
+        assert(lineWidth > 0);
+    }
     float lineWidth;
 };

[kkaefer]

What problem does the indexed access solve here? It seems like the code worked before without this as well?

[jfirebaugh]

• Less verbose -- std::get<typename As::SomeInnerType>(tuple) becomes tuple.template get<As>().
• Eliminates the need to use a separate helper and std::index_sequence in Attributes::binder.
• Will be consistent with use in [core] Properties refactor #6868.

[kkaefer]

Why switch to indexLength? You're saying "Segment` now tracks offset and length of indices, rather than primitives. This is more natural.", but I'm unsure what "natural" is supposed to mean.

[jfirebaugh]

Fundamentally, a vertex/index buffer is a buffer of vertices/indexes, not a buffer of triangles, or lines, or whatever. Conceptually, that's how OpenGL works, and I found it easier to reason about things when I stopped trying to think in terms of buffers of primitives.

Implementation wise, it simplifies calculations like this.

It matches the internal implementation of the new {Vertex,Index}Vector -- I think if we were to have std::vector<Triangle<Vertex>>, the packing might come out wrong. You don't want to get in a situation where there might be extra padding every third vertex. {Vertex,Index}Vector still enforces filling the vector with the right sized groups though.

[kkaefer]

Can we move ignore to the mbgl::util namespace to avoid confusion with local variables or mistaking it for a language keyword?

[kkaefer]

One more thing that I'd like to see (but that is out of scope for this PR) is to somehow move shaders to the context. Background is that for Node.js, we'd like to use a shared context, and ideally we can share the shaders as well to avoid having all of these Programs that are just duplicates.

[kkaefer]

Can we factor out the attributes stuff into its own objects to be in line with the others?

[jfirebaugh]

Rebasing this is getting to be a real drag. Can I get this approved and we can follow up on nits in other PRs?

[brunoabinader]

Thank you @jfirebaugh - confirming that overdraw debug mode is operational again on Linux: