Browse Source

GL: fix off-by-one line numbers in Shader error messages.

The #line directive behaves differently on GLSL < 330. Who would have
thought. Another "fun" implication is that I didn't notice this until
now -- seems like I didn't really write any shader since that
needed compatibility with old GL since 2012?? Heh.
pull/605/head
Vladimír Vondruš 4 years ago
parent
commit
3e968d3504
  1. 2
      doc/changelog.dox
  2. 29
      src/Magnum/GL/Shader.cpp
  3. 14
      src/Magnum/GL/Shader.h
  4. 14
      src/Magnum/GL/Test/ShaderGLTest.cpp

2
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

29
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));

14
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<std::string> _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;
}

14
src/Magnum/GL/Test/ShaderGLTest.cpp

@ -234,9 +234,11 @@ void ShaderGLTest::addSource() {
#ifndef MAGNUM_TARGET_GLES
CORRADE_COMPARE(shader.sources(), (std::vector<std::string>{
"#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<std::string>{
"",
/* 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<std::string>{
"#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

Loading…
Cancel
Save