From c0c712cc332b859ad2db59c2c1c86817d94863f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 6 Sep 2022 18:30:15 +0200 Subject: [PATCH] GL: deprecate list-taking Shader::compile() and ShaderProgram::link(). There's no reason for those to exist anymore -- origiinally they were added in a hopeful attempt to make use of parallel shader compilation, but in practice that meant compiling at most two or three shaders at once and still stalling until that was done, so not that great at all. The new APIs provide much better opportunities for parallelism. Fun fact: CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && frag.compile()); is actually one character shorter than CORRADE_INTERNAL_ASSERT_OUTPUT(GL::Shader::compile({vert, frag})); so not even typing convenience would be a reason to keep these. --- doc/changelog.dox | 10 +++++++ doc/snippets/MagnumGL.cpp | 4 +-- src/Magnum/DebugTools/TextureImage.cpp | 2 +- src/Magnum/GL/AbstractShaderProgram.cpp | 7 ++++- src/Magnum/GL/AbstractShaderProgram.h | 18 ++++++++---- src/Magnum/GL/Shader.cpp | 9 +++++- src/Magnum/GL/Shader.h | 23 +++++++++------ .../GL/Test/AbstractShaderProgramGLTest.cpp | 28 ++++++++++--------- src/Magnum/GL/Test/MeshGLTest.cpp | 12 ++++---- src/Magnum/GL/Test/RendererGLTest.cpp | 2 +- src/Magnum/GL/Test/SampleQueryGLTest.cpp | 2 +- .../GL/Test/TransformFeedbackGLTest.cpp | 5 ++-- .../Test/FullScreenTriangleGLTest.cpp | 2 +- src/Magnum/TextureTools/DistanceField.cpp | 2 +- 14 files changed, 83 insertions(+), 43 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 404b267b3..676d9749c 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -821,6 +821,16 @@ See also: @ref DebugTools::FrameProfilerGL. The new name plays better with IDE autocompletion and makes the GL-specific class appear next to the API-independent base in alphabetically sorted lists. +- List-taking @cpp GL::Shader::compile() @ce and + @cpp GL::AbstractShaderProgram::link() @ce functions are deprecated in + favor of new @ref GL::Shader::submitCompile(), + @relativeref{GL::Shader,checkCompile()}, + @ref GL::AbstractShaderProgram::submitLink() and + @relativeref{GL::AbstractShaderProgram,checkLink()} APIs. These were + originally meant to make use of parallel shader compilation, but in + practice that meant compiling at most two or three shaders at once. The new + API allows for much larger parallelism as well as an ability to query + completion status. - @cpp Shaders::DistanceFieldVector @ce, @cpp Shaders::Flat @ce, @cpp Shaders::Generic @ce, @cpp Shaders::MeshVisualizer2D @ce, @cpp Shaders::MeshVisualizer3D @ce, @cpp Shaders::Phong @ce, diff --git a/doc/snippets/MagnumGL.cpp b/doc/snippets/MagnumGL.cpp index 98755e21a..26a25311f 100644 --- a/doc/snippets/MagnumGL.cpp +++ b/doc/snippets/MagnumGL.cpp @@ -147,8 +147,8 @@ explicit MyShader() { vert.addFile("MyShader.vert"); frag.addFile("MyShader.frag"); - /* Invoke parallel compilation for best performance */ - CORRADE_INTERNAL_ASSERT_OUTPUT(GL::Shader::compile({vert, frag})); + /* Compile them */ + CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && frag.compile()); /* Attach the shaders */ attachShaders({vert, frag}); diff --git a/src/Magnum/DebugTools/TextureImage.cpp b/src/Magnum/DebugTools/TextureImage.cpp index 64987f240..253394413 100644 --- a/src/Magnum/DebugTools/TextureImage.cpp +++ b/src/Magnum/DebugTools/TextureImage.cpp @@ -87,7 +87,7 @@ FloatReinterpretShader::FloatReinterpretShader() { vert.addSource(rs.getString("TextureImage.vert")); frag.addSource(rs.getString("TextureImage.frag")); - CORRADE_INTERNAL_ASSERT_OUTPUT(GL::Shader::compile({vert, frag})); + CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && frag.compile()); attachShaders({vert, frag}); if(!GL::Context::current().isExtensionSupported()) { diff --git a/src/Magnum/GL/AbstractShaderProgram.cpp b/src/Magnum/GL/AbstractShaderProgram.cpp index c26f0cab9..dcc1b4915 100644 --- a/src/Magnum/GL/AbstractShaderProgram.cpp +++ b/src/Magnum/GL/AbstractShaderProgram.cpp @@ -586,7 +586,10 @@ void AbstractShaderProgram::transformFeedbackVaryingsImplementationDanglingWorka #endif #endif -bool AbstractShaderProgram::link() { return link({*this}); } +bool AbstractShaderProgram::link() { + submitLink(); + return checkLink({}); +} void AbstractShaderProgram::submitLink() { glLinkProgram(_id); @@ -637,12 +640,14 @@ bool AbstractShaderProgram::checkLink(const Containers::Iterable shaders return success; } +#ifdef MAGNUM_BUILD_DEPRECATED bool AbstractShaderProgram::link(std::initializer_list> shaders) { for(AbstractShaderProgram& shader: shaders) shader.submitLink(); bool allSuccess = true; for(AbstractShaderProgram& shader: shaders) allSuccess = allSuccess && shader.checkLink({}); return allSuccess; } +#endif bool AbstractShaderProgram::isLinkFinished() { GLint success; diff --git a/src/Magnum/GL/AbstractShaderProgram.h b/src/Magnum/GL/AbstractShaderProgram.h index 10b804461..c01eb6e6f 100644 --- a/src/Magnum/GL/AbstractShaderProgram.h +++ b/src/Magnum/GL/AbstractShaderProgram.h @@ -43,6 +43,7 @@ #endif #ifdef MAGNUM_BUILD_DEPRECATED +#include /* For label() / setLabel(), which used to be a std::string */ #include #endif @@ -1345,15 +1346,22 @@ class MAGNUM_GL_EXPORT AbstractShaderProgram: public AbstractObject { bool isLinkFinished(); protected: - /** @brief Link the shaders + #ifdef MAGNUM_BUILD_DEPRECATED + /** + * @brief Link multiple shaders simultaenously + * @m_deprecated_since_latest Originally meant to batch multiple link + * operations together in a way that allowed the driver to perform + * the linking in multiple threads. Superseded by @ref submitLink() + * and @ref checkLink(), use either those or the zero-argument + * @ref link() instead. See @ref GL-AbstractShaderProgram-async + * for more information. * * Calls @ref submitLink() on all shaders first, then @ref checkLink(). * Returns @cpp false @ce if linking of any shader failed, @cpp true @ce - * if everything succeeded. The operation is batched in a - * way that allows the driver to link multiple shaders simultaneously - * (i.e. in multiple threads). + * if everything succeeded. */ - static bool link(std::initializer_list> shaders); + static CORRADE_DEPRECATED("use either submitLink() and checkLink() or the zero-argument link() instead") bool link(std::initializer_list> shaders); + #endif #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) /** diff --git a/src/Magnum/GL/Shader.cpp b/src/Magnum/GL/Shader.cpp index 9966b75cf..40e5cf2dc 100644 --- a/src/Magnum/GL/Shader.cpp +++ b/src/Magnum/GL/Shader.cpp @@ -27,7 +27,9 @@ #include "Shader.h" #include +#ifdef MAGNUM_BUILD_DEPRECATED #include +#endif #ifndef MAGNUM_TARGET_WEBGL #include #endif @@ -748,7 +750,10 @@ Shader& Shader::addFile(const std::string& filename) { return *this; } -bool Shader::compile() { return compile({*this}); } +bool Shader::compile() { + submitCompile(); + return checkCompile(); +} void Shader::submitCompile() { CORRADE_ASSERT(_sources.size() > 1, "GL::Shader::compile(): no files added", ); @@ -808,6 +813,7 @@ bool Shader::checkCompile() { return success; } +#ifdef MAGNUM_BUILD_DEPRECATED bool Shader::compile(std::initializer_list> shaders) { /* Invoke (possibly parallel) compilation on all shaders */ for(Shader& shader: shaders) shader.submitCompile(); @@ -815,6 +821,7 @@ bool Shader::compile(std::initializer_list> shader for(Shader& shader: shaders) allSuccess = allSuccess && shader.checkCompile(); return allSuccess; } +#endif bool Shader::isCompileFinished() { GLint success; diff --git a/src/Magnum/GL/Shader.h b/src/Magnum/GL/Shader.h index 5850e0ba6..b9385e877 100644 --- a/src/Magnum/GL/Shader.h +++ b/src/Magnum/GL/Shader.h @@ -39,6 +39,7 @@ #include "Magnum/GL/GL.h" #ifdef MAGNUM_BUILD_DEPRECATED +#include /* For label() / setLabel(), which used to be a std::string. Not ideal for the return type, but at least something. */ #include @@ -508,16 +509,22 @@ class MAGNUM_GL_EXPORT Shader: public AbstractObject { static Int maxCombinedUniformComponents(Type type); #endif + #ifdef MAGNUM_BUILD_DEPRECATED /** * @brief Compile multiple shaders simultaneously - * - * Calls @ref submitCompile() on all shaders first, then @ref checkCompile(). - * Returns @cpp false @ce if compilation of any shader failed, - * @cpp true @ce if everything succeeded. The operation is batched in a way that - * allows the driver to perform multiple compilations simultaneously - * (i.e. in multiple threads). - */ - static bool compile(std::initializer_list> shaders); + * @m_deprecated_since_latest Originally meant to batch multiple + * compile operations together in a way that allowed the driver to + * perform the compilation in multiple threads. Superseded by + * @ref submitCompile() and @ref checkCompile(), use either those + * or the zero-argument @ref compile() instead. See + * @ref GL-AbstractShaderProgram-async for more information. + * + * Calls @ref submitCompile() on all shaders first, then + * @ref checkCompile(). Returns @cpp false @ce if compilation of any + * shader failed, @cpp true @ce if everything succeeded. + */ + static CORRADE_DEPRECATED("use either submitCompile() and checkCompile() or the zero-argument compile() instead") bool compile(std::initializer_list> shaders); + #endif /** * @brief Constructor diff --git a/src/Magnum/GL/Test/AbstractShaderProgramGLTest.cpp b/src/Magnum/GL/Test/AbstractShaderProgramGLTest.cpp index 9b90f75da..201e5fee4 100644 --- a/src/Magnum/GL/Test/AbstractShaderProgramGLTest.cpp +++ b/src/Magnum/GL/Test/AbstractShaderProgramGLTest.cpp @@ -652,7 +652,8 @@ void AbstractShaderProgramGLTest::uniformNotFound() { #endif ); - CORRADE_VERIFY(Shader::compile({vert, frag})); + CORRADE_VERIFY(vert.compile() && frag.compile()); + program.attachShaders({vert, frag}); CORRADE_VERIFY(program.link()); @@ -702,10 +703,10 @@ MyShader::MyShader() { Version::GLES200 #endif , Shader::Type::Fragment); - vert.addSource(rs.getString("MyShader.vert")); - frag.addSource(rs.getString("MyShader.frag")); - - Shader::compile({vert, frag}); + vert.addSource(rs.getString("MyShader.vert")) + .compile(); + frag.addSource(rs.getString("MyShader.frag")) + .compile(); attachShaders({vert, frag}); @@ -786,10 +787,10 @@ MyDoubleShader::MyDoubleShader() { Shader vert(Version::GL320, Shader::Type::Vertex); Shader frag(Version::GL320, Shader::Type::Fragment); - vert.addSource(rs.getString("MyDoubleShader.vert")); - frag.addSource(rs.getString("MyDoubleShader.frag")); - - Shader::compile({vert, frag}); + vert.addSource(rs.getString("MyDoubleShader.vert")) + .compile(); + frag.addSource(rs.getString("MyDoubleShader.frag")) + .compile(); attachShaders({vert, frag}); @@ -948,8 +949,8 @@ void AbstractShaderProgramGLTest::uniformBlockIndexNotFound() { vert.addSource("void main() { gl_Position = vec4(0.0); }"); frag.addSource("out lowp vec4 color;\n" "void main() { color = vec4(1.0); }"); + CORRADE_VERIFY(vert.compile() && frag.compile()); - CORRADE_VERIFY(Shader::compile({vert, frag})); program.attachShaders({vert, frag}); CORRADE_VERIFY(program.link()); @@ -989,10 +990,11 @@ UniformBlockShader::UniformBlockShader() { Version::GLES300 #endif , Shader::Type::Fragment); - vert.addSource(rs.getString("UniformBlockShader.vert")); - frag.addSource(rs.getString("UniformBlockShader.frag")); + vert.addSource(rs.getString("UniformBlockShader.vert")) + .compile(); + frag.addSource(rs.getString("UniformBlockShader.frag")) + .compile(); - Shader::compile({vert, frag}); attachShaders({vert, frag}); link(); diff --git a/src/Magnum/GL/Test/MeshGLTest.cpp b/src/Magnum/GL/Test/MeshGLTest.cpp index 85137729b..a56257f88 100644 --- a/src/Magnum/GL/Test/MeshGLTest.cpp +++ b/src/Magnum/GL/Test/MeshGLTest.cpp @@ -1023,7 +1023,7 @@ FloatShader::FloatShader(const std::string& type, const std::string& conversion) "#endif\n" "void main() { result = " + conversion + "; }\n"); - CORRADE_INTERNAL_ASSERT_OUTPUT(Shader::compile({vert, frag})); + CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && frag.compile()); attachShaders({vert, frag}); @@ -1065,7 +1065,7 @@ IntegerShader::IntegerShader(const std::string& type) { "out mediump " + type + " result;\n" "void main() { result = valueInterpolated; }\n"); - CORRADE_INTERNAL_ASSERT_OUTPUT(Shader::compile({vert, frag})); + CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && frag.compile()); attachShaders({vert, frag}); @@ -1100,7 +1100,7 @@ DoubleShader::DoubleShader(const std::string& type, const std::string& outputTyp "out " + outputType + " result;\n" "void main() { result = valueInterpolated; }\n"); - CORRADE_INTERNAL_ASSERT_OUTPUT(Shader::compile({vert, frag})); + CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && frag.compile()); attachShaders({vert, frag}); @@ -2147,7 +2147,7 @@ MultipleShader::MultipleShader() { "#endif\n" "void main() { result = valueInterpolated; }\n"); - CORRADE_INTERNAL_ASSERT_OUTPUT(Shader::compile({vert, frag})); + CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && frag.compile()); attachShaders({vert, frag}); @@ -3836,7 +3836,7 @@ MultiDrawShader::MultiDrawShader(bool vertexId, bool drawId) { "#endif\n" "void main() { result.r = valueInterpolated; }\n"); - CORRADE_INTERNAL_ASSERT_OUTPUT(Shader::compile({vert, frag})); + CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && frag.compile()); attachShaders({vert, frag}); @@ -4641,7 +4641,7 @@ MultiDrawInstancedShader::MultiDrawInstancedShader(bool vertexId, bool drawId "#endif\n" "void main() { result.r = valueInterpolated; }\n"); - CORRADE_INTERNAL_ASSERT_OUTPUT(Shader::compile({vert, frag})); + CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && frag.compile()); attachShaders({vert, frag}); diff --git a/src/Magnum/GL/Test/RendererGLTest.cpp b/src/Magnum/GL/Test/RendererGLTest.cpp index fd8084748..5b93a64c5 100644 --- a/src/Magnum/GL/Test/RendererGLTest.cpp +++ b/src/Magnum/GL/Test/RendererGLTest.cpp @@ -203,7 +203,7 @@ void RendererGLTest::pointCoord() { color = vec4(gl_PointCoord.x, gl_PointCoord.y, 0.0, 1.0); })GLSL"); - CORRADE_INTERNAL_ASSERT_OUTPUT(Shader::compile({vert, frag})); + CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && frag.compile()); attachShaders({vert, frag}); diff --git a/src/Magnum/GL/Test/SampleQueryGLTest.cpp b/src/Magnum/GL/Test/SampleQueryGLTest.cpp index 1929de2fb..8df0ac97f 100644 --- a/src/Magnum/GL/Test/SampleQueryGLTest.cpp +++ b/src/Magnum/GL/Test/SampleQueryGLTest.cpp @@ -207,7 +207,7 @@ MyShader::MyShader() { " color = vec4(1.0, 1.0, 1.0, 1.0);\n" "}\n"); - CORRADE_INTERNAL_ASSERT_OUTPUT(Shader::compile({vert, frag})); + CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && frag.compile()); attachShaders({vert, frag}); diff --git a/src/Magnum/GL/Test/TransformFeedbackGLTest.cpp b/src/Magnum/GL/Test/TransformFeedbackGLTest.cpp index 25a3b008f..ad6268e87 100644 --- a/src/Magnum/GL/Test/TransformFeedbackGLTest.cpp +++ b/src/Magnum/GL/Test/TransformFeedbackGLTest.cpp @@ -635,7 +635,8 @@ void TransformFeedbackGLTest::draw() { else geom.addSource( " EmitVertex();\n"); geom.addSource("}\n"); - CORRADE_INTERNAL_ASSERT_OUTPUT(Shader::compile({vert, geom})); + CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && geom.compile()); + attachShaders({vert, geom}); setTransformFeedbackOutputs({"geomOutput"}, TransformFeedbackBufferMode::SeparateAttributes); CORRADE_INTERNAL_ASSERT_OUTPUT(link()); @@ -696,8 +697,8 @@ void TransformFeedbackGLTest::draw() { "void main() {\n" " outputData = interleaved.x + 2*interleaved.y;\n" "}\n"); + CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && frag.compile()); - CORRADE_INTERNAL_ASSERT_OUTPUT(Shader::compile({vert, frag})); attachShaders({vert, frag}); bindAttributeLocation(Input::Location, "inputData"); CORRADE_INTERNAL_ASSERT_OUTPUT(link()); diff --git a/src/Magnum/MeshTools/Test/FullScreenTriangleGLTest.cpp b/src/Magnum/MeshTools/Test/FullScreenTriangleGLTest.cpp index 3bed21b7d..ebfab2401 100644 --- a/src/Magnum/MeshTools/Test/FullScreenTriangleGLTest.cpp +++ b/src/Magnum/MeshTools/Test/FullScreenTriangleGLTest.cpp @@ -102,7 +102,7 @@ void main() { } )"); - CORRADE_INTERNAL_ASSERT_OUTPUT(GL::Shader::compile({vert, frag})); + CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && frag.compile()); attachShaders({vert, frag}); diff --git a/src/Magnum/TextureTools/DistanceField.cpp b/src/Magnum/TextureTools/DistanceField.cpp index 9865b7dea..c600dc2af 100644 --- a/src/Magnum/TextureTools/DistanceField.cpp +++ b/src/Magnum/TextureTools/DistanceField.cpp @@ -104,7 +104,7 @@ DistanceFieldShader::DistanceFieldShader(const UnsignedInt radius) { frag.addSource(Utility::formatString("#define RADIUS {}\n", radius)) .addSource(rs.getString("DistanceFieldShader.frag")); - CORRADE_INTERNAL_ASSERT_OUTPUT(GL::Shader::compile({vert, frag})); + CORRADE_INTERNAL_ASSERT_OUTPUT(vert.compile() && frag.compile()); attachShaders({vert, frag});