From ad4ae11de7b53e9c15737cf35bcdd825b94723be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 25 Jul 2023 11:48:28 +0200 Subject: [PATCH] GL: define driver workaround macros in the Shader constructor. As this is now documented, it means 3rd party code can now directly make use of these without having to reinvent the same logic, or worse, rediscover the same driver bugs. The compatibility.glsl file however stays private -- I don't expect real-world projects needing *that much* diversity in their supported GLSL versions, often the baseline is GLES 3.0 which makes a large part of the file unnecessary, and the projects might choose to for example always have implicitly queried uniform locations to not have to maintain two code paths. --- doc/changelog.dox | 3 + src/Magnum/GL/Shader.cpp | 42 ++++++++- src/Magnum/GL/Shader.h | 42 ++++++++- src/Magnum/GL/Test/ShaderGLTest.cpp | 93 +++++++++++-------- .../CreateCompatibilityShader.h | 15 --- 5 files changed, 131 insertions(+), 64 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 93d77e7b8..a78250cd3 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -556,6 +556,9 @@ See also: - @ref GL::Renderer::Feature::DepthClamp is now exposed on OpenGL ES as well, using the @gl_extension{EXT,depth_clamp} extension - Added @ref GL::Shader::wrap() and @relativeref{GL::Shader,release()} +- @ref GL::Shader now adds defines for driver workarounds that affect shader + code. Until now this was done only internally for builtin shaders, now it's + done for all. - New @ref MAGNUM_GL_ABSTRACTSHADERPROGRAM_SUBCLASS_DRAW_IMPLEMENTATION() and @ref MAGNUM_GL_ABSTRACTSHADERPROGRAM_SUBCLASS_DISPATCH_IMPLEMENTATION() macros for easier and more robust implementation of method chaining in diff --git a/src/Magnum/GL/Shader.cpp b/src/Magnum/GL/Shader.cpp index 46dadfe3b..176a423e4 100644 --- a/src/Magnum/GL/Shader.cpp +++ b/src/Magnum/GL/Shader.cpp @@ -684,20 +684,50 @@ Shader::Shader(const Version version, const Type type): _type{type}, _flags{Obje case Version::GLES320: versionString = "#version 320 es\n"_s; break; #endif - /* The user is responsible for (not) adding #version directive */ + /* The user is responsible for (not) adding a #version directive, and + also any workarounds that depend on it */ case Version::None: versionString = ""_s; break; } CORRADE_ASSERT(versionString, "GL::Shader::Shader(): unsupported version" << version, ); - if(*versionString) + if(*versionString) { arrayAppend(_sources, Containers::String::nullTerminatedGlobalView(*versionString)); + + #if !defined(MAGNUM_TARGET_GLES) || !defined(MAGNUM_TARGET_GLES2) + Context& context = Context::current(); + #endif + + /* Supply macros for functionality advertised by the GLSL compiler that + isn't actually working on given version. Search for these macros in + Implementation/driverSpecific.cpp for more information. */ + #ifndef MAGNUM_TARGET_GLES + if(context.isExtensionDisabled(version)) + arrayAppend(_sources, Containers::String::nullTerminatedGlobalView("#define MAGNUM_DISABLE_GL_ARB_explicit_attrib_location\n"_s)); + if(context.isExtensionDisabled(version)) + arrayAppend(_sources, Containers::String::nullTerminatedGlobalView("#define MAGNUM_DISABLE_GL_ARB_shading_language_420pack\n"_s)); + if(context.isExtensionDisabled(version)) + arrayAppend(_sources, Containers::String::nullTerminatedGlobalView("#define MAGNUM_DISABLE_GL_ARB_explicit_uniform_location\n"_s)); + #endif + + #ifndef MAGNUM_TARGET_GLES2 + if(type == Type::Vertex && context.isExtensionDisabled(version)) + arrayAppend(_sources, Containers::String::nullTerminatedGlobalView("#define MAGNUM_DISABLE_GL_MAGNUM_shader_vertex_id\n"_s)); + #endif + + /* Remember how many initial sources were added to have consistent + #line numbering later */ + _fileIndexOffset = _sources.size() - 1; + } else { + _fileIndexOffset = 0; + } } -Shader::Shader(const Type type, const GLuint id, ObjectFlags flags) noexcept: _type{type}, _id{id}, _flags{flags} +Shader::Shader(const Type type, const GLuint id, ObjectFlags flags) noexcept: _type{type}, _id{id}, _flags{flags}, #ifndef MAGNUM_TARGET_GLES - , _offsetLineByOneOnOldGlsl{} + _offsetLineByOneOnOldGlsl{}, #endif + _fileIndexOffset{} {} Shader::Shader(NoCreateT) noexcept: _type{}, _id{0} {} @@ -706,6 +736,7 @@ Shader::Shader(Shader&& other) noexcept: _type{other._type}, _id{other._id}, _fl #ifndef MAGNUM_TARGET_GLES _offsetLineByOneOnOldGlsl{other._flags}, #endif + _fileIndexOffset{other._fileIndexOffset}, _sources{std::move(other._sources)} { other._id = 0; @@ -726,6 +757,7 @@ Shader& Shader::operator=(Shader&& other) noexcept { #ifndef MAGNUM_TARGET_GLES swap(_offsetLineByOneOnOldGlsl, other._offsetLineByOneOnOldGlsl); #endif + swap(_fileIndexOffset, other._fileIndexOffset); swap(_sources, other._sources); return *this; } @@ -778,7 +810,7 @@ Shader& Shader::addSourceInternal(Containers::String&& source) { _offsetLineByOneOnOldGlsl ? "#line 0 {}\n" : #endif "#line 1 {}\n", - (_sources.size() + 1)/2)); + (_sources.size() + 1 - _fileIndexOffset)/2)); else arrayAppend(_sources, Containers::String::nullTerminatedGlobalView(""_s)); Context::current().state().shader.addSourceImplementation(*this, std::move(source)); diff --git a/src/Magnum/GL/Shader.h b/src/Magnum/GL/Shader.h index 965f4d905..c5b608f6d 100644 --- a/src/Magnum/GL/Shader.h +++ b/src/Magnum/GL/Shader.h @@ -50,7 +50,41 @@ namespace Implementation { struct ShaderState; } /** @brief Shader -See @ref AbstractShaderProgram for usage information. +See @ref AbstractShaderProgram for high-level usage information. + +@section GL-Shader-workarounds Driver workarounds + +Some driver workarounds used by Magnum affect also shader code, and the class +is implicitly defining the following macros depending on the @ref Version +passed to the constructor: + +- @cpp #define MAGNUM_DISABLE_GL_ARB_explicit_attrib_location @ce if the + @gl_extension{ARB,explicit_attrib_location} desktop extension is reported + as supported by the shader compiler but isn't usable on given GLSL version. + In this case @ref GL::Context::isExtensionSupported(Version) const also + returns @cpp false @ce for this extension to be able to make appropriate + adjustments during shader compilation. +- @cpp #define MAGNUM_DISABLE_GL_ARB_shading_language_420pack @ce if the + @gl_extension{ARB,shading_language_420pack} desktop extension is reported + as supported by the shader compiler but isn't usable on given GLSL version. + In this case @ref GL::Context::isExtensionSupported(Version) const also + returns @cpp false @ce for this extension to be able to make appropriate + adjustments during shader compilation. +- @cpp #define MAGNUM_DISABLE_GL_ARB_explicit_uniform_location @ce if the + @gl_extension{ARB,explicit_attrib_location} desktop extension is reported + as supported by the shader compiler but isn't usable on given GLSL version. + In this case @ref GL::Context::isExtensionSupported(Version) const also + returns @cpp false @ce for this extension to be able to make appropriate + adjustments during shader compilation. +- @cpp #define MAGNUM_DISABLE_GL_MAGNUM_shader_vertex_id @ce if the + @glsl gl_VertexID @ce builtin should be present on given GLSL version but + isn't working correctly. You can use the artificial + `GL_MAGNUM_shader_vertex_id` desktop, ES and WebGL extension to check for + this case in @ref GL::Context::isExtensionSupported(Version) const. + +See @ref opengl-workarounds for concrete information about driver workarounds +used. If @ref Version::None is passed to the constructor, none of the above +defines are added. @section GL-Shader-errors Compilation error reporting @@ -64,8 +98,9 @@ error location, usually in a form `():` or `::`. Unfortunately GLSL only allows specifying a file *number*, not a name. Source number `0` is a @glsl #version @ce directive added in the constructor -(unless @ref Version::None is specified). which means the first added source -has a number `1`. +together with potential workaround defines listed above, which means the first +added source has a number `1`. If @ref Version::None is specified, the first +added source has a number `0` instead. @section GL-Shader-performance-optimizations Performance optimizations @@ -789,6 +824,7 @@ class MAGNUM_GL_EXPORT Shader: public AbstractObject { /* Used by addSource() / addFile(), see there for details */ bool _offsetLineByOneOnOldGlsl; #endif + UnsignedByte _fileIndexOffset; Containers::Array _sources; }; diff --git a/src/Magnum/GL/Test/ShaderGLTest.cpp b/src/Magnum/GL/Test/ShaderGLTest.cpp index aed49aaca..b4a9d7f7d 100644 --- a/src/Magnum/GL/Test/ShaderGLTest.cpp +++ b/src/Magnum/GL/Test/ShaderGLTest.cpp @@ -25,6 +25,7 @@ */ #include +#include #include #include #include /* StringHasPrefix / StringHasSuffix */ @@ -127,14 +128,13 @@ void ShaderGLTest::construct() { MAGNUM_VERIFY_NO_GL_ERROR(); CORRADE_VERIFY(shader.id() > 0); CORRADE_COMPARE(shader.type(), Shader::Type::Fragment); + /* There may be various workaround defines after, so check just that + the first source is the version definition */ + CORRADE_VERIFY(!shader.sources().isEmpty()); #ifndef MAGNUM_TARGET_GLES - CORRADE_COMPARE_AS(shader.sources(), - Containers::StringIterable{"#version 130\n"}, - TestSuite::Compare::Container); + CORRADE_COMPARE(shader.sources()[0], "#version 130\n"); #else - CORRADE_COMPARE_AS(shader.sources(), - Containers::StringIterable{"#version 300 es\n"}, - TestSuite::Compare::Container); + CORRADE_COMPARE(shader.sources()[0], "#version 300 es\n"); #endif } @@ -162,14 +162,13 @@ void ShaderGLTest::constructMove() { CORRADE_COMPARE(a.id(), 0); CORRADE_COMPARE(b.id(), id); CORRADE_COMPARE(b.type(), Shader::Type::Fragment); + /* There may be various workaround defines after, so check just that the + first source is the version definition */ + CORRADE_VERIFY(!b.sources().isEmpty()); #ifndef MAGNUM_TARGET_GLES - CORRADE_COMPARE_AS(b.sources(), - Containers::StringIterable{"#version 130\n"}, - TestSuite::Compare::Container); + CORRADE_COMPARE(b.sources()[0], "#version 130\n"); #else - CORRADE_COMPARE_AS(b.sources(), - Containers::StringIterable{"#version 300 es\n"}, - TestSuite::Compare::Container); + CORRADE_COMPARE(b.sources()[0], "#version 300 es\n"); #endif #ifndef MAGNUM_TARGET_GLES @@ -185,14 +184,13 @@ void ShaderGLTest::constructMove() { CORRADE_COMPARE(b.id(), cId); CORRADE_COMPARE(c.id(), id); CORRADE_COMPARE(c.type(), Shader::Type::Fragment); + /* There may be various workaround defines after, so check just that the + first source is the version definition */ + CORRADE_VERIFY(!c.sources().isEmpty()); #ifndef MAGNUM_TARGET_GLES - CORRADE_COMPARE_AS(c.sources(), - Containers::StringIterable{"#version 130\n"}, - TestSuite::Compare::Container); + CORRADE_COMPARE(c.sources()[0], "#version 130\n"); #else - CORRADE_COMPARE_AS(c.sources(), - Containers::StringIterable{"#version 300 es\n"}, - TestSuite::Compare::Container); + CORRADE_COMPARE(c.sources()[0], "#version 300 es\n"); #endif CORRADE_VERIFY(std::is_nothrow_move_constructible::value); @@ -245,6 +243,14 @@ void ShaderGLTest::addSource() { Shader shader(Version::GLES200, Shader::Type::Fragment); #endif + /* The initial shader sources contain driver-specific workarounds, so + just copy them to the expected array */ + /** @todo some StringIterable slicing during comparison instead of this? */ + Containers::Array expected; + for(Containers::StringView i: shader.sources()) + arrayAppend(expected, i); + const UnsignedInt workaroundCount = expected.size() - 1; + const char* data = "// r-value String\n"; shader.addSource("// global, null-terminated\n"_s) @@ -262,12 +268,7 @@ void ShaderGLTest::addSource() { #else #define _zero " 1 " #endif - Containers::StringView expected[]{ - #ifndef MAGNUM_TARGET_GLES - "#version 120\n", // 0 - #else - "#version 100\n", // 0 - #endif + Containers::StringView expectedSuffix[]{ "#line" _zero "1\n", "// global, null-terminated\n", // 2 "#line" _zero "2\n", @@ -283,26 +284,31 @@ void ShaderGLTest::addSource() { "void main() {}\n" // 12 }; #undef _zero + arrayAppend(expected, expectedSuffix); + CORRADE_COMPARE_AS(shader.sources(), Containers::StringIterable{expected}, TestSuite::Compare::Container); /* Verify that strings get copied only when not null terminated or not - global, and when not moved */ + global, and when not moved. Exclude the workaround defines added at the + front when comparing. */ for(std::size_t i: {0, 2, 12}) { CORRADE_ITERATION(i); - CORRADE_COMPARE(shader.sources()[i].flags(), Containers::StringViewFlag::NullTerminated|Containers::StringViewFlag::Global); + CORRADE_COMPARE(shader.sources()[i + workaroundCount].flags(), Containers::StringViewFlag::NullTerminated|Containers::StringViewFlag::Global); } for(std::size_t i: {1, 3, 4, 5, 6, 7, 8, 9, 10, 11}) { CORRADE_ITERATION(i); - CORRADE_COMPARE(shader.sources()[i].flags(), Containers::StringViewFlag::NullTerminated); + CORRADE_COMPARE(shader.sources()[i + workaroundCount].flags(), Containers::StringViewFlag::NullTerminated); } - CORRADE_VERIFY(shader.sources()[10].data() == data); + CORRADE_VERIFY(shader.sources()[10 + workaroundCount].data() == data); } void ShaderGLTest::addSourceNoVersion() { Shader shader(Version::None, Shader::Type::Fragment); + /* Unlike above, the initial shader sources are empty in this case */ + #ifndef MAGNUM_TARGET_GLES shader.addSource("#version 120\n"_s); #else @@ -354,28 +360,33 @@ void ShaderGLTest::addFile() { Shader shader(Version::GLES200, Shader::Type::Fragment); #endif + /* The initial shader sources contain driver-specific workarounds, so + just copy them to the expected array */ + /** @todo some StringIterable slicing during comparison instead of this? */ + Containers::Array expected; + for(Containers::StringView i: shader.sources()) + arrayAppend(expected, i); + const UnsignedInt workaroundCount = expected.size() - 1; + shader.addFile(Utility::Path::join(SHADERGLTEST_FILES_DIR, "shader.glsl")); - #ifndef MAGNUM_TARGET_GLES - CORRADE_COMPARE_AS(shader.sources(), (Containers::StringIterable{ - "#version 120\n", + arrayAppend(expected, { + #ifndef MAGNUM_TARGET_GLES /* On (desktop) GLSL < 330 the #line affect next line, not current line; see compileFailure() for a correctness verification */ "#line 0 1\n", - "void main() {}\n" - }), TestSuite::Compare::Container); - #else - CORRADE_COMPARE_AS(shader.sources(), (Containers::StringIterable{ - "#version 100\n", + #else "#line 1 1\n", + #endif "void main() {}\n" - }), TestSuite::Compare::Container); - #endif + }); + CORRADE_COMPARE_AS(shader.sources(), expected, + TestSuite::Compare::Container); /* The file source and the line number isn't global */ - CORRADE_COMPARE(shader.sources()[0].flags(), Containers::StringViewFlag::NullTerminated|Containers::StringViewFlag::Global); - CORRADE_COMPARE(shader.sources()[1].flags(), Containers::StringViewFlag::NullTerminated); - CORRADE_COMPARE(shader.sources()[2].flags(), Containers::StringViewFlag::NullTerminated); + CORRADE_COMPARE(shader.sources()[0 + workaroundCount].flags(), Containers::StringViewFlag::NullTerminated|Containers::StringViewFlag::Global); + CORRADE_COMPARE(shader.sources()[1 + workaroundCount].flags(), Containers::StringViewFlag::NullTerminated); + CORRADE_COMPARE(shader.sources()[2 + workaroundCount].flags(), Containers::StringViewFlag::NullTerminated); } void ShaderGLTest::addFileNonexistent() { diff --git a/src/Magnum/Shaders/Implementation/CreateCompatibilityShader.h b/src/Magnum/Shaders/Implementation/CreateCompatibilityShader.h index d7b1a0151..8779506bc 100644 --- a/src/Magnum/Shaders/Implementation/CreateCompatibilityShader.h +++ b/src/Magnum/Shaders/Implementation/CreateCompatibilityShader.h @@ -46,21 +46,6 @@ inline GL::Shader createCompatibilityShader(const Utility::Resource& rs, GL::Ver using namespace Containers::Literals; GL::Shader shader(version, type); - - #ifndef MAGNUM_TARGET_GLES - if(GL::Context::current().isExtensionDisabled(version)) - shader.addSource("#define MAGNUM_DISABLE_GL_ARB_explicit_attrib_location\n"_s); - if(GL::Context::current().isExtensionDisabled(version)) - shader.addSource("#define MAGNUM_DISABLE_GL_ARB_shading_language_420pack\n"_s); - if(GL::Context::current().isExtensionDisabled(version)) - shader.addSource("#define MAGNUM_DISABLE_GL_ARB_explicit_uniform_location\n"_s); - #endif - - #ifndef MAGNUM_TARGET_GLES2 - if(type == GL::Shader::Type::Vertex && GL::Context::current().isExtensionDisabled(version)) - shader.addSource("#define MAGNUM_DISABLE_GL_MAGNUM_shader_vertex_id\n"_s); - #endif - shader.addSource(rs.getString("compatibility.glsl"_s)); return shader; }