diff --git a/doc/changelog.dox b/doc/changelog.dox index e648d3d87..c36eac040 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -139,6 +139,11 @@ See also: - A new @cpp "android-generic-hostname-might-be-swiftshader" @ce workaround needed for testing against Android Emulator SwiftShader GPU. See @ref opengl-workarounds for more information. +- A new @cpp "angle-instanced-attributes-always-draw-instanced" @ce + workaround for ANGLE's draw validation bug, where it complained about + instanced buffers being too small when drawing such mesh as non-instanced. + See @ref opengl-workarounds for more information, see also + [mosra/magnum#539](https://github.com/mosra/magnum/pull/539). @subsubsection changelog-latest-new-math Math library diff --git a/src/Magnum/GL/Implementation/MeshState.cpp b/src/Magnum/GL/Implementation/MeshState.cpp index f9719984c..b635f441b 100644 --- a/src/Magnum/GL/Implementation/MeshState.cpp +++ b/src/Magnum/GL/Implementation/MeshState.cpp @@ -42,6 +42,7 @@ MeshState::MeshState(Context& context, ContextState& contextState, Containers::S , maxElementIndex{0}, maxElementsIndices{0}, maxElementsVertices{0} #endif { + /* Vertex array object implementation */ #ifndef MAGNUM_TARGET_GLES if(context.isExtensionSupported()) #elif defined(MAGNUM_TARGET_GLES2) @@ -87,7 +88,18 @@ MeshState::MeshState(Context& context, ContextState& contextState, Containers::S #endif { createImplementation = &Mesh::createImplementationVAO; - attributePointerImplementation = &Mesh::attributePointerImplementationVAO; + + /* ANGLE is ... also special! Equivalently below for the non-VAO + case. */ + #ifdef MAGNUM_TARGET_GLES + if((context.detectedDriver() & Context::DetectedDriver::Angle) && !context.isDriverWorkaroundDisabled("angle-instanced-attributes-always-draw-instanced"_s)) { + attributePointerImplementation = &Mesh::attributePointerImplementationVAOAngleAlwaysInstanced; + } else + #endif + { + attributePointerImplementation = &Mesh::attributePointerImplementationVAO; + } + bindIndexBufferImplementation = &Mesh::bindIndexBufferImplementationVAO; } @@ -105,7 +117,17 @@ MeshState::MeshState(Context& context, ContextState& contextState, Containers::S moveConstructImplementation = &Mesh::moveConstructImplementationDefault; moveAssignImplementation = &Mesh::moveAssignImplementationDefault; destroyImplementation = &Mesh::destroyImplementationDefault; - attributePointerImplementation = &Mesh::attributePointerImplementationDefault; + + /* ANGLE is ... also special! Equivalently above for the VAO case. */ + #ifdef MAGNUM_TARGET_GLES + if((context.detectedDriver() & Context::DetectedDriver::Angle) && !context.isDriverWorkaroundDisabled("angle-instanced-attributes-always-draw-instanced"_s)) { + attributePointerImplementation = &Mesh::attributePointerImplementationDefaultAngleAlwaysInstanced; + } else + #endif + { + attributePointerImplementation = &Mesh::attributePointerImplementationDefault; + } + acquireVertexBufferImplementation = &Mesh::acquireVertexBufferImplementationDefault; bindIndexBufferImplementation = &Mesh::bindIndexBufferImplementationDefault; bindVAOImplementation = &Mesh::bindVAOImplementationDefault; diff --git a/src/Magnum/GL/Implementation/driverSpecific.cpp b/src/Magnum/GL/Implementation/driverSpecific.cpp index f224cf568..52fdff88f 100644 --- a/src/Magnum/GL/Implementation/driverSpecific.cpp +++ b/src/Magnum/GL/Implementation/driverSpecific.cpp @@ -64,6 +64,18 @@ constexpr Containers::StringView KnownWorkarounds[]{ "angle-chatty-shader-compiler"_s, #endif +#ifdef MAGNUM_TARGET_GLES +/* ANGLE has a buggy bounds validation when drawing a mesh with instanced + attributes added (with divisor set) using non-instanced glDraw*() APIs (in + particular, when instance count is 1). This should be allowed according to + the GL spec, which describes e.g. glDrawElements() as a special case of + the "virtual" glDrawElementsOneInstance(). To work around the validation + bug, gl*Draw*Instanced() is used unconditionally for all meshes that have + instanced attributes. A test that triggers this issue is in + MeshGLTest::drawInstancedAttributeSingleInstance(). */ +"angle-instanced-attributes-always-draw-instanced"_s, +#endif + #if defined(CORRADE_TARGET_APPLE) && !defined(MAGNUM_TARGET_GLES) /* Calling glBufferData(), glMapBuffer(), glMapBufferRange() or glUnmapBuffer() on ANY buffer when ANY buffer is attached to a currently bound diff --git a/src/Magnum/GL/Mesh.cpp b/src/Magnum/GL/Mesh.cpp index dab4e7629..f367f1af4 100644 --- a/src/Magnum/GL/Mesh.cpp +++ b/src/Magnum/GL/Mesh.cpp @@ -792,7 +792,13 @@ void Mesh::drawInternal(Int count, Int baseVertex, Int instanceCount, GLintptr i (this->*state.bindImplementation)(); /* Non-instanced mesh */ - if(instanceCount == 1) { + if(instanceCount == 1 + #ifdef MAGNUM_TARGET_GLES + /* See the "angle-instanced-attributes-always-draw-instanced" + workaround */ + && !_instanced + #endif + ) { /* Non-indexed mesh */ if(!_indexBuffer.id()) { glDrawArrays(GLenum(_primitive), baseVertex, count); @@ -1170,6 +1176,19 @@ void Mesh::attributePointerImplementationVAODSAIntelWindows(AttributeLayout&& at #endif #endif +#ifdef MAGNUM_TARGET_GLES +/* See the "angle-instanced-attributes-always-draw-instanced" workaround for + these two */ +void Mesh::attributePointerImplementationDefaultAngleAlwaysInstanced(AttributeLayout&& attribute) { + if(attribute.divisor) _instanced = true; + return attributePointerImplementationDefault(std::move(attribute)); +} +void Mesh::attributePointerImplementationVAOAngleAlwaysInstanced(AttributeLayout&& attribute) { + if(attribute.divisor) _instanced = true; + return attributePointerImplementationVAO(std::move(attribute)); +} +#endif + void Mesh::vertexAttribPointer(AttributeLayout& attribute) { glEnableVertexAttribArray(attribute.location); attribute.buffer.bindInternal(Buffer::TargetHint::Array); diff --git a/src/Magnum/GL/Mesh.h b/src/Magnum/GL/Mesh.h index 2952201b3..33d90f89c 100644 --- a/src/Magnum/GL/Mesh.h +++ b/src/Magnum/GL/Mesh.h @@ -1278,6 +1278,10 @@ class MAGNUM_GL_EXPORT Mesh: public AbstractObject { void MAGNUM_GL_LOCAL attributePointerImplementationVAODSAIntelWindows(AttributeLayout&& attribute); #endif #endif + #ifdef MAGNUM_TARGET_GLES + void MAGNUM_GL_LOCAL attributePointerImplementationDefaultAngleAlwaysInstanced(AttributeLayout&& attribute); + void MAGNUM_GL_LOCAL attributePointerImplementationVAOAngleAlwaysInstanced(AttributeLayout&& attribute); + #endif void MAGNUM_GL_LOCAL vertexAttribPointer(AttributeLayout& attribute); #ifndef MAGNUM_TARGET_GLES @@ -1358,6 +1362,10 @@ class MAGNUM_GL_EXPORT Mesh: public AbstractObject { /* Whether the _attributes storage was constructed (it's not when the object is constructed using NoCreate). Also fits in the gap. */ bool _constructed{}; + #ifdef MAGNUM_TARGET_GLES + /* See the "angle-instanced-attributes-always-draw-instanced" workaround */ + bool _instanced{}; + #endif Int _count{}, _baseVertex{}, _instanceCount{1}; #ifndef MAGNUM_TARGET_GLES2 UnsignedInt _baseInstance{}; diff --git a/src/Magnum/GL/Test/MeshGLTest.cpp b/src/Magnum/GL/Test/MeshGLTest.cpp index e173c93ba..8658b4913 100644 --- a/src/Magnum/GL/Test/MeshGLTest.cpp +++ b/src/Magnum/GL/Test/MeshGLTest.cpp @@ -3,6 +3,7 @@ Copyright © 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021 Vladimír Vondruš + Copyright © 2022 Pablo Escobar Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), @@ -182,6 +183,7 @@ struct MeshGLTest: OpenGLTester { void addVertexBufferInstancedDouble(); #endif void resetDivisorAfterInstancedDraw(); + void drawInstancedAttributeSingleInstance(); void multiDraw(); void multiDrawSparseArrays(); @@ -670,7 +672,8 @@ MeshGLTest::MeshGLTest() { #ifndef MAGNUM_TARGET_GLES &MeshGLTest::addVertexBufferInstancedDouble, #endif - &MeshGLTest::resetDivisorAfterInstancedDraw}); + &MeshGLTest::resetDivisorAfterInstancedDraw, + &MeshGLTest::drawInstancedAttributeSingleInstance}); addInstancedTests({&MeshGLTest::multiDraw, &MeshGLTest::multiDrawSparseArrays, @@ -3704,6 +3707,66 @@ void MeshGLTest::resetDivisorAfterInstancedDraw() { } } +void MeshGLTest::drawInstancedAttributeSingleInstance() { + #ifndef MAGNUM_TARGET_GLES + if(!Context::current().isExtensionSupported()) + CORRADE_SKIP(Extensions::ARB::draw_instanced::string() << "is not supported."); + if(!Context::current().isExtensionSupported()) + CORRADE_SKIP(Extensions::ARB::instanced_arrays::string() << "is not supported."); + #elif defined(MAGNUM_TARGET_GLES2) + #ifndef MAGNUM_TARGET_WEBGL + if(!Context::current().isExtensionSupported() && + !Context::current().isExtensionSupported() && + !Context::current().isExtensionSupported()) + CORRADE_SKIP("Required extension is not supported."); + #else + if(!Context::current().isExtensionSupported()) + CORRADE_SKIP(Extensions::ANGLE::instanced_arrays::string() << "is not supported."); + #endif + #endif + + typedef Attribute<0, Float> Attribute; + + const Float data[]{ + Math::unpack(96) + }; + Buffer buffer; + /* The ANGLE validation error can be only reproduced with DynamicDraw used + here, not StaticDraw. Interesting. */ + buffer.setData(data, BufferUsage::DynamicDraw); + + Renderbuffer renderbuffer; + renderbuffer.setStorage( + #ifndef MAGNUM_TARGET_GLES2 + RenderbufferFormat::RGBA8, + #else + RenderbufferFormat::RGBA4, + #endif + Vector2i(1)); + Framebuffer framebuffer{{{}, Vector2i(1)}}; + framebuffer.attachRenderbuffer(Framebuffer::ColorAttachment{0}, renderbuffer) + .bind(); + + FloatShader shader{"float", "vec4(valueInterpolated, 0.0, 0.0, 0.0)"}; + + MAGNUM_VERIFY_NO_GL_ERROR(); + + /* Create a mesh with (implicitly) one instance and the buffer added as + instanced. Drawing it 16 times should always draw 96 with no error. + On ANGLE w/o the "angle-instanced-attributes-always-draw-instanced" + workaround this would trigger a validation error where it would complain + that the 4-byte buffer is not large enough to draw 16 vertices. */ + Mesh mesh; + mesh.addVertexBufferInstanced(buffer, 1, 0, Attribute{}) + .setPrimitive(MeshPrimitive::Points) + .setCount(16); + shader.draw(mesh); + + MAGNUM_VERIFY_NO_GL_ERROR(); + + CORRADE_COMPARE(Containers::arrayCast(framebuffer.read({{}, Vector2i{1}}, {PixelFormat::RGBA, PixelType::UnsignedByte}).data())[0], 96); +} + struct MultiDrawShader: AbstractShaderProgram { typedef Attribute<0, Vector2> Position; typedef Attribute<1, Vector4> Value;