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

@@ -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

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<Extensions::ARB::explicit_attrib_location>(version))
+            arrayAppend(_sources, Containers::String::nullTerminatedGlobalView("#define MAGNUM_DISABLE_GL_ARB_explicit_attrib_location\n"_s));
+        if(context.isExtensionDisabled<Extensions::ARB::shading_language_420pack>(version))
+            arrayAppend(_sources, Containers::String::nullTerminatedGlobalView("#define MAGNUM_DISABLE_GL_ARB_shading_language_420pack\n"_s));
+        if(context.isExtensionDisabled<Extensions::ARB::explicit_uniform_location>(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<Extensions::MAGNUM::shader_vertex_id>(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));

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 `<file>(<line>):` or `<file>:<line>:`.
     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<Containers::String> _sources;
 };

src/Magnum/GL/Test/ShaderGLTest.cpp

@@ -25,6 +25,7 @@
 */
 
 #include <sstream>
+#include <Corrade/Containers/GrowableArray.h>
 #include <Corrade/Containers/String.h>
 #include <Corrade/Containers/StringIterable.h>
 #include <Corrade/Containers/StringStl.h> /* 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<Shader>::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<Containers::StringView> 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<Containers::StringView> 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() {

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<GL::Extensions::ARB::explicit_attrib_location>(version))
-        shader.addSource("#define MAGNUM_DISABLE_GL_ARB_explicit_attrib_location\n"_s);
-    if(GL::Context::current().isExtensionDisabled<GL::Extensions::ARB::shading_language_420pack>(version))
-        shader.addSource("#define MAGNUM_DISABLE_GL_ARB_shading_language_420pack\n"_s);
-    if(GL::Context::current().isExtensionDisabled<GL::Extensions::ARB::explicit_uniform_location>(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<GL::Extensions::MAGNUM::shader_vertex_id>(version))
-        shader.addSource("#define MAGNUM_DISABLE_GL_MAGNUM_shader_vertex_id\n"_s);
-    #endif
-
     shader.addSource(rs.getString("compatibility.glsl"_s));
     return shader;
 }