From 5d7a7d4d929914730a99abcff27558201bbb3f8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 26 May 2021 22:04:57 +0200 Subject: [PATCH] Shaders: ensure the ObjectId and Bitangent attribs are not used together. These deliberately share the same binding (because there's very little space), but the shader wasn't guarding that. Discovered completely by accident when adding tests for "multidraw with all the things" -- Mesa gives just a warning, but ANGLE straight out fails the shader compilation, so better have an assert there. --- src/Magnum/Shaders/MeshVisualizerGL.cpp | 5 +++ src/Magnum/Shaders/PhongGL.cpp | 5 +++ .../Shaders/Test/MeshVisualizerGLTest.cpp | 10 ++++- src/Magnum/Shaders/Test/PhongGLTest.cpp | 44 +++++++++++++++---- 4 files changed, 53 insertions(+), 11 deletions(-) diff --git a/src/Magnum/Shaders/MeshVisualizerGL.cpp b/src/Magnum/Shaders/MeshVisualizerGL.cpp index 6bd1b7200..5c9488408 100644 --- a/src/Magnum/Shaders/MeshVisualizerGL.cpp +++ b/src/Magnum/Shaders/MeshVisualizerGL.cpp @@ -572,6 +572,11 @@ MeshVisualizerGL3D::MeshVisualizerGL3D(const Flags flags "Shaders::MeshVisualizerGL3D: at least Flag::Wireframe has to be enabled", ); #endif + #if !defined(MAGNUM_TARGET_GLES2) && !defined(MAGNUM_TARGET_WEBGL) + CORRADE_ASSERT(!(flags & Flag::InstancedObjectId) || !(flags & Flag::BitangentDirection), + "Shaders::MeshVisualizerGL3D: Bitangent attribute binding conflicts with the ObjectId attribute, use a Tangent4 attribute with instanced object ID rendering instead", ); + #endif + /* Has to be here and not in the base class in order to have it exit the constructor when testing for asserts -- GLSL compilation would fail otherwise */ diff --git a/src/Magnum/Shaders/PhongGL.cpp b/src/Magnum/Shaders/PhongGL.cpp index 87d62e4bd..00e1083da 100644 --- a/src/Magnum/Shaders/PhongGL.cpp +++ b/src/Magnum/Shaders/PhongGL.cpp @@ -89,6 +89,11 @@ PhongGL::PhongGL(const Flags flags, const UnsignedInt lightCount CORRADE_ASSERT(!(flags & Flag::TextureTransformation) || (flags & (Flag::AmbientTexture|Flag::DiffuseTexture|Flag::SpecularTexture|Flag::NormalTexture)), "Shaders::PhongGL: texture transformation enabled but the shader is not textured", ); + #ifndef MAGNUM_TARGET_GLES2 + CORRADE_ASSERT(!(flags & Flag::InstancedObjectId) || !(flags & Flag::Bitangent), + "Shaders::PhongGL: Bitangent attribute binding conflicts with the ObjectId attribute, use a Tangent4 attribute with instanced object ID rendering instead", ); + #endif + #ifndef MAGNUM_TARGET_GLES2 CORRADE_ASSERT(!(flags >= Flag::UniformBuffers) || materialCount, "Shaders::PhongGL: material count can't be zero", ); diff --git a/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp b/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp index 8e7d462c1..0b6bdbd92 100644 --- a/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp +++ b/src/Magnum/Shaders/Test/MeshVisualizerGLTest.cpp @@ -275,7 +275,10 @@ constexpr struct { {"wireframe + vertex id", MeshVisualizerGL3D::Flag::Wireframe|MeshVisualizerGL3D::Flag::VertexId}, {"wireframe + t/n direction", MeshVisualizerGL3D::Flag::Wireframe|MeshVisualizerGL3D::Flag::TangentDirection|MeshVisualizerGL3D::Flag::NormalDirection}, {"wireframe + object id + t/n direction", MeshVisualizerGL3D::Flag::Wireframe|MeshVisualizerGL3D::Flag::InstancedObjectId|MeshVisualizerGL3D::Flag::TangentDirection|MeshVisualizerGL3D::Flag::NormalDirection}, - {"wireframe + vertex id + t/b direction", MeshVisualizerGL3D::Flag::Wireframe|MeshVisualizerGL3D::Flag::VertexId|MeshVisualizerGL3D::Flag::TangentDirection|MeshVisualizerGL3D::Flag::BitangentDirection} + {"wireframe + vertex id + t/b direction", MeshVisualizerGL3D::Flag::Wireframe|MeshVisualizerGL3D::Flag::VertexId|MeshVisualizerGL3D::Flag::TangentDirection|MeshVisualizerGL3D::Flag::BitangentDirection}, + /* InstancedObjectId|BitangentDirection is disallowed (checked in + ConstructInvalidData3D), but this should work */ + {"object id + bitangent from tangent direction", MeshVisualizerGL3D::Flag::InstancedObjectId|MeshVisualizerGL3D::Flag::BitangentFromTangentDirection}, #endif }; @@ -381,7 +384,10 @@ constexpr struct { "3D: geometry shader has to be enabled when rendering TBN direction"}, {"conflicting bitangent input", MeshVisualizerGL3D::Flag::BitangentFromTangentDirection|MeshVisualizerGL3D::Flag::BitangentDirection, - "3D: Flag::BitangentDirection and Flag::BitangentFromTangentDirection are mutually exclusive"} + "3D: Flag::BitangentDirection and Flag::BitangentFromTangentDirection are mutually exclusive"}, + {"conflicting bitangent and instanced object id attribute", + MeshVisualizerGL3D::Flag::BitangentDirection|MeshVisualizerGL3D::Flag::InstancedObjectId, + "3D: Bitangent attribute binding conflicts with the ObjectId attribute, use a Tangent4 attribute with instanced object ID rendering instead"}, #endif }; diff --git a/src/Magnum/Shaders/Test/PhongGLTest.cpp b/src/Magnum/Shaders/Test/PhongGLTest.cpp index c82f7944f..4a952bce2 100644 --- a/src/Magnum/Shaders/Test/PhongGLTest.cpp +++ b/src/Magnum/Shaders/Test/PhongGLTest.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include "Magnum/Image.h" #include "Magnum/ImageView.h" @@ -58,8 +59,6 @@ #include "Magnum/Trade/MeshData.h" #ifndef MAGNUM_TARGET_GLES2 -#include - #include "Magnum/GL/MeshView.h" #include "Magnum/MeshTools/Concatenate.h" #include "Magnum/MeshTools/GenerateIndices.h" @@ -85,7 +84,7 @@ struct PhongGLTest: GL::OpenGLTester { void constructMoveUniformBuffers(); #endif - void constructTextureTransformationNotTextured(); + void constructInvalid(); #ifndef MAGNUM_TARGET_GLES2 void constructUniformBuffersInvalid(); #endif @@ -220,7 +219,12 @@ constexpr struct { {"zero lights", {}, 0}, {"instanced transformation", PhongGL::Flag::InstancedTransformation, 3}, {"instanced specular texture offset", PhongGL::Flag::SpecularTexture|PhongGL::Flag::InstancedTextureOffset, 3}, - {"instanced normal texture offset", PhongGL::Flag::NormalTexture|PhongGL::Flag::InstancedTextureOffset, 3} + {"instanced normal texture offset", PhongGL::Flag::NormalTexture|PhongGL::Flag::InstancedTextureOffset, 3}, + #ifndef MAGNUM_TARGET_GLES2 + /* InstancedObjectId|Bitangent is disallowed (checked in + ConstructInvalidData), but this should work */ + {"object ID + normal texture with bitangent from tangent", PhongGL::Flag::InstancedObjectId|PhongGL::Flag::NormalTexture, 1} + #endif }; #ifndef MAGNUM_TARGET_GLES2 @@ -242,7 +246,24 @@ constexpr struct { {"alpha mask", PhongGL::Flag::UniformBuffers|PhongGL::Flag::AlphaMask, 1, 1, 1}, {"object ID", PhongGL::Flag::UniformBuffers|PhongGL::Flag::ObjectId, 1, 1, 1} }; +#endif +constexpr struct { + const char* name; + PhongGL::Flags flags; + const char* message; +} ConstructInvalidData[] { + {"texture transformation but not textured", + PhongGL::Flag::TextureTransformation, + "texture transformation enabled but the shader is not textured"}, + #ifndef MAGNUM_TARGET_GLES2 + {"conflicting bitangent and instanced object id attribute", + PhongGL::Flag::Bitangent|PhongGL::Flag::InstancedObjectId, + "Bitangent attribute binding conflicts with the ObjectId attribute, use a Tangent4 attribute with instanced object ID rendering instead"}, + #endif +}; + +#ifndef MAGNUM_TARGET_GLES2 constexpr struct { const char* name; PhongGL::Flags flags; @@ -613,8 +634,10 @@ PhongGLTest::PhongGLTest() { #ifndef MAGNUM_TARGET_GLES2 &PhongGLTest::constructMoveUniformBuffers, #endif + }); - &PhongGLTest::constructTextureTransformationNotTextured}); + addInstancedTests({&PhongGLTest::constructInvalid}, + Containers::arraySize(ConstructInvalidData)); #ifndef MAGNUM_TARGET_GLES2 addInstancedTests({ @@ -922,16 +945,19 @@ void PhongGLTest::constructMoveUniformBuffers() { } #endif -void PhongGLTest::constructTextureTransformationNotTextured() { +void PhongGLTest::constructInvalid() { + auto&& data = ConstructInvalidData[testCaseInstanceId()]; + setTestCaseDescription(data.name); + #ifdef CORRADE_NO_ASSERT CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); #endif std::ostringstream out; Error redirectError{&out}; - PhongGL{PhongGL::Flag::TextureTransformation}; - CORRADE_COMPARE(out.str(), - "Shaders::PhongGL: texture transformation enabled but the shader is not textured\n"); + PhongGL{data.flags}; + CORRADE_COMPARE(out.str(), Utility::formatString( + "Shaders::PhongGL: {}\n", data.message)); } #ifndef MAGNUM_TARGET_GLES2