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

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

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", );

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

src/Magnum/Shaders/Test/PhongGLTest.cpp

@@ -31,6 +31,7 @@
 #include <Corrade/TestSuite/Compare/Numeric.h>
 #include <Corrade/Utility/DebugStl.h>
 #include <Corrade/Utility/Directory.h>
+#include <Corrade/Utility/FormatStl.h>
 
 #include "Magnum/Image.h"
 #include "Magnum/ImageView.h"
@@ -58,8 +59,6 @@
 #include "Magnum/Trade/MeshData.h"
 
 #ifndef MAGNUM_TARGET_GLES2
-#include <Corrade/Utility/FormatStl.h>
-
 #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