MeshTools: allow smaller-to-larger array copying in concatenate().

THe remaining elements are zero-filled, consistently with the behavior with zeroing out attributes that are missing in subsequent meshes. One use case is concatenating a mesh with 4 JointIds / Weights into a mesh that has 8 instead. Right now, copying larger arrays to smaller arrays is still disallowed to avoid data loss. That is inconsistent with behavior for attributes (which are silently left out when not present in the first mesh), I'll need to think about this more and solve the inconsistency somehow. Either by allowing array slicing or by failing for unhandled attributes as well, and the latter makes more sense from the perspective of avoiding data loss.
3 years ago · b8c2f8b317
4 changed files with 114 additions and 37 deletions
--- a/doc/changelog.dox
+++ b/doc/changelog.dox
@ -516,6 +516,12 @@ See also:
    @ref MeshTools::concatenate(const Containers::Iterable<const Trade::MeshData>&, InterleaveFlags)
    optionally take a @ref MeshTools::InterleaveFlags parameter affecting the
    output, in particular whether to preserve the original interleaved layout.
+-   @ref MeshTools::concatenate() now allows concatenating an array attribute
+    into an array attribute with more elements. The remaining elements are
+    zero-filled, similarly to how missing attributes are handled. To avoid
+    accidents and data loss, concatenating a bigger array into a smaller one
+    isn't allowed. There's also a restriction that non-array attributes can't
+    be concatenated into arrays, even though they would fit.
 -   Support for new @ref Trade::MeshAttribute::JointIds and
    @ref Trade::MeshAttribute::Weights in @ref MeshTools::compile() as well as
    a new @ref MeshTools::compiledPerVertexJointCount() helper utility (see
--- a/src/Magnum/MeshTools/Concatenate.cpp
+++ b/src/Magnum/MeshTools/Concatenate.cpp
@ -169,14 +169,25 @@ Trade::MeshData concatenate(Containers::Array<char>&& indexData, const UnsignedI
            CORRADE_ASSERT(out.attributeFormat(dst) == mesh.attributeFormat(src),
                assertPrefix << "expected" << out.attributeFormat(dst) << "for attribute" << dst << "(" << Debug::nospace << out.attributeName(dst) << Debug::nospace << ") but got" << mesh.attributeFormat(src) << "in mesh" << i << "attribute" << src,
                (Trade::MeshData{MeshPrimitive{}, 0}));
-            CORRADE_ASSERT(out.attributeArraySize(dst) == mesh.attributeArraySize(src),
-                assertPrefix << "expected array size" << out.attributeArraySize(dst) << "for attribute" << dst << "(" << Debug::nospace << out.attributeName(dst) << Debug::nospace << ") but got" << mesh.attributeArraySize(src) << "in mesh" << i << "attribute" << src,
+            CORRADE_ASSERT(!out.attributeArraySize(dst) == !mesh.attributeArraySize(src),
+                assertPrefix << "attribute" << dst << "(" << Debug::nospace << out.attributeName(dst) << Debug::nospace << ")" << (out.attributeArraySize(dst) ? "is" : "isn't") << "an array but attribute" << src << "in mesh" << i << (mesh.attributeArraySize(src) ? "is" : "isn't"),
                (Trade::MeshData{MeshPrimitive{}, 0}));
+            CORRADE_ASSERT(out.attributeArraySize(dst) >= mesh.attributeArraySize(src),
+                assertPrefix << "expected array size" << out.attributeArraySize(dst) << "or less for attribute" << dst << "(" << Debug::nospace << out.attributeName(dst) << Debug::nospace << ") but got" << mesh.attributeArraySize(src) << "in mesh" << i << "attribute" << src,
+                (Trade::MeshData{MeshPrimitive{}, 0}));
+
+            const Containers::StridedArrayView2D<const char> srcAttribute = mesh.attribute(src);
+            const Containers::StridedArrayView2D<char> dstAttribute = out.mutableAttribute(dst);

            /* Copy the data to a slice of the output, mark the attribute as
-               copied */
-            Utility::copy(mesh.attribute(src), out.mutableAttribute(dst)
-                .slice(vertexOffset, vertexOffset + mesh.vertexCount()));
+               copied. For non-array attributes the second dimension should be
+               matching (because the format is matching), for array attributes
+               we may be copying to just a prefix of the elements in
+               dstAttribute. */
+            CORRADE_INTERNAL_ASSERT(out.attributeArraySize(dst) || srcAttribute.size()[1] == dstAttribute.size()[1]);
+            Utility::copy(srcAttribute, dstAttribute.sliceSize(
+                {vertexOffset, 0},
+                {mesh.vertexCount(), srcAttribute.size()[1]}));
            found->second.second = true;
        }

--- a/src/Magnum/MeshTools/Concatenate.h
+++ b/src/Magnum/MeshTools/Concatenate.h
@ -64,11 +64,14 @@ All attributes from the first mesh are taken, expected to not have an
 implementation-specific format. For each following mesh attributes present in
 the first are copied, superfluous attributes ignored and missing attributes
 zeroed out. Matching attributes are expected to have the same type, all meshes
-are expected to have the same primitive. The vertex data are concatenated in
-the same order as passed, with no duplicate removal. Returned instance vertex
-and index data flags always have both @ref Trade::DataFlag::Owned and
-@ref Trade::DataFlag::Mutable to guarante mutable access to particular parts of
-the concatenated mesh --- for example for applying transformations.
+are expected to have the same primitive. In case of array attributes,
+attributes in subsequent meshes are expected to be arrays as well and have the
+same or smaller array size. Unused components at the end are zeroed out. The
+vertex data are concatenated in the same order as passed, with no duplicate
+removal. Returned instance vertex and index data flags always have both
+@ref Trade::DataFlag::Owned and @ref Trade::DataFlag::Mutable to guarante
+mutable access to particular parts of the concatenated mesh --- for example for
+applying transformations.

 The data layouting is done by @ref interleavedLayout() with the @p flags
 parameter propagated to it, see its documentation for detailed behavior
--- a/src/Magnum/MeshTools/Test/ConcatenateTest.cpp
+++ b/src/Magnum/MeshTools/Test/ConcatenateTest.cpp
@ -25,8 +25,10 @@

 #include <sstream>
 #include <Corrade/Containers/Optional.h>
+#include <Corrade/Containers/StringStl.h> /** @todo remove when Debug is stream-free */
 #include <Corrade/TestSuite/Tester.h>
 #include <Corrade/TestSuite/Compare/Container.h>
+#include <Corrade/TestSuite/Compare/String.h>
 #include <Corrade/Utility/DebugStl.h>

 #include "Magnum/Math/Color.h"
@ -50,7 +52,8 @@ struct ConcatenateTest: TestSuite::Tester {
    void concatenateUnsupportedPrimitive();
    void concatenateInconsistentPrimitive();
    void concatenateInconsistentAttributeFormat();
-    void concatenateInconsistentAttributeArraySize();
+    void concatenateInconsistentArrayAttribute();
+    void concatenateTooLargeAttributeArraySize();
    void concatenateImplementationSpecificIndexType();
    void concatenateImplementationSpecificVertexFormat();
    void concatenateIntoNoMeshes();
@ -81,7 +84,8 @@ ConcatenateTest::ConcatenateTest() {
              &ConcatenateTest::concatenateUnsupportedPrimitive,
              &ConcatenateTest::concatenateInconsistentPrimitive,
              &ConcatenateTest::concatenateInconsistentAttributeFormat,
-              &ConcatenateTest::concatenateInconsistentAttributeArraySize,
+              &ConcatenateTest::concatenateInconsistentArrayAttribute,
+              &ConcatenateTest::concatenateTooLargeAttributeArraySize,
              &ConcatenateTest::concatenateImplementationSpecificIndexType,
              &ConcatenateTest::concatenateImplementationSpecificVertexFormat,
              &ConcatenateTest::concatenateIntoNoMeshes});
@ -94,7 +98,7 @@ struct VertexDataA {
    Vector2 texcoords2;
    Int:32;
    Vector3 position;
-    Short data[2];
+    Short data[3];
 };

 void ConcatenateTest::concatenate() {
@ -106,8 +110,8 @@ void ConcatenateTest::concatenate() {
    /* First is non-indexed, this layout (including the gap) will be
       preserved */
    const VertexDataA vertexDataA[]{
-        {{0.1f, 0.2f}, {0.5f, 0.6f}, {1.0f, 2.0f, 3.0f}, {15, 3}},
-        {{0.3f, 0.4f}, {0.7f, 0.8f}, {4.0f, 5.0f, 6.0f}, {14, 2}}
+        {{0.1f, 0.2f}, {0.5f, 0.6f}, {1.0f, 2.0f, 3.0f}, {15, 3, -1}},
+        {{0.3f, 0.4f}, {0.7f, 0.8f}, {4.0f, 5.0f, 6.0f}, {14, 2, -4}}
    };
    Containers::StridedArrayView1D<const VertexDataA> verticesA = vertexDataA;
    Trade::MeshData a{MeshPrimitive::Points, {}, vertexDataA, {
@ -119,7 +123,7 @@ void ConcatenateTest::concatenate() {
            verticesA.slice(&VertexDataA::position)},
        /* Array attribute to verify it's correctly propagated */
        Trade::MeshAttributeData{Trade::meshAttributeCustom(42),
-            VertexFormat::Short, verticesA.slice(&VertexDataA::data), 2}
+            VertexFormat::Short, verticesA.slice(&VertexDataA::data), 3}
    }};

    /* Second is indexed, has only one texture coordinate of the two, an extra
@ -127,13 +131,13 @@ void ConcatenateTest::concatenate() {
       zero-filled) */
    const struct VertexDataB {
        Color4 color;
-        Short data[2];
+        Short data[3];
        Vector2 texcoords1;
    } vertexDataB[]{
-        {0x112233_rgbf, {28, -15}, {0.15f, 0.25f}},
-        {0x445566_rgbf, {29, -16}, {0.35f, 0.45f}},
-        {0x778899_rgbf, {30, -17}, {0.55f, 0.65f}},
-        {0xaabbcc_rgbf, {40, -18}, {0.75f, 0.85f}}
+        {0x112233_rgbf, {28, -15, 0}, {0.15f, 0.25f}},
+        {0x445566_rgbf, {29, -16, 1}, {0.35f, 0.45f}},
+        {0x778899_rgbf, {30, -17, 2}, {0.55f, 0.65f}},
+        {0xaabbcc_rgbf, {40, -18, 3}, {0.75f, 0.85f}}
    };
    Containers::StridedArrayView1D<const VertexDataB> verticesB = vertexDataB;
    const UnsignedShort indicesB[]{0, 2, 1, 0, 3, 2};
@ -143,7 +147,7 @@ void ConcatenateTest::concatenate() {
                verticesB.slice(&VertexDataB::color)},
            /* Array attribute to verify it's correctly propagated */
            Trade::MeshAttributeData{Trade::meshAttributeCustom(42),
-                VertexFormat::Short, verticesB.slice(&VertexDataB::data), 2},
+                VertexFormat::Short, verticesB.slice(&VertexDataB::data), 3},
            Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates,
                verticesB.slice(&VertexDataB::texcoords1)}
        }};
@ -157,10 +161,11 @@ void ConcatenateTest::concatenate() {
        Vector3 position;
        Vector2 texcoords3;
        Vector2 texcoords1;
+        Short data[1];
    } vertexDataC[]{
-        {{0.425f, 0.475f}, {1.5f, 2.5f, 3.5f}, {0.725f, 0.775f}, {0.125f, 0.175f}},
-        {{0.525f, 0.575f}, {4.5f, 5.5f, 6.5f}, {0.825f, 0.875f}, {0.225f, 0.275f}},
-        {{0.625f, 0.675f}, {7.5f, 8.5f, 9.5f}, {0.925f, 0.975f}, {0.325f, 0.375f}},
+        {{0.425f, 0.475f}, {1.5f, 2.5f, 3.5f}, {0.725f, 0.775f}, {0.125f, 0.175f}, {320}},
+        {{0.525f, 0.575f}, {4.5f, 5.5f, 6.5f}, {0.825f, 0.875f}, {0.225f, 0.275f}, {3200}},
+        {{0.625f, 0.675f}, {7.5f, 8.5f, 9.5f}, {0.925f, 0.975f}, {0.325f, 0.375f}, {32000}},
    };
    Containers::StridedArrayView1D<const VertexDataC> verticesC = vertexDataC;
    Trade::MeshData c{MeshPrimitive::Points, {}, vertexDataC, {
@ -172,6 +177,9 @@ void ConcatenateTest::concatenate() {
            verticesC.slice(&VertexDataC::texcoords2)},
        Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates,
            verticesC.slice(&VertexDataC::texcoords3)},
+        /* Array attribute with less elements. The rest will be zero-filled. */
+        Trade::MeshAttributeData{Trade::meshAttributeCustom(42),
+            VertexFormat::Short, verticesC.slice(&VertexDataC::data), 1},
    }};

    /* To catch when the default argument becomes different */
@ -213,12 +221,13 @@ void ConcatenateTest::concatenate() {
        }), TestSuite::Compare::Container);
    CORRADE_COMPARE(dst.attributeName(3), Trade::meshAttributeCustom(42));
    CORRADE_COMPARE(dst.attributeFormat(3), VertexFormat::Short);
-    CORRADE_COMPARE(dst.attributeArraySize(3), 2);
-    CORRADE_COMPARE_AS((Containers::arrayCast<1, const Vector2s>(dst.attribute<Short[]>(3))),
-        Containers::arrayView<Vector2s>({
-            {15, 3}, {14, 2},
-            {28, -15}, {29, -16}, {30, -17}, {40, -18},
-            {}, {}, {}, /* Missing in the third mesh */
+    CORRADE_COMPARE(dst.attributeArraySize(3), 3);
+    CORRADE_COMPARE_AS((Containers::arrayCast<1, const Vector3s>(dst.attribute<Short[]>(3))),
+        Containers::arrayView<Vector3s>({
+            {15, 3, -1}, {14, 2, -4},
+            {28, -15, 0}, {29, -16, 1}, {30, -17, 2}, {40, -18, 3},
+            /* Last two components missing in the third mesh, kept at zeros */
+            {320, 0, 0}, {3200, 0, 0}, {32000, 0, 0},
        }), TestSuite::Compare::Container);
    CORRADE_VERIFY(dst.isIndexed());
    CORRADE_COMPARE(dst.indexType(), MeshIndexType::UnsignedInt);
@ -239,7 +248,7 @@ void ConcatenateTest::concatenate() {
        CORRADE_COMPARE(dst.attributeOffset(3), 2*sizeof(Vector2) + 4 + sizeof(Vector3));
    } else {
        /* Everything gets tightly packed */
-        CORRADE_COMPARE(dst.attributeStride(0), 2*sizeof(Vector2) + sizeof(Vector3) + 2*sizeof(Short));
+        CORRADE_COMPARE(dst.attributeStride(0), 2*sizeof(Vector2) + sizeof(Vector3) + 3*sizeof(Short));
        CORRADE_COMPARE(dst.attributeOffset(0), 0);
        CORRADE_COMPARE(dst.attributeOffset(1), sizeof(Vector2));
        CORRADE_COMPARE(dst.attributeOffset(2), 2*sizeof(Vector2));
@ -622,7 +631,7 @@ void ConcatenateTest::concatenateInconsistentAttributeFormat() {
        "MeshTools::concatenateInto(): expected VertexFormat::Vector3ubNormalized for attribute 2 (Trade::MeshAttribute::Color) but got VertexFormat::Vector3usNormalized in mesh 3 attribute 1\n");
 }

-void ConcatenateTest::concatenateInconsistentAttributeArraySize() {
+void ConcatenateTest::concatenateInconsistentArrayAttribute() {
    CORRADE_SKIP_IF_NO_ASSERT();

    /* Things are a bit duplicated to test correct numbering */
@ -632,22 +641,70 @@ void ConcatenateTest::concatenateInconsistentAttributeArraySize() {
        Trade::MeshAttributeData{Trade::MeshAttribute::Position,
            VertexFormat::Vector3, nullptr},
        Trade::MeshAttributeData{Trade::meshAttributeCustom(42),
-            VertexFormat::ByteNormalized, nullptr, 5}
+            VertexFormat::ByteNormalized, nullptr, 4}
+    }};
+    /* Copy of `a` so we can (destructively) concatenateInto() it and then use
+       `a` again in another assert */
+    Trade::MeshData a2{MeshPrimitive::Lines, nullptr, {
+        Trade::MeshAttributeData{Trade::MeshAttribute::Position,
+            VertexFormat::Vector3, nullptr},
+        Trade::MeshAttributeData{Trade::MeshAttribute::Position,
+            VertexFormat::Vector3, nullptr},
+        Trade::MeshAttributeData{Trade::meshAttributeCustom(42),
+            VertexFormat::ByteNormalized, nullptr, 4}
    }};
    Trade::MeshData b{MeshPrimitive::Lines, nullptr, {
+        Trade::MeshAttributeData{Trade::MeshAttribute::Position,
+            VertexFormat::Vector3, nullptr},
+        Trade::MeshAttributeData{Trade::meshAttributeCustom(42),
+            VertexFormat::ByteNormalized, nullptr}
+    }};
+
+    std::ostringstream out;
+    Error redirectError{&out};
+    MeshTools::concatenate({a, a, a, a, b});
+    MeshTools::concatenate({b, b, b, b, a});
+    MeshTools::concatenateInto(a2, {a, a, a, b});
+    MeshTools::concatenateInto(b, {b, b, b, a});
+    CORRADE_COMPARE_AS(out.str(),
+        "MeshTools::concatenate(): attribute 2 (Trade::MeshAttribute::Custom(42)) is an array but attribute 1 in mesh 4 isn't\n"
+        "MeshTools::concatenate(): attribute 1 (Trade::MeshAttribute::Custom(42)) isn't an array but attribute 2 in mesh 4 is\n"
+        "MeshTools::concatenateInto(): attribute 2 (Trade::MeshAttribute::Custom(42)) is an array but attribute 1 in mesh 3 isn't\n"
+        "MeshTools::concatenateInto(): attribute 1 (Trade::MeshAttribute::Custom(42)) isn't an array but attribute 2 in mesh 3 is\n",
+        TestSuite::Compare::String);
+}
+
+void ConcatenateTest::concatenateTooLargeAttributeArraySize() {
+    CORRADE_SKIP_IF_NO_ASSERT();
+
+    /* Things are a bit duplicated to test correct numbering */
+    Trade::MeshData a{MeshPrimitive::Lines, nullptr, {
+        Trade::MeshAttributeData{Trade::MeshAttribute::Position,
+            VertexFormat::Vector3, nullptr},
        Trade::MeshAttributeData{Trade::MeshAttribute::Position,
            VertexFormat::Vector3, nullptr},
        Trade::MeshAttributeData{Trade::meshAttributeCustom(42),
            VertexFormat::ByteNormalized, nullptr, 4}
    }};
+    Trade::MeshData b{MeshPrimitive::Lines, nullptr, {
+        Trade::MeshAttributeData{Trade::MeshAttribute::Position,
+            VertexFormat::Vector3, nullptr},
+        Trade::MeshAttributeData{Trade::meshAttributeCustom(42),
+            VertexFormat::ByteNormalized, nullptr, 5}
+    }};
+
+    /* Using a lower array size in subsequent meshes is fine (tested in
+       concatenate() above) */
+    MeshTools::concatenate({b, a});

    std::ostringstream out;
    Error redirectError{&out};
    MeshTools::concatenate({a, a, a, a, b});
    MeshTools::concatenateInto(a, {a, a, a, b});
-    CORRADE_COMPARE(out.str(),
-        "MeshTools::concatenate(): expected array size 5 for attribute 2 (Trade::MeshAttribute::Custom(42)) but got 4 in mesh 4 attribute 1\n"
-        "MeshTools::concatenateInto(): expected array size 5 for attribute 2 (Trade::MeshAttribute::Custom(42)) but got 4 in mesh 3 attribute 1\n");
+    CORRADE_COMPARE_AS(out.str(),
+        "MeshTools::concatenate(): expected array size 4 or less for attribute 2 (Trade::MeshAttribute::Custom(42)) but got 5 in mesh 4 attribute 1\n"
+        "MeshTools::concatenateInto(): expected array size 4 or less for attribute 2 (Trade::MeshAttribute::Custom(42)) but got 5 in mesh 3 attribute 1\n",
+        TestSuite::Compare::String);
 }

 void ConcatenateTest::concatenateImplementationSpecificIndexType() {