From d6e0186dbfa35443dc52fe0d21f332f84196f39b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Fri, 1 Mar 2019 00:10:14 +0100 Subject: [PATCH] GL: actually fix the VAO / element array buffer binding state interaction. Fixing the new (and now failing test) from the previous commit. In setIndexBuffer(), I was resetting the state tracker to a VAO state that was about to be set in the very next step, and then, when doing that next step the state tracker "optimized away" the state change because it thought it was already done (even though it wasn't). Reordering the two operations fixes it. In comparison to how this was meant to be done in the original 169031fb7bf30e2c98c81cbf865aa5a4662aa716, the new way should do the same but additionally avoid a bunch of redundant state calls. Let's hope no more bugs related to this appear. --- doc/changelog.dox | 3 +++ src/Magnum/GL/Mesh.cpp | 14 ++++++++++---- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 674bcc38c..63cb1b67d 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -111,6 +111,9 @@ See also: - The @cb{.cmake} android_create_apk() @ce macro for @ref platforms-android-apps-cmake "Gradle-less Android builds" now properly creates the library output directory beforehand +- Fixed a bad interaction of @ref GL::Mesh and @ref GL::Buffer state trackers + resulting in corrupted rendering on ES/WebGL platforms and systems without + @gl_extension{ARB,direct_state_access} / @gl_extension{EXT,direct_state_access} - Updated the `base-qt` bootstrap project to correctly reset state tracker when going from and back to Qt code, and to use the framebuffer provided by Qt instead of @ref GL::defaultFramebuffer (see diff --git a/src/Magnum/GL/Mesh.cpp b/src/Magnum/GL/Mesh.cpp index 09a60d9c3..4ddc5e235 100644 --- a/src/Magnum/GL/Mesh.cpp +++ b/src/Magnum/GL/Mesh.cpp @@ -340,6 +340,11 @@ Mesh& Mesh::setIndexBuffer(Buffer&& buffer, GLintptr offset, MeshIndexType type, "GL::Mesh::setIndexBuffer(): the buffer has unexpected target hint, expected" << Buffer::TargetHint::ElementArray << "but got" << buffer.targetHint(), *this); #endif + /* It's IMPORTANT to do this *before* the _indexBuffer is set, since the + bindVAO() function called from here is resetting element buffer state + tracker to _indexBuffer.id(). */ + (this->*Context::current().state().mesh->bindIndexBufferImplementation)(buffer); + _indexBuffer = std::move(buffer); _indexOffset = offset; _indexType = type; @@ -350,7 +355,6 @@ Mesh& Mesh::setIndexBuffer(Buffer&& buffer, GLintptr offset, MeshIndexType type, static_cast(start); static_cast(end); #endif - (this->*Context::current().state().mesh->bindIndexBufferImplementation)(_indexBuffer); return *this; } @@ -547,9 +551,11 @@ void Mesh::bindVAO() { /* Reset element buffer binding, because binding a different VAO with a different index buffer will change that binding as well. (GL state, - what the hell.) We could also theoretically reset the binding - directly to _indexBuffer->id(), but let's play safe and force the - rebind every time. */ + what the hell.). The _indexBuffer.id() is the index buffer that's + already attached to this particular VAO (or 0, if there's none). In + particular, the setIndexBuffer() buffers call this function *and + then* sets the _indexBuffer, which means at this point the ID will + be still 0. */ Context::current().state().buffer->bindings[Implementation::BufferState::indexForTarget(Buffer::TargetHint::ElementArray)] = _indexBuffer.id(); } }