From 10f29d2c8141d03eaedc4584cda0702823f5db01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 17 Jun 2020 20:02:36 +0200 Subject: [PATCH] MeshTools: change concatenate() to do only what it's designed for. This is a breaking change that changes the signature, sorry -- if you were using concatenate() for mesh concatenation before, enjoy the new less strange signature, if you were using it for making the mesh owned before (which was a strange and not very well thought out use case), please use the recently added owned() instead. I thought about adding an overload for backwards compatibility, but it would need to allocate to work. This way with the breakage it's ensured you actually change to the right API. This also cleans up a lot of ugly code in the internals and resolves one XFAIL in removeDuplicates(). --- doc/snippets/MagnumMeshTools.cpp | 9 -- src/Magnum/MeshTools/Concatenate.cpp | 85 +++++++------------ src/Magnum/MeshTools/Concatenate.h | 46 +++------- src/Magnum/MeshTools/RemoveDuplicates.cpp | 20 ++--- src/Magnum/MeshTools/Test/ConcatenateTest.cpp | 57 ++++++------- .../MeshTools/Test/RemoveDuplicatesTest.cpp | 39 ++++----- 6 files changed, 93 insertions(+), 163 deletions(-) diff --git a/doc/snippets/MagnumMeshTools.cpp b/doc/snippets/MagnumMeshTools.cpp index 7709f11b3..6f7ca8cf0 100644 --- a/doc/snippets/MagnumMeshTools.cpp +++ b/doc/snippets/MagnumMeshTools.cpp @@ -80,15 +80,6 @@ std::pair, MeshIndexType> result = /* [compressIndices-offset] */ } -{ -/* [concatenate-make-mutable] */ -/* Flip triangles on a cube primitive so it's counterclockwise from the inside - in order to render a cube map */ -Trade::MeshData mesh = MeshTools::concatenate(Primitives::cubeSolid()); -MeshTools::flipFaceWindingInPlace(mesh.mutableIndices()); -/* [concatenate-make-mutable] */ -} - #ifdef MAGNUM_BUILD_DEPRECATED { CORRADE_IGNORE_DEPRECATED_PUSH diff --git a/src/Magnum/MeshTools/Concatenate.cpp b/src/Magnum/MeshTools/Concatenate.cpp index 210325cac..38a7bc130 100644 --- a/src/Magnum/MeshTools/Concatenate.cpp +++ b/src/Magnum/MeshTools/Concatenate.cpp @@ -33,10 +33,10 @@ namespace Magnum { namespace MeshTools { namespace Implementation { -std::pair concatenateIndexVertexCount(const Trade::MeshData& first, const Containers::ArrayView> next) { - UnsignedInt indexCount = first.isIndexed() ? first.indexCount() : 0; - UnsignedInt vertexCount = first.vertexCount(); - for(const Trade::MeshData& mesh: next) { +std::pair concatenateIndexVertexCount(Containers::ArrayView> meshes) { + UnsignedInt indexCount = 0; + UnsignedInt vertexCount = 0; + for(const Trade::MeshData& mesh: meshes) { /* If the mesh is indexed, add to index count. If this is the first indexed mesh, all previous meshes will have a trivial index buffer generated for all their vertices */ @@ -62,10 +62,9 @@ struct MeshAttributeHash: std::hash&& indexData, const UnsignedInt vertexCount, Containers::Array&& vertexData, Containers::Array&& attributeData, const Trade::MeshData& first, const Containers::ArrayView> next, const char* const assertPrefix, const std::size_t meshIndexOffset) { +Trade::MeshData concatenate(Containers::Array&& indexData, const UnsignedInt vertexCount, Containers::Array&& vertexData, Containers::Array&& attributeData, const Containers::ArrayView> meshes, const char* const assertPrefix) { #ifdef CORRADE_NO_ASSERT static_cast(assertPrefix); - static_cast(meshIndexOffset); #endif /* Convert the attributes from offset-only and zero vertex count to @@ -83,17 +82,17 @@ Trade::MeshData concatenate(Containers::Array&& indexData, const UnsignedI /** @todo delegate to `indexTriangleStrip()` (`duplicate*()`?) etc when those are done */ CORRADE_ASSERT( - first.primitive() != MeshPrimitive::LineStrip && - first.primitive() != MeshPrimitive::LineLoop && - first.primitive() != MeshPrimitive::TriangleStrip && - first.primitive() != MeshPrimitive::TriangleFan, - assertPrefix << first.primitive() << "is not supported, turn it into a plain indexed mesh first", + meshes.front()->primitive() != MeshPrimitive::LineStrip && + meshes.front()->primitive() != MeshPrimitive::LineLoop && + meshes.front()->primitive() != MeshPrimitive::TriangleStrip && + meshes.front()->primitive() != MeshPrimitive::TriangleFan, + assertPrefix << meshes.front()->primitive() << "is not supported, turn it into a plain indexed mesh first", (Trade::MeshData{MeshPrimitive{}, 0})); /* Populate the resulting instance with what we have. It'll be used below for convenient access to vertex / index data */ auto indices = Containers::arrayCast(indexData); - Trade::MeshData out{first.primitive(), + Trade::MeshData out{meshes.front()->primitive(), /* If the index array is empty, we're creating a non-indexed mesh (not an indexed mesh with zero indices) */ std::move(indexData), indices.empty() ? @@ -107,19 +106,16 @@ Trade::MeshData concatenate(Containers::Array&& indexData, const UnsignedI for(UnsignedInt i = 0; i != out.attributeCount(); ++i) attributeMap.emplace(out.attributeName(i), std::make_pair(i, false)); - /* Go through all meshes and put all attributes and index arrays together. - The first mesh might get separately and thus can't be a part of the - view, so abuse the *defined* unsigned integer overflow to add it to the - loop. This probably breaks all coding guidelines on earth tho. */ + /* Go through all meshes and put all attributes and index arrays together. */ std::size_t indexOffset = 0; std::size_t vertexOffset = 0; - for(std::size_t i = ~std::size_t{}; i != next.size(); ++i) { - const Trade::MeshData& mesh = i == ~std::size_t{} ? first : next[i].get(); + for(std::size_t i = 0; i != meshes.size(); ++i) { + const Trade::MeshData& mesh = meshes[i]; /* This won't fire for i == ~std::size_t{}, as that's where out.primitive() comes from */ CORRADE_ASSERT(mesh.primitive() == out.primitive(), - assertPrefix << "expected" << out.primitive() << "but got" << mesh.primitive() << "in mesh" << i + meshIndexOffset, + assertPrefix << "expected" << out.primitive() << "but got" << mesh.primitive() << "in mesh" << i, (Trade::MeshData{MeshPrimitive{}, 0})); /* If the mesh is indexed, copy the indices over, expanded to 32bit */ @@ -167,10 +163,10 @@ Trade::MeshData concatenate(Containers::Array&& indexData, const UnsignedI /* Check format compatibility. This won't fire for i == ~std::size_t{}, as that's where out.primitive() comes from */ 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 + meshIndexOffset << "attribute" << 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 + meshIndexOffset << "attribute" << 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, (Trade::MeshData{MeshPrimitive{}, 0})); /* Copy the data to a slice of the output, mark the attribute as @@ -189,16 +185,10 @@ Trade::MeshData concatenate(Containers::Array&& indexData, const UnsignedI } -Trade::MeshData concatenate(Trade::MeshData&& first, const Containers::ArrayView> next) { - /* If there's just a single non-empty mesh and its data is owned, pass it - through, as it passes the guarantee that the returned data is always - owned. If it's empty, it doesn't matter that we drag it through the rest - as there will be no heavy allocation / copy made (and that also makes - tests easier to write). */ - if(first.indexDataFlags() & Trade::DataFlag::Owned && - first.vertexDataFlags() & Trade::DataFlag::Owned && - first.attributeCount() && first.vertexCount() && next.empty()) - return std::move(first); +Trade::MeshData concatenate(const Containers::ArrayView> meshes) { + CORRADE_ASSERT(!meshes.empty(), + "MeshTools::concatenate(): expected at least one mesh", + (Trade::MeshData{MeshPrimitive::Points, 0})); /* Calculate final attribute stride and offsets. Make a non-owning copy of the attribute data to avoid interleavedLayout() stealing the original @@ -206,40 +196,27 @@ Trade::MeshData concatenate(Trade::MeshData&& first, const Containers::ArrayView no attributes in the original array, pass just vertex count --- otherwise MeshData will assert on that to avoid it getting lost. */ Containers::Array attributeData; - if(first.attributeCount()) - attributeData = Implementation::interleavedLayout(Trade::MeshData{first.primitive(), - {}, first.vertexData(), - Trade::meshAttributeDataNonOwningArray(first.attributeData())}, {}); + if(meshes.front()->attributeCount()) + attributeData = Implementation::interleavedLayout(Trade::MeshData{meshes.front()->primitive(), + {}, meshes.front()->vertexData(), + Trade::meshAttributeDataNonOwningArray(meshes.front()->attributeData())}, {}); else attributeData = - Implementation::interleavedLayout(Trade::MeshData{first.primitive(), - first.vertexCount()}, {}); + Implementation::interleavedLayout(Trade::MeshData{meshes.front()->primitive(), + meshes.front()->vertexCount()}, {}); /* Calculate total index/vertex count and allocate the target memory. Index data are allocated with NoInit as the whole array will be written, however vertex data might have holes and thus it's zero-initialized. */ - const std::pair indexVertexCount = Implementation::concatenateIndexVertexCount(first, next); + const std::pair indexVertexCount = Implementation::concatenateIndexVertexCount(meshes); Containers::Array indexData{Containers::NoInit, indexVertexCount.first*sizeof(UnsignedInt)}; Containers::Array vertexData{Containers::ValueInit, attributeData.empty() ? 0 : (attributeData[0].stride()*indexVertexCount.second)}; - return Implementation::concatenate(std::move(indexData), indexVertexCount.second, std::move(vertexData), std::move(attributeData), first, next, "MeshTools::concatenate():", 0); + return Implementation::concatenate(std::move(indexData), indexVertexCount.second, std::move(vertexData), std::move(attributeData), meshes, "MeshTools::concatenate():"); } -Trade::MeshData concatenate(Trade::MeshData&& first, std::initializer_list> next) { - return concatenate(std::move(first), Containers::arrayView(next)); -} - -Trade::MeshData concatenate(const Trade::MeshData& first, const Containers::ArrayView> next) { - return concatenate(Trade::MeshData{first.primitive(), - /* If data is not indexed, the reference will be also non-indexed */ - {}, first.indexData(), Trade::MeshIndexData{first.indices()}, - {}, first.vertexData(), Trade::meshAttributeDataNonOwningArray(first.attributeData()), - first.vertexCount(), - }, next); -} - -Trade::MeshData concatenate(const Trade::MeshData& first, std::initializer_list> next) { - return concatenate(first, Containers::arrayView(next)); +Trade::MeshData concatenate(std::initializer_list> meshes) { + return concatenate(Containers::arrayView(meshes)); } }} diff --git a/src/Magnum/MeshTools/Concatenate.h b/src/Magnum/MeshTools/Concatenate.h index e888d5a44..089a12134 100644 --- a/src/Magnum/MeshTools/Concatenate.h +++ b/src/Magnum/MeshTools/Concatenate.h @@ -39,8 +39,8 @@ namespace Magnum { namespace MeshTools { namespace Implementation { - MAGNUM_MESHTOOLS_EXPORT std::pair concatenateIndexVertexCount(const Trade::MeshData& first, const Containers::ArrayView> next); - MAGNUM_MESHTOOLS_EXPORT Trade::MeshData concatenate(Containers::Array&& indexData, UnsignedInt vertexCount, Containers::Array&& vertexData, Containers::Array&& attributeData, const Trade::MeshData& first, const Containers::ArrayView> next, const char* assertPrefix, std::size_t meshIndexOffset); + MAGNUM_MESHTOOLS_EXPORT std::pair concatenateIndexVertexCount(Containers::ArrayView> meshes); + MAGNUM_MESHTOOLS_EXPORT Trade::MeshData concatenate(Containers::Array&& indexData, UnsignedInt vertexCount, Containers::Array&& vertexData, Containers::Array&& attributeData, Containers::ArrayView> meshes, const char* assertPrefix); } /** @@ -54,10 +54,11 @@ any mesh has indices out of bounds for its particular vertex count. Meshes with @ref MeshPrimitive::LineStrip, @ref MeshPrimitive::LineLoop, @ref MeshPrimitive::TriangleStrip and @ref MeshPrimitive::TriangleFan can't be concatenated --- use @ref generateIndices() to turn them into -@ref MeshPrimitive::Lines or @ref MeshPrimitive::Triangles first. +@ref MeshPrimitive::Lines or @ref MeshPrimitive::Triangles first. The @p meshes +array is expected to have at least one item. -All attributes from the @p first mesh are taken; for each mesh in @p next, -attributes present in @p first are copied, superfluous attributes ignored and +All attributes from the first mesh are taken; 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. @@ -69,36 +70,15 @@ applying transformations. If an index buffer is needed, @ref MeshIndexType::UnsignedInt is always used. Call @ref compressIndices(const Trade::MeshData&, MeshIndexType) on the result to compress it to a smaller type, if desired. -@see @ref concatenate(Trade::MeshData&&, const Containers::ArrayView>), - @ref concatenateInto() +@see @ref concatenateInto() */ -MAGNUM_MESHTOOLS_EXPORT Trade::MeshData concatenate(const Trade::MeshData& first, const Containers::ArrayView> next = {}); +MAGNUM_MESHTOOLS_EXPORT Trade::MeshData concatenate(Containers::ArrayView> meshes); /** * @overload * @m_since_latest */ -MAGNUM_MESHTOOLS_EXPORT Trade::MeshData concatenate(const Trade::MeshData& first, std::initializer_list> next); - -/** -@brief Concatenate meshes together -@m_since_latest - -Compared to @ref concatenate(const Trade::MeshData&, const Containers::ArrayView>), -if @p first has both vertex and index data owned and @p next is empty, it's -passed through without any extra allocations or other work. This can be used -for example to ensure a mesh is mutable in order to do various modifications on -its data: - -@snippet MagnumMeshTools.cpp concatenate-make-mutable -*/ -MAGNUM_MESHTOOLS_EXPORT Trade::MeshData concatenate(Trade::MeshData&& first, const Containers::ArrayView> next = {}); - -/** - * @overload - * @m_since_latest - */ -MAGNUM_MESHTOOLS_EXPORT Trade::MeshData concatenate(Trade::MeshData&& first, std::initializer_list> next); +MAGNUM_MESHTOOLS_EXPORT Trade::MeshData concatenate(std::initializer_list> meshes); /** @brief Concatenate a list of meshes into a pre-existing destination, enlarging it if necessary @@ -108,8 +88,8 @@ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData concatenate(Trade::MeshData&& first, std @param[in] meshes Meshes to concatenate @m_since_latest -Compared to @ref concatenate(const Trade::MeshData&, const Containers::ArrayView>) this -function resizes existing index and vertex buffers in @p destination using +Compared to @ref concatenate(Containers::ArrayView>) +this function resizes existing index and vertex buffers in @p destination using @ref Containers::arrayResize() and given @p allocator, and reuses its atttribute data array instead of always allocating new ones. Only the attribute layout from @p destination is used, all vertex/index data are taken from @@ -119,7 +99,7 @@ template class Allocator = Containers::ArrayAllocator> void conc CORRADE_ASSERT(!meshes.empty(), "MeshTools::concatenateInto(): no meshes passed", ); - std::pair indexVertexCount = Implementation::concatenateIndexVertexCount(meshes[0], meshes.suffix(1)); + std::pair indexVertexCount = Implementation::concatenateIndexVertexCount(meshes); Containers::Array indexData; if(indexVertexCount.first) { @@ -141,7 +121,7 @@ template class Allocator = Containers::ArrayAllocator> void conc Containers::arrayResize(vertexData, Containers::ValueInit, attributeStride*indexVertexCount.second); } - destination = Implementation::concatenate(std::move(indexData), indexVertexCount.second, std::move(vertexData), std::move(attributeData), meshes[0], meshes.suffix(1), "MeshTools::concatenateInto():", 1); + destination = Implementation::concatenate(std::move(indexData), indexVertexCount.second, std::move(vertexData), std::move(attributeData), meshes, "MeshTools::concatenateInto():"); } /** diff --git a/src/Magnum/MeshTools/RemoveDuplicates.cpp b/src/Magnum/MeshTools/RemoveDuplicates.cpp index 9e4fcc443..2e0afcc19 100644 --- a/src/Magnum/MeshTools/RemoveDuplicates.cpp +++ b/src/Magnum/MeshTools/RemoveDuplicates.cpp @@ -36,7 +36,7 @@ #include "Magnum/Math/FunctionsBatch.h" #include "Magnum/Math/Range.h" -#include "Magnum/MeshTools/Concatenate.h" +#include "Magnum/MeshTools/Reference.h" #include "Magnum/MeshTools/Duplicate.h" #include "Magnum/MeshTools/Interleave.h" #include "Magnum/Trade/MeshData.h" @@ -409,13 +409,11 @@ Trade::MeshData removeDuplicates(Trade::MeshData&& data) { (Trade::MeshData{MeshPrimitive::Points, 0})); /* Turn the passed data into an interleaved owned mutable instance we can - operate on -- concatenate() alone only makes the data owned, - interleave() alone only makes the data interleaved (but those can stay - non-owned). There's a chance the original data are already like this, in - which case this will be just a passthrough. */ - /** @todo concatenate() causes the resulting index type to be UnsignedInt - always, replace with owned() or some such when that's done */ - Trade::MeshData ownedInterleaved = interleave(concatenate(std::move(data))); + operate on -- owned() alone only makes the data owned, interleave() + alone only makes the data interleaved (but those can stay non-owned). + There's a chance the original data are already like this, in which case + this will be just a passthrough. */ + Trade::MeshData ownedInterleaved = owned(interleave(std::move(data))); const Containers::StridedArrayView2D vertexData = MeshTools::interleavedMutableData(ownedInterleaved); @@ -467,9 +465,7 @@ Trade::MeshData removeDuplicatesFuzzy(const Trade::MeshData& data, const Float f /* Turn the passed data into an owned mutable instance we can operate on. There's a chance the original data are already like this, in which case this will be just a passthrough. */ - /** @todo concatenate() causes the resulting index type to be UnsignedInt - always, replace with owned() or some such when that's done */ - Trade::MeshData owned = concatenate(std::move(data)); + Trade::MeshData owned = MeshTools::owned(std::move(data)); /* Allocate an interleaved index array for all attribs. If the mesh is already indexed, use the existing index count and copy the original @@ -491,7 +487,7 @@ Trade::MeshData removeDuplicatesFuzzy(const Trade::MeshData& data, const Float f for(UnsignedInt i = 0; i != owned.attributeCount(); ++i) { const VertexFormat format = owned.attributeFormat(i); CORRADE_ASSERT(!isVertexFormatImplementationSpecific(format), - "MeshTools::removeDuplicatesFuzzy(): can't remove duplicates in" << format, + "MeshTools::removeDuplicatesFuzzy(): can't remove duplicates in an implementation-specific format" << reinterpret_cast(vertexFormatUnwrap(format)), (Trade::MeshData{MeshPrimitive::Points, 0})); const Containers::StridedArrayView1D outputIndices = perAttributeIndices[i]; diff --git a/src/Magnum/MeshTools/Test/ConcatenateTest.cpp b/src/Magnum/MeshTools/Test/ConcatenateTest.cpp index 0490a30ec..c901338de 100644 --- a/src/Magnum/MeshTools/Test/ConcatenateTest.cpp +++ b/src/Magnum/MeshTools/Test/ConcatenateTest.cpp @@ -41,7 +41,7 @@ struct ConcatenateTest: TestSuite::Tester { void concatenateNoAttributes(); void concatenateNoAttributesNotIndexed(); void concatenateOne(); - void concatenateOneRvalue(); + void concatenateNone(); void concatenateInto(); void concatenateIntoNoIndexArray(); void concatenateIntoNonOwnedAttributeArray(); @@ -59,7 +59,7 @@ ConcatenateTest::ConcatenateTest() { &ConcatenateTest::concatenateNoAttributes, &ConcatenateTest::concatenateNoAttributesNotIndexed, &ConcatenateTest::concatenateOne, - &ConcatenateTest::concatenateOneRvalue, + &ConcatenateTest::concatenateNone, &ConcatenateTest::concatenateInto, &ConcatenateTest::concatenateIntoNoIndexArray, &ConcatenateTest::concatenateIntoNonOwnedAttributeArray, @@ -165,7 +165,7 @@ void ConcatenateTest::concatenate() { &vertexDataC[0].texcoords3, 3, sizeof(VertexDataC))}, }}; - Trade::MeshData dst = MeshTools::concatenate(a, {b, c}); + Trade::MeshData dst = MeshTools::concatenate({a, b, c}); CORRADE_COMPARE(dst.primitive(), MeshPrimitive::Points); CORRADE_COMPARE(dst.attributeCount(), 4); CORRADE_COMPARE_AS(dst.attribute(Trade::MeshAttribute::Position), @@ -244,7 +244,7 @@ void ConcatenateTest::concatenateNotIndexed() { Containers::arrayView(positionB)} }}; - Trade::MeshData dst = MeshTools::concatenate(a, {b, b}); + Trade::MeshData dst = MeshTools::concatenate({a, b, b}); CORRADE_COMPARE(dst.primitive(), MeshPrimitive::Points); CORRADE_COMPARE(dst.attributeCount(), 1); CORRADE_COMPARE_AS(dst.attribute(Trade::MeshAttribute::Position), @@ -272,7 +272,7 @@ void ConcatenateTest::concatenateNoAttributes() { const UnsignedByte indicesC[]{1, 0, 1, 0}; Trade::MeshData c{MeshPrimitive::Points, {}, indicesC, Trade::MeshIndexData{indicesC}, 2}; - Trade::MeshData dst = MeshTools::concatenate(a, {b, c}); + Trade::MeshData dst = MeshTools::concatenate({a, b, c}); CORRADE_COMPARE(dst.primitive(), MeshPrimitive::Points); CORRADE_COMPARE(dst.attributeCount(), 0); CORRADE_COMPARE(dst.vertexCount(), 10); @@ -292,7 +292,7 @@ void ConcatenateTest::concatenateNoAttributesNotIndexed() { Trade::MeshData b{MeshPrimitive::Points, 6}; Trade::MeshData c{MeshPrimitive::Points, 2}; - Trade::MeshData dst = MeshTools::concatenate(a, {b, c}); + Trade::MeshData dst = MeshTools::concatenate({a, b, c}); CORRADE_COMPARE(dst.primitive(), MeshPrimitive::Points); CORRADE_COMPARE(dst.attributeCount(), 0); CORRADE_COMPARE(dst.vertexCount(), 11); @@ -330,7 +330,9 @@ void ConcatenateTest::concatenateOne() { Containers::arrayView(vertexData[0].position)}, }}; - Trade::MeshData dst = MeshTools::concatenate(a); + /* This is a rather pointless use case, but could happen in generic code + that filters the input meshes and ends up with just one */ + Trade::MeshData dst = MeshTools::concatenate({a}); CORRADE_COMPARE(dst.primitive(), MeshPrimitive::Points); CORRADE_COMPARE(dst.attributeCount(), 3); CORRADE_COMPARE_AS(dst.attribute(Trade::MeshAttribute::Position), @@ -362,26 +364,15 @@ void ConcatenateTest::concatenateOne() { CORRADE_COMPARE(dst.vertexDataFlags(), Trade::DataFlag::Owned|Trade::DataFlag::Mutable); } -void ConcatenateTest::concatenateOneRvalue() { - Containers::Array vertexData{sizeof(Vector2)*4}; - auto positions = Containers::arrayCast(vertexData); - Containers::Array indexData{sizeof(UnsignedInt)*6}; - auto indices = Containers::arrayCast(indexData); - Trade::MeshAttributeData attributeData[]{ - Trade::MeshAttributeData{Trade::MeshAttribute::Position, positions} - }; +void ConcatenateTest::concatenateNone() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif - /* The result should be just a pass-through, as both index and vertex data - are already owned */ - Trade::MeshData dst = MeshTools::concatenate(Trade::MeshData{ - MeshPrimitive::Triangles, - std::move(indexData), Trade::MeshIndexData{indices}, - std::move(vertexData), Trade::meshAttributeDataNonOwningArray(attributeData)}, - /* Explicitly pass an empty init list to ensure this overload is - covered as well */ - std::initializer_list>{}); - CORRADE_COMPARE(dst.indexData().data(), static_cast(indices.data())); - CORRADE_COMPARE(dst.vertexData().data(), static_cast(positions.data())); + std::ostringstream out; + Error redirectError{&out}; + MeshTools::concatenate({}); + CORRADE_COMPARE(out.str(), "MeshTools::concatenate(): expected at least one mesh\n"); } void ConcatenateTest::concatenateInto() { @@ -561,7 +552,7 @@ void ConcatenateTest::concatenateUnsupportedPrimitive() { std::ostringstream out; Error redirectError{&out}; - MeshTools::concatenate(a); + MeshTools::concatenate({a}); MeshTools::concatenateInto(a, {a}); CORRADE_COMPARE(out.str(), "MeshTools::concatenate(): MeshPrimitive::TriangleStrip is not supported, turn it into a plain indexed mesh first\n" @@ -579,10 +570,10 @@ void ConcatenateTest::concatenateInconsistentPrimitive() { std::ostringstream out; Error redirectError{&out}; - MeshTools::concatenate(a, {a, b}); + MeshTools::concatenate({a, a, b}); MeshTools::concatenateInto(a, {a, b}); CORRADE_COMPARE(out.str(), - "MeshTools::concatenate(): expected MeshPrimitive::Triangles but got MeshPrimitive::Lines in mesh 1\n" + "MeshTools::concatenate(): expected MeshPrimitive::Triangles but got MeshPrimitive::Lines in mesh 2\n" "MeshTools::concatenateInto(): expected MeshPrimitive::Triangles but got MeshPrimitive::Lines in mesh 1\n"); } @@ -609,10 +600,10 @@ void ConcatenateTest::concatenateInconsistentAttributeType() { std::ostringstream out; Error redirectError{&out}; - MeshTools::concatenate(a, {a, a, a, b}); + MeshTools::concatenate({a, a, a, a, b}); MeshTools::concatenateInto(a, {a, a, a, b}); CORRADE_COMPARE(out.str(), - "MeshTools::concatenate(): expected VertexFormat::Vector3ubNormalized for attribute 2 (Trade::MeshAttribute::Color) but got VertexFormat::Vector3usNormalized in mesh 3 attribute 1\n" + "MeshTools::concatenate(): expected VertexFormat::Vector3ubNormalized for attribute 2 (Trade::MeshAttribute::Color) but got VertexFormat::Vector3usNormalized in mesh 4 attribute 1\n" "MeshTools::concatenateInto(): expected VertexFormat::Vector3ubNormalized for attribute 2 (Trade::MeshAttribute::Color) but got VertexFormat::Vector3usNormalized in mesh 3 attribute 1\n"); } @@ -639,10 +630,10 @@ void ConcatenateTest::concatenateInconsistentAttributeArraySize() { std::ostringstream out; Error redirectError{&out}; - MeshTools::concatenate(a, {a, a, a, b}); + 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 3 attribute 1\n" + "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"); } diff --git a/src/Magnum/MeshTools/Test/RemoveDuplicatesTest.cpp b/src/Magnum/MeshTools/Test/RemoveDuplicatesTest.cpp index 8d75680f3..04d0fae21 100644 --- a/src/Magnum/MeshTools/Test/RemoveDuplicatesTest.cpp +++ b/src/Magnum/MeshTools/Test/RemoveDuplicatesTest.cpp @@ -673,13 +673,9 @@ void RemoveDuplicatesTest::removeDuplicatesMeshData() { CORRADE_VERIFY(unique.isIndexed()); if(data.indexed) { CORRADE_COMPARE(unique.indexCount(), mesh.indexCount()); - { - CORRADE_EXPECT_FAIL("The function currently doesn't preserve the index type."); - CORRADE_COMPARE(unique.indexType(), MeshIndexType::UnsignedShort); - } - CORRADE_COMPARE(unique.indexType(), MeshIndexType::UnsignedInt); - CORRADE_COMPARE_AS(unique.indices(), - Containers::arrayView({1, 2, 5, 7, 1, 6, 4, 1, 5, 0, 3, 2, 3}), + CORRADE_COMPARE(unique.indexType(), MeshIndexType::UnsignedShort); + CORRADE_COMPARE_AS(unique.indices(), + Containers::arrayView({1, 2, 5, 7, 1, 6, 4, 1, 5, 0, 3, 2, 3}), TestSuite::Compare::Container); } else { CORRADE_COMPARE(unique.indexCount(), mesh.vertexCount()); @@ -820,11 +816,13 @@ void RemoveDuplicatesTest::removeDuplicatesMeshDataFuzzy() { CORRADE_COMPARE(unique.primitive(), MeshPrimitive::Lines); CORRADE_VERIFY(unique.isIndexed()); - if(data.indexed) + if(data.indexed) { CORRADE_COMPARE(unique.indexCount(), mesh.indexCount()); - else + CORRADE_COMPARE(unique.indexType(), MeshIndexType::UnsignedShort); + } else { CORRADE_COMPARE(unique.indexCount(), mesh.vertexCount()); - CORRADE_COMPARE(unique.indexType(), MeshIndexType::UnsignedInt); + CORRADE_COMPARE(unique.indexType(), MeshIndexType::UnsignedInt); + } CORRADE_COMPARE(unique.attributeCount(), 3 + data.attributes.size()); @@ -857,8 +855,8 @@ void RemoveDuplicatesTest::removeDuplicatesMeshDataFuzzy() { /* The data differ depending on how much is actually removed */ if(data.vertexCount == 7) { - if(data.indexed) CORRADE_COMPARE_AS(unique.indices(), - Containers::arrayView({0, 1, 3, 6, 5, 4, 3, 5, 3, 0, 2, 5, 2}), + if(data.indexed) CORRADE_COMPARE_AS(unique.indices(), + Containers::arrayView({0, 1, 3, 6, 5, 4, 3, 5, 3, 0, 2, 5, 2}), TestSuite::Compare::Container); else CORRADE_COMPARE_AS(unique.indices(), Containers::arrayView({0, 0, 1, 2, 3, 3, 4, 5, 5, 6}), @@ -912,8 +910,8 @@ void RemoveDuplicatesTest::removeDuplicatesMeshDataFuzzy() { TestSuite::Compare::Container); } else if(data.vertexCount == 9) { - if(data.indexed) CORRADE_COMPARE_AS(unique.indices(), - Containers::arrayView({1, 2, 4, 8, 6, 5, 4, 6, 4, 0, 3, 7, 3}), + if(data.indexed) CORRADE_COMPARE_AS(unique.indices(), + Containers::arrayView({1, 2, 4, 8, 6, 5, 4, 6, 4, 0, 3, 7, 3}), TestSuite::Compare::Container); else CORRADE_COMPARE_AS(unique.indices(), Containers::arrayView({0, 1, 2, 3, 4, 4, 5, 6, 7, 8}), @@ -1059,14 +1057,11 @@ void RemoveDuplicatesTest::removeDuplicatesMeshDataFuzzyImplementationSpecific() std::ostringstream out; Error redirectError{&out}; - CORRADE_EXPECT_FAIL("The function currently uses concatenate() to make data owned, which fails earlier with a different (and confusing) assert message."); - CORRADE_VERIFY(false); - -// MeshTools::removeDuplicatesFuzzy(Trade::MeshData{MeshPrimitive::Points, -// nullptr, {Trade::MeshAttributeData{Trade::MeshAttribute::Position, -// vertexFormatWrap(0x1234), nullptr}}}); -// CORRADE_COMPARE(out.str(), -// "MeshTools::removeDuplicatesFuzzy(): can't remove duplicates in an implementation-specific format 0x1234\n"); + MeshTools::removeDuplicatesFuzzy(Trade::MeshData{MeshPrimitive::Points, + nullptr, {Trade::MeshAttributeData{Trade::MeshAttribute::Position, + vertexFormatWrap(0x1234), nullptr}}}); + CORRADE_COMPARE(out.str(), + "MeshTools::removeDuplicatesFuzzy(): can't remove duplicates in an implementation-specific format 0x1234\n"); } void RemoveDuplicatesTest::soakTest() {