From df8b6f7ef59b80a4616b0ae84e3bc4507689e02f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 25 Jan 2023 22:50:31 +0100 Subject: [PATCH] Shaders: fix ObjectId output tests on WebGL. Apparently a framebuffer attachment can be bound only if it's actually written to by a shader, thus instanced test cases that had it set up always were failing with a GL error if the shader didn't have ObjectId output enabled. I unified both into a single renderSetup()/renderTeardown() routine, where the ObjectId renderbuffer is attached always, but then only the test cases that actually use it are mapping it explicitly. And clearing as well, because apparently it can be cleared only if it's mapped. Huh. --- src/Magnum/Shaders/Test/FlatGLTest.cpp | 143 +++++++++++++++--------- src/Magnum/Shaders/Test/PhongGLTest.cpp | 131 ++++++++++++---------- 2 files changed, 165 insertions(+), 109 deletions(-) diff --git a/src/Magnum/Shaders/Test/FlatGLTest.cpp b/src/Magnum/Shaders/Test/FlatGLTest.cpp index 1c7a8d5ab..b8abceffd 100644 --- a/src/Magnum/Shaders/Test/FlatGLTest.cpp +++ b/src/Magnum/Shaders/Test/FlatGLTest.cpp @@ -153,9 +153,6 @@ struct FlatGLTest: GL::OpenGLTester { template void renderAlpha3D(); #ifndef MAGNUM_TARGET_GLES2 - void renderObjectIdSetup(); - void renderObjectIdTeardown(); - template void renderObjectId2D(); template void renderObjectId3D(); #endif @@ -961,8 +958,8 @@ FlatGLTest::FlatGLTest() { &FlatGLTest::renderObjectId3D, &FlatGLTest::renderObjectId3D}, Containers::arraySize(RenderObjectIdData), - &FlatGLTest::renderObjectIdSetup, - &FlatGLTest::renderObjectIdTeardown); + &FlatGLTest::renderSetup, + &FlatGLTest::renderTeardown); #endif #ifndef MAGNUM_TARGET_GLES2 @@ -989,14 +986,8 @@ FlatGLTest::FlatGLTest() { #endif }, Containers::arraySize(RenderInstancedData), - #ifndef MAGNUM_TARGET_GLES2 - &FlatGLTest::renderObjectIdSetup, - &FlatGLTest::renderObjectIdTeardown - #else &FlatGLTest::renderSetup, - &FlatGLTest::renderTeardown - #endif - ); + &FlatGLTest::renderTeardown); #ifndef MAGNUM_TARGET_GLES2 /* MSVC needs explicit type due to default template args */ @@ -1013,8 +1004,8 @@ FlatGLTest::FlatGLTest() { addInstancedTests({&FlatGLTest::renderMulti2D, &FlatGLTest::renderMulti3D}, Containers::arraySize(RenderMultiData), - &FlatGLTest::renderObjectIdSetup, - &FlatGLTest::renderObjectIdTeardown); + &FlatGLTest::renderSetup, + &FlatGLTest::renderTeardown); addInstancedTests({&FlatGLTest::renderMultiSkinning2D, &FlatGLTest::renderMultiSkinning3D}, @@ -1646,11 +1637,34 @@ void FlatGLTest::renderSetup() { _framebuffer.attachRenderbuffer(GL::Framebuffer::ColorAttachment{0}, _color) .clear(GL::FramebufferClear::Color) .bind(); + + #ifndef MAGNUM_TARGET_GLES2 + /* If we don't have EXT_gpu_shader4, we likely don't have integer + framebuffers either (Mesa's Zink), so skip setting up integer + attachments to avoid GL errors */ + #ifndef MAGNUM_TARGET_GLES + if(GL::Context::current().isExtensionSupported()) + #endif + { + _objectId = GL::Renderbuffer{}; + _objectId.setStorage(GL::RenderbufferFormat::R32UI, RenderSize); + _framebuffer.attachRenderbuffer(GL::Framebuffer::ColorAttachment{1}, _objectId) + .mapForDraw({ + {FlatGL2D::ColorOutput, GL::Framebuffer::ColorAttachment{0}} + /* ObjectIdOutput is mapped (and cleared) in test cases that + actually draw to it, otherwise it causes an error on WebGL + due to the shader not rendering to all outputs */ + }); + } + #endif } void FlatGLTest::renderTeardown() { _framebuffer = GL::Framebuffer{NoCreate}; _color = GL::Renderbuffer{NoCreate}; + #ifndef MAGNUM_TARGET_GLES2 + _objectId = GL::Renderbuffer{NoCreate}; + #endif } template void FlatGLTest::renderDefaults2D() { @@ -2813,43 +2827,6 @@ template void FlatGLTest::renderAlpha3D() { } #ifndef MAGNUM_TARGET_GLES2 -void FlatGLTest::renderObjectIdSetup() { - GL::Renderer::enable(GL::Renderer::Feature::FaceCulling); - - _color = GL::Renderbuffer{}; - _color.setStorage(GL::RenderbufferFormat::RGBA8, RenderSize); - _framebuffer = GL::Framebuffer{{{}, RenderSize}}; - _framebuffer.attachRenderbuffer(GL::Framebuffer::ColorAttachment{0}, _color) - /* Pick a color that's directly representable on RGBA4 as well to - reduce artifacts (well, and this needs to be consistent with other - tests that *need* to run on WebGL 1) */ - .clearColor(0, 0x111111_rgbf) - .bind(); - - /* If we don't have EXT_gpu_shader4, we likely don't have integer - framebuffers either (Mesa's Zink), so skip setting up integer - attachments to avoid GL errors */ - #ifndef MAGNUM_TARGET_GLES - if(GL::Context::current().isExtensionSupported()) - #endif - { - _objectId = GL::Renderbuffer{}; - _objectId.setStorage(GL::RenderbufferFormat::R32UI, RenderSize); - _framebuffer.attachRenderbuffer(GL::Framebuffer::ColorAttachment{1}, _objectId) - .mapForDraw({ - {FlatGL2D::ColorOutput, GL::Framebuffer::ColorAttachment{0}}, - {FlatGL2D::ObjectIdOutput, GL::Framebuffer::ColorAttachment{1}} - }) - .clearColor(1, Vector4ui{27}); - } -} - -void FlatGLTest::renderObjectIdTeardown() { - _color = GL::Renderbuffer{NoCreate}; - _objectId = GL::Renderbuffer{NoCreate}; - _framebuffer = GL::Framebuffer{NoCreate}; -} - template void FlatGLTest::renderObjectId2D() { auto&& data = RenderObjectIdData[testCaseInstanceId()]; setTestCaseDescription(data.name); @@ -2917,6 +2894,16 @@ template void FlatGLTest::renderObjectId2D() { } } + /* Map ObjectIdOutput so we can draw to it. Mapping it always causes an + error on WebGL when the shader does not render to it; however if not + bound we can't even clear it on WebGL, so it has to be cleared after. */ + _framebuffer + .mapForDraw({ + {FlatGL2D::ColorOutput, GL::Framebuffer::ColorAttachment{0}}, + {FlatGL2D::ObjectIdOutput, GL::Framebuffer::ColorAttachment{1}} + }) + .clearColor(1, Vector4ui{27}); + if(flag == FlatGL2D::Flag{}) { if(data.textureTransformation != Matrix3{}) shader.setTextureMatrix(data.textureTransformation); @@ -3050,6 +3037,16 @@ template void FlatGLTest::renderObjectId3D() { } } + /* Map ObjectIdOutput so we can draw to it. Mapping it always causes an + error on WebGL when the shader does not render to it; however if not + bound we can't even clear it on WebGL, so it has to be cleared after. */ + _framebuffer + .mapForDraw({ + {FlatGL3D::ColorOutput, GL::Framebuffer::ColorAttachment{0}}, + {FlatGL3D::ObjectIdOutput, GL::Framebuffer::ColorAttachment{1}} + }) + .clearColor(1, Vector4ui{27}); + if(flag == FlatGL3D::Flag{}) { if(data.textureTransformation != Matrix3{}) shader.setTextureMatrix(data.textureTransformation); @@ -3586,6 +3583,18 @@ template void FlatGLTest::renderInstanced2D() { } #endif + #ifndef MAGNUM_TARGET_GLES2 + /* Map ObjectIdOutput so we can draw to it. Mapping it always causes an + error on WebGL when the shader does not render to it; however if not + bound we can't even clear it on WebGL, so it has to be cleared after. */ + if(data.flags & FlatGL2D::Flag::ObjectId) _framebuffer + .mapForDraw({ + {FlatGL2D::ColorOutput, GL::Framebuffer::ColorAttachment{0}}, + {FlatGL2D::ObjectIdOutput, GL::Framebuffer::ColorAttachment{1}} + }) + .clearColor(1, Vector4ui{27}); + #endif + if(flag == FlatGL2D::Flag{}) { shader .setColor(data.flags & FlatGL2D::Flag::Textured ? 0xffffff_rgbf : 0xffff00_rgbf) @@ -3900,6 +3909,18 @@ template void FlatGLTest::renderInstanced3D() { } #endif + #ifndef MAGNUM_TARGET_GLES2 + /* Map ObjectIdOutput so we can draw to it. Mapping it always causes an + error on WebGL when the shader does not render to it; however if not + bound we can't even clear it on WebGL, so it has to be cleared after. */ + if(data.flags & FlatGL3D::Flag::ObjectId) _framebuffer + .mapForDraw({ + {FlatGL3D::ColorOutput, GL::Framebuffer::ColorAttachment{0}}, + {FlatGL3D::ObjectIdOutput, GL::Framebuffer::ColorAttachment{1}} + }) + .clearColor(1, Vector4ui{27}); + #endif + if(flag == FlatGL3D::Flag{}) { shader .setColor(data.flags & FlatGL2D::Flag::Textured ? 0xffffff_rgbf : 0xffff00_rgbf) @@ -4505,6 +4526,16 @@ void FlatGLTest::renderMulti2D() { .setObjectId(36363); GL::Buffer drawUniform{GL::Buffer::TargetHint::Uniform, drawData}; + /* Map ObjectIdOutput so we can draw to it. Mapping it always causes an + error on WebGL when the shader does not render to it; however if not + bound we can't even clear it on WebGL, so it has to be cleared after. */ + if(data.flags & FlatGL2D::Flag::ObjectId) _framebuffer + .mapForDraw({ + {FlatGL2D::ColorOutput, GL::Framebuffer::ColorAttachment{0}}, + {FlatGL2D::ObjectIdOutput, GL::Framebuffer::ColorAttachment{1}} + }) + .clearColor(1, Vector4ui{27}); + /* Just one draw, rebinding UBOs each time */ if(data.drawCount == 1) { shader.bindMaterialBuffer(materialUniform, @@ -4850,6 +4881,16 @@ void FlatGLTest::renderMulti3D() { .setObjectId(36363); GL::Buffer drawUniform{GL::Buffer::TargetHint::Uniform, drawData}; + /* Map ObjectIdOutput so we can draw to it. Mapping it always causes an + error on WebGL when the shader does not render to it; however if not + bound we can't even clear it on WebGL, so it has to be cleared after. */ + if(data.flags & FlatGL3D::Flag::ObjectId) _framebuffer + .mapForDraw({ + {FlatGL3D::ColorOutput, GL::Framebuffer::ColorAttachment{0}}, + {FlatGL3D::ObjectIdOutput, GL::Framebuffer::ColorAttachment{1}} + }) + .clearColor(1, Vector4ui{27}); + /* Just one draw, rebinding UBOs each time */ if(data.drawCount == 1) { shader.bindMaterialBuffer(materialUniform, diff --git a/src/Magnum/Shaders/Test/PhongGLTest.cpp b/src/Magnum/Shaders/Test/PhongGLTest.cpp index b6f27eb75..cdd4c05be 100644 --- a/src/Magnum/Shaders/Test/PhongGLTest.cpp +++ b/src/Magnum/Shaders/Test/PhongGLTest.cpp @@ -155,9 +155,6 @@ struct PhongGLTest: GL::OpenGLTester { template void renderAlpha(); #ifndef MAGNUM_TARGET_GLES2 - void renderObjectIdSetup(); - void renderObjectIdTeardown(); - template void renderObjectId(); #endif @@ -1274,8 +1271,8 @@ PhongGLTest::PhongGLTest() { &PhongGLTest::renderObjectId, &PhongGLTest::renderObjectId}, Containers::arraySize(RenderObjectIdData), - &PhongGLTest::renderObjectIdSetup, - &PhongGLTest::renderObjectIdTeardown); + &PhongGLTest::renderSetup, + &PhongGLTest::renderTeardown); #endif /* MSVC needs explicit type due to default template args */ @@ -1306,14 +1303,8 @@ PhongGLTest::PhongGLTest() { &PhongGLTest::renderZeroLights #endif }, - #ifndef MAGNUM_TARGET_GLES2 - &PhongGLTest::renderObjectIdSetup, - &PhongGLTest::renderObjectIdTeardown - #else &PhongGLTest::renderSetup, - &PhongGLTest::renderTeardown - #endif - ); + &PhongGLTest::renderTeardown); #ifndef MAGNUM_TARGET_GLES2 /* MSVC needs explicit type due to default template args */ @@ -1333,14 +1324,8 @@ PhongGLTest::PhongGLTest() { #endif }, Containers::arraySize(RenderInstancedData), - #ifndef MAGNUM_TARGET_GLES2 - &PhongGLTest::renderObjectIdSetup, - &PhongGLTest::renderObjectIdTeardown - #else &PhongGLTest::renderSetup, - &PhongGLTest::renderTeardown - #endif - ); + &PhongGLTest::renderTeardown); #ifndef MAGNUM_TARGET_GLES2 /* MSVC needs explicit type due to default template args */ @@ -1354,8 +1339,8 @@ PhongGLTest::PhongGLTest() { #ifndef MAGNUM_TARGET_GLES2 addInstancedTests({&PhongGLTest::renderMulti}, Containers::arraySize(RenderMultiData), - &PhongGLTest::renderObjectIdSetup, - &PhongGLTest::renderObjectIdTeardown); + &PhongGLTest::renderSetup, + &PhongGLTest::renderTeardown); addInstancedTests({&PhongGLTest::renderMultiSkinning}, Containers::arraySize(RenderMultiSkinningData), @@ -2067,11 +2052,34 @@ void PhongGLTest::renderSetup() { .attachRenderbuffer(GL::Framebuffer::ColorAttachment{0}, _color) .clear(GL::FramebufferClear::Color) .bind(); + + #ifndef MAGNUM_TARGET_GLES2 + /* If we don't have EXT_gpu_shader4, we likely don't have integer + framebuffers either (Mesa's Zink), so skip setting up integer + attachments to avoid GL errors */ + #ifndef MAGNUM_TARGET_GLES + if(GL::Context::current().isExtensionSupported()) + #endif + { + _objectId = GL::Renderbuffer{}; + _objectId.setStorage(GL::RenderbufferFormat::R32UI, RenderSize); + _framebuffer.attachRenderbuffer(GL::Framebuffer::ColorAttachment{1}, _objectId) + .mapForDraw({ + {PhongGL::ColorOutput, GL::Framebuffer::ColorAttachment{0}} + /* ObjectIdOutput is mapped (and cleared) in test cases that + actually draw to it, otherwise it causes an error on WebGL + due to the shader not rendering to all outputs */ + }); + } + #endif } void PhongGLTest::renderTeardown() { _framebuffer = GL::Framebuffer{NoCreate}; _color = GL::Renderbuffer{NoCreate}; + #ifndef MAGNUM_TARGET_GLES2 + _objectId = GL::Renderbuffer{NoCreate}; + #endif } template void PhongGLTest::renderDefaults() { @@ -3321,43 +3329,6 @@ template void PhongGLTest::renderAlpha() { } #ifndef MAGNUM_TARGET_GLES2 -void PhongGLTest::renderObjectIdSetup() { - GL::Renderer::enable(GL::Renderer::Feature::FaceCulling); - - _color = GL::Renderbuffer{}; - _color.setStorage(GL::RenderbufferFormat::RGBA8, RenderSize); - _framebuffer = GL::Framebuffer{{{}, RenderSize}}; - _framebuffer.attachRenderbuffer(GL::Framebuffer::ColorAttachment{0}, _color) - /* Pick a color that's directly representable on RGBA4 as well to - reduce artifacts (well, and this needs to be consistent with other - tests that *need* to run on WebGL 1) */ - .clearColor(0, 0x111111_rgbf) - .bind(); - - /* If we don't have EXT_gpu_shader4, we likely don't have integer - framebuffers either (Mesa's Zink), so skip setting up integer - attachments to avoid GL errors */ - #ifndef MAGNUM_TARGET_GLES - if(GL::Context::current().isExtensionSupported()) - #endif - { - _objectId = GL::Renderbuffer{}; - _objectId.setStorage(GL::RenderbufferFormat::R32UI, RenderSize); - _framebuffer.attachRenderbuffer(GL::Framebuffer::ColorAttachment{1}, _objectId) - .mapForDraw({ - {PhongGL::ColorOutput, GL::Framebuffer::ColorAttachment{0}}, - {PhongGL::ObjectIdOutput, GL::Framebuffer::ColorAttachment{1}} - }) - .clearColor(1, Vector4ui{27}); - } -} - -void PhongGLTest::renderObjectIdTeardown() { - _color = GL::Renderbuffer{NoCreate}; - _objectId = GL::Renderbuffer{NoCreate}; - _framebuffer = GL::Framebuffer{NoCreate}; -} - template void PhongGLTest::renderObjectId() { auto&& data = RenderObjectIdData[testCaseInstanceId()]; setTestCaseDescription(data.name); @@ -3426,6 +3397,16 @@ template void PhongGLTest::renderObjectId() { } } + /* Map ObjectIdOutput so we can draw to it. Mapping it always causes an + error on WebGL when the shader does not render to it; however if not + bound we can't even clear it on WebGL, so it has to be cleared after. */ + _framebuffer + .mapForDraw({ + {PhongGL::ColorOutput, GL::Framebuffer::ColorAttachment{0}}, + {PhongGL::ObjectIdOutput, GL::Framebuffer::ColorAttachment{1}} + }) + .clearColor(1, Vector4ui{27}); + if(flag == PhongGL::Flag{}) { if(data.textureTransformation != Matrix3{}) shader.setTextureMatrix(data.textureTransformation); @@ -3859,6 +3840,18 @@ template void PhongGLTest::renderZeroLights() { shader.bindAmbientTexture(ambient); + #ifndef MAGNUM_TARGET_GLES2 + /* Map ObjectIdOutput so we can draw to it. Mapping it always causes an + error on WebGL when the shader does not render to it; however if not + bound we can't even clear it on WebGL, so it has to be cleared after. */ + _framebuffer + .mapForDraw({ + {PhongGL::ColorOutput, GL::Framebuffer::ColorAttachment{0}}, + {PhongGL::ObjectIdOutput, GL::Framebuffer::ColorAttachment{1}} + }) + .clearColor(1, Vector4ui{27}); + #endif + if(flag == PhongGL::Flag{}) { shader .setAmbientColor(0x9999ff_rgbf) @@ -4388,6 +4381,18 @@ template void PhongGLTest::renderInstanced() { } #endif + #ifndef MAGNUM_TARGET_GLES2 + /* Map ObjectIdOutput so we can draw to it. Mapping it always causes an + error on WebGL when the shader does not render to it; however if not + bound we can't even clear it on WebGL, so it has to be cleared after. */ + if(data.flags & PhongGL::Flag::ObjectId) _framebuffer + .mapForDraw({ + {PhongGL::ColorOutput, GL::Framebuffer::ColorAttachment{0}}, + {PhongGL::ObjectIdOutput, GL::Framebuffer::ColorAttachment{1}} + }) + .clearColor(1, Vector4ui{27}); + #endif + if(flag == PhongGL::Flag{}) { shader .setLightPositions({{-3.0f, -3.0f, 2.0f, 0.0f}, @@ -4940,6 +4945,16 @@ void PhongGLTest::renderMulti() { shader.bindProjectionBuffer(projectionUniform); + /* Map ObjectIdOutput so we can draw to it. Mapping it always causes an + error on WebGL when the shader does not render to it; however if not + bound we can't even clear it on WebGL, so it has to be cleared after. */ + if(data.flags & PhongGL::Flag::ObjectId) _framebuffer + .mapForDraw({ + {PhongGL::ColorOutput, GL::Framebuffer::ColorAttachment{0}}, + {PhongGL::ObjectIdOutput, GL::Framebuffer::ColorAttachment{1}} + }) + .clearColor(1, Vector4ui{27}); + /* Just one draw, rebinding UBOs each time */ if(data.drawCount == 1) { shader.bindMaterialBuffer(materialUniform,