From 14a213f6eb8f785ce523922013a7bf226162b383 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 14 Feb 2018 13:06:42 +0100 Subject: [PATCH] Improve state tracker resetting functionality. --- doc/changelog.dox | 1 + src/Magnum/Context.cpp | 2 ++ src/Magnum/Context.h | 66 +++++++++++++++++++++++++++++----- src/Magnum/Mesh.h | 2 +- src/Magnum/Test/MeshGLTest.cpp | 48 +++++++++++++++++++++++++ 5 files changed, 110 insertions(+), 9 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 3b3d6b7cc..73c7ce5e7 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -346,6 +346,7 @@ See also: @ref Image::release() instead. - @ref Buffer::map() now returns @ref Corrade::Containers::ArrayView instead of a plain pointer for better security +- Improved @ref Context::resetState() to better handle corner cases with VAOs - Graceful handling of broken GL contexts - Behavior of @ref Version::GLES200 and upwards on desktop OpenGL is changed to request an ES dialect of GLSL when used in @ref Shader (instead of a diff --git a/src/Magnum/Context.cpp b/src/Magnum/Context.cpp index ca307bdf1..7e4f5de85 100644 --- a/src/Magnum/Context.cpp +++ b/src/Magnum/Context.cpp @@ -836,6 +836,8 @@ void Context::resetState(const States states) { _state->framebuffer->reset(); if(states & State::Meshes) _state->mesh->reset(); + if(states & State::MeshVao) + _state->mesh->bindVAOImplementation(0); if(states & State::PixelStorage) { _state->renderer->unpackPixelStorage.reset(); diff --git a/src/Magnum/Context.h b/src/Magnum/Context.h index 91f90a4ec..a050593b9 100644 --- a/src/Magnum/Context.h +++ b/src/Magnum/Context.h @@ -178,7 +178,7 @@ class MAGNUM_EXPORT Context { /** * @brief State to reset * - * @see @ref States, @ref resetState() + * @see @ref States, @ref resetState(), @ref opengl-state-tracking */ enum class State: UnsignedInt { /** Reset tracked buffer-related bindings and state */ @@ -190,22 +190,63 @@ class MAGNUM_EXPORT Context { /** Reset tracked mesh-related bindings */ Meshes = 1 << 2, + /** + * Unbind currently bound VAO. + * + * Magnum by default uses VAOs --- each time a @ref Mesh is drawn + * or configured, its VAO is bound, but it is *not* unbound + * afterwards to avoid needless state changes. This may introduce + * problems when using third-party OpenGL code --- it may break + * internal state of a mesh that was used the most recently. + * Similar issue can happen the other way. Calling @ref resetState() + * with @ref State::MeshVao included unbounds any currently bound + * VAO to fix such case. + */ + MeshVao = 1 << 3, + /** Reset tracked pixel storage-related state */ - PixelStorage = 1 << 3, + PixelStorage = 1 << 4, /** Reset tracked renderer-related state */ - Renderer = 1 << 4, + Renderer = 1 << 5, /** Reset tracked shader-related bindings */ - Shaders = 1 << 5, + Shaders = 1 << 6, /** Reset tracked texture-related bindings and state */ - Textures = 1 << 6, + Textures = 1 << 7, #ifndef MAGNUM_TARGET_GLES2 /** Reset tracked transform feedback-related bindings */ - TransformFeedback = 1 << 7 + TransformFeedback = 1 << 8, #endif + + /** + * Reset state on entering section with external OpenGL code. + * + * Resets all state that could cause external code to accidentally + * modify Magnum objects. This includes only @ref State::MeshVao. + */ + EnterExternal = UnsignedInt(State::MeshVao), + + /** + * Reset state on exiting section with external OpenGL code. + * + * Resets Magnum state tracker to avoid being confused by external + * state changes. This resets all states. + */ + ExitExternal = + UnsignedInt(State::Buffers)| + UnsignedInt(State::Framebuffers)| + UnsignedInt(State::Meshes)| + UnsignedInt(State::MeshVao)| + UnsignedInt(State::PixelStorage)| + UnsignedInt(State::Renderer)| + UnsignedInt(State::Shaders)| + UnsignedInt(State::Textures) + #ifndef MAGNUM_TARGET_GLES2 + |UnsignedInt(State::TransformFeedback) + #endif }; /** @@ -554,12 +595,21 @@ class MAGNUM_EXPORT Context { /** * @brief Reset internal state tracker - * @param states Tracked states to reset. Default is all state. * * The engine internally tracks object bindings and other state to * avoid redundant OpenGL calls. In some cases (e.g. when non-Magnum * code makes GL calls) the internal tracker no longer reflects actual - * state and needs to be reset to avoid strange issues. + * state. Equivalently the third party code can cause accidental + * modifications of Magnum objects. It's thus advised to call this + * function as a barrier between Magnum code and third-party GL code. + * + * The default, when calling this function with no parameters, will + * reset all state. That's the safest option, but may have considerable + * performance impact when third-party and Magnum code is combined very + * often. For greater control it's possible to reset only particular + * states from the @ref State enum. + * + * See also @ref opengl-state-tracking for more information. */ void resetState(States states = ~States{}); diff --git a/src/Magnum/Mesh.h b/src/Magnum/Mesh.h index 0dcaf3d38..bf4f833d3 100644 --- a/src/Magnum/Mesh.h +++ b/src/Magnum/Mesh.h @@ -1044,7 +1044,7 @@ class MAGNUM_EXPORT Mesh: public AbstractObject { #endif /* Unconditionally binds a specified VAO and updates the state tracker. - Used also in Buffer::bindSomewhereInternal(). */ + Used also in Buffer::bindSomewhereInternal() and Context::resetState(). */ static void MAGNUM_LOCAL bindVAOImplementationDefault(GLuint id); static void MAGNUM_LOCAL bindVAOImplementationVAO(GLuint id); diff --git a/src/Magnum/Test/MeshGLTest.cpp b/src/Magnum/Test/MeshGLTest.cpp index a60c4574d..7d78f3a99 100644 --- a/src/Magnum/Test/MeshGLTest.cpp +++ b/src/Magnum/Test/MeshGLTest.cpp @@ -113,6 +113,7 @@ struct MeshGLTest: OpenGLTester { void setIndexBufferUnsignedInt(); void unbindVAOWhenSettingIndexBufferData(); + void unbindVAOBeforeEnteringExternalSection(); #ifndef MAGNUM_TARGET_GLES void setBaseVertex(); @@ -209,6 +210,7 @@ MeshGLTest::MeshGLTest() { &MeshGLTest::setIndexBufferUnsignedInt, &MeshGLTest::unbindVAOWhenSettingIndexBufferData, + &MeshGLTest::unbindVAOBeforeEnteringExternalSection, #ifndef MAGNUM_TARGET_GLES &MeshGLTest::setBaseVertex, @@ -1741,6 +1743,52 @@ void MeshGLTest::unbindVAOWhenSettingIndexBufferData() { CORRADE_COMPARE(value, 92); } +void MeshGLTest::unbindVAOBeforeEnteringExternalSection() { + #ifndef MAGNUM_TARGET_GLES + if(!Context::current().isExtensionSupported()) + CORRADE_SKIP(Extensions::GL::ARB::vertex_array_object::string() + std::string(" is not available.")); + #elif defined(MAGNUM_TARGET_GLES2) + if(!Context::current().isExtensionSupported()) + CORRADE_SKIP(Extensions::GL::OES::vertex_array_object::string() + std::string(" is not available.")); + #endif + + typedef Attribute<0, Float> Attribute; + + const Float data[] = { -0.7f, Math::unpack(92), Math::unpack(32) }; + Buffer buffer{Buffer::TargetHint::Array}; + buffer.setData(data, BufferUsage::StaticDraw); + + Buffer indices{Buffer::TargetHint::ElementArray}; + indices.setData(std::vector{5, 0}, BufferUsage::StaticDraw); + + Mesh mesh; + mesh.addVertexBuffer(buffer, 4, Attribute{}) + .setIndexBuffer(indices, 0, Mesh::IndexType::UnsignedByte); + + { + /* Comment this out to watch the world burn */ + Context::current().resetState(Context::State::MeshVao); + + glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0); + + /* Be nice to the other tests */ + Context::current().resetState(Context::State::ExitExternal); + } + + MAGNUM_VERIFY_NO_ERROR(); + + const auto value = Checker(FloatShader("float", "vec4(valueInterpolated, 0.0, 0.0, 0.0)"), + #ifndef MAGNUM_TARGET_GLES2 + RenderbufferFormat::RGBA8, + #else + RenderbufferFormat::RGBA4, + #endif + mesh).get(PixelFormat::RGBA, PixelType::UnsignedByte); + + MAGNUM_VERIFY_NO_ERROR(); + CORRADE_COMPARE(value, 92); +} + #ifndef MAGNUM_TARGET_GLES void MeshGLTest::setBaseVertex() { if(!Context::current().isExtensionSupported())