MeshTools: print a warning if compile() ignores conflicting attributes.

Which is currently the case for any two attributes of the same name
(such as two texture coordinate sets), or bitangents and object ID used
together.

Plus extending the check for matrix attributes (such as instanced
transformation conflicting with secondary joint IDs and weights).
None of these attributes are builtin though, so the check can't be
verified at the moment.

And also documenting the behavior, for some reason the comment was
apparently not updated since the MeshData2D/MeshData3D days!

src/Magnum/MeshTools/Compile.cpp

@@ -26,6 +26,7 @@
 #include "Compile.h"
 
 #include <Corrade/Containers/Optional.h>
+#include <Corrade/Containers/StaticArray.h>
 #include <Corrade/Containers/StridedArrayView.h>
 
 #include "Magnum/GL/Buffer.h"
@@ -68,8 +69,11 @@ GL::Mesh compileInternal(const Trade::MeshData& meshData, GL::Buffer&& indices,
     GL::Buffer verticesRef = GL::Buffer::wrap(vertices.id(), GL::Buffer::TargetHint::Array);
 
     /* Ensure each known attribute gets bound only once. There's 16 generic
-       attribs at most. */
-    Math::BitVector<16> boundAttributes;
+       attributes at most, for each remember the mesh attribute index that got
+       bound to it first, or ~UnsignedInt{} if none yet. */
+    /** @todo revisit when there are secondary generic texture coordinates,
+        colors, etc */
+    Containers::StaticArray<16, UnsignedInt> boundAttributes{DirectInit, ~UnsignedInt{}};
 
     for(UnsignedInt i = 0; i != meshData.attributeCount(); ++i) {
         Containers::Optional<GL::DynamicAttribute> attribute;
@@ -137,13 +141,20 @@ GL::Mesh compileInternal(const Trade::MeshData& meshData, GL::Buffer&& indices,
             continue;
         }
 
-        /* Ensure each attribute gets bound only once -- so for example when
-           there are two texture coordinate sets, we don't bind them both to
-           the same slot, effectively ignoring the first one */
-        /** @todo revisit when there are secondary generic texture coordinates */
-        if(boundAttributes[attribute->location()])
+        /* Ensure each attribute slot gets bound only once -- so for example
+           when there are two texture coordinate sets, we don't bind them both
+           to the same slot, effectively ignoring the first one. Similarly
+           warn if an attribute has a location conflicting with another one
+           (such as ObjectId and Bitangent). */
+        if(boundAttributes[attribute->location()] != ~UnsignedInt{}) {
+            Warning{} << "MeshTools::compile(): ignoring" << meshData.attributeName(i) << meshData.attributeId(i) << "as its biding slot is already occupied by" << meshData.attributeName(boundAttributes[attribute->location()]) << meshData.attributeId(boundAttributes[attribute->location()]);
             continue;
-        boundAttributes.set(attribute->location(), true);
+        }
+
+        /* Remeber where this attribute got bound, including all subsequent
+           vectors for matrix attributes */
+        for(UnsignedInt j = 0; j != attribute->vectors(); ++j)
+            boundAttributes[attribute->location() + j] = i;
 
         /* Negative strides are not supported by GL, zero strides are
            understood as tightly packed instead of all attributes having the

src/Magnum/MeshTools/Compile.h

@@ -101,17 +101,31 @@ CORRADE_ENUMSET_OPERATORS(CompileFlags)
 Configures a mesh for a @ref Shaders::GenericGL shader with a vertex buffer and
 possibly also an index buffer, if the mesh is indexed.
 
--   If the mesh contains positions, these are bound to the
-    @ref Shaders::GenericGL2D::Position attribute if they are 2D or to
+-   If the mesh contains @ref Trade::MeshAttribute::Position, these are bound
+    to the @ref Shaders::GenericGL2D::Position attribute if they are 2D or to
     @ref Shaders::GenericGL3D::Position if they are 3D.
--   If the mesh contains normals or if @ref CompileFlag::GenerateFlatNormals /
+-   If the mesh contains @ref Trade::MeshAttribute::Tangent, these are bound to
+    @ref Shaders::GenericGL3D::Tangent4 or @ref Shaders::GenericGL3D::Tangent
+    based on their type.
+-   If the mesh contains @ref Trade::MeshAttribute::Bitangent, these are bound
+    to @ref Shaders::GenericGL3D::Bitangent. However, if the mesh contains a
+    @ref Trade::MeshAttribute::ObjectId as well, only the first appearing of
+    the two is bound. The second is ignored with a warning as they share the
+    same binding slot.
+-   If the mesh contains @ref Trade::MeshAttribute::Normal or if
+    @ref CompileFlag::GenerateFlatNormals /
     @ref CompileFlag::GenerateSmoothNormals is set, these are bound to
     @ref Shaders::GenericGL3D::Normal.
--   If the mesh contains texture coordinates, these are bound to
-    @ref Shaders::GenericGL::TextureCoordinates.
--   If the mesh contains colors, these are bound to
-    @ref Shaders::GenericGL::Color3 / @ref Shaders::GenericGL::Color4 based on
-    their type.
+-   If the mesh contains @ref Trade::MeshAttribute::TextureCoordinates, these
+    are bound to @ref Shaders::GenericGL::TextureCoordinates.
+-   If the mesh contains @ref Trade::MeshAttribute::Color, these are bound to
+    @ref Shaders::GenericGL::Color3 / @relativeref{Shaders::GenericGL,Color4}
+    based on their type.
+-   If the mesh contains @ref Trade::MeshAttribute::ObjectId, these are bound
+    to @ref Shaders::GenericGL::ObjectId. However, if the mesh contains a
+    @ref Trade::MeshAttribute::Bitangent as well, only the first appearing of
+    the two is bound. The second is ignored with a warning as they share the
+    same binding slot.
 -   Custom attributes and known attributes of implementation-specific vertex
     formats are ignored with a warning. See
     @ref compile(const Trade::MeshData&, GL::Buffer&, GL::Buffer&)

src/Magnum/MeshTools/Test/CompileGLTest.cpp

@@ -27,8 +27,10 @@
 #include <Corrade/Containers/EnumSet.h>
 #include <Corrade/Containers/GrowableArray.h>
 #include <Corrade/Containers/String.h>
+#include <Corrade/Containers/StringIterable.h>
 #include <Corrade/PluginManager/Manager.h>
 #include <Corrade/Utility/DebugStl.h>
+#include <Corrade/Utility/FormatStl.h>
 #include <Corrade/Utility/Path.h>
 
 #include "Magnum/Image.h"
@@ -102,9 +104,9 @@ struct CompileGLTest: GL::OpenGLTester {
         template<class T> void twoDimensions();
         template<class T> void threeDimensions();
 
-        void multipleAttributes();
         void packedAttributes();
 
+        void conflictingAttributes();
         void unsupportedIndexStride();
 
         void customAttribute();
@@ -201,6 +203,51 @@ const struct {
     {"positions, object id, nonindexed", Flag::ObjectId|Flag::NonIndexed, {}}
 };
 
+constexpr std::ptrdiff_t ConflictingAttributesDataStride = 3*sizeof(Vector2) + sizeof(UnsignedInt) + sizeof(Vector3);
+
+const struct {
+    const char* name;
+    Containers::Array<Trade::MeshAttributeData> attributes;
+    Flags flags;
+    UnsignedInt expectedObjectId;
+    const char* expected;
+    const char* expectedMessage;
+} ConflictingAttributesData[]{
+    /* The position atribute is added in the test */
+    {"multiple texture coordinates", {InPlaceInit, {
+        Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates,
+            VertexFormat::Vector2, sizeof(Vector2),
+            9, ConflictingAttributesDataStride},
+        Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates,
+            VertexFormat::Vector2, 2*sizeof(Vector2),
+            9, ConflictingAttributesDataStride},
+    }}, Flag::TextureCoordinates2D, 0, "textured2D.tga",
+        "ignoring Trade::MeshAttribute::TextureCoordinates 1 as its biding slot is already occupied by Trade::MeshAttribute::TextureCoordinates 0"},
+    #ifndef MAGNUM_TARGET_GLES2
+    {"bitangents + object ID", {InPlaceInit, {
+        Trade::MeshAttributeData{Trade::MeshAttribute::Bitangent,
+            VertexFormat::Vector3, 3*sizeof(Vector2) + sizeof(UnsignedInt),
+            9, ConflictingAttributesDataStride},
+        Trade::MeshAttributeData{Trade::MeshAttribute::ObjectId,
+            VertexFormat::UnsignedInt, 3*sizeof(Vector2),
+            9, ConflictingAttributesDataStride},
+    }}, Flag::ObjectId, 0, "flat2D.tga",
+        "ignoring Trade::MeshAttribute::ObjectId 0 as its biding slot is already occupied by Trade::MeshAttribute::Bitangent 0"},
+    {"object ID + bitangents", {InPlaceInit, {
+        Trade::MeshAttributeData{Trade::MeshAttribute::ObjectId,
+            VertexFormat::UnsignedInt, 3*sizeof(Vector2),
+            9, ConflictingAttributesDataStride},
+        Trade::MeshAttributeData{Trade::MeshAttribute::Bitangent,
+            VertexFormat::Vector3, 3*sizeof(Vector2) + sizeof(UnsignedInt),
+            9, ConflictingAttributesDataStride},
+    }}, Flag::ObjectId, 26234, "flat2D.tga",
+        "ignoring Trade::MeshAttribute::Bitangent 0 as its biding slot is already occupied by Trade::MeshAttribute::ObjectId 0"},
+    #endif
+    /** @todo test also a conflict with instanced transformation + secondary
+        joints & weights, once instanced transformation is a builtin
+        attribute */
+};
+
 constexpr struct {
     const char* name;
     CompileFlags flags;
@@ -262,8 +309,12 @@ CompileGLTest::CompileGLTest() {
     CORRADE_IGNORE_DEPRECATED_POP
     #endif
 
-    addTests({&CompileGLTest::multipleAttributes,
-              &CompileGLTest::packedAttributes},
+    addTests({&CompileGLTest::packedAttributes},
+        &CompileGLTest::renderSetup,
+        &CompileGLTest::renderTeardown);
+
+    addInstancedTests({&CompileGLTest::conflictingAttributes},
+        Containers::arraySize(ConflictingAttributesData),
         &CompileGLTest::renderSetup,
         &CompileGLTest::renderTeardown);
 
@@ -902,66 +953,6 @@ template<class T> void CompileGLTest::threeDimensions() {
     }
 }
 
-void CompileGLTest::multipleAttributes() {
-    struct Vertex {
-        Vector2 position;
-        Vector2 textureCoordinates1, textureCoordinates2;
-    } vertexData[]{
-        {{-0.75f, -0.75f}, {0.0f, 0.0f}, {0.0f, 0.0f}},
-        {{ 0.00f, -0.75f}, {0.5f, 0.0f}, {5.0f, 0.0f}},
-        {{ 0.75f, -0.75f}, {1.0f, 0.0f}, {10.0f, 0.0f}},
-
-        {{-0.75f,  0.00f}, {0.0f, 0.5f}, {0.0f, 5.0f}},
-        {{ 0.00f,  0.00f}, {0.5f, 0.5f}, {5.0f, 5.0f}},
-        {{ 0.75f,  0.00f}, {1.0f, 0.5f}, {10.0f, 5.0f}},
-
-        {{-0.75f,  0.75f}, {0.0f, 1.0f}, {0.0f, 10.0f}},
-        {{ 0.0f,   0.75f}, {0.5f, 1.0f}, {5.0f, 10.0f}},
-        {{ 0.75f,  0.75f}, {1.0f, 1.0f}, {10.0f, 10.0f}}
-    };
-    auto vertices = Containers::stridedArrayView(vertexData);
-
-    const UnsignedInt indexData[]{
-        0, 1, 4, 0, 4, 3,
-        1, 2, 5, 1, 5, 4,
-        3, 4, 7, 3, 7, 6,
-        4, 5, 8, 4, 8, 7
-    };
-
-    Trade::MeshData meshData{MeshPrimitive::Triangles,
-        {}, indexData, Trade::MeshIndexData{indexData},
-        {}, vertexData, {
-            Trade::MeshAttributeData{Trade::MeshAttribute::Position,
-                vertices.slice(&Vertex::position)},
-            Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates,
-                vertices.slice(&Vertex::textureCoordinates1)},
-            Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates,
-                vertices.slice(&Vertex::textureCoordinates2)},
-        }};
-
-    GL::Mesh mesh = compile(meshData);
-    MAGNUM_VERIFY_NO_GL_ERROR();
-
-    if(!(_manager.loadState("AnyImageImporter") & PluginManager::LoadState::Loaded) ||
-       !(_manager.loadState("TgaImporter") & PluginManager::LoadState::Loaded))
-        CORRADE_SKIP("AnyImageImporter / TgaImporter plugins not found.");
-
-    _framebuffer.clear(GL::FramebufferClear::Color);
-    _flatTextured2D
-        .bindTexture(_texture)
-        .draw(mesh);
-
-    /* The output should be the same as in the textured case of twoDimensions()
-       -- i.e., the second texture coordinate set not affecting anything */
-    MAGNUM_VERIFY_NO_GL_ERROR();
-    CORRADE_COMPARE_WITH(
-        _framebuffer.read({{}, {32, 32}}, {PixelFormat::RGBA8Unorm}),
-        Utility::Path::join(MESHTOOLS_TEST_DIR, "CompileTestFiles/textured2D.tga"),
-        /* SwiftShader has some minor off-by-one precision differences,
-            llvmpipe as well */
-        (DebugTools::CompareImageToFile{_manager, 1.75f, 0.22f}));
-}
-
 /* Can't be inline because MSVC 2015 doesn't like anonymous bitfields in local
    structs */
 struct PackedVertex {
@@ -1149,6 +1140,103 @@ void CompileGLTest::packedAttributes() {
     #endif
 }
 
+void CompileGLTest::conflictingAttributes() {
+    auto&& data = ConflictingAttributesData[testCaseInstanceId()];
+    setTestCaseDescription(data.name);
+
+    struct Vertex {
+        Vector2 position;
+        Vector2 textureCoordinates1, textureCoordinates2;
+        UnsignedInt objectId;
+        Vector3 bitangent;
+    } vertexData[]{
+        {{-0.75f, -0.75f}, {0.0f, 0.0f}, { 0.0f,  0.0f}, 26234, {}},
+        {{ 0.00f, -0.75f}, {0.5f, 0.0f}, { 5.0f,  0.0f}, 26234, {}},
+        {{ 0.75f, -0.75f}, {1.0f, 0.0f}, {10.0f,  0.0f}, 26234, {}},
+
+        {{-0.75f,  0.00f}, {0.0f, 0.5f}, { 0.0f,  5.0f}, 26234, {}},
+        {{ 0.00f,  0.00f}, {0.5f, 0.5f}, { 5.0f,  5.0f}, 26234, {}},
+        {{ 0.75f,  0.00f}, {1.0f, 0.5f}, {10.0f,  5.0f}, 26234, {}},
+
+        {{-0.75f,  0.75f}, {0.0f, 1.0f}, { 0.0f, 10.0f}, 26234, {}},
+        {{ 0.0f,   0.75f}, {0.5f, 1.0f}, { 5.0f, 10.0f}, 26234, {}},
+        {{ 0.75f,  0.75f}, {1.0f, 1.0f}, {10.0f, 10.0f}, 26234, {}}
+    };
+    auto vertices = Containers::stridedArrayView(vertexData);
+
+    const UnsignedInt indexData[]{
+        0, 1, 4, 0, 4, 3,
+        1, 2, 5, 1, 5, 4,
+        3, 4, 7, 3, 7, 6,
+        4, 5, 8, 4, 8, 7
+    };
+
+    Containers::Array<Trade::MeshAttributeData> attributeData;
+    arrayAppend(attributeData, InPlaceInit, Trade::MeshAttribute::Position,
+        vertices.slice(&Vertex::position));
+    arrayAppend(attributeData, data.attributes);
+
+    Trade::MeshData meshData{MeshPrimitive::Triangles,
+        {}, indexData, Trade::MeshIndexData{indexData},
+        {}, vertexData, std::move(attributeData)};
+
+    GL::Mesh mesh{NoCreate};
+
+    std::ostringstream out;
+    {
+        Warning redirectWarning{&out};
+        mesh = compile(meshData);
+    }
+    MAGNUM_VERIFY_NO_GL_ERROR();
+    CORRADE_COMPARE(out.str(), Utility::formatString("MeshTools::compile(): {}\n", data.expectedMessage));
+
+    if(!(_manager.loadState("AnyImageImporter") & PluginManager::LoadState::Loaded) ||
+       !(_manager.loadState("TgaImporter") & PluginManager::LoadState::Loaded))
+        CORRADE_SKIP("AnyImageImporter / TgaImporter plugins not found.");
+
+    _framebuffer.clear(GL::FramebufferClear::Color);
+    #ifndef MAGNUM_TARGET_GLES2
+    if(data.flags & Flag::ObjectId)
+        _framebuffer.clearColor(1, Vector4ui{27});
+    #endif
+    if(data.flags & Flag::TextureCoordinates2D)
+        _flatTextured2D
+            .bindTexture(_texture)
+            .draw(mesh);
+    #ifndef MAGNUM_TARGET_GLES2
+    else if(data.flags & Flag::ObjectId)
+        _flatObjectId2D
+            .setColor(0xff3366_rgbf)
+            .draw(mesh);
+    #endif
+    else
+        _flat2D
+            .setColor(0xff3366_rgbf)
+            .draw(mesh);
+
+    /* The output should be the same as in the textured case of twoDimensions()
+       -- i.e., the second texture coordinate set not affecting anything */
+    MAGNUM_VERIFY_NO_GL_ERROR();
+    CORRADE_COMPARE_WITH(
+        _framebuffer.read({{}, {32, 32}}, {PixelFormat::RGBA8Unorm}),
+        Utility::Path::join({MESHTOOLS_TEST_DIR, "CompileTestFiles", data.expected}),
+        /* SwiftShader has some minor off-by-one precision differences,
+            llvmpipe as well */
+        (DebugTools::CompareImageToFile{_manager, 1.75f, 0.22f}));
+
+    #ifndef MAGNUM_TARGET_GLES2
+    if(data.flags & Flag::ObjectId) {
+        _framebuffer.mapForRead(GL::Framebuffer::ColorAttachment{1});
+        CORRADE_COMPARE(_framebuffer.checkStatus(GL::FramebufferTarget::Read), GL::Framebuffer::Status::Complete);
+        Image2D image = _framebuffer.read(_framebuffer.viewport(), {PixelFormat::R32UI});
+        MAGNUM_VERIFY_NO_GL_ERROR();
+        /* Outside of the object, cleared to 27 */
+        CORRADE_COMPARE(image.pixels<UnsignedInt>()[2][2], 27);
+        CORRADE_COMPARE(image.pixels<UnsignedInt>()[11][18], data.expectedObjectId);
+    }
+    #endif
+}
+
 void CompileGLTest::unsupportedIndexStride() {
     CORRADE_SKIP_IF_NO_ASSERT();