From faf063764aeff1fd4b9af9b3f2ff5e61c2de5d90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Fri, 21 Jan 2022 18:46:03 +0100 Subject: [PATCH] MeshTools: make interleave() aware of impl-spec index types. As implementation-specific index types cause the index buffer to be strided. which is not allowed by default, these work only if InterleaveFlag::PreserveStridedIndices is set. Originally I thought about straight-out disallowing implementation-specific types here, unfortunately that broke some valid use cases so I backtracked on that decision. In the future I might change the default to preserve implementation-specific indices with sane strides (i.e., positive powers of 2), but that will only be done once similar treatment is finished for attributes (currently zero and negative strides are just disallowed completely). --- src/Magnum/MeshTools/Interleave.cpp | 11 ++++-- src/Magnum/MeshTools/Interleave.h | 12 ++++-- src/Magnum/MeshTools/Test/InterleaveTest.cpp | 41 +++++++++++++++----- 3 files changed, 47 insertions(+), 17 deletions(-) diff --git a/src/Magnum/MeshTools/Interleave.cpp b/src/Magnum/MeshTools/Interleave.cpp index 778802ccf..219fc71c1 100644 --- a/src/Magnum/MeshTools/Interleave.cpp +++ b/src/Magnum/MeshTools/Interleave.cpp @@ -265,11 +265,10 @@ Trade::MeshData interleave(Trade::MeshData&& data, const Containers::ArrayView{ data.indexData(), @@ -291,8 +290,14 @@ Trade::MeshData interleave(Trade::MeshData&& data, const Containers::ArrayView(meshIndexTypeUnwrap(indexType)) << Debug::nospace << ", enable MeshTools::InterleaveFlag::PreserveStridedIndices to pass the array through unchanged", + (Trade::MeshData{MeshPrimitive{}, 0})); + + const std::size_t indexTypeSize = meshIndexTypeSize(indexType); indexData = Containers::Array{NoInit, data.indexCount()*indexTypeSize}; Containers::StridedArrayView2D out{indexData, {data.indexCount(), indexTypeSize}, diff --git a/src/Magnum/MeshTools/Interleave.h b/src/Magnum/MeshTools/Interleave.h index 4420aabc1..ece30f231 100644 --- a/src/Magnum/MeshTools/Interleave.h +++ b/src/Magnum/MeshTools/Interleave.h @@ -219,12 +219,14 @@ enum class InterleaveFlag: UnsignedInt { * * 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. + * padding before or after the original index view. In such case however, + * the index type is not allowed to be implementation-specific. * * Has no effect when passed to @ref interleavedLayout(const Trade::MeshData&, UnsignedInt, Containers::ArrayView, InterleaveFlags) "interleavedLayout()" * as that function doesn't preserve the index buffer. Has no effect when * passed to @ref concatenate(Containers::ArrayView>, InterleaveFlags) "concatenate()" * as that function allocates a new combined index buffer anyway. + * @see @ref isMeshIndexTypeImplementationSpecific() */ PreserveStridedIndices = 1 << 1 }; @@ -361,8 +363,9 @@ 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. +Indices (if any) are kept as-is only if they're tightly packed and not with an +implementation-specific type. 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 @@ -373,7 +376,8 @@ to avoid that copy. All attributes in both @p data and @p extra are expected to not have an implementation-specific format, except for @p data attributes in case @p data is already interleaved, then the layout is untouched. -@see @ref isInterleaved(), @ref isVertexFormatImplementationSpecific(), +@see @ref isInterleaved(), @ref isMeshIndexTypeImplementationSpecific(), + @ref isVertexFormatImplementationSpecific(), @ref Trade::MeshData::attributeData() */ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData interleave(const Trade::MeshData& data, Containers::ArrayView extra = {}, InterleaveFlags flags = InterleaveFlag::PreserveInterleavedAttributes); diff --git a/src/Magnum/MeshTools/Test/InterleaveTest.cpp b/src/Magnum/MeshTools/Test/InterleaveTest.cpp index 834b54917..9308fd6ad 100644 --- a/src/Magnum/MeshTools/Test/InterleaveTest.cpp +++ b/src/Magnum/MeshTools/Test/InterleaveTest.cpp @@ -86,6 +86,7 @@ struct InterleaveTest: Corrade::TestSuite::Tester { void interleaveMeshData(); void interleaveMeshDataIndexed(); + void interleaveMeshDataImplementationSpecificIndexType(); void interleaveMeshDataImplementationSpecificVertexFormat(); void interleaveMeshDataExtra(); void interleaveMeshDataExtraEmpty(); @@ -112,13 +113,15 @@ const struct { const struct { const char* name; + MeshIndexType indexType; Containers::Optional 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} + {"", MeshIndexType::UnsignedShort, {}, false, false, true}, + {"strided indices", MeshIndexType::UnsignedShort, {}, true, false, false}, + {"strided indices, preserved", MeshIndexType::UnsignedShort, InterleaveFlag::PreserveInterleavedAttributes|InterleaveFlag::PreserveStridedIndices, true, true, true}, + {"strided indices, implementation-specific index type, preserved", meshIndexTypeWrap(0xbaf), InterleaveFlag::PreserveInterleavedAttributes|InterleaveFlag::PreserveStridedIndices, true, true, true} }; InterleaveTest::InterleaveTest() { @@ -171,7 +174,8 @@ InterleaveTest::InterleaveTest() { addInstancedTests({&InterleaveTest::interleaveMeshDataIndexed}, Containers::arraySize(StridedIndicesData)); - addTests({&InterleaveTest::interleaveMeshDataImplementationSpecificVertexFormat, + addTests({&InterleaveTest::interleaveMeshDataImplementationSpecificIndexType, + &InterleaveTest::interleaveMeshDataImplementationSpecificVertexFormat, &InterleaveTest::interleaveMeshDataExtra, &InterleaveTest::interleaveMeshDataExtraEmpty, &InterleaveTest::interleaveMeshDataExtraOriginalEmpty, @@ -1192,7 +1196,7 @@ void InterleaveTest::interleaveMeshDataIndexed() { Vector2 positions[]{{1.3f, 0.3f}, {0.87f, 1.1f}, {1.0f, -0.5f}}; Trade::MeshData mesh{MeshPrimitive::TriangleFan, - {}, Containers::arrayView(indexData), Trade::MeshIndexData{indices}, + {}, Containers::arrayView(indexData), Trade::MeshIndexData{data.indexType, indices}, {}, Containers::arrayView(positions), { Trade::MeshAttributeData{Trade::MeshAttribute::Position, Containers::arrayView(positions)} }}; @@ -1204,8 +1208,8 @@ void InterleaveTest::interleaveMeshDataIndexed() { CORRADE_COMPARE(interleaved.primitive(), MeshPrimitive::TriangleFan); CORRADE_VERIFY(interleaved.isIndexed()); - CORRADE_COMPARE(interleaved.indexType(), MeshIndexType::UnsignedShort); - CORRADE_COMPARE_AS(interleaved.indices(), + CORRADE_COMPARE(interleaved.indexType(), data.indexType); + CORRADE_COMPARE_AS((Containers::arrayCast<1, const UnsignedShort>(interleaved.indices())), Containers::arrayView({0, 2, 1}), TestSuite::Compare::Container); @@ -1227,6 +1231,23 @@ void InterleaveTest::interleaveMeshDataIndexed() { TestSuite::Compare::Container); } +void InterleaveTest::interleaveMeshDataImplementationSpecificIndexType() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + Trade::MeshData data{MeshPrimitive::Points, + nullptr, Trade::MeshIndexData{meshIndexTypeWrap(0xcaca), Containers::StridedArrayView1D{}}, + nullptr, { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, VertexFormat::Vector2, nullptr} + }}; + + std::ostringstream out; + Error redirectError{&out}; + MeshTools::interleave(data); + CORRADE_COMPARE(out.str(), "MeshTools::interleave(): mesh has an implementation-specific index type 0xcaca, enable MeshTools::InterleaveFlag::PreserveStridedIndices to pass the array through unchanged\n"); +} + void InterleaveTest::interleaveMeshDataImplementationSpecificVertexFormat() { #ifdef CORRADE_NO_ASSERT CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); @@ -1438,7 +1459,7 @@ void InterleaveTest::interleaveMeshDataAlreadyInterleavedMoveIndices() { const Trade::MeshAttributeData* attributePointer = attributeData; Trade::MeshData mesh{MeshPrimitive::TriangleFan, - std::move(indexData), Trade::MeshIndexData{indices}, + std::move(indexData), Trade::MeshIndexData{data.indexType,indices}, std::move(vertexData), std::move(attributeData)}; CORRADE_VERIFY(MeshTools::isInterleaved(mesh)); @@ -1448,8 +1469,8 @@ void InterleaveTest::interleaveMeshDataAlreadyInterleavedMoveIndices() { MeshTools::interleave(std::move(mesh)); CORRADE_VERIFY(MeshTools::isInterleaved(interleaved)); - CORRADE_COMPARE(interleaved.indexType(), MeshIndexType::UnsignedShort); - CORRADE_COMPARE_AS(interleaved.indices(), + CORRADE_COMPARE(interleaved.indexType(), data.indexType); + CORRADE_COMPARE_AS((Containers::arrayCast<1, const UnsignedShort>(interleaved.indices())), Containers::arrayView({0, 2, 1}), TestSuite::Compare::Container);