C++ bindings for OpenGL #6468

jfirebaugh · 2016-09-27T00:43:33Z

This PR introduces mid-level bindings for OpenGL, specifically a procedural API that wraps a single primitive draw operation, with all state affecting the result provided via method parameters rather than the OpenGL default of implicit state and bindings. The primary design reference for this API was glium, with secondary inspiration from regl.

This parameters for a draw operation are the following:

Program: A compiled vertex and fragment shader.
Parameters: General OpenGL state such as depth test, stencil test, and blend mode. The variety of OpenGL methods for configuring the depth test, stencil test, and blend mode have been rationalized with an algebraic data type for each of Depth, Stencil, and Blend.
AttributeSources: A collection of sources of vertex attribute data. A source can be a vertex buffer or a slice of one (glVertexAttribPointer), or a constant scalar or vector value (glVertexAttrib*). (It remains to be seen if we use the latter for DDS or go the route of GL JS and generate shader source code that uses attributes or uniforms as appropriate.) A buffer or buffer slice comes with a specification of its layout.
Uniforms: A collection of uniform locations and values. Values are typed, and valid types include scalars and vectors as well as texture objects. For the latter, the implementation handles activating an unused texture unit.
Primitives: The specification of what geometry to draw. This concept unifies unindexed (glDrawArrays) and indexed (glDrawElements) drawing. Input can be array buffer or element buffer objects or slices thereof. Buffer types are templated on their geometry, e.g. ElementBuffer<Triangles, uint16_t>.

Type safety is generally provided at the level of individual parameters, and notably not at the level of matching types between shader source and uniform or attribute types, or between attribute types and C++ object layout. Some additional safety can be obtained by writing higher-level bindings atop this API, e.g. along the lines of the API for uniforms proposed by @kkaefer here.

Likewise, we can layer a declarative, object-based API more like regl atop this one, as suggested in this gl-js issue. Doing so may simplify the communication pathway between workers and the renderer, and provide a clearer path to use VAOs transparently than with a purely procedural approach.

TODOs

The remaining items are numerous; I'll edit to flesh out this section.

Probably the first item is to land the unification of gl::ObjectStore and gl::Config (Merge gl::ObjectStore and gl::Config #6470). The prepare and draw functions I've sketched here should be methods on this object, and refer to its internal state to avoid no-op OpenGL operations as gl::Config currently does.

cc @kkaefer @lucaswoj

mention-bot · 2016-09-27T00:43:35Z

@jfirebaugh, thanks for your PR! By analyzing this pull request, we identified @ansis to be potential reviewers.

jfirebaugh · 2016-09-27T00:48:57Z

include/mbgl/gl/gl_values.hpp

@@ -76,7 +76,7 @@ struct DepthMask {
    }
 };

-struct ColorMask {
+struct ColorMaskValue {


Renames here to avoid conflicts with new type names. I envision that all the structs in gl_values.hpp will eventually go away.

The goal of these values, and their surrounding Value objects is to prevent duplicate state from being set. It seems like this is no longer the case with this code? Will it come back as work progresses?

Yep, I was just waiting for #6470 to land.

kkaefer · 2016-09-27T09:12:33Z

include/mbgl/gl/gl_values.hpp

@@ -76,7 +76,7 @@ struct DepthMask {
    }
 };

-struct ColorMask {
+struct ColorMaskValue {


The goal of these values, and their surrounding Value objects is to prevent duplicate state from being set. It seems like this is no longer the case with this code? Will it come back as work progresses?

kkaefer · 2016-09-27T09:13:21Z

src/mbgl/renderer/painter_clipping.cpp

+                 gl::Uniforms {
+                     gl::UniformMatrix<4> { program.u_matrix, matrix }
+                 },
+                 tileStencilBuffer);


How does this draw call know that it should draw triangles?

I was thinking that vertex buffer types would be tagged with a primitive type, but in some cases we want to reuse vertexes in multiple primitive types. So I think this API will probably need to be revised to accept vertex sources and index source in separate parameters, with a marker value for the latter to indicate unindexed drawing (e.g. Unindexed<Triangles>()). This is what the glium API does.

Yeah, in some cases we're using the same buffer for different draw operations.

kkaefer · 2016-09-27T09:15:41Z

src/mbgl/renderer/painter_clipping.cpp

+                         gl::Stencil::Replace
+                     },
+                     gl::Blend::disabled(),
+                     gl::ColorMask::disabled()


Do all of these have to be specified? Can we use some defaults here?

Yes, we could allow defaults. However, given that part of the motivation for moving to this API is to eliminate errors caused by forgetting to specify the desired state, I suggest that we start by making all parameters required. We can define additional helpers in the vein of gl::*::disabled() to package up common values. For example I anticipate we'll want gl::Blend::alphaTransparency() and functions for the most common depth and stencil settings.

kkaefer · 2016-09-27T09:15:44Z

src/mbgl/renderer/painter_clipping.cpp

+                 gl::AttributeSources {},
+                 gl::Uniforms {
+                     gl::UniformMatrix<4> { program.u_matrix, matrix }
+                 },


Is there a way to ensure that all uniforms in a program can be set? In the past, we've seen issues with not all uniforms for a draw call being set so we didn't have predictable results.

Yes, I think we can accomplish this with a higher-level API that layers on top of this one.

kkaefer · 2016-09-27T09:18:57Z

src/mbgl/gl/attributes.hpp

+template <> struct AttributeType<int32_t>  : std::integral_constant<GLenum, GL_INT> {};
+template <> struct AttributeType<uint32_t> : std::integral_constant<GLenum, GL_UNSIGNED_INT> {};
+template <> struct AttributeType<float>    : std::integral_constant<GLenum, GL_FLOAT> {};
+template <> struct AttributeType<double>   : std::integral_constant<GLenum, GL_DOUBLE> {};


At some point, we should hide the GL_*, and gl* symbols so users of this library don't need to include gl.h.

kkaefer · 2016-09-27T09:20:33Z

src/mbgl/gl/attributes.hpp

+
+using AttributeSources = std::vector<AttributeSource>;
+
+namespace detail {


Ideally, we'd move all of the detail code that calls actual GL functions to an implementation file

I think this is doable. It means that all polymorphic input types have to be wrapped in a variant at some point -- no full monomorphization via template methods. I think that's fine, it was already mostly that way.

Or did you just mean move them to an _impl.hpp file? I assumed you meant move them to a .cpp file.

Yeah, .cpp file.

kkaefer · 2016-09-27T09:22:44Z

src/mbgl/gl/parameters.hpp

+    bool r;
+    bool g;
+    bool b;
+    bool a;


For types like this, we should either default them, or write explicit initializers that require all parameters to prevent accidental misuse, which results in uninitialized values. Valgrind may catch them, but it's always better to have a compiler error slapped in the face.

clang gives a warning if you omit parameters in brace initialization:

warning: missing field 'b' initializer [-Wmissing-field-initializers] return ColorMask { false, false }; ^

This will be an error once we fix #6487.

For consecutive bool values we could save some memory space by using bit field e.g.:

bool r : 1; bool g : 1; bool b : 1; bool a : 1;

lucaswoj · 2016-09-27T18:22:18Z

This is excellent work @jfirebaugh! I am excited to iterate on GL Native and GL JS rendering architecture together as we work on mapbox/mapbox-gl-js#145.

jfirebaugh · 2016-10-25T22:16:54Z

@kkaefer Rebased on top of #6596 and ready for re-review.

jfirebaugh · 2016-10-25T22:24:46Z

src/mbgl/renderer/painter_debug.cpp

-    MBGL_CHECK_ERROR(glDrawArrays(GL_LINE_STRIP, 0, static_cast<GLsizei>(tileLineStripVertexBuffer.vertexCount)));
-}
-
-#ifndef NDEBUG


I removed Painter::renderClipMasks and Painter::renderDepthBuffer in an early step to avoid dealing with them at the time, and need to go back and convert them now.

kkaefer · 2016-10-26T00:32:01Z

src/mbgl/gl/attribute.hpp

@@ -27,10 +25,8 @@ class AttributeBinding {
          type(DataTypeOf<T>::value),
          count(N),
          offset(O) {
-        static_assert(std::is_standard_layout<Vertex>::value, "vertex type must use standard layout");


Why are these removed?

This one moved to the Drawable constructor. I just dropped "vertex type is too big", because it was kind of silly.

kkaefer · 2016-10-26T00:36:33Z

src/mbgl/gl/context.cpp

+        blend = true;
+        blendColor = color.blendColor;
+        apply_visitor([&] (const auto& blendFunction) {
+            // TODO: blendEquation = blendFunction.equation;


Previous code didn't call glBlendEquation anywhere and we always want the default of GL_FUNC_ADD. But we should set it explicitly right? In case a custom layer changes it.

kkaefer · 2016-10-26T00:38:18Z

src/mbgl/gl/context.cpp

+
+    drawable.bindUniforms();
+
+    for (const auto& segment : drawable.segments) {


Is it possible to have zero drawable segments? In that case, we shouldn't set any of the state prior to this whatsoever.

I think it'll be short circuited prior to getting here, but I'll add an internal guard as well.

kkaefer · 2016-10-26T17:06:16Z

src/mbgl/gl/context.cpp

+        };
+
+        if (needAttributeBindings()) {
+            vertexBuffer.setDirty();


why setDirty here?

It's for the case where a VAO is being initialized. VAOs don't inherit the existing buffer bindings; we need to ensure that glBindBuffer actually gets called. But it can be moved up into the needAttributeBindings lambda and commented. I'll do so.

kkaefer · 2016-10-26T17:15:36Z

src/mbgl/gl/mode.hpp

+    TriangleStrip>;
+
+} // namespace gl
+} // namespace mbgl


Can we use a different name? Mode feels way too generic for this. I know that the parameter is called mode in OpenGL as well, but maybe we can use a name like PrimitiveType instead?

PrimitiveMode?

Or:

Mode ⇢ DrawMode

Depth ⇢ DepthMode

Stencil ⇢ StencilMode

Color ⇢ ColorMode

Either work for me. Maybe adding the Mode to all avoids ambiguity?

kkaefer · 2016-10-26T17:21:10Z

src/mbgl/gl/drawable.hpp

+    Mode mode;
+    Depth depth;
+    Stencil stencil;
+    Color color;


Rename to blendColor?

I chose Color for parallelism with Depth and Stencil -- each holds the configuration that affects the respective GL buffer attachment of that type. For Color, that's both blend settings and the color mask.

kkaefer · 2016-10-26T17:38:52Z

src/mbgl/shader/circle_uniforms.hpp

+    u_extrude_scale,
+    u_devicepixelratio> {};
+
+} // namespace mbgl


Can we merge all of the _uniforms.hpp files into the shader headers? It seems like they are very short, and are always used together, so having them in the same file makes the code more readable. Maybe even make them a child class of the shader class?

There's not a one-to-one relationship -- e.g. FillShader, FillPatternShader, FillOutlineShader, FillOutlinePatternShader. I think there's some consolidation to be done -- perhaps replacing explicit shader subclasses with a single Shader template -- but I'd like to defer it to a future PR.

kkaefer · 2016-10-26T17:50:33Z

src/mbgl/shader/circle_uniforms.hpp

+
+namespace mbgl {
+
+MBGL_DEFINE_UNIFORM_SCALAR(float, u_radius);


These are going to define global symbols, and I assume it's just a coincidence that we don't have any name clashes for different symbols. Instead we should define them inside the Shader class.

Where multiple shaders use the same uniform name, I've hoisted the definition to a shared file.

Yeah, I've seen that; I'm referring to the situation when two shaders use the same name for different uniform types.

Ok -- there aren't any cases of that currently, and I think we should avoid introducing any. Besides link errors, it would just be confusing to use the same uniform name for different types.

Okay; but it still feels weird to have them in the main mbgl namespace so we have symbols like mbgl::u_radius. Can we at least move them to another namespace?

kkaefer · 2016-10-26T17:58:20Z

src/mbgl/gl/context.cpp

+            VertexArrayID id = 0;
+            MBGL_CHECK_ERROR(gl::GenVertexArrays(1, &id));
+            vertexArrayObject = id;
+            vaos.emplace(vaoKey, UniqueVertexArray(std::move(id), { this }));


I'm not sure I like this design. Can we come up with a version that does the VAO management statically instead of during runtime?

Well, the static design is the previously existing one, where each buffer Segment holds a corresponding VAO, and binding is done explicitly. That approach seems to be error-prone.

kkaefer · 2016-10-29T00:03:24Z

Can you add the [core] prefix to the two last commits?

jfirebaugh · 2016-10-29T00:17:27Z

Squashed one, prefixed the other.

I think it's prudent for this not to go into iOS v3.4.0 or Android v4.2.0, as it's an invasive change. iOS is on a release branch already and not planning any more merges from master. @mapbox/android, do you have plans to create a release branch soon, so that this PR can be merged to master?

tobrun · 2016-10-29T06:08:06Z

@jfirebaugh feel free to merge to master, we have a release branch called release-android-v4.2.0.

kkaefer · 2016-10-31T10:42:10Z

🎉

jfirebaugh commented Sep 27, 2016

View reviewed changes

kkaefer reviewed Sep 27, 2016

View reviewed changes

jfirebaugh force-pushed the gl-bindings branch 2 times, most recently from ecc3ae0 to 3bf08b4 Compare September 28, 2016 14:43

jfirebaugh mentioned this pull request Sep 28, 2016

Buffer refactor #6504

Merged

jfirebaugh force-pushed the gl-bindings branch from 3bf08b4 to 12112c6 Compare September 29, 2016 04:27

kkaefer added refactor Core The cross-platform C++ core, aka mbgl labels Sep 29, 2016

kkaefer mentioned this pull request Sep 29, 2016

Remove access to gl.h for most code #6511

Merged

jfirebaugh force-pushed the gl-bindings branch 7 times, most recently from 2bc6035 to c0b3c9f Compare October 5, 2016 19:38

jfirebaugh force-pushed the gl-bindings branch 12 times, most recently from e2bd2e6 to 9d229b7 Compare October 10, 2016 20:51

jfirebaugh mentioned this pull request Oct 11, 2016

collision debug doesn't work reliably #5055

Closed

friedbunny mentioned this pull request Oct 11, 2016

iOS build times out #6653

Closed

jfirebaugh force-pushed the gl-bindings branch from 3ef3084 to 49a01e8 Compare October 12, 2016 17:52

jfirebaugh mentioned this pull request Oct 12, 2016

Crash rendering the map — mbgl::FillBucket::drawElements, glrAGXRenderVertexArray #5731

Closed

jfirebaugh force-pushed the gl-bindings branch 2 times, most recently from 7dba523 to ce34d4e Compare October 20, 2016 23:39

jfirebaugh force-pushed the gl-bindings branch from ce34d4e to 8ac0eec Compare October 25, 2016 22:16

jfirebaugh commented Oct 25, 2016

View reviewed changes

kkaefer reviewed Oct 26, 2016

View reviewed changes

kkaefer approved these changes Oct 27, 2016

View reviewed changes

kkaefer mentioned this pull request Oct 28, 2016

Move OpenGL code away from Buckets #5257

Closed

jfirebaugh mentioned this pull request Oct 28, 2016

OpenGL state management system mapbox/mapbox-gl-js#145

Closed

jfirebaugh force-pushed the gl-bindings branch 2 times, most recently from cd43b2c to 713e8b7 Compare October 28, 2016 22:43

kkaefer approved these changes Oct 29, 2016

View reviewed changes

jfirebaugh added 2 commits October 28, 2016 17:14

[core] Modern C++ bindings for OpenGL

0e57d10

[core] Use VAOs internally

39d5cb3

jfirebaugh force-pushed the gl-bindings branch from c1dd5ad to 39d5cb3 Compare October 29, 2016 00:14

jfirebaugh merged commit 39d5cb3 into master Oct 30, 2016

jfirebaugh deleted the gl-bindings branch October 30, 2016 02:25

jfirebaugh mentioned this pull request Oct 30, 2016

Shaders refactor #6856

Merged

1 task

brunoabinader mentioned this pull request Oct 30, 2016

[core] Transform{,State} cleanups #6790

Merged

jfirebaugh mentioned this pull request Oct 30, 2016

[linux] Overdraw debug is broken #6799

Closed

jfirebaugh mentioned this pull request Nov 2, 2016

Merge branch release-ios-v3.3.5 into master #6884

Merged

jfirebaugh mentioned this pull request Nov 14, 2016

Return to static, per-segment approach to VAOs #7056

Merged


		using AttributeSources = std::vector<AttributeSource>;

		namespace detail {


		drawable.bindUniforms();

		for (const auto& segment : drawable.segments) {


		namespace mbgl {

		MBGL_DEFINE_UNIFORM_SCALAR(float, u_radius);

C++ bindings for OpenGL #6468

C++ bindings for OpenGL #6468

Conversation

jfirebaugh commented Sep 27, 2016 • edited Loading

TODOs

mention-bot commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkaefer Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucaswoj commented Sep 27, 2016

jfirebaugh commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkaefer commented Oct 29, 2016

jfirebaugh commented Oct 29, 2016

tobrun commented Oct 29, 2016

kkaefer commented Oct 31, 2016

jfirebaugh commented Sep 27, 2016 •

edited

Loading

kkaefer Sep 27, 2016 •

edited

Loading