diff --git a/doc/changelog.dox b/doc/changelog.dox index 030c9e9e5..c502fc45a 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -725,6 +725,8 @@ See also: - @ref GL::Shader limit queries for a particular shader stage on desktop were out-of-bounds array accesses, causing wrong or random values being returned for most shader-related limits +- Fixed @ref GL::Shader reporting errors and warnings with line numbers + off-by-one on desktop GLSL < 330. - Fixed assertions related to OpenGL driver workarounds when the proprietary AMDGPU PRO drivers are used on Linux - Fixed an assertion when using @ref MeshTools::removeDuplicates() on an diff --git a/src/Magnum/GL/Shader.cpp b/src/Magnum/GL/Shader.cpp index 721caad30..eeb435856 100644 --- a/src/Magnum/GL/Shader.cpp +++ b/src/Magnum/GL/Shader.cpp @@ -648,6 +648,17 @@ Int Shader::maxCombinedUniformComponents(const Type type) { Shader::Shader(const Version version, const Type type): _type{type}, _flags{ObjectFlag::DeleteOnDestruction|ObjectFlag::Created} { _id = glCreateShader(GLenum(_type)); + /* Used by addSource() / addFile() to insert either `#line 0` (for GLSL < + 330, which interprets it as affecting this line) or `#line 1` (for GLSL + >= 330 which interprets it as affecting next line). GLSL ES has it like + GLSL >= 330 always. */ + #ifndef MAGNUM_TARGET_GLES + if(version < Version::GL330) + _offsetLineByOneOnOldGlsl = true; + else + _offsetLineByOneOnOldGlsl = false; + #endif + switch(version) { #ifndef MAGNUM_TARGET_GLES case Version::GL210: _sources.emplace_back("#version 120\n"); return; @@ -678,7 +689,11 @@ Shader::Shader(const Version version, const Type type): _type{type}, _flags{Obje CORRADE_ASSERT_UNREACHABLE("GL::Shader::Shader(): unsupported version" << version, ); } -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{} + #endif + {} Shader::~Shader() { /* Moved out or not deleting on destruction, nothing to do */ @@ -723,7 +738,17 @@ Shader& Shader::addSource(std::string source) { order to avoid complex logic in compile() where we assert for at least some user-provided source, an empty string is added here instead. */ - if(!_sources.empty()) (this->*addSource)("#line 1 " + std::to_string((_sources.size()+1)/2) + '\n'); + if(!_sources.empty()) (this->*addSource)( + /* GLSL < 330 interprets #line 0 as the next line being line 1, + while GLSL >= 330 which interprets #line 1 as next line being + line 1; the latter is consistent with the C preprocessor. GLSL ES + behaves like GLSL >= 330 always. */ + ( + #ifndef MAGNUM_TARGET_GLES + _offsetLineByOneOnOldGlsl ? "#line 0 " : + #endif + "#line 1 " + ) + std::to_string((_sources.size()+1)/2) + '\n'); else (this->*addSource)({}); (this->*addSource)(std::move(source)); diff --git a/src/Magnum/GL/Shader.h b/src/Magnum/GL/Shader.h index 544e6d6c6..db2e4f699 100644 --- a/src/Magnum/GL/Shader.h +++ b/src/Magnum/GL/Shader.h @@ -760,6 +760,10 @@ class MAGNUM_GL_EXPORT Shader: public AbstractObject { Type _type; GLuint _id; ObjectFlags _flags; + #ifndef MAGNUM_TARGET_GLES + /* Used by addSource() / addFile(), see there for details */ + bool _offsetLineByOneOnOldGlsl; + #endif std::vector _sources; }; @@ -767,7 +771,12 @@ class MAGNUM_GL_EXPORT Shader: public AbstractObject { /** @debugoperatorclassenum{Shader,Shader::Type} */ MAGNUM_GL_EXPORT Debug& operator<<(Debug& debug, Shader::Type value); -inline Shader::Shader(Shader&& other) noexcept: _type{other._type}, _id{other._id}, _flags{other._flags}, _sources{std::move(other._sources)} { +inline Shader::Shader(Shader&& other) noexcept: _type{other._type}, _id{other._id}, _flags{other._flags}, + #ifndef MAGNUM_TARGET_GLES + _offsetLineByOneOnOldGlsl{other._flags}, + #endif + _sources{std::move(other._sources)} +{ other._id = 0; } @@ -776,6 +785,9 @@ inline Shader& Shader::operator=(Shader&& other) noexcept { swap(_type, other._type); swap(_id, other._id); swap(_flags, other._flags); + #ifndef MAGNUM_TARGET_GLES + swap(_offsetLineByOneOnOldGlsl, other._offsetLineByOneOnOldGlsl); + #endif swap(_sources, other._sources); return *this; } diff --git a/src/Magnum/GL/Test/ShaderGLTest.cpp b/src/Magnum/GL/Test/ShaderGLTest.cpp index e80f56896..03fda08c5 100644 --- a/src/Magnum/GL/Test/ShaderGLTest.cpp +++ b/src/Magnum/GL/Test/ShaderGLTest.cpp @@ -234,9 +234,11 @@ void ShaderGLTest::addSource() { #ifndef MAGNUM_TARGET_GLES CORRADE_COMPARE(shader.sources(), (std::vector{ "#version 120\n", - "#line 1 1\n", + /* On (desktop) GLSL < 330 the #line affect next line, not current + line; see compileFailure() for a correctness verification */ + "#line 0 1\n", "#define FOO BAR\n", - "#line 1 2\n", + "#line 0 2\n", "void main() {}\n" })); #else @@ -264,6 +266,10 @@ void ShaderGLTest::addSourceNoVersion() { #ifndef MAGNUM_TARGET_GLES CORRADE_COMPARE(shader.sources(), (std::vector{ "", + /* Here, even though there's #version 120 eventually added by the user, + it assumes the specified version was new GLSL, not old. Explicitly + specified old GLSL is such a rare use case that I don't bother + looking for the #version directive and adjusting. */ "#version 120\n", "#line 1 1\n", "#define FOO BAR\n", @@ -294,7 +300,9 @@ void ShaderGLTest::addFile() { #ifndef MAGNUM_TARGET_GLES CORRADE_COMPARE(shader.sources(), (std::vector{ "#version 120\n", - "#line 1 1\n", + /* 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" })); #else