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; }