MeshTools: interleave() preserves strided indices only if asked.

Otherwise it makes a tightly-packed copy. This also means that arbitrary
padding before/after the index buffer is preserved only in case it would
mean the data can be transferred without a copy -- otherwise it's faster
to just drop the padding and copy only the used part.

src/Magnum/MeshTools/Interleave.cpp

@@ -265,15 +265,41 @@ Trade::MeshData interleave(Trade::MeshData&& data, const Containers::ArrayView<c
     Containers::Array<char> indexData;
     Trade::MeshIndexData indices;
     if(data.isIndexed()) {
-        /* If we can steal the data, do it */
-        if(data.indexDataFlags() & Trade::DataFlag::Owned) {
-            indices = Trade::MeshIndexData{data.indices()};
+        const MeshIndexType indexType = data.indexType();
+        const std::size_t indexTypeSize = meshIndexTypeSize(indexType);
+
+        /* If we can steal the data and we're allowed to preserve a strided
+           layout or it's tightly packed, do the steal */
+        if((data.indexDataFlags() & Trade::DataFlag::Owned) && ((flags & InterleaveFlag::PreserveStridedIndices) || data.indexStride() == Int(indexTypeSize))) {
+            indices = Trade::MeshIndexData{indexType,
+                Containers::StridedArrayView1D<const void>{
+                    data.indexData(),
+                    data.indexData().data() + data.indexOffset(),
+                    data.indexCount(),
+                    data.indexStride()}};
             indexData = data.releaseIndexData();
-        } else {
-            indexData = Containers::Array<char>{data.indexData().size()};
+
+        /* Otherwise, if we can't steal the data but we're told to preserve
+           strided indices, make a full copy including any extra offsets and
+           paddings */
+        } else if(flags & InterleaveFlag::PreserveStridedIndices) {
+            indexData = Containers::Array<char>{NoInit, data.indexData().size()};
+            indices = Trade::MeshIndexData{indexType,
+                Containers::StridedArrayView1D<const void>{
+                    indexData,
+                    indexData.data() + data.indexOffset(),
+                    data.indexCount(),
+                    data.indexStride()}};
             Utility::copy(data.indexData(), indexData);
-            indices = Trade::MeshIndexData{data.indexType(),
-                Containers::ArrayView<const void>{indexData + data.indexOffset(), data.indices().size()[0]*data.indices().size()[1]}};
+
+        /* Otherwise, make a tightly packed copy */
+        } else {
+            indexData = Containers::Array<char>{NoInit, data.indexCount()*indexTypeSize};
+            Containers::StridedArrayView2D<char> out{indexData,
+                {data.indexCount(), indexTypeSize},
+                {std::ptrdiff_t(indexTypeSize), 1}};
+            indices = Trade::MeshIndexData{out};
+            Utility::copy(data.indices(), out);
         }
     }
 

src/Magnum/MeshTools/Interleave.h

@@ -209,7 +209,24 @@ enum class InterleaveFlag: UnsignedInt {
      * removing all padding. In that case an implementation-specific
      * @ref VertexFormat can't be used for any attribute.
      */
-    PreserveInterleavedAttributes = 1 << 0
+    PreserveInterleavedAttributes = 1 << 0,
+
+    /**
+     * If a mesh is indexed, makes @ref interleave(const Trade::MeshData&, Containers::ArrayView<const Trade::MeshAttributeData>, InterleaveFlags)
+     * preserve the index buffer even if it's not tightly packed. Since such
+     * data layouts are not commonly supported by GPU APIs, this flag is not
+     * set by default.
+     *
+     * If not set and the index buffer is strided, a tightly packed copy with
+     * the same index type is allocated for the output, dropping also any
+     * padding before or after the original index view.
+     *
+     * Has no effect when passed to @ref interleavedLayout(const Trade::MeshData&, UnsignedInt, Containers::ArrayView<const Trade::MeshAttributeData>, InterleaveFlags) "interleavedLayout()"
+     * as that function doesn't preserve the index buffer. Has no effect when
+     * passed to @ref concatenate(Containers::ArrayView<const Containers::Reference<const Trade::MeshData>>, InterleaveFlags) "concatenate()"
+     * as that function allocates a new combined index buffer anyway.
+     */
+    PreserveStridedIndices = 1 << 1
 };
 
 /**
@@ -335,14 +352,17 @@ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData interleavedLayout(Trade::MeshData&& data
 @brief Interleave mesh data
 @m_since{2020,06}
 
-Returns a copy of @p data with all attributes interleaved. Indices (if any) are
-kept as-is. The @p extra attributes, if any, are interleaved together with
-existing attributes (or, in case the attribute view is empty, only the
-corresponding space for given attribute type is reserved, with memory left
-uninitialized). The data layouting is done by @ref interleavedLayout() with the
-@p flags parameter propagated to it, see its documentation for detailed
-behavior description. Note that offset-only @ref Trade::MeshAttributeData
-instances are not supported in the @p extra array.
+Returns a copy of @p data with all attributes interleaved. The @p extra
+attributes, if any, are interleaved together with existing attributes (or, in
+case the attribute view is empty, only the corresponding space for given
+attribute type is reserved, with memory left uninitialized). The data layouting
+is done by @ref interleavedLayout() with the @p flags parameter propagated to
+it, see its documentation for detailed behavior description. Note that
+offset-only @ref Trade::MeshAttributeData instances are not supported in the
+@p extra array.
+
+Indices (if any) are kept as-is only if they're tightly packed. Otherwise the
+behavior depends on presence of @ref InterleaveFlag::PreserveStridedIndices.
 
 Expects that each attribute in @p extra has either the same amount of elements
 as @p data vertex count or has none. This function will unconditionally make a

src/Magnum/MeshTools/Test/InterleaveTest.cpp

@@ -28,6 +28,7 @@
 #include <Corrade/Containers/Optional.h>
 #include <Corrade/TestSuite/Tester.h>
 #include <Corrade/TestSuite/Compare/Container.h>
+#include <Corrade/Utility/Algorithms.h>
 #include <Corrade/Utility/Endianness.h>
 #include <Corrade/Utility/Debug.h>
 #include <Corrade/Utility/DebugStl.h>
@@ -93,6 +94,7 @@ struct InterleaveTest: Corrade::TestSuite::Tester {
     void interleaveMeshDataExtraOffsetOnly();
     void interleaveMeshDataExtraImplementationSpecificVertexFormat();
     void interleaveMeshDataAlreadyInterleavedMove();
+    void interleaveMeshDataAlreadyInterleavedMoveIndices();
     void interleaveMeshDataAlreadyInterleavedMoveNonOwned();
     void interleaveMeshDataNothing();
 };
@@ -108,6 +110,17 @@ const struct {
     {"don't preserve layout", VertexFormat::Vector3, InterleaveFlags{}, false}
 };
 
+const struct {
+    const char* name;
+    Containers::Optional<InterleaveFlags> flags;
+    bool flip;
+    bool shouldPreserveLayoutInCopy, shouldPreserveLayoutInMove;
+} StridedIndicesData[]{
+    {"", {}, false, false, true},
+    {"strided indices", {}, true, false, false},
+    {"strided indices, preserved", InterleaveFlag::PreserveInterleavedAttributes|InterleaveFlag::PreserveStridedIndices, true, true, true}
+};
+
 InterleaveTest::InterleaveTest() {
     addTests({&InterleaveTest::attributeCount,
               &InterleaveTest::attributeCountGaps,
@@ -153,9 +166,12 @@ InterleaveTest::InterleaveTest() {
     addTests({&InterleaveTest::interleavedLayoutNothing,
               &InterleaveTest::interleavedLayoutRvalue,
 
-              &InterleaveTest::interleaveMeshData,
-              &InterleaveTest::interleaveMeshDataIndexed,
-              &InterleaveTest::interleaveMeshDataImplementationSpecificVertexFormat,
+              &InterleaveTest::interleaveMeshData});
+
+    addInstancedTests({&InterleaveTest::interleaveMeshDataIndexed},
+        Containers::arraySize(StridedIndicesData));
+
+    addTests({&InterleaveTest::interleaveMeshDataImplementationSpecificVertexFormat,
               &InterleaveTest::interleaveMeshDataExtra,
               &InterleaveTest::interleaveMeshDataExtraEmpty,
               &InterleaveTest::interleaveMeshDataExtraOriginalEmpty,
@@ -166,6 +182,9 @@ InterleaveTest::InterleaveTest() {
     addInstancedTests({&InterleaveTest::interleaveMeshDataAlreadyInterleavedMove},
         Containers::arraySize(AlreadyInterleavedData));
 
+    addInstancedTests({&InterleaveTest::interleaveMeshDataAlreadyInterleavedMoveIndices},
+        Containers::arraySize(StridedIndicesData));
+
     addTests({&InterleaveTest::interleaveMeshDataAlreadyInterleavedMoveNonOwned,
               &InterleaveTest::interleaveMeshDataNothing});
 }
@@ -1162,27 +1181,46 @@ void InterleaveTest::interleaveMeshData() {
 }
 
 void InterleaveTest::interleaveMeshDataIndexed() {
+    auto&& data = StridedIndicesData[testCaseInstanceId()];
+    setTestCaseDescription(data.name);
+
     /* Testing also offset */
     UnsignedShort indexData[50 + 3];
-    indexData[50] = 0;
-    indexData[51] = 2;
-    indexData[52] = 1;
+    Containers::StridedArrayView1D<UnsignedShort> indices = Containers::arrayView(indexData).suffix(50);
+    if(data.flip) indices = indices.flipped<0>();
+    Utility::copy({0, 2, 1}, indices);
+
     Vector2 positions[]{{1.3f, 0.3f}, {0.87f, 1.1f}, {1.0f, -0.5f}};
-    Trade::MeshData data{MeshPrimitive::TriangleFan,
-        {}, Containers::arrayView(indexData), Trade::MeshIndexData{Containers::arrayView(indexData).suffix(50)},
+    Trade::MeshData mesh{MeshPrimitive::TriangleFan,
+        {}, Containers::arrayView(indexData), Trade::MeshIndexData{indices},
         {}, Containers::arrayView(positions), {
             Trade::MeshAttributeData{Trade::MeshAttribute::Position, Containers::arrayView(positions)}
         }};
 
-    Trade::MeshData interleaved = MeshTools::interleave(data);
+    Trade::MeshData interleaved = data.flags ?
+        MeshTools::interleave(mesh, {}, *data.flags) :
+        MeshTools::interleave(mesh);
     CORRADE_VERIFY(MeshTools::isInterleaved(interleaved));
     CORRADE_COMPARE(interleaved.primitive(), MeshPrimitive::TriangleFan);
+
     CORRADE_VERIFY(interleaved.isIndexed());
     CORRADE_COMPARE(interleaved.indexType(), MeshIndexType::UnsignedShort);
-    CORRADE_COMPARE(interleaved.indexData().size(), 106);
     CORRADE_COMPARE_AS(interleaved.indices<UnsignedShort>(),
-        Containers::arrayView(indexData).suffix(50),
+        Containers::arrayView<UnsignedShort>({0, 2, 1}),
         TestSuite::Compare::Container);
+
+    if(data.shouldPreserveLayoutInCopy) {
+        CORRADE_COMPARE(interleaved.indexData().size(), sizeof(indexData));
+        CORRADE_COMPARE(interleaved.indexOffset(), static_cast<const char*>(indices.data()) - reinterpret_cast<const char*>(indexData));
+        CORRADE_COMPARE(interleaved.indexStride(), indices.stride());
+    } else {
+        /* Only the actually used part of the index buffer gets transferred and
+           is tightly packed */
+        CORRADE_COMPARE(interleaved.indexData().size(), 3*sizeof(UnsignedShort));
+        CORRADE_COMPARE(interleaved.indexOffset(), 0);
+        CORRADE_COMPARE(interleaved.indexStride(), sizeof(UnsignedShort));
+    }
+
     CORRADE_COMPARE(interleaved.attributeCount(), 1);
     CORRADE_COMPARE_AS(interleaved.attribute<Vector2>(Trade::MeshAttribute::Position),
         Containers::stridedArrayView(positions),
@@ -1381,6 +1419,64 @@ void InterleaveTest::interleaveMeshDataAlreadyInterleavedMove() {
     }
 }
 
+void InterleaveTest::interleaveMeshDataAlreadyInterleavedMoveIndices() {
+    auto&& data = StridedIndicesData[testCaseInstanceId()];
+    setTestCaseDescription(data.name);
+
+    /* Testing also offset */
+    Containers::Array<char> indexData{(50 + 3)*sizeof(UnsignedShort)};
+    Containers::StridedArrayView1D<UnsignedShort> indices = Containers::arrayCast<UnsignedShort>(indexData).suffix(50);
+    if(data.flip) indices = indices.flipped<0>();
+    Utility::copy({0, 2, 1}, indices);
+
+    Containers::Array<char> vertexData{3*8};
+    Containers::StridedArrayView1D<Vector2> positionView = Containers::arrayCast<Vector2>(vertexData);
+    auto attributeData = Containers::array({
+        Trade::MeshAttributeData{Trade::MeshAttribute::Position, positionView},
+    });
+    const void* indexPointer = indexData;
+    const Trade::MeshAttributeData* attributePointer = attributeData;
+
+    Trade::MeshData mesh{MeshPrimitive::TriangleFan,
+        std::move(indexData), Trade::MeshIndexData{indices},
+        std::move(vertexData), std::move(attributeData)};
+    CORRADE_VERIFY(MeshTools::isInterleaved(mesh));
+
+    /* To catch when the default argument becomes different */
+    Trade::MeshData interleaved = data.flags ?
+        MeshTools::interleave(std::move(mesh), {}, *data.flags) :
+        MeshTools::interleave(std::move(mesh));
+
+    CORRADE_VERIFY(MeshTools::isInterleaved(interleaved));
+    CORRADE_COMPARE(interleaved.indexType(), MeshIndexType::UnsignedShort);
+    CORRADE_COMPARE_AS(interleaved.indices<UnsignedShort>(),
+        Containers::arrayView<UnsignedShort>({0, 2, 1}),
+        TestSuite::Compare::Container);
+
+    if(data.shouldPreserveLayoutInMove) {
+        /* Indices got just moved without copying, with all metadata
+           preserved */
+        CORRADE_VERIFY(interleaved.indexData().data() == indexPointer);
+        CORRADE_COMPARE(interleaved.indexData().size(), (50 + 3)*sizeof(UnsignedShort));
+        CORRADE_COMPARE(interleaved.indexOffset(), static_cast<const char*>(indices.data()) - reinterpret_cast<const char*>(indexPointer));
+        CORRADE_COMPARE(interleaved.indexStride(), indices.stride());
+    } else {
+        /* Only the actually used part of the index buffer gets transferred and
+           is tightly packed */
+        CORRADE_VERIFY(interleaved.indexData().data() != indexData);
+        CORRADE_COMPARE(interleaved.indexData().size(), 3*sizeof(UnsignedShort));
+        CORRADE_COMPARE(interleaved.indexOffset(), 0);
+        CORRADE_COMPARE(interleaved.indexStride(), sizeof(UnsignedShort));
+    }
+
+    CORRADE_COMPARE(interleaved.attributeCount(), 1);
+    CORRADE_COMPARE(interleaved.attributeStride(0), 8);
+    CORRADE_VERIFY(interleaved.attributeData().data() == attributePointer);
+
+    CORRADE_COMPARE(interleaved.vertexCount(), 3);
+    CORRADE_VERIFY(interleaved.vertexData().data() == positionView.data());
+}
+
 void InterleaveTest::interleaveMeshDataAlreadyInterleavedMoveNonOwned() {
     Containers::Array<char> indexData{4};
     auto indexView = Containers::arrayCast<UnsignedShort>(indexData);