From 32fc6f003d084f7d71b263d10555eb6a39406f0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 11 Nov 2024 20:06:48 +0100 Subject: [PATCH] MeshTools: add a MeshData interleave() overload for loose arrays. It went for so long without such a thing, causing so much suffering. --- doc/changelog.dox | 3 + doc/snippets/MeshTools.cpp | 15 +++ src/Magnum/MeshTools/Interleave.cpp | 50 ++++++++- src/Magnum/MeshTools/Interleave.h | 75 +++++++++++-- src/Magnum/MeshTools/Test/InterleaveTest.cpp | 106 ++++++++++++++++++- 5 files changed, 236 insertions(+), 13 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index b9ab544de..b018316e4 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -318,6 +318,9 @@ See also: - New @ref MeshTools::compileLines() utility for creating meshes compatible with the new @ref Shaders::LineGL. See also [mosra/magnum#601](https://github.com/mosra/magnum/pull/601). +- New @ref MeshTools::interleave(MeshPrimitive, const Trade::MeshIndexData&, Containers::ArrayView) + overload for conveniently creating an interleaved mesh out of loose index + and attribute arrays @subsubsection changelog-latest-new-platform Platform libraries diff --git a/doc/snippets/MeshTools.cpp b/doc/snippets/MeshTools.cpp index 794e3a72d..6051d2571 100644 --- a/doc/snippets/MeshTools.cpp +++ b/doc/snippets/MeshTools.cpp @@ -154,6 +154,21 @@ auto data = MeshTools::interleave(positions, weights, 2, vertexColors, 1); /* [interleave2] */ } +{ +/* [interleave-meshdata] */ +Containers::ArrayView indices = DOXYGEN_ELLIPSIS({}); +Containers::ArrayView positions = DOXYGEN_ELLIPSIS({}); +Containers::ArrayView normals = DOXYGEN_ELLIPSIS({}); + +Trade::MeshData mesh = MeshTools::interleave( + MeshPrimitive::Triangles, + Trade::MeshIndexData{indices}, { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, positions}, + Trade::MeshAttributeData{Trade::MeshAttribute::Normal, normals}, + }); +/* [interleave-meshdata] */ +} + { Trade::MeshData data{MeshPrimitive::Lines, 0}; UnsignedInt vertexCount{}; diff --git a/src/Magnum/MeshTools/Interleave.cpp b/src/Magnum/MeshTools/Interleave.cpp index a061e5728..d45b202dd 100644 --- a/src/Magnum/MeshTools/Interleave.cpp +++ b/src/Magnum/MeshTools/Interleave.cpp @@ -350,7 +350,7 @@ Trade::MeshData interleave(Trade::MeshData&& mesh, const Containers::ArrayView attributes) { + /* Get vertex count from the first non-padding attribute. Checking that all + arrays have the same size etc is done in the delegated-to function. */ + UnsignedInt vertexCount = ~UnsignedInt{}; + for(const Trade::MeshAttributeData& attribute: attributes) { + if(attribute.format() != VertexFormat{}) { + vertexCount = attribute.data().size(); + break; + } + } + CORRADE_ASSERT(vertexCount != ~UnsignedInt{}, + "MeshTools::interleave(): only padding found among" << attributes.size() << "attributes, can't infer vertex count", + (Trade::MeshData{MeshPrimitive::Triangles, 0})); + + /* Check that indices aren't implementation-specific. The assert inside the + delegated-to interleave() suggests PreserveStridedIndices, which would + be confusing as here it's no such argument */ + CORRADE_ASSERT(indices.type() == MeshIndexType{} || !isMeshIndexTypeImplementationSpecific(indices.type()), + "MeshTools::interleave(): implementation-specific index type" << Debug::hex << meshIndexTypeUnwrap(indices.type()), + (Trade::MeshData{MeshPrimitive{}, 0})); + + return interleave(Trade::MeshData{primitive, + /* Pass indices as non-owned so they get copied. We can say the index + data is the whole memory as it's not going to get used because the + indices get tightly packed. */ + {}, + indices.type() == MeshIndexType{} ? + nullptr : Containers::ArrayView{nullptr, ~std::size_t{}}, + indices, + vertexCount}, + attributes, + /* Explicitly *not* PreserveStridedIndices to ensure the indices get + tightly packed */ + InterleaveFlags{}); +} + +Trade::MeshData interleave(const MeshPrimitive primitive, const Trade::MeshIndexData& indices, const std::initializer_list attributes) { + return interleave(primitive, indices, Containers::arrayView(attributes)); +} + +Trade::MeshData interleave(const MeshPrimitive primitive, const Containers::ArrayView attributes) { + return interleave(primitive, Trade::MeshIndexData{}, attributes); +} + +Trade::MeshData interleave(const MeshPrimitive primitive, const std::initializer_list attributes) { + return interleave(primitive, Containers::arrayView(attributes)); +} + }} diff --git a/src/Magnum/MeshTools/Interleave.h b/src/Magnum/MeshTools/Interleave.h index ce5f79926..8426910b9 100644 --- a/src/Magnum/MeshTools/Interleave.h +++ b/src/Magnum/MeshTools/Interleave.h @@ -303,16 +303,18 @@ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData interleavedLayout(Trade::MeshData&& mesh Returns a copy of @p mesh 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 and not with an -implementation-specific type. Otherwise the behavior depends on presence of -@ref InterleaveFlag::PreserveStridedIndices. +case the attribute view is null, only the corresponding space for given +attribute type is reserved, with memory left uninitialized). See the +@ref interleave(MeshPrimitive, const Trade::MeshIndexData&, Containers::ArrayView) +overload if you only have loose attributes and want to interleave them +together. + +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 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 mesh vertex count or has none. This function will unconditionally make a @@ -356,6 +358,59 @@ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData interleave(Trade::MeshData&& mesh, Conta */ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData interleave(Trade::MeshData&& mesh, std::initializer_list extra, InterleaveFlags flags = InterleaveFlag::PreserveInterleavedAttributes); +/** +@brief Create an indexed interleaved mesh +@m_since_latest + +Creates a mesh data instance out of given indices and attributes. Usage +example: + +@snippet MeshTools.cpp interleave-meshdata + +The @ref interleave(MeshPrimitive, Containers::ArrayView) +overload creates a non-indexed mesh. This function is a convenience shorthand +for calling @ref interleave(const Trade::MeshData&, Containers::ArrayView, InterleaveFlags) +with a @ref Trade::MeshData instance created out of @p primitive and +@p indices and vertex count matching @p attributes. If a particular attribute +view is null, only the corresponding space for given attribute type is +reserved, with memory left uninitialized. The attribute can also be a are a +padding value created with @ref Trade::MeshAttributeData::MeshAttributeData(Int), +see documentation of @ref interleavedLayout() for an example snippet. + +Expects that @p attributes all have the same amount of elements or have none, +there's at least one non-padding attribute, none of them have an +implementation-specific format and none of them are offset-only +@ref Trade::MeshAttributeData instances. The @p indices, if present, are +assumed to not have an implementation-specific type. Returned instance vertex +and index data flags have both @ref Trade::DataFlag::Mutable and +@ref Trade::DataFlag::Owned, so mutable attribute access is guaranteed. +*/ +MAGNUM_MESHTOOLS_EXPORT Trade::MeshData interleave(MeshPrimitive primitive, const Trade::MeshIndexData& indices, Containers::ArrayView attributes); + +/** +@overload +@m_since_latest +*/ +MAGNUM_MESHTOOLS_EXPORT Trade::MeshData interleave(MeshPrimitive primitive, const Trade::MeshIndexData& indices, std::initializer_list attributes); + +/** +@brief Create a non-indexed interleaved mesh +@m_since_latest + +Same as calling @ref interleave(MeshPrimitive, const Trade::MeshIndexData&, Containers::ArrayView) +with a default-constructed @ref Trade::MeshIndexData instance. See its +documentation for more information and a usage example. +*/ +/* No InterleaveFlags as there's no index array for PreserveStridedIndices, + and PreserveInterleavedAttributes makes sense only for an input mesh */ +MAGNUM_MESHTOOLS_EXPORT Trade::MeshData interleave(MeshPrimitive primitive, Containers::ArrayView attributes); + +/** +@overload +@m_since_latest +*/ +MAGNUM_MESHTOOLS_EXPORT Trade::MeshData interleave(MeshPrimitive primitive, std::initializer_list attributes); + namespace Implementation { /* Used internally by interleavedLayout() and concatenate() */ diff --git a/src/Magnum/MeshTools/Test/InterleaveTest.cpp b/src/Magnum/MeshTools/Test/InterleaveTest.cpp index cdce91f37..a35b92c0c 100644 --- a/src/Magnum/MeshTools/Test/InterleaveTest.cpp +++ b/src/Magnum/MeshTools/Test/InterleaveTest.cpp @@ -27,12 +27,14 @@ #include #include #include +#include /** @todo remove once Debug is stream-free */ #include #include +#include #include #include #include -#include +#include /** @todo remove once Debug is stream-free */ #include "Magnum/Math/Vector3.h" #include "Magnum/MeshTools/Interleave.h" @@ -99,6 +101,10 @@ struct InterleaveTest: Corrade::TestSuite::Tester { void interleaveMeshDataAlreadyInterleavedMoveIndices(); void interleaveMeshDataAlreadyInterleavedMoveNonOwned(); void interleaveMeshDataNothing(); + + void interleaveMeshDataLooseAttributes(); + void interleaveMeshDataLooseAttributesIndexed(); + void interleaveMeshDataLooseAttributesInvalid(); }; const struct { @@ -191,7 +197,11 @@ InterleaveTest::InterleaveTest() { Containers::arraySize(StridedIndicesData)); addTests({&InterleaveTest::interleaveMeshDataAlreadyInterleavedMoveNonOwned, - &InterleaveTest::interleaveMeshDataNothing}); + &InterleaveTest::interleaveMeshDataNothing, + + &InterleaveTest::interleaveMeshDataLooseAttributes, + &InterleaveTest::interleaveMeshDataLooseAttributesIndexed, + &InterleaveTest::interleaveMeshDataLooseAttributesInvalid}); } void InterleaveTest::attributeCount() { @@ -1343,6 +1353,7 @@ void InterleaveTest::interleaveMeshDataExtraOriginalEmpty() { CORRADE_VERIFY(!interleaved.isIndexed()); /* No reason to not be like this */ CORRADE_COMPARE(interleaved.vertexDataFlags(), Trade::DataFlag::Mutable|Trade::DataFlag::Owned); + CORRADE_COMPARE(interleaved.attributeStride(0), sizeof(Vector2) + 4); CORRADE_COMPARE(interleaved.attributeCount(), 1); CORRADE_COMPARE_AS(interleaved.attribute(Trade::MeshAttribute::Position), Containers::stridedArrayView(positions), @@ -1544,6 +1555,97 @@ void InterleaveTest::interleaveMeshDataNothing() { CORRADE_COMPARE(interleaved.vertexData().size(), 0); } +void InterleaveTest::interleaveMeshDataLooseAttributes() { + /* Same as interleaveMeshDataExtraOriginalEmpty(), but testing the + convenience overload instead */ + + Vector2 positions[]{{1.3f, 0.3f}, {0.87f, 1.1f}, {1.0f, -0.5f}}; + Trade::MeshData interleaved = MeshTools::interleave(MeshPrimitive::TriangleFan, { + Trade::MeshAttributeData{4}, + Trade::MeshAttributeData{Trade::MeshAttribute::Position, Containers::arrayView(positions)} + }); + + CORRADE_VERIFY(MeshTools::isInterleaved(interleaved)); + CORRADE_COMPARE(interleaved.primitive(), MeshPrimitive::TriangleFan); + CORRADE_VERIFY(!interleaved.isIndexed()); + /* No reason to not be like this */ + CORRADE_COMPARE(interleaved.vertexDataFlags(), Trade::DataFlag::Mutable|Trade::DataFlag::Owned); + CORRADE_COMPARE(interleaved.attributeCount(), 1); + CORRADE_COMPARE(interleaved.attributeStride(0), sizeof(Vector2) + 4); + CORRADE_COMPARE_AS(interleaved.attribute(Trade::MeshAttribute::Position), + Containers::stridedArrayView(positions), + TestSuite::Compare::Container); +} + +void InterleaveTest::interleaveMeshDataLooseAttributesIndexed() { + /* Same as interleaveMeshDataExtraOriginalEmpty(), but testing the + convenience overload instead */ + + struct Index { + UnsignedShort index; + Short dummy; /* MSVC 2015 doesn't like Short:16 in local structs */ + } indices[]{{3, 0}, {6, 0}, {7, 0}, {9, 0}}; + Vector2 positions[]{{1.3f, 0.3f}, {0.87f, 1.1f}, {1.0f, -0.5f}}; + Trade::MeshData interleaved = MeshTools::interleave( + MeshPrimitive::TriangleStrip, + Trade::MeshIndexData{Containers::stridedArrayView(indices).slice(&Index::index)}, { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, Containers::arrayView(positions)}, + Trade::MeshAttributeData{4} + }); + + CORRADE_VERIFY(MeshTools::isInterleaved(interleaved)); + CORRADE_COMPARE(interleaved.primitive(), MeshPrimitive::TriangleStrip); + + CORRADE_VERIFY(interleaved.isIndexed()); + CORRADE_COMPARE(interleaved.indexDataFlags(), Trade::DataFlag::Mutable|Trade::DataFlag::Owned); + /* Indices get copied and made tightly packed */ + CORRADE_COMPARE(interleaved.indexStride(), 2); + CORRADE_COMPARE_AS(interleaved.indices(), + Containers::stridedArrayView(indices).slice(&Index::index), + TestSuite::Compare::Container); + + CORRADE_COMPARE(interleaved.vertexDataFlags(), Trade::DataFlag::Mutable|Trade::DataFlag::Owned); + CORRADE_COMPARE(interleaved.attributeCount(), 1); + CORRADE_COMPARE(interleaved.attributeStride(0), sizeof(Vector2) + 4); + CORRADE_COMPARE_AS(interleaved.attribute(Trade::MeshAttribute::Position), + Containers::stridedArrayView(positions), + TestSuite::Compare::Container); +} + +void InterleaveTest::interleaveMeshDataLooseAttributesInvalid() { + CORRADE_SKIP_IF_NO_ASSERT(); + + UnsignedShort indices[]{3, 6, 7, 9}; + + /* Null views are fine */ + CORRADE_COMPARE(MeshTools::interleave(MeshPrimitive::Triangles, { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, Containers::ArrayView{nullptr, 3}} + }).vertexCount(), 3); + CORRADE_COMPARE(MeshTools::interleave(MeshPrimitive::Triangles, { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, Containers::ArrayView{nullptr, 0}} + }).vertexCount(), 0); + + std::ostringstream out; + Error redirectError{&out}; + MeshTools::interleave(MeshPrimitive::Triangles, + Trade::MeshIndexData{indices}, { + Trade::MeshAttributeData{4} + }); + MeshTools::interleave(MeshPrimitive::Triangles, { + Trade::MeshAttributeData{4}, + Trade::MeshAttributeData{4} + }); + MeshTools::interleave(MeshPrimitive::Triangles, + Trade::MeshIndexData{meshIndexTypeWrap(0xcece), Containers::stridedArrayView(indices)}, { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, Containers::ArrayView{nullptr, 3}} + }); + CORRADE_COMPARE_AS(out.str(), + "MeshTools::interleave(): only padding found among 1 attributes, can't infer vertex count\n" + "MeshTools::interleave(): only padding found among 2 attributes, can't infer vertex count\n" + "MeshTools::interleave(): implementation-specific index type 0xcece\n", + TestSuite::Compare::String); +} + }}}} CORRADE_TEST_MAIN(Magnum::MeshTools::Test::InterleaveTest)