Buffer refactor #6504

jfirebaugh · 2016-09-30T00:31:31Z

As part of the general OpenGL code refactor effort, we should redesign how we handle buffers. The main design goal is to split the current responsibilities of the current Buffer class template.

Building/filling an array of data (in the worker) should be done with plain std::vector<Vertex> (or std::array<Vertex, N> for fixed-sized arrays). Vertex is a stand in for any user-defined, plain-old-struct type with appropriate data members for the vertex attributes.
Separately, (on the main thread) create an object representing the OpenGL buffer, passing the vector or array. Creation of this object handles OpenGL buffer allocation and data upload in one step.

Secondary design goals:

Separate, dedicated types for vertex buffers and index buffers. (Technically we have this already since Buffer is templated on GLenum target, but we want to remove GLenum from the public interface.)
Set up for the vertex buffer type to carry layout information about its elements. (Necessary for C++ bindings for OpenGL #6468.)

API sketch:

// Vertex buffers

template <class ElementType>
class VertexBufferSlice {
public:
    std::size_t offset() const;
    std::size_t size() const;

    const VertexBuffer<ElementType>& buffer;
};

template <class ElementType>
class VertexBuffer {
public:
    std::size_t size() const;
    VertexBufferSlice<ElementType> slice(std::size_t offset, std::size_t len) const;
    operator VertexBufferSlice<ElementType> () const { return slice(0, size()); }

private:
    UniqueBuffer buffer;
};

template <class Arraylike> // see https://github.com/mapbox/jni.hpp/blob/master/include/jni/arraylike.hpp
VertexBuffer<ArraylikeElement<ArrayLike>> Context::createVertexBuffer(const Arraylike& data);

// Index (a.k.a. element) buffers

template <class Primitive, class IndexType = uint16_t>
class IndexBufferSlice { ... };

template <class Primitive, class IndexType = uint16_t>
class IndexBuffer { ... };

// Where Primitive is one of `Points`, `Lines`, `LineStrip`, `Triangles`:
// https://github.com/mapbox/mapbox-gl-native/blob/3bf08b4cd89b71010bc25dbf74fe3c7752918faa/src/mbgl/gl/primitives.hpp#L14-L17

template <class Primitive, class IndexType>
using IndexBufferElementType;

template <> using IndexBufferElementType<Points> = IndexType;
template <> using IndexBufferElementType<Lines> = IndexType;
template <> using IndexBufferElementType<LineStrip> = IndexType;
template <> using IndexBufferElementType<Triangles> = std::array<IndexType, 3>;

template <class Primitive, class Arraylike>
IndexBuffer<Primitive, IndexType> Context::createIndexBuffer(const Arraylike& data) {
    static_assert(ArraylikeElement<ArrayLike> == IndexBufferElementType<Primitive>);
    ...
}

kkaefer · 2016-09-29T08:40:15Z

This could also include a refactor that uses structs as the members of the buffers instead of writing explicit accessors (using #pragma pack(1)).

With MTLVertexDescriptor, the Metal API has a similar concept for describing the buffer layout and how they map to a shader's attributes.

mention-bot · 2016-09-30T00:31:32Z

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

jfirebaugh · 2016-09-30T00:40:31Z

src/mbgl/renderer/circle_bucket.hpp

-    CircleVertexBuffer vertexBuffer_;
-    TriangleElementsBuffer elementsBuffer_;
+    std::vector<CircleVertex> vertexes;
+    std::vector<gl::Triangle<uint16_t>> indexes;


@kkaefer Should we hard-code uint16_t as the index type? So a declaration like this would be:

std::vector<gl::Triangle> indexes;

OpenGL only supports 16bit unsigned indices anyway

Ah, this is an OpenGL ES limitation and I was looking at generic OpenGL documentation for glDrawElements. OpenGL ES also supports uint8_t indexes, but I don't think we'll ever use those. So I'll deparameterize this.

kkaefer · 2016-09-30T09:12:37Z

src/mbgl/gl/bytes.hpp

+namespace gl {
+
+struct ElementOffset {
+    std::size_t value;


size_t is 8 bytes on 64-bit platforms, can we use uint32_t here?

Why? If you want to match GLsizei, it should be int32_t, or actually uint16_t because this must be a non-negative value.

kkaefer · 2016-09-30T09:23:20Z

src/mbgl/gl/index_buffer.hpp

+
+template <class Primitive>
+class IndexBuffer {
+public:


Could we static_assert that Primitive is one of Line and Triangle?

kkaefer · 2016-09-30T09:28:34Z

src/mbgl/gl/vertex_buffer.hpp

+template <class Vertex>
+class VertexBuffer {
+public:
+    static constexpr std::size_t vertexSize = sizeof(Vertex);


Could we use using vertexSize = std::integral_constant<std::size_t, sizeof(Vertex)>; to avoid static data members that need initialization?

kkaefer · 2016-09-30T09:29:08Z

src/mbgl/layout/symbol_layout.cpp

+                                    static_cast<uint16_t>(triangleIndex + 2)});
+        buffer.triangles.push_back({static_cast<uint16_t>(triangleIndex + 1),
+                                    static_cast<uint16_t>(triangleIndex + 2),
+                                    static_cast<uint16_t>(triangleIndex + 3)});


why push_back and not emplace_back?

emplace_back doesn't work with aggregate initialization. I'll add constructors to Line and Triangle so we can use it.

kkaefer · 2016-09-30T09:30:18Z

src/mbgl/renderer/symbol_bucket.hpp


    struct TextBuffer {
-        TextVertexBuffer vertices;
-        TriangleElementsBuffer triangles;
+        std::vector<TextureRectVertex> vertexes;


Why switch from vertices/indices to vertexes/indexes?

For indexes I actually meant to use lines or triangles depending on the primitive. I'll switch back to vertices.

kkaefer · 2016-09-30T09:32:09Z

src/mbgl/shader/circle_vertex.cpp

+void CircleVertex::bind(const int8_t* offset) {
+    static_assert(sizeof(CircleVertex) == 4, "expected CircleVertex size");
+
+    MBGL_BIND_VERTEX_ATTRIBUTE(CircleVertex, a_pos, offset);


This is an incorrect place for using a_pos; those names/attributes are shader-specific and not Vertex type specific.

Vertex attribute binding involves some shader-specific things and some vertex-specific things:

Shader-specific: attribute name, index, and GLSL type.

Vertex-specific: member name, offset, and C++ type.

I'd like to come up with a design where:

A given C++ vertex type can be used with multiple shaders.

A shader requires one particular vertex type, enforced at compile time.

Attribute indexes are defined per-Shader-subclass, rather than in Shader, as they are now. Not all shaders have all attributes.

Debug builds verify that the GLSL and native types are consistent, using glGetActiveAttrib to introspect the GLSL type. (We should do this for uniforms as well.)

I'm open to a design that doesn't enforce that the GLSL attribute name matches the C++ member name, but it also doesn't seem bad to do so. Why would we need them to differ?

I think the starting point here is to introduce a counterpart to uniform.{hpp,cpp} for attributes and add members to each Shader subclass -- e.g.:

AttributeVector<int16_t, 2> a_pos = { "a_pos", *this };

Then see where that leads the binding code.

Sketched here but currently stuck on a tricky problem with type erasing pointer-to-members.

We still have the issue of Vertex types having to have the same names as the shaders. In our code, we (almost?) always do it like that, but there's no reason why we need to do it like that. I.e. VertexType::a_pos could be bound to Shader::a_pos, but we could also name the position in the VertexType something else.

kkaefer · 2016-09-30T09:45:45Z

include/mbgl/gl/gl.hpp

+
+// This has to be a macro because it uses the offsetof macro, which is the only legal way to get a member offset.
+#define MBGL_BIND_VERTEX_ATTRIBUTE(VertexType, member, offset) \
+    ::mbgl::gl::bindVertexAttribute(Shader::member, &VertexType::member, offset, offsetof(VertexType, member))


We should be using std::is_standard_layout to ensure that we can safely use offsetof. We should also assert that all values we get from offsetof are multiples of 4.

It also assumes that the shader and the vertex type have identically named member names. I feel a little bit uneasy about that, but can't quite put my finger on it.

kkaefer · 2016-09-30T09:46:55Z

src/mbgl/shader/raster_vertex.hpp

+          } {}
+
+    int16_t a_pos[2];
+    int16_t a_texture_pos[2];


Can we make all of these members const?

kkaefer · 2016-09-30T09:49:10Z

src/mbgl/shader/raster_shader.hpp

 class RasterShader : public Shader {
 public:
    RasterShader(gl::Context&, Defines defines = None);

-    void bind(int8_t* offset) final;
+    void bind(const gl::VertexBuffer<RasterVertex>&,
+              const int8_t* offset);


Can we rename offset to something like begin to avoid confusion of the offset within a buffer with the offset of an attribute within a vertex?

My plan here is to replace both parameters with const gl::VertexBufferSlice<RasterVertex>&, but I'd like to put that off until a followup PR.

jfirebaugh · 2016-10-03T20:07:17Z

@kkaefer Re-review?

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.

kkaefer · 2016-10-05T09:13:16Z

include/mbgl/gl/gl.hpp

+    static_assert(std::is_standard_layout<V>::value, "vertex type must use standard layout");
+    static_assert(memberOffset % 4 == 0, "vertex attribute must be optimally aligned");
+    static_assert(1 <= N && N <= 4, "count must be 1, 2, 3, or 4");
+    static_assert(sizeof(V) <= std::numeric_limits<GLsizei>::max(), "vertex type is too big");


kkaefer · 2016-10-05T09:23:40Z

src/mbgl/gl/element_group.hpp

+
+    ElementGroup(uint32_t vertex_length_ = 0, uint32_t elements_length_ = 0)
+        : vertex_length(vertex_length_)
+        , elements_length(elements_length_)


Our current formatting guide does not have leading commas in initializer lists. We should either change this formatting or the formatting guidelines.

This was fixed in a followup commit.

kkaefer · 2016-10-05T09:43:48Z

src/mbgl/renderer/debug_bucket.cpp

-      modified(std::move(modified_)),
-      expires(std::move(expires_)),
-      debugMode(debugMode_) {
+std::vector<PlainVertex> buildTextVertexes(const OverscaledTileID& id,


There are still a few instances of vertexes in the code.

kkaefer · 2016-10-05T09:47:54Z

src/mbgl/shader/circle_vertex.cpp

+void CircleVertex::bind(const int8_t* offset) {
+    static_assert(sizeof(CircleVertex) == 4, "expected CircleVertex size");
+
+    MBGL_BIND_VERTEX_ATTRIBUTE(CircleVertex, a_pos, offset);


We still have the issue of Vertex types having to have the same names as the shaders. In our code, we (almost?) always do it like that, but there's no reason why we need to do it like that. I.e. VertexType::a_pos could be bound to Shader::a_pos, but we could also name the position in the VertexType something else.

jfirebaugh self-assigned this Sep 29, 2016

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 buffer-refactor branch from f65f62e to ee3dd65 Compare September 30, 2016 00:34

jfirebaugh commented Sep 30, 2016

View reviewed changes

jfirebaugh force-pushed the buffer-refactor branch 2 times, most recently from db104b0 to a327bf5 Compare September 30, 2016 01:37

kkaefer reviewed Sep 30, 2016

View reviewed changes

jfirebaugh force-pushed the buffer-refactor branch 9 times, most recently from 18ddf48 to 95bbe9e Compare October 3, 2016 18:37

jfirebaugh added 6 commits October 4, 2016 14:01

[core] Don't refer to geometry coordinates as "vertices"

c8c7472

[core] Refactor Buffer

e7971da

[core] Move shader and uniform to gl directory

bef14f7

[core] Put VertexArrayObject in gl namespace

496aebc

[core] Improve attribute binding API

8d4362d

jfirebaugh force-pushed the buffer-refactor branch from 3390bba to 39c4cb0 Compare October 4, 2016 21:11

[core] TextureRectVertex ⇢ SymbolVertex

120d0c6

kkaefer approved these changes Oct 5, 2016

View reviewed changes

[core] Adhere to vertices/vertexBuffer naming convention

fc9e3d9

jfirebaugh merged commit f438bb6 into master Oct 5, 2016

jfirebaugh deleted the buffer-refactor branch October 5, 2016 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Buffer refactor #6504

Buffer refactor #6504

jfirebaugh commented Sep 30, 2016

kkaefer commented Sep 29, 2016

mention-bot commented Sep 30, 2016

jfirebaugh Sep 30, 2016

kkaefer Sep 30, 2016

jfirebaugh Sep 30, 2016

kkaefer Sep 30, 2016

jfirebaugh Sep 30, 2016

kkaefer Sep 30, 2016

kkaefer Sep 30, 2016

kkaefer Sep 30, 2016

jfirebaugh Sep 30, 2016

kkaefer Sep 30, 2016

jfirebaugh Sep 30, 2016

kkaefer Sep 30, 2016

jfirebaugh Sep 30, 2016

jfirebaugh Sep 30, 2016

jfirebaugh Oct 1, 2016

kkaefer Oct 5, 2016

kkaefer Sep 30, 2016

kkaefer Sep 30, 2016

kkaefer Sep 30, 2016

jfirebaugh Sep 30, 2016

jfirebaugh commented Oct 3, 2016

kkaefer Oct 5, 2016

kkaefer Oct 5, 2016

jfirebaugh Oct 5, 2016

kkaefer Oct 5, 2016

kkaefer Oct 5, 2016

Buffer refactor #6504

Buffer refactor #6504

Conversation

jfirebaugh commented Sep 30, 2016

kkaefer commented Sep 29, 2016

mention-bot commented Sep 30, 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

jfirebaugh commented Oct 3, 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