MeshTools: assert on impl-specific vert format in combine*Attributes().

This asserted on some random nondescript hard-to-track-down place
before, which wasn't great. Also explicitly listing the requirement in
the docs.

src/Magnum/MeshTools/Combine.cpp

@@ -40,14 +40,25 @@ namespace Magnum { namespace MeshTools {
 
 namespace {
 
-Trade::MeshData combineIndexedImplementation(const MeshPrimitive primitive, Containers::Array<char>& combinedIndices, const UnsignedInt indexCount, const UnsignedInt indexStride, const Containers::ArrayView<const Containers::Reference<const Trade::MeshData>> data) {
+Trade::MeshData combineIndexedImplementation(
+    #ifndef CORRADE_NO_ASSERT
+    const char* assertPrefix,
+    #endif
+    const MeshPrimitive primitive, Containers::Array<char>& combinedIndices, const UnsignedInt indexCount, const UnsignedInt indexStride, const Containers::ArrayView<const Containers::Reference<const Trade::MeshData>> data)
+{
     /* Calculate attribute count and vertex stride */
     UnsignedInt attributeCount = 0;
     UnsignedInt vertexStride = 0;
     for(std::size_t i = 0; i != data.size(); ++i) {
         attributeCount += data[i]->attributeCount();
-        for(UnsignedInt j = 0; j != data[i]->attributeCount(); ++j)
+        for(UnsignedInt j = 0; j != data[i]->attributeCount(); ++j) {
-            vertexStride += vertexFormatSize(data[i]->attributeFormat(j))*Math::max(data[i]->attributeArraySize(j), UnsignedShort{1});
+            const VertexFormat format = data[i]->attributeFormat(j);
+            CORRADE_ASSERT(!isVertexFormatImplementationSpecific(format),
+                assertPrefix << "attribute" << j << "of mesh" << i << "has an implementation-specific format" << reinterpret_cast<void*>(vertexFormatUnwrap(format)),
+                (Trade::MeshData{MeshPrimitive::Points, 0}));
+
+            vertexStride += vertexFormatSize(format)*Math::max(data[i]->attributeArraySize(j), UnsignedShort{1});
+        }
     }
 
     /* Make the combined index array unique */
@@ -149,7 +160,11 @@ Trade::MeshData combineIndexedAttributes(const Containers::ArrayView<const Conta
         CORRADE_INTERNAL_ASSERT(indexOffset == indexStride);
     }
 
-    return combineIndexedImplementation(primitive, combinedIndices, indexCount, indexStride, data);
+    return combineIndexedImplementation(
+        #ifndef CORRADE_NO_ASSERT
+        "MeshTools::combineIndexedAttributes():",
+        #endif
+        primitive, combinedIndices, indexCount, indexStride, data);
 }
 
 Trade::MeshData combineIndexedAttributes(std::initializer_list<Containers::Reference<const Trade::MeshData>> data) {
@@ -203,8 +218,11 @@ Trade::MeshData combineFaceAttributes(const Trade::MeshData& mesh, const Trade::
     Utility::copy(combinedFaceIndices[0], combinedFaceIndices[2]);
 
     /* Then combine the two into a single buffer */
-    return combineIndexedImplementation(mesh.primitive(), combinedIndices,
+    return combineIndexedImplementation(
-        meshIndexCount, indexStride,
+        #ifndef CORRADE_NO_ASSERT
+        "MeshTools::combineFaceAttributes():",
+        #endif
+        mesh.primitive(), combinedIndices, meshIndexCount, indexStride,
         Containers::arrayView<const Containers::Reference<const Trade::MeshData>>({
             mesh, faceAttributes
         }));

src/Magnum/MeshTools/Combine.h

@@ -83,7 +83,9 @@ index count. All inputs have to be indexed, although the particular
 @ref MeshIndexType doesn't matter. For non-indexed attributes combining can be
 done much more efficiently using @ref duplicate(const Trade::MeshData&, Containers::ArrayView<const Trade::MeshAttributeData>),
 alternatively you can turn a non-indexed attribute to an indexed one first
-using @ref removeDuplicatesInPlace() and then call this function.
+using @ref removeDuplicatesInPlace() and then call this function. Attributes in
+all meshes are expected to not have an implementation-specific format.
+@see @ref isVertexFormatImplementationSpecific()
 */
 MAGNUM_MESHTOOLS_EXPORT Trade::MeshData combineIndexedAttributes(const Containers::ArrayView<const Containers::Reference<const Trade::MeshData>> data);
 
@@ -107,8 +109,9 @@ Expects that @p mesh is indexed @ref MeshPrimitive::Triangles and
 latter corresponding to triangle count of the former. If @p faceAttributes is
 indexed, it's assumed to have the data unique; if it's not indexed, it's first
 made unique using @ref removeDuplicates() and in that case it's expected to
-be interleaved.
+be interleaved. Attributes in both meshes are expected to not have an
-@see @ref isInterleaved()
+implementation-specific format.
+@see @ref isInterleaved(), @ref isVertexFormatImplementationSpecific()
 */
 MAGNUM_MESHTOOLS_EXPORT Trade::MeshData combineFaceAttributes(const Trade::MeshData& mesh, const Trade::MeshData& faceAttributes);
 

src/Magnum/MeshTools/Test/CombineTest.cpp

@@ -47,6 +47,7 @@ struct CombineTest: TestSuite::Tester {
     void combineIndexedAttributesNotIndexed();
     void combineIndexedAttributesDifferentPrimitive();
     void combineIndexedAttributesDifferentIndexCount();
+    void combineIndexedAttributesImplementationSpecificVertexFormat();
 
     void combineFaceAttributes();
     void combineFaceAttributesMeshNotIndexed();
@@ -54,6 +55,7 @@ struct CombineTest: TestSuite::Tester {
     void combineFaceAttributesUnexpectedFaceCount();
     void combineFaceAttributesFacesNotInterleaved();
     void combineFaceAttributesFaceAttributeOffsetOnly();
+    void combineFaceAttributesImplementationSpecificVertexFormat();
 };
 
 constexpr struct {
@@ -72,7 +74,8 @@ CombineTest::CombineTest() {
               &CombineTest::combineIndexedAttributesNoMeshes,
               &CombineTest::combineIndexedAttributesNotIndexed,
               &CombineTest::combineIndexedAttributesDifferentPrimitive,
-              &CombineTest::combineIndexedAttributesDifferentIndexCount});
+              &CombineTest::combineIndexedAttributesDifferentIndexCount,
+              &CombineTest::combineIndexedAttributesImplementationSpecificVertexFormat});
 
     addInstancedTests({&CombineTest::combineFaceAttributes},
         Containers::arraySize(CombineFaceAttributesData));
@@ -81,7 +84,8 @@ CombineTest::CombineTest() {
               &CombineTest::combineFaceAttributesUnexpectedPrimitive,
               &CombineTest::combineFaceAttributesUnexpectedFaceCount,
               &CombineTest::combineFaceAttributesFacesNotInterleaved,
-              &CombineTest::combineFaceAttributesFaceAttributeOffsetOnly});
+              &CombineTest::combineFaceAttributesFaceAttributeOffsetOnly,
+              &CombineTest::combineFaceAttributesImplementationSpecificVertexFormat});
 }
 
 void CombineTest::combineIndexedAttributes() {
@@ -243,6 +247,31 @@ void CombineTest::combineIndexedAttributesDifferentIndexCount() {
     CORRADE_COMPARE(out.str(), "MeshTools::combineIndexedAttributes(): data 2 has 4 indices but expected 5\n");
 }
 
+void CombineTest::combineIndexedAttributesImplementationSpecificVertexFormat() {
+    #ifdef CORRADE_NO_ASSERT
+    CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions");
+    #endif
+
+    Trade::MeshData a{MeshPrimitive::Points,
+        nullptr, Trade::MeshIndexData{MeshIndexType::UnsignedShort, nullptr},
+        nullptr, {
+            Trade::MeshAttributeData{Trade::MeshAttribute::Position, VertexFormat::Vector3, nullptr},
+            Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates, VertexFormat::Vector2, nullptr},
+        }};
+    Trade::MeshData b{MeshPrimitive::Points,
+        nullptr, Trade::MeshIndexData{MeshIndexType::UnsignedShort, nullptr},
+        nullptr, {
+            Trade::MeshAttributeData{Trade::MeshAttribute::Position, VertexFormat::Vector3, nullptr},
+            Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates, VertexFormat::Vector2, nullptr},
+            Trade::MeshAttributeData{Trade::meshAttributeCustom(3), vertexFormatWrap(0xcaca), nullptr}
+        }};
+
+    std::ostringstream out;
+    Error redirectError{&out};
+    MeshTools::combineIndexedAttributes({a, b});
+    CORRADE_COMPARE(out.str(), "MeshTools::combineIndexedAttributes(): attribute 2 of mesh 1 has an implementation-specific format 0xcaca\n");
+}
+
 void CombineTest::combineFaceAttributes() {
     auto&& data = CombineFaceAttributesData[testCaseInstanceId()];
     setTestCaseDescription(data.name);
@@ -418,6 +447,48 @@ void CombineTest::combineFaceAttributesUnexpectedFaceCount() {
         "MeshTools::combineFaceAttributes(): expected 1 face entries for 3 indices but got 2\n");
 }
 
+void CombineTest::combineFaceAttributesImplementationSpecificVertexFormat() {
+    #ifdef CORRADE_NO_ASSERT
+    CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions");
+    #endif
+
+    Trade::MeshData triangles{MeshPrimitive::Triangles,
+        nullptr, Trade::MeshIndexData{MeshIndexType::UnsignedShort, nullptr},
+        nullptr, {
+            Trade::MeshAttributeData{Trade::MeshAttribute::Position, VertexFormat::Vector3, nullptr},
+            Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates, VertexFormat::Vector2, nullptr},
+        }};
+    Trade::MeshData trianglesImplementationSpecific{MeshPrimitive::Triangles,
+        nullptr, Trade::MeshIndexData{MeshIndexType::UnsignedShort, nullptr},
+        nullptr, {
+            Trade::MeshAttributeData{Trade::MeshAttribute::Normal, VertexFormat::Vector3, nullptr},
+            Trade::MeshAttributeData{Trade::MeshAttribute::Color, VertexFormat::Vector4, nullptr},
+            Trade::MeshAttributeData{Trade::MeshAttribute::Tangent, vertexFormatWrap(0xcaca), nullptr}
+        }};
+
+    Trade::MeshData faces{MeshPrimitive::Faces,
+        nullptr, Trade::MeshIndexData{MeshIndexType::UnsignedShort, nullptr},
+        nullptr, {
+            Trade::MeshAttributeData{Trade::MeshAttribute::Position, VertexFormat::Vector3, nullptr},
+            Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates, VertexFormat::Vector2, nullptr},
+        }};
+    Trade::MeshData facesImplementationSpecific{MeshPrimitive::Faces,
+        nullptr, Trade::MeshIndexData{MeshIndexType::UnsignedShort, nullptr},
+        nullptr, {
+            Trade::MeshAttributeData{Trade::MeshAttribute::Normal, VertexFormat::Vector3, nullptr},
+            Trade::MeshAttributeData{Trade::MeshAttribute::Color, VertexFormat::Vector4, nullptr},
+            Trade::MeshAttributeData{Trade::MeshAttribute::Tangent, vertexFormatWrap(0xcaca), nullptr}
+        }};
+
+    std::ostringstream out;
+    Error redirectError{&out};
+    MeshTools::combineFaceAttributes(triangles, facesImplementationSpecific);
+    MeshTools::combineFaceAttributes(trianglesImplementationSpecific, faces);
+    CORRADE_COMPARE(out.str(),
+        "MeshTools::combineFaceAttributes(): attribute 2 of mesh 1 has an implementation-specific format 0xcaca\n"
+        "MeshTools::combineFaceAttributes(): attribute 2 of mesh 0 has an implementation-specific format 0xcaca\n");
+}
+
 void CombineTest::combineFaceAttributesFacesNotInterleaved() {
     #ifdef CORRADE_NO_ASSERT
     CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions");