diff --git a/src/Magnum/MeshTools/Compile.cpp b/src/Magnum/MeshTools/Compile.cpp index f8a8f13f6..654ca8559 100644 --- a/src/Magnum/MeshTools/Compile.cpp +++ b/src/Magnum/MeshTools/Compile.cpp @@ -26,6 +26,7 @@ #include "Compile.h" #include +#include #include #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 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 diff --git a/src/Magnum/MeshTools/Compile.h b/src/Magnum/MeshTools/Compile.h index 9faa673fa..1bf658826 100644 --- a/src/Magnum/MeshTools/Compile.h +++ b/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&) diff --git a/src/Magnum/MeshTools/Test/CompileGLTest.cpp b/src/Magnum/MeshTools/Test/CompileGLTest.cpp index 46e38e2bc..8701e36a3 100644 --- a/src/Magnum/MeshTools/Test/CompileGLTest.cpp +++ b/src/Magnum/MeshTools/Test/CompileGLTest.cpp @@ -27,8 +27,10 @@ #include #include #include +#include #include #include +#include #include #include "Magnum/Image.h" @@ -102,9 +104,9 @@ struct CompileGLTest: GL::OpenGLTester { template void twoDimensions(); template 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 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 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 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()[2][2], 27); + CORRADE_COMPARE(image.pixels()[11][18], data.expectedObjectId); + } + #endif +} + void CompileGLTest::unsupportedIndexStride() { CORRADE_SKIP_IF_NO_ASSERT();