GL: clean up the horrific mess in internal Mesh attribute storage.

This "type erased std::vector member" was done in the times before
growable arrays were a thing, and kind of made sense to go the extra way
to avoid a <vector> include in the header. Except that it made rather
unportable assumptions about std::vector size, which weren't correct for
example with _GLIBXX_ASSERTIONS set.

But what was *completely* unacceptable was that the vector was of one or
another type depending on the GL feature set present in the current
context. Apart from adding a lot of extra *nasty* logic to construction,
moves and destruction, this approach led to the mesh instance asking the
current context on destruction in order to know whether a destructor
should be called on std::vector<Buffer> or std::vector<AttributeLayout>.
Ugh.

Now it's a regular Array member (which isn't *that* heavy to need such
type-erased treatment, although it eventually could be), and thanks to
the AttributeLayout packing improvements in previous commits it's no
longer prohibitively wasteful to just abuse AttributeLayout instances to
store just owning Buffer instances alone -- doing so now wastes only 16
bytes per buffer, compared to 36 before. Given there's usually just one
or two vertex buffers per mesh (compared to attributes, which are
usually 4 or more), it should be fine.

The MeshGLTest::destructMovedOutInstance() test added few commits back
also no longer asserts on no GL context being present.

doc/changelog.dox

@@ -1036,6 +1036,10 @@ See also:
     passed the arguments to the base @relativeref{Corrade,TestSuite::Tester}
     implementation in a wrong order, failing to compile if this function was
     used in a test
+-   Destructing a moved-out @ref GL::Mesh was querying the GL context to decide
+    what to destroy in its internal state, now it's a proper no-op that works
+    also with the GL context already gone, like with all other GL object
+    wrappers.
 -   @ref GL::Renderer::MemoryBarrier::ShaderStorage had an incorrect value
 -   @ref GL::Shader limit queries for a particular shader stage on desktop were
     out-of-bounds array accesses, causing wrong or random values being returned

src/Magnum/GL/Implementation/MeshState.cpp

@@ -113,8 +113,6 @@ MeshState::MeshState(Context& context, ContextState& contextState, Containers::S
             bindIndexBufferImplementation = &Mesh::bindIndexBufferImplementationVAO;
         }
 
-        moveConstructImplementation = &Mesh::moveConstructImplementationVAO;
-        moveAssignImplementation = &Mesh::moveAssignImplementationVAO;
         destroyImplementation = &Mesh::destroyImplementationVAO;
         acquireVertexBufferImplementation = &Mesh::acquireVertexBufferImplementationVAO;
         bindVAOImplementation = &Mesh::bindVAOImplementationVAO;
@@ -124,8 +122,6 @@ MeshState::MeshState(Context& context, ContextState& contextState, Containers::S
     #if !defined(MAGNUM_TARGET_GLES) || defined(MAGNUM_TARGET_GLES2)
     else {
         createImplementation = &Mesh::createImplementationDefault;
-        moveConstructImplementation = &Mesh::moveConstructImplementationDefault;
-        moveAssignImplementation = &Mesh::moveAssignImplementationDefault;
         destroyImplementation = &Mesh::destroyImplementationDefault;
 
         /* ANGLE is ... also special! Equivalently above for the VAO case. */

src/Magnum/GL/Implementation/MeshState.h

@@ -39,10 +39,8 @@ struct MeshState {
 
     void reset();
 
-    void(*createImplementation)(Mesh&, bool);
-    void(*moveConstructImplementation)(Mesh&, Mesh&&);
-    void(*moveAssignImplementation)(Mesh&, Mesh&&);
-    void(*destroyImplementation)(Mesh&, bool);
+    void(*createImplementation)(Mesh&);
+    void(*destroyImplementation)(Mesh&);
     void(*attributePointerImplementation)(Mesh&, Mesh::AttributeLayout&&);
     #if !defined(MAGNUM_TARGET_GLES) || defined(MAGNUM_TARGET_GLES2)
     void(*vertexAttribDivisorImplementation)(Mesh&, GLuint, GLuint);

src/Magnum/GL/Mesh.cpp

@@ -25,7 +25,7 @@
 
 #include "Mesh.h"
 
-#include <vector>
+#include <Corrade/Containers/GrowableArray.h>
 #include <Corrade/Containers/StridedArrayView.h>
 #ifndef MAGNUM_TARGET_WEBGL
 #include <Corrade/Containers/String.h>
@@ -165,6 +165,11 @@ Debug& operator<<(Debug& debug, const MeshIndexType value) {
 #endif
 
 struct Mesh::AttributeLayout {
+    /* Records attribute layout with a non-owning Buffer reference. Used as a
+       temporary data holder when VAOs are used, saved to the _attributes
+       member when not. If a Buffer instance needs to be owned, it's
+       subsequently moved in (usually with ObjectFlag::DeleteOnDestruction
+       set). */
     explicit AttributeLayout(const Buffer& buffer, GLuint location, GLint size, GLenum type, DynamicAttribute::Kind kind, GLintptr offset, GLsizei stride, GLuint divisor) noexcept: buffer{Buffer::wrap(buffer.id())}, location{UnsignedByte(location)}, kindSize{UnsignedByte(kind)}, type{UnsignedShort(type)}, divisor{divisor}, offsetStride{(UnsignedLong(offset) << 16)|stride} {
         CORRADE_INTERNAL_ASSERT(location < 256 && type < 65536 && UnsignedLong(offset) < (1ull << 48) && stride < 65536);
         #ifndef MAGNUM_TARGET_GLES
@@ -178,6 +183,13 @@ struct Mesh::AttributeLayout {
         }
     }
 
+    /* Takes ownership of a Buffer instance. Abuses the _attributes storage in
+       cases where VAOs are used. That wastes a bit of space as only 8 out of
+       the 24 bytes is actually used, but that should be okay as there's likely
+       only very few buffers (compared to attributes, which can be quite
+       many). */
+    explicit AttributeLayout(Buffer&& buffer): buffer{Utility::move(buffer)}, location{}, kindSize{}, type{}, divisor{}, offsetStride{} {}
+
     AttributeLayout(AttributeLayout&&) noexcept = default;
     AttributeLayout(const AttributeLayout&) noexcept = delete;
     AttributeLayout& operator=(AttributeLayout&&) noexcept = default;
@@ -319,22 +331,19 @@ Int Mesh::maxElementsVertices() {
 #endif
 
 Mesh::Mesh(const MeshPrimitive primitive): _primitive{primitive}, _flags{ObjectFlag::DeleteOnDestruction} {
-    Context::current().state().mesh.createImplementation(*this, true);
+    Context::current().state().mesh.createImplementation(*this);
 }
 
 Mesh::Mesh(NoCreateT) noexcept: _id{0}, _primitive{MeshPrimitive::Triangles}, _flags{ObjectFlag::DeleteOnDestruction} {}
 
 Mesh::~Mesh() {
-    const bool deleteObject = _id && (_flags & ObjectFlag::DeleteOnDestruction);
-
     /* Moved out or not deleting on destruction, nothing to do */
-    if(deleteObject) {
-        /* Remove current vao from the state */
-        GLuint& current = Context::current().state().mesh.currentVAO;
-        if(current == _id) current = 0;
-    }
+    if(!_id || !(_flags & ObjectFlag::DeleteOnDestruction))
+        return;
 
-    if(_constructed) Context::current().state().mesh.destroyImplementation(*this, deleteObject);
+    /* Remove current vao from the state */
+    GLuint& current = Context::current().state().mesh.currentVAO;
+    if(current == _id) current = 0;
 }
 
 Mesh::Mesh(Mesh&& other) noexcept: _id(other._id), _primitive(other._primitive), _flags{other._flags}, _countSet{other._countSet}, _count(other._count), _baseVertex{other._baseVertex}, _instanceCount{other._instanceCount},
@@ -344,10 +353,9 @@ Mesh::Mesh(Mesh&& other) noexcept: _id(other._id), _primitive(other._primitive),
     #ifndef MAGNUM_TARGET_GLES2
     _indexStart(other._indexStart), _indexEnd(other._indexEnd),
     #endif
-    _indexType(other._indexType), _indexBufferOffset(other._indexBufferOffset), _indexOffset(other._indexOffset), _indexBuffer{Utility::move(other._indexBuffer)}
+    _indexType(other._indexType), _indexBufferOffset(other._indexBufferOffset), _indexOffset(other._indexOffset), _indexBuffer{Utility::move(other._indexBuffer)},
+    _attributes{Utility::move(other._attributes)}
 {
-    if(_constructed || other._constructed)
-        Context::current().state().mesh.moveConstructImplementation(*this, Utility::move(other));
     other._id = 0;
 }
 
@@ -371,16 +379,12 @@ Mesh& Mesh::operator=(Mesh&& other) noexcept {
     swap(_indexType, other._indexType);
     swap(_indexOffset, other._indexOffset);
     swap(_indexBuffer, other._indexBuffer);
-
-    if(_constructed || other._constructed)
-        Context::current().state().mesh.moveAssignImplementation(*this, Utility::move(other));
+    swap(_attributes, other._attributes);
 
     return *this;
 }
 
-Mesh::Mesh(const GLuint id, const MeshPrimitive primitive, const ObjectFlags flags): _id{id}, _primitive{primitive}, _flags{flags} {
-    Context::current().state().mesh.createImplementation(*this, false);
-}
+Mesh::Mesh(const GLuint id, const MeshPrimitive primitive, const ObjectFlags flags): _id{id}, _primitive{primitive}, _flags{flags} {}
 
 inline void Mesh::createIfNotAlready() {
     /* If VAO extension is not available, the following is always true */
@@ -1084,102 +1088,35 @@ void Mesh::bindVAO() {
     }
 }
 
-void Mesh::createImplementationDefault(Mesh& self, bool) {
+void Mesh::createImplementationDefault(Mesh& self) {
     self._id = 0;
     self._flags |= ObjectFlag::Created;
-
-    static_assert(sizeof(_attributes) >= sizeof(std::vector<AttributeLayout>),
-        "attribute storage buffer size too small");
-    new(&self._attributes) std::vector<AttributeLayout>;
-    self._constructed = true;
 }
 
-void Mesh::createImplementationVAO(Mesh& self, const bool createObject) {
-    if(createObject) {
-        #ifndef MAGNUM_TARGET_GLES2
-        glGenVertexArrays(1, &self._id);
-        #else
-        glGenVertexArraysOES(1, &self._id);
-        #endif
-        CORRADE_INTERNAL_ASSERT(self._id != Implementation::State::DisengagedBinding);
-    }
-
-    static_assert(sizeof(_attributes) >= sizeof(std::vector<Buffer>),
-        "attribute storage buffer size too small");
-    new(&self._attributes) std::vector<Buffer>;
-    self._constructed = true;
+void Mesh::createImplementationVAO(Mesh& self) {
+    #ifndef MAGNUM_TARGET_GLES2
+    glGenVertexArrays(1, &self._id);
+    #else
+    glGenVertexArraysOES(1, &self._id);
+    #endif
+    CORRADE_INTERNAL_ASSERT(self._id != Implementation::State::DisengagedBinding);
 }
 
 #ifndef MAGNUM_TARGET_GLES
-void Mesh::createImplementationVAODSA(Mesh& self, const bool createObject) {
-    if(createObject) {
-        glCreateVertexArrays(1, &self._id);
-        self._flags |= ObjectFlag::Created;
-    }
-
-    static_assert(sizeof(_attributes) >= sizeof(std::vector<Buffer>),
-        "attribute storage buffer size too small");
-    new(&self._attributes) std::vector<Buffer>;
-    self._constructed = true;
+void Mesh::createImplementationVAODSA(Mesh& self) {
+    glCreateVertexArrays(1, &self._id);
+    self._flags |= ObjectFlag::Created;
 }
 #endif
 
-void Mesh::moveConstructImplementationDefault(Mesh& self, Mesh&& other) {
-    new(&self._attributes) std::vector<AttributeLayout>{Utility::move(*reinterpret_cast<std::vector<AttributeLayout>*>(&other._attributes))};
-    self._constructed = true;
-}
+void Mesh::destroyImplementationDefault(Mesh&) {}
 
-void Mesh::moveConstructImplementationVAO(Mesh& self, Mesh&& other) {
-    new(&self._attributes) std::vector<Buffer>{Utility::move(*reinterpret_cast<std::vector<Buffer>*>(&other._attributes))};
-    self._constructed = true;
-}
-
-/* In the move construction / assignment we need to stay noexcept, which means
-   it's only possible to swap or move-construct the vector, can't delete or
-   allocate. So, in the particular case where `this` is constructed and `other`
-   is not, we do a "partial swap" -- `other` gets our data and our data gets
-   emptied. */
-
-void Mesh::moveAssignImplementationDefault(Mesh& self, Mesh&& other) {
-    if(self._constructed && other._constructed)
-        Utility::swap(*reinterpret_cast<std::vector<AttributeLayout>*>(&self._attributes),
-            *reinterpret_cast<std::vector<AttributeLayout>*>(&other._attributes));
-    else if(self._constructed && !other._constructed) {
-        other._constructed = true;
-        new(&other._attributes) std::vector<AttributeLayout>{Utility::move(*reinterpret_cast<std::vector<AttributeLayout>*>(&self._attributes))};
-    } else if(!self._constructed && other._constructed) {
-        self._constructed = true;
-        new(&self._attributes) std::vector<AttributeLayout>{Utility::move(*reinterpret_cast<std::vector<AttributeLayout>*>(&other._attributes))};
-    }
-}
-
-void Mesh::moveAssignImplementationVAO(Mesh& self, Mesh&& other) {
-    if(self._constructed && other._constructed)
-        Utility::swap(*reinterpret_cast<std::vector<Buffer>*>(&self._attributes),
-            *reinterpret_cast<std::vector<Buffer>*>(&other._attributes));
-    else if(self._constructed && !other._constructed) {
-        other._constructed = true;
-        new(&other._attributes) std::vector<Buffer>{Utility::move(*reinterpret_cast<std::vector<Buffer>*>(&self._attributes))};
-    } else if(!self._constructed && other._constructed) {
-        self._constructed = true;
-        new(&self._attributes) std::vector<Buffer>{Utility::move(*reinterpret_cast<std::vector<Buffer>*>(&other._attributes))};
-    }
-}
-
-void Mesh::destroyImplementationDefault(Mesh& self, bool) {
-    reinterpret_cast<std::vector<AttributeLayout>*>(&self._attributes)->~vector();
-}
-
-void Mesh::destroyImplementationVAO(Mesh& self, const bool deleteObject) {
-    if(deleteObject) {
-        #ifndef MAGNUM_TARGET_GLES2
-        glDeleteVertexArrays(1, &self._id);
-        #else
-        glDeleteVertexArraysOES(1, &self._id);
-        #endif
-    }
-
-    reinterpret_cast<std::vector<Buffer>*>(&self._attributes)->~vector();
+void Mesh::destroyImplementationVAO(Mesh& self) {
+    #ifndef MAGNUM_TARGET_GLES2
+    glDeleteVertexArrays(1, &self._id);
+    #else
+    glDeleteVertexArraysOES(1, &self._id);
+    #endif
 }
 
 void Mesh::attributePointerInternal(const Buffer& buffer, const GLuint location, const GLint size, const GLenum type, const DynamicAttribute::Kind kind, const GLintptr offset, const GLsizei stride, const GLuint divisor) {
@@ -1198,7 +1135,7 @@ void Mesh::attributePointerImplementationDefault(Mesh& self, AttributeLayout&& a
         "GL::Mesh::addVertexBuffer(): the buffer has unexpected target hint, expected" << Buffer::TargetHint::Array << "but got" << attribute.buffer.targetHint(), );
     #endif
 
-    reinterpret_cast<std::vector<AttributeLayout>*>(&self._attributes)->push_back(Utility::move(attribute));
+    arrayAppend(self._attributes, Utility::move(attribute));
 }
 
 void Mesh::attributePointerImplementationVAO(Mesh& self, AttributeLayout&& attribute) {
@@ -1325,17 +1262,19 @@ void Mesh::acquireVertexBuffer(Buffer&& buffer) {
 
 void Mesh::acquireVertexBufferImplementationDefault(Mesh& self, Buffer&& buffer) {
     /* The last added buffer should be this one, replace it with a owning one */
-    auto& attributes = *reinterpret_cast<std::vector<AttributeLayout>*>(&self._attributes);
-    CORRADE_INTERNAL_ASSERT(!attributes.empty() && attributes.back().buffer.id() == buffer.id() && buffer.id());
-    attributes.back().buffer.release(); /* so we swap back a zero ID */
-    attributes.back().buffer = Utility::move(buffer);
+    CORRADE_INTERNAL_ASSERT(!self._attributes.isEmpty() && self._attributes.back().buffer.id() == buffer.id() && buffer.id());
+    self._attributes.back().buffer.release(); /* so we swap back a zero ID */
+    self._attributes.back().buffer = Utility::move(buffer);
 }
 
 void Mesh::acquireVertexBufferImplementationVAO(Mesh& self, Buffer&& buffer) {
     CORRADE_INTERNAL_ASSERT(buffer.id());
-    /* With VAOs we are not maintaining the attribute list, so just store the
-       buffer directly */
-    reinterpret_cast<std::vector<Buffer>*>(&self._attributes)->emplace_back(Utility::move(buffer));
+    /* With VAOs we are not maintaining the attribute list, so abuse the
+       storage for just owning the buffer. That wastes a bit of space as only 8
+       out of the 24 bytes is actually used, but that should be okay as there's
+       likely only very few buffers (compared to attributes, which can be quite
+       many). */
+    arrayAppend(self._attributes, InPlaceInit, Utility::move(buffer));
 }
 
 void Mesh::bindIndexBufferImplementationDefault(Mesh&, Buffer&) {}
@@ -1356,7 +1295,7 @@ void Mesh::bindIndexBufferImplementationVAODSA(Mesh& self, Buffer& buffer) {
 
 void Mesh::bindImplementationDefault(Mesh& self) {
     /* Specify vertex attributes */
-    for(AttributeLayout& attribute: *reinterpret_cast<std::vector<AttributeLayout>*>(&self._attributes))
+    for(AttributeLayout& attribute: self._attributes)
         self.vertexAttribPointer(attribute);
 
     /* Bind index buffer, if the mesh is indexed */
@@ -1368,7 +1307,7 @@ void Mesh::bindImplementationVAO(Mesh& self) {
 }
 
 void Mesh::unbindImplementationDefault(Mesh& self) {
-    for(const AttributeLayout& attribute: *reinterpret_cast<std::vector<AttributeLayout>*>(&self._attributes)) {
+    for(const AttributeLayout& attribute: self._attributes) {
         glDisableVertexAttribArray(attribute.location);
 
         /* Reset also the divisor back so it doesn't affect  */

src/Magnum/GL/Mesh.h

@@ -29,6 +29,7 @@
  * @brief Class @ref Magnum::GL::Mesh, enum @ref Magnum::GL::MeshPrimitive, @ref Magnum::GL::MeshIndexType, function @ref Magnum::GL::meshPrimitive(), @ref Magnum::GL::meshIndexType(), @ref Magnum::GL::meshIndexTypeSize()
  */
 
+#include <Corrade/Containers/Array.h>
 #include <Corrade/Utility/ConfigurationValue.h>
 
 #include "Magnum/Tags.h"
@@ -1195,6 +1196,7 @@ class MAGNUM_GL_EXPORT Mesh: public AbstractObject {
 
         struct MAGNUM_GL_LOCAL AttributeLayout;
 
+        /* Used by wrap() */
         explicit Mesh(GLuint id, MeshPrimitive primitive, ObjectFlags flags);
 
         void MAGNUM_GL_LOCAL createIfNotAlready();
@@ -1315,19 +1317,14 @@ class MAGNUM_GL_EXPORT Mesh: public AbstractObject {
         MAGNUM_GL_LOCAL void drawInternal(TransformFeedback& xfb, UnsignedInt stream, Int instanceCount);
         #endif
 
-        static void MAGNUM_GL_LOCAL createImplementationDefault(Mesh& self, bool);
-        static void MAGNUM_GL_LOCAL createImplementationVAO(Mesh& self, bool createObject);
+        static void MAGNUM_GL_LOCAL createImplementationDefault(Mesh& self);
+        static void MAGNUM_GL_LOCAL createImplementationVAO(Mesh& self);
         #ifndef MAGNUM_TARGET_GLES
-        static void MAGNUM_GL_LOCAL createImplementationVAODSA(Mesh& self, bool createObject);
+        static void MAGNUM_GL_LOCAL createImplementationVAODSA(Mesh& self);
         #endif
 
-        static void MAGNUM_GL_LOCAL moveConstructImplementationDefault(Mesh& self, Mesh&& other);
-        static void MAGNUM_GL_LOCAL moveConstructImplementationVAO(Mesh& self, Mesh&& other);
-        static void MAGNUM_GL_LOCAL moveAssignImplementationDefault(Mesh& self, Mesh&& other);
-        static void MAGNUM_GL_LOCAL moveAssignImplementationVAO(Mesh& self, Mesh&& other);
-
-        static void MAGNUM_GL_LOCAL destroyImplementationDefault(Mesh& self, bool);
-        static void MAGNUM_GL_LOCAL destroyImplementationVAO(Mesh& self, bool deleteObject);
+        static void MAGNUM_GL_LOCAL destroyImplementationDefault(Mesh& self);
+        static void MAGNUM_GL_LOCAL destroyImplementationVAO(Mesh& self);
 
         void attributePointerInternal(const Buffer& buffer, GLuint location, GLint size, GLenum type, DynamicAttribute::Kind kind, GLintptr offset, GLsizei stride, GLuint divisor);
         void MAGNUM_GL_LOCAL attributePointerInternal(AttributeLayout&& attribute);
@@ -1420,9 +1417,7 @@ class MAGNUM_GL_EXPORT Mesh: public AbstractObject {
         /* using a separate bool for _count instead of Optional to make use of
            the 3-byte gap after _flags */
         bool _countSet{};
-        /* Whether the _attributes storage was constructed (it's not when the
-           object is constructed using NoCreate). Also fits in the gap. */
-        bool _constructed{};
+        /* 1 byte free */
         #ifdef MAGNUM_TARGET_GLES
         /* See the "angle-instanced-attributes-always-draw-instanced" workaround */
         bool _instanced{};
@@ -1437,10 +1432,9 @@ class MAGNUM_GL_EXPORT Mesh: public AbstractObject {
         GLintptr _indexBufferOffset{}, _indexOffset{};
         Buffer _indexBuffer{NoCreate};
 
-        /* Storage for either std::vector<AttributeLayout> (in case of no VAOs)
-           or std::vector<Buffer> (in case of VAOs). 4 pointers should be one
-           pointer more than enough. */
-        struct { std::intptr_t data[4]; } _attributes;
+        /* Stores attribute layouts in case VAOs are not supported or disabled,
+           abused for capturing buffer ownership if VAOs are supported. */
+        Containers::Array<AttributeLayout> _attributes;
 };
 
 /** @debugoperatorenum{MeshPrimitive} */