From 9474b3e3fc6fe75d343e605ca96aac077f815d8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 19 May 2020 18:08:35 +0200 Subject: [PATCH] MeshTools: rename removeDuplicates{ => Fuzzy}[Indexed]InPlace(). A breaking change, sorry, but I don't want to add yet another layer of backwards compatibility on APIs that are in master for just a month or so. This is a test for how many people actually use these APIs -- if nobody complains, great! Conflating the fuzzy operation with the discrete one wasn't a good idea, as people could be unintentionally using the (slower) fuzzy variant on data that could be easily deduplicated using the discrete variant. One such case is in the icosphere primitive, and I'm going to look at that right now. --- doc/changelog.dox | 5 +- src/Magnum/MeshTools/RemoveDuplicates.h | 50 +++++++++---------- .../MeshTools/Test/RemoveDuplicatesTest.cpp | 22 ++++---- .../SubdivideRemoveDuplicatesBenchmark.cpp | 4 +- src/Magnum/Primitives/Icosphere.cpp | 2 +- 5 files changed, 42 insertions(+), 41 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 1f4e8054c..f3c162297 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -624,8 +624,9 @@ See also: @cpp MeshTools::removeDuplicates() @ce and @cpp MeshTools::subdivide() @ce operating on a @ref std::vector are deprecated, use the STL-free @ref MeshTools::compressIndices(), @ref MeshTools::duplicate(), - @ref MeshTools::removeDuplicatesInPlace() and @ref MeshTools::subdivide() / - @ref MeshTools::subdivideInPlace() overloads instead + @ref MeshTools::removeDuplicatesFuzzyInPlace() and + @ref MeshTools::subdivide() / @ref MeshTools::subdivideInPlace() overloads + instead - @cpp Primitives::CapsuleTextureCoords @ce, @cpp Primitives::CircleTextureCoords @ce, @cpp Primitives::PlaneTextureCoords @ce, diff --git a/src/Magnum/MeshTools/RemoveDuplicates.h b/src/Magnum/MeshTools/RemoveDuplicates.h index 40d7ee3e3..b668df147 100644 --- a/src/Magnum/MeshTools/RemoveDuplicates.h +++ b/src/Magnum/MeshTools/RemoveDuplicates.h @@ -64,9 +64,9 @@ namespace Implementation { Removes duplicate data from given array by comparing the second dimension of each item, the second dimension is expected to be contiguous. A plain bit-exact matching is used, if you need fuzzy comparison for floating-point data, use -@ref removeDuplicatesInPlace(const Containers::StridedArrayView1D&, typename Vector::Type) -instead. If you want to remove duplicate data from an already indexed array, -use @ref removeDuplicatesIndexedInPlace(const Containers::StridedArrayView1D&, const Containers::StridedArrayView2D&) +@ref removeDuplicatesFuzzyInPlace() instead. If you want to remove duplicate +data from an already indexed array, use +@ref removeDuplicatesIndexedInPlace(const Containers::StridedArrayView1D&, const Containers::StridedArrayView2D&) instead. Usage example: @snippet MagnumMeshTools.cpp removeDuplicates @@ -152,16 +152,16 @@ MAGNUM_MESHTOOLS_EXPORT std::size_t removeDuplicatesIndexedInPlace(const Contain Expects that the second dimension of @p indices is contiguous and represents the actual 1/2/4-byte index type. Based on its size then calls one of the -@ref removeDuplicatesIndexedInPlace(const Containers::StridedArrayView1D&) +@ref removeDuplicatesIndexedInPlace(const Containers::StridedArrayView1D&, const Containers::StridedArrayView2D&) etc. overloads. */ MAGNUM_MESHTOOLS_EXPORT std::size_t removeDuplicatesIndexedInPlace(const Containers::StridedArrayView2D& indices, const Containers::StridedArrayView2D& data); /** -@brief Remove duplicate floating-point vector data from given array in-place +@brief Remove duplicate data from given array using fuzzy comparison in-place @param[in,out] data Data array, duplicate items will be cut away with order preserved -@param[in] epsilon Epsilon value, vertices closer than this distance will be +@param[in] epsilon Epsilon value, data closer than this distance will be melt together @return Size of unique prefix in the cleaned up @p data array and the resulting index array @@ -175,23 +175,23 @@ bit-exact matching is sufficient use @ref removeDuplicatesInPlace(const Containe instead. If you want to remove duplicate data from an already indexed array, use -@ref removeDuplicatesIndexedInPlace(const Containers::StridedArrayView1D&, const Containers::StridedArrayView1D&, typename Vector::Type) instead. +@ref removeDuplicatesFuzzyIndexedInPlace(const Containers::StridedArrayView1D&, const Containers::StridedArrayView1D&, typename Vector::Type) instead. If you want to remove duplicates in multiple incidental arrays, first remove duplicates in each array separately and then combine the resulting index arrays back into a single one using @ref combineIndexedAttributes(). */ -template std::pair, std::size_t> removeDuplicatesInPlace(const Containers::StridedArrayView1D& data, typename Vector::Type epsilon = Math::TypeTraits::epsilon()); +template std::pair, std::size_t> removeDuplicatesFuzzyInPlace(const Containers::StridedArrayView1D& data, typename Vector::Type epsilon = Math::TypeTraits::epsilon()); #ifdef MAGNUM_BUILD_DEPRECATED /** -@brief Remove duplicate floating-point vector data from a STL vector in-place +@brief Remove duplicate data from a STL vector using fuzzy comparison in-place @param[in,out] data Data array, duplicate items will be cut away with order preserved and the size shrunk to just the unique prefix @param[in] epsilon Epsilon value, vertices closer than this distance will be melt together @return Resulting index array -@m_deprecated_since_latest Use @ref removeDuplicatesInPlace() instead. +@m_deprecated_since_latest Use @ref removeDuplicatesFuzzyInPlace() instead. Similar to the above, except that it's operating on a @ref std::vector, which gets shrunk as a result (instead of the prefix size being returned). This @@ -206,7 +206,7 @@ template CORRADE_DEPRECATED("use removeDuplicatesInPlace() instead #endif /** -@brief Remove duplicates from indexed floating-point vector data in-place +@brief Remove duplicates from indexed data using fuzzy comparison in-place @param[in,out] indices Index array, which will get remapped to list just unique vertices @param[in,out] data Data array, duplicate items will be cut away with order @@ -216,14 +216,14 @@ template CORRADE_DEPRECATED("use removeDuplicatesInPlace() instead @return Size of unique prefix in the cleaned up @p data array @m_since_latest -Compared to @ref removeDuplicatesInPlace(const Containers::StridedArrayView1D&, typename Vector::Type) +Compared to @ref removeDuplicatesFuzzyInPlace(const Containers::StridedArrayView1D&, typename Vector::Type) this variant is more suited for data that is already indexed as it works on the existing index array instead of allocating a new one. */ -template std::size_t removeDuplicatesIndexedInPlace(const Containers::StridedArrayView1D& indices, const Containers::StridedArrayView1D& data, typename Vector::Type epsilon = Math::TypeTraits::epsilon()) { +template std::size_t removeDuplicatesFuzzyIndexedInPlace(const Containers::StridedArrayView1D& indices, const Containers::StridedArrayView1D& data, typename Vector::Type epsilon = Math::TypeTraits::epsilon()) { /* Somehow ~IndexType{} doesn't work for < 4byte types, as the result is int(-1) instead of the type I want */ - CORRADE_ASSERT(data.size() <= IndexType(-1), "MeshTools::removeDuplicatesIndexedInPlace(): a" << sizeof(IndexType) << Debug::nospace << "-byte index type is too small for" << data.size() << "vertices", {}); + CORRADE_ASSERT(data.size() <= IndexType(-1), "MeshTools::removeDuplicatesFuzzyIndexedInPlace(): a" << sizeof(IndexType) << Debug::nospace << "-byte index type is too small for" << data.size() << "vertices", {}); /* Get bounds. When NaNs appear, those will get collapsed together when you're lucky, or cause the whole data to disappear when you're not -- it @@ -286,23 +286,23 @@ template std::size_t removeDuplicatesIndexedInPla } /** -@brief Remove duplicates from indexed data in-place on a type-erased index array +@brief Remove duplicates from indexed data using fuzzy comparison in-place on a type-erased index array @m_since_latest Expects that the second dimension of @p indices is contiguous and represents the actual 1/2/4-byte index type. Based on its size then calls -@ref removeDuplicatesIndexedInPlace(const Containers::StridedArrayView1D&, const Containers::StridedArrayView1D&, typename Vector::Type) +@ref removeDuplicatesFuzzyIndexedInPlace(const Containers::StridedArrayView1D&, const Containers::StridedArrayView1D&, typename Vector::Type) with a concrete index type. */ -template std::size_t removeDuplicatesIndexedInPlace(const Containers::StridedArrayView2D& indices, const Containers::StridedArrayView1D& data, typename Vector::Type epsilon = Math::TypeTraits::epsilon()) { - CORRADE_ASSERT(indices.isContiguous<1>(), "MeshTools::removeDuplicatesIndexedInPlace(): second index view dimension is not contiguous", {}); +template std::size_t removeDuplicatesFuzzyIndexedInPlace(const Containers::StridedArrayView2D& indices, const Containers::StridedArrayView1D& data, typename Vector::Type epsilon = Math::TypeTraits::epsilon()) { + CORRADE_ASSERT(indices.isContiguous<1>(), "MeshTools::removeDuplicatesFuzzyIndexedInPlace(): second index view dimension is not contiguous", {}); if(indices.size()[1] == 4) - return removeDuplicatesIndexedInPlace(Containers::arrayCast<1, UnsignedInt>(indices), data, epsilon); + return removeDuplicatesFuzzyIndexedInPlace(Containers::arrayCast<1, UnsignedInt>(indices), data, epsilon); else if(indices.size()[1] == 2) - return removeDuplicatesIndexedInPlace(Containers::arrayCast<1, UnsignedShort>(indices), data, epsilon); + return removeDuplicatesFuzzyIndexedInPlace(Containers::arrayCast<1, UnsignedShort>(indices), data, epsilon); else { - CORRADE_ASSERT(indices.size()[1] == 1, "MeshTools::removeDuplicatesIndexedInPlace(): expected index type size 1, 2 or 4 but got" << indices.size()[1], {}); - return removeDuplicatesIndexedInPlace(Containers::arrayCast<1, UnsignedByte>(indices), data, epsilon); + CORRADE_ASSERT(indices.size()[1] == 1, "MeshTools::removeDuplicatesFuzzyIndexedInPlace(): expected index type size 1, 2 or 4 but got" << indices.size()[1], {}); + return removeDuplicatesFuzzyIndexedInPlace(Containers::arrayCast<1, UnsignedByte>(indices), data, epsilon); } } @@ -336,11 +336,11 @@ data. */ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData removeDuplicates(Trade::MeshData&& data); -template std::pair, std::size_t> removeDuplicatesInPlace(const Containers::StridedArrayView1D& data, typename Vector::Type epsilon) { +template std::pair, std::size_t> removeDuplicatesFuzzyInPlace(const Containers::StridedArrayView1D& data, typename Vector::Type epsilon) { /* A trivial index array that'll be remapped and returned after */ Containers::Array indices{Containers::NoInit, data.size()}; std::iota(indices.begin(), indices.end(), 0); - const std::size_t size = removeDuplicatesIndexedInPlace(Containers::stridedArrayView(indices), data, epsilon); + const std::size_t size = removeDuplicatesFuzzyIndexedInPlace(Containers::stridedArrayView(indices), data, epsilon); return {std::move(indices), size}; } @@ -349,7 +349,7 @@ template std::vector removeDuplicates(std::vector indices(data.size()); std::iota(indices.begin(), indices.end(), 0); - const std::size_t size = removeDuplicatesIndexedInPlace(Containers::stridedArrayView(indices), Containers::stridedArrayView(data), epsilon); + const std::size_t size = removeDuplicatesFuzzyIndexedInPlace(Containers::stridedArrayView(indices), Containers::stridedArrayView(data), epsilon); data.resize(size); return indices; } diff --git a/src/Magnum/MeshTools/Test/RemoveDuplicatesTest.cpp b/src/Magnum/MeshTools/Test/RemoveDuplicatesTest.cpp index 7b8cfde85..31f36632c 100644 --- a/src/Magnum/MeshTools/Test/RemoveDuplicatesTest.cpp +++ b/src/Magnum/MeshTools/Test/RemoveDuplicatesTest.cpp @@ -278,7 +278,7 @@ void RemoveDuplicatesTest::removeDuplicatesFuzzyInPlace() { {1, 5} }; - std::pair, std::size_t> result = MeshTools::removeDuplicatesInPlace(Containers::stridedArrayView(data), 2); + std::pair, std::size_t> result = MeshTools::removeDuplicatesFuzzyInPlace(Containers::stridedArrayView(data), 2); CORRADE_COMPARE_AS(Containers::arrayView(result.first), Containers::arrayView({0, 0, 1, 1}), TestSuite::Compare::Container); @@ -321,7 +321,7 @@ template void RemoveDuplicatesTest::removeDuplicatesFuzzyIndexedInPlace {1, 5} }; - std::size_t count = MeshTools::removeDuplicatesIndexedInPlace( + std::size_t count = MeshTools::removeDuplicatesFuzzyIndexedInPlace( Containers::stridedArrayView(indices), Containers::stridedArrayView(data), 2); CORRADE_COMPARE_AS(Containers::arrayView(indices), @@ -342,10 +342,10 @@ void RemoveDuplicatesTest::removeDuplicatesFuzzyIndexedInPlaceSmallType() { UnsignedByte indices[1]; Vector2i data[256]{}; - MeshTools::removeDuplicatesIndexedInPlace( + MeshTools::removeDuplicatesFuzzyIndexedInPlace( Containers::stridedArrayView(indices), Containers::stridedArrayView(data), 2); - CORRADE_COMPARE(out.str(), "MeshTools::removeDuplicatesIndexedInPlace(): a 1-byte index type is too small for 256 vertices\n"); + CORRADE_COMPARE(out.str(), "MeshTools::removeDuplicatesFuzzyIndexedInPlace(): a 1-byte index type is too small for 256 vertices\n"); } void RemoveDuplicatesTest::removeDuplicatesFuzzyIndexedInPlaceEmptyIndices() { @@ -356,7 +356,7 @@ void RemoveDuplicatesTest::removeDuplicatesFuzzyIndexedInPlaceEmptyIndices() { {1, 5} }; - std::size_t count = MeshTools::removeDuplicatesIndexedInPlace( + std::size_t count = MeshTools::removeDuplicatesFuzzyIndexedInPlace( Containers::StridedArrayView1D{}, Containers::stridedArrayView(data), 2); CORRADE_COMPARE_AS(Containers::arrayView(data).prefix(count), @@ -365,7 +365,7 @@ void RemoveDuplicatesTest::removeDuplicatesFuzzyIndexedInPlaceEmptyIndices() { } void RemoveDuplicatesTest::removeDuplicatesFuzzyIndexedInPlaceEmptyIndicesVertices() { - CORRADE_COMPARE((MeshTools::removeDuplicatesIndexedInPlace({}, {}, 2)), 0); + CORRADE_COMPARE((MeshTools::removeDuplicatesFuzzyIndexedInPlace({}, {}, 2)), 0); } template void RemoveDuplicatesTest::removeDuplicatesFuzzyIndexedInPlaceErased() { @@ -380,7 +380,7 @@ template void RemoveDuplicatesTest::removeDuplicatesFuzzyIndexedInPlace {1, 5} }; - std::size_t count = MeshTools::removeDuplicatesIndexedInPlace( + std::size_t count = MeshTools::removeDuplicatesFuzzyIndexedInPlace( Containers::arrayCast<2, char>(Containers::arrayView(indices)), Containers::stridedArrayView(data), 2); CORRADE_COMPARE_AS(Containers::arrayView(indices), @@ -401,11 +401,11 @@ void RemoveDuplicatesTest::removeDuplicatesFuzzyIndexedInPlaceErasedNonContiguou std::stringstream out; Error redirectError{&out}; - MeshTools::removeDuplicatesIndexedInPlace( + MeshTools::removeDuplicatesFuzzyIndexedInPlace( Containers::StridedArrayView2D{indices, {6, 2}, {4, 2}}, Containers::stridedArrayView(data), 2); CORRADE_COMPARE(out.str(), - "MeshTools::removeDuplicatesIndexedInPlace(): second index view dimension is not contiguous\n"); + "MeshTools::removeDuplicatesFuzzyIndexedInPlace(): second index view dimension is not contiguous\n"); } void RemoveDuplicatesTest::removeDuplicatesFuzzyIndexedInPlaceErasedWrongIndexSize() { @@ -418,11 +418,11 @@ void RemoveDuplicatesTest::removeDuplicatesFuzzyIndexedInPlaceErasedWrongIndexSi std::stringstream out; Error redirectError{&out}; - MeshTools::removeDuplicatesIndexedInPlace( + MeshTools::removeDuplicatesFuzzyIndexedInPlace( Containers::StridedArrayView2D{indices, {6, 3}}, Containers::stridedArrayView(data), 2); CORRADE_COMPARE(out.str(), - "MeshTools::removeDuplicatesIndexedInPlace(): expected index type size 1, 2 or 4 but got 3\n"); + "MeshTools::removeDuplicatesFuzzyIndexedInPlace(): expected index type size 1, 2 or 4 but got 3\n"); } void RemoveDuplicatesTest::removeDuplicatesMeshData() { diff --git a/src/Magnum/MeshTools/Test/SubdivideRemoveDuplicatesBenchmark.cpp b/src/Magnum/MeshTools/Test/SubdivideRemoveDuplicatesBenchmark.cpp index 4d6f5ab50..b476bcdb6 100644 --- a/src/Magnum/MeshTools/Test/SubdivideRemoveDuplicatesBenchmark.cpp +++ b/src/Magnum/MeshTools/Test/SubdivideRemoveDuplicatesBenchmark.cpp @@ -93,7 +93,7 @@ void SubdivideRemoveDuplicatesBenchmark::subdivideAndRemoveDuplicatesAfter() { MeshTools::subdivide(indices, positions, interpolator); /* Remove duplicates after */ - arrayResize(positions, MeshTools::removeDuplicatesIndexedInPlace( + arrayResize(positions, MeshTools::removeDuplicatesFuzzyIndexedInPlace( stridedArrayView(indices), stridedArrayView(positions))); } } @@ -120,7 +120,7 @@ void SubdivideRemoveDuplicatesBenchmark::subdivideAndRemoveDuplicatesInBetween() /* Subdivide 5 times and remove duplicates during the operation */ for(std::size_t i = 0; i != 5; ++i) { MeshTools::subdivide(indices, positions, interpolator); - arrayResize(positions, MeshTools::removeDuplicatesIndexedInPlace( + arrayResize(positions, MeshTools::removeDuplicatesFuzzyIndexedInPlace( stridedArrayView(indices), stridedArrayView(positions))); } } diff --git a/src/Magnum/Primitives/Icosphere.cpp b/src/Magnum/Primitives/Icosphere.cpp index 0fd6ff1af..b09a9871b 100644 --- a/src/Magnum/Primitives/Icosphere.cpp +++ b/src/Magnum/Primitives/Icosphere.cpp @@ -116,7 +116,7 @@ Trade::MeshData icosphereSolid(const UnsignedInt subdivisions) { /** @todo i need arrayShrinkAndGiveUpMemoryIfItDoesntCauseRealloc() */ Containers::arrayResize(vertexData, - MeshTools::removeDuplicatesIndexedInPlace(Containers::stridedArrayView(indices), Containers::stridedArrayView(positions))*sizeof(Vertex)); + MeshTools::removeDuplicatesFuzzyIndexedInPlace(Containers::stridedArrayView(indices), Containers::stridedArrayView(positions))*sizeof(Vertex)); } /* Build up the views again with correct size, fill the normals */