diff --git a/src/Magnum/GL/Implementation/MeshState.h b/src/Magnum/GL/Implementation/MeshState.h index e5f778e70..9868483c6 100644 --- a/src/Magnum/GL/Implementation/MeshState.h +++ b/src/Magnum/GL/Implementation/MeshState.h @@ -40,10 +40,10 @@ struct MeshState { void reset(); - void(Mesh::*createImplementation)(); + void(Mesh::*createImplementation)(bool); void(Mesh::*moveConstructImplementation)(Mesh&&); void(Mesh::*moveAssignImplementation)(Mesh&&); - void(Mesh::*destroyImplementation)(); + void(Mesh::*destroyImplementation)(bool); void(Mesh::*attributePointerImplementation)(Mesh::AttributeLayout&&); #if !defined(MAGNUM_TARGET_GLES) || defined(MAGNUM_TARGET_GLES2) void(Mesh::*vertexAttribDivisorImplementation)(GLuint, GLuint); diff --git a/src/Magnum/GL/Mesh.cpp b/src/Magnum/GL/Mesh.cpp index 60e8680eb..a09e85ac8 100644 --- a/src/Magnum/GL/Mesh.cpp +++ b/src/Magnum/GL/Mesh.cpp @@ -208,20 +208,22 @@ std::size_t Mesh::indexSize(Magnum::MeshIndexType type) { #endif Mesh::Mesh(const MeshPrimitive primitive): _primitive{primitive}, _flags{ObjectFlag::DeleteOnDestruction} { - (this->*Context::current().state().mesh->createImplementation)(); + (this->*Context::current().state().mesh->createImplementation)(true); } Mesh::Mesh(NoCreateT) noexcept: _id{0}, _primitive{MeshPrimitive::Triangles}, _flags{ObjectFlag::DeleteOnDestruction} {} Mesh::~Mesh() { - /* Moved out or not deleting on destruction, nothing to do */ - if(!_id || !(_flags & ObjectFlag::DeleteOnDestruction)) return; + const bool deleteObject = _id && (_flags & ObjectFlag::DeleteOnDestruction); - /* Remove current vao from the state */ - GLuint& current = Context::current().state().mesh->currentVAO; - if(current == _id) current = 0; + /* 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; + } - (this->*Context::current().state().mesh->destroyImplementation)(); + if(_constructed) (this->*Context::current().state().mesh->destroyImplementation)(deleteObject); } 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}, @@ -233,7 +235,8 @@ Mesh::Mesh(Mesh&& other) noexcept: _id(other._id), _primitive(other._primitive), #endif _indexOffset(other._indexOffset), _indexType(other._indexType), _indexBuffer{std::move(other._indexBuffer)} { - (this->*Context::current().state().mesh->moveConstructImplementation)(std::move(other)); + if(_constructed || other._constructed) + (this->*Context::current().state().mesh->moveConstructImplementation)(std::move(other)); other._id = 0; } @@ -256,12 +259,16 @@ Mesh& Mesh::operator=(Mesh&& other) noexcept { swap(_indexOffset, other._indexOffset); swap(_indexType, other._indexType); swap(_indexBuffer, other._indexBuffer); - (this->*Context::current().state().mesh->moveAssignImplementation)(std::move(other)); + + if(_constructed || other._constructed) + (this->*Context::current().state().mesh->moveAssignImplementation)(std::move(other)); return *this; } -Mesh::Mesh(const GLuint id, const MeshPrimitive primitive, const ObjectFlags flags): _id{id}, _primitive{primitive}, _flags{flags} {} +Mesh::Mesh(const GLuint id, const MeshPrimitive primitive, const ObjectFlags flags): _id{id}, _primitive{primitive}, _flags{flags} { + (this->*Context::current().state().mesh->createImplementation)(false); +} inline void Mesh::createIfNotAlready() { /* If VAO extension is not available, the following is always true */ @@ -538,67 +545,100 @@ void Mesh::bindVAO() { } } -void Mesh::createImplementationDefault() { +void Mesh::createImplementationDefault(bool) { _id = 0; _flags |= ObjectFlag::Created; static_assert(sizeof(_attributes) >= sizeof(std::vector), "attribute storage buffer size too small"); new(&_attributes) std::vector; + _constructed = true; } -void Mesh::createImplementationVAO() { - #ifndef MAGNUM_TARGET_GLES2 - glGenVertexArrays(1, &_id); - #else - glGenVertexArraysOES(1, &_id); - #endif - CORRADE_INTERNAL_ASSERT(_id != Implementation::State::DisengagedBinding); +void Mesh::createImplementationVAO(const bool createObject) { + if(createObject) { + #ifndef MAGNUM_TARGET_GLES2 + glGenVertexArrays(1, &_id); + #else + glGenVertexArraysOES(1, &_id); + #endif + CORRADE_INTERNAL_ASSERT(_id != Implementation::State::DisengagedBinding); + } static_assert(sizeof(_attributes) >= sizeof(std::vector), "attribute storage buffer size too small"); new(&_attributes) std::vector; + _constructed = true; } #ifndef MAGNUM_TARGET_GLES -void Mesh::createImplementationVAODSA() { - glCreateVertexArrays(1, &_id); - _flags |= ObjectFlag::Created; +void Mesh::createImplementationVAODSA(const bool createObject) { + if(createObject) { + glCreateVertexArrays(1, &_id); + _flags |= ObjectFlag::Created; + } static_assert(sizeof(_attributes) >= sizeof(std::vector), "attribute storage buffer size too small"); new(&_attributes) std::vector; + _constructed = true; } #endif void Mesh::moveConstructImplementationDefault(Mesh&& other) { new(&_attributes) std::vector{std::move(*reinterpret_cast*>(&other._attributes))}; + _constructed = true; } void Mesh::moveConstructImplementationVAO(Mesh&& other) { new(&_attributes) std::vector{std::move(*reinterpret_cast*>(&other._attributes))}; + _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&& other) { - std::swap(*reinterpret_cast*>(&_attributes), - *reinterpret_cast*>(&other._attributes)); + if(_constructed && other._constructed) + std::swap(*reinterpret_cast*>(&_attributes), + *reinterpret_cast*>(&other._attributes)); + else if(_constructed && !other._constructed) { + other._constructed = true; + new(&other._attributes) std::vector{std::move(*reinterpret_cast*>(&_attributes))}; + } else if(!_constructed && other._constructed) { + _constructed = true; + new(&_attributes) std::vector{std::move(*reinterpret_cast*>(&other._attributes))}; + } } void Mesh::moveAssignImplementationVAO(Mesh&& other) { - std::swap(*reinterpret_cast*>(&_attributes), - *reinterpret_cast*>(&other._attributes)); + if(_constructed && other._constructed) + std::swap(*reinterpret_cast*>(&_attributes), + *reinterpret_cast*>(&other._attributes)); + else if(_constructed && !other._constructed) { + other._constructed = true; + new(&other._attributes) std::vector{std::move(*reinterpret_cast*>(&_attributes))}; + } else if(!_constructed && other._constructed) { + _constructed = true; + new(&_attributes) std::vector{std::move(*reinterpret_cast*>(&other._attributes))}; + } } -void Mesh::destroyImplementationDefault() { +void Mesh::destroyImplementationDefault(bool) { reinterpret_cast*>(&_attributes)->~vector(); } -void Mesh::destroyImplementationVAO() { - #ifndef MAGNUM_TARGET_GLES2 - glDeleteVertexArrays(1, &_id); - #else - glDeleteVertexArraysOES(1, &_id); - #endif +void Mesh::destroyImplementationVAO(const bool deleteObject) { + if(deleteObject) { + #ifndef MAGNUM_TARGET_GLES2 + glDeleteVertexArrays(1, &_id); + #else + glDeleteVertexArraysOES(1, &_id); + #endif + } reinterpret_cast*>(&_attributes)->~vector(); } diff --git a/src/Magnum/GL/Mesh.h b/src/Magnum/GL/Mesh.h index 0508f7880..56cce077b 100644 --- a/src/Magnum/GL/Mesh.h +++ b/src/Magnum/GL/Mesh.h @@ -1084,10 +1084,10 @@ class MAGNUM_GL_EXPORT Mesh: public AbstractObject { void drawInternal(TransformFeedback& xfb, UnsignedInt stream, Int instanceCount); #endif - void MAGNUM_GL_LOCAL createImplementationDefault(); - void MAGNUM_GL_LOCAL createImplementationVAO(); + void MAGNUM_GL_LOCAL createImplementationDefault(bool); + void MAGNUM_GL_LOCAL createImplementationVAO(bool createObject); #ifndef MAGNUM_TARGET_GLES - void MAGNUM_GL_LOCAL createImplementationVAODSA(); + void MAGNUM_GL_LOCAL createImplementationVAODSA(bool createObject); #endif void MAGNUM_GL_LOCAL moveConstructImplementationDefault(Mesh&& other); @@ -1095,8 +1095,8 @@ class MAGNUM_GL_EXPORT Mesh: public AbstractObject { void MAGNUM_GL_LOCAL moveAssignImplementationDefault(Mesh&& other); void MAGNUM_GL_LOCAL moveAssignImplementationVAO(Mesh&& other); - void MAGNUM_GL_LOCAL destroyImplementationDefault(); - void MAGNUM_GL_LOCAL destroyImplementationVAO(); + void MAGNUM_GL_LOCAL destroyImplementationDefault(bool); + void MAGNUM_GL_LOCAL destroyImplementationVAO(bool deleteObject); 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); @@ -1149,9 +1149,12 @@ class MAGNUM_GL_EXPORT Mesh: public AbstractObject { GLuint _id; MeshPrimitive _primitive; ObjectFlags _flags; - /* using a separate bool instead of Optional to make use of the 3-byte - gap after _flags */ + /* 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{}; Int _count{}, _baseVertex{}, _instanceCount{1}; #ifndef MAGNUM_TARGET_GLES UnsignedInt _baseInstance{}; diff --git a/src/Magnum/GL/Test/MeshGLTest.cpp b/src/Magnum/GL/Test/MeshGLTest.cpp index d09ebb229..db1de0fb2 100644 --- a/src/Magnum/GL/Test/MeshGLTest.cpp +++ b/src/Magnum/GL/Test/MeshGLTest.cpp @@ -328,11 +328,13 @@ void MeshGLTest::constructMove() { CORRADE_VERIFY(id > 0); } + /* Move construct */ Mesh b(std::move(a)); CORRADE_COMPARE(a.id(), 0); CORRADE_COMPARE(b.id(), id); + /* Move assign */ Mesh c; c.addVertexBuffer(buffer2, 1, Attribute<1, Float>{}); const Int cId = c.id(); @@ -352,6 +354,16 @@ void MeshGLTest::constructMove() { CORRADE_COMPARE(b.id(), cId); CORRADE_COMPARE(c.id(), id); + /* Move assign to a NoCreate instance */ + Mesh d{NoCreate}; + d = std::move(c); + + CORRADE_COMPARE(c.id(), 0); + CORRADE_COMPARE(d.id(), id); + + /* Destroy */ + b = Mesh{NoCreate}; + /* Test that drawing still works properly */ { MAGNUM_VERIFY_NO_GL_ERROR(); @@ -369,7 +381,7 @@ void MeshGLTest::constructMove() { .bind(); FloatShader shader{"float", "vec4(valueInterpolated, 0.0, 0.0, 0.0)"}; - c.setPrimitive(MeshPrimitive::Points) + d.setPrimitive(MeshPrimitive::Points) .setCount(1) .draw(shader); diff --git a/src/Magnum/GL/Test/MeshTest.cpp b/src/Magnum/GL/Test/MeshTest.cpp index 3d189f91c..79bb0ef13 100644 --- a/src/Magnum/GL/Test/MeshTest.cpp +++ b/src/Magnum/GL/Test/MeshTest.cpp @@ -43,6 +43,7 @@ struct MeshTest: TestSuite::Tester { void constructViewNoCreate(); void constructCopy(); + void constructMoveNoCreate(); /* View *is* copyable */ void drawCountNotSet(); @@ -69,6 +70,7 @@ MeshTest::MeshTest() { &MeshTest::constructViewNoCreate, &MeshTest::constructCopy, + &MeshTest::constructMoveNoCreate, &MeshTest::drawCountNotSet, &MeshTest::drawViewCountNotSet, @@ -113,6 +115,23 @@ void MeshTest::constructCopy() { CORRADE_VERIFY(!(std::is_assignable{})); } +void MeshTest::constructMoveNoCreate() { + /* Neither of these should be accessing the GL context */ + { + Mesh a{NoCreate}; + CORRADE_COMPARE(a.id(), 0); + + Mesh b{std::move(a)}; + CORRADE_COMPARE(b.id(), 0); + + Mesh c{NoCreate}; + c = std::move(b); + CORRADE_COMPARE(c.id(), 0); + } + + CORRADE_VERIFY(true); +} + namespace { struct Shader: AbstractShaderProgram { explicit Shader(NoCreateT): AbstractShaderProgram{NoCreate} {}