From e4962412907eb16d1440e433a6865be8f559edaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 18 Jan 2022 21:14:04 +0100 Subject: [PATCH] MeshTools: unconditionally repack the input in removeDuplicates(). Since the main speed advantage of the function is that it hashes a *whole* vertex together instead of going through the attribute arrays one by one, it can't really operate on whatever funny interleaved layout it was given as there may be padding bytes with random content, breaking the deduplication. Since checking that a layout is really padding-less is rather complex, the repacking is now performed always. This also means the && overload makes no sense anymore and thus it was dropped. This makes the test added in the previous commit not assert anymore, and behave the same as the padding-less case. --- doc/changelog.dox | 11 +++++++++++ src/Magnum/MeshTools/RemoveDuplicates.cpp | 24 +++++++++++------------ src/Magnum/MeshTools/RemoveDuplicates.h | 17 +++------------- 3 files changed, 26 insertions(+), 26 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index e1b297ad7..b4b22f3fe 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -527,6 +527,17 @@ See also: for most shader-related limits - Fixed assertions related to OpenGL driver workarounds when the proprietary AMDGPU PRO drivers are used on Linux +- Fixed an assertion when using @ref MeshTools::removeDuplicates() on an + interleaved @ref Trade::MeshData that included padding at the beginning or + end of each vertex +- Fixed @ref MeshTools::removeDuplicates() to not take into account random + padding bytes and filtered-out attributes in interleaved source + @ref Trade::MeshData. This was a particularly glaring issue when using + @ref magnum-sceneconverter "magnum-sceneconverter" with both + `--only-attributes` and `--remove-duplicates` specified together, where it + usually just didn't remove any duplicate whatsoever while + `--remove-duplicates-fuzzy` did, no matter how ridiculously low epsilon + was used. - @ref Platform::EmscriptenApplication randomly created antialiased contexts due to an uninitialized variable in its @ref Platform::EmscriptenApplication::GLConfiguration "GLConfiguration" diff --git a/src/Magnum/MeshTools/RemoveDuplicates.cpp b/src/Magnum/MeshTools/RemoveDuplicates.cpp index e5581124f..101691e80 100644 --- a/src/Magnum/MeshTools/RemoveDuplicates.cpp +++ b/src/Magnum/MeshTools/RemoveDuplicates.cpp @@ -397,13 +397,6 @@ std::size_t removeDuplicatesFuzzyIndexedInPlace(const Containers::StridedArrayVi } Trade::MeshData removeDuplicates(const Trade::MeshData& data) { - return removeDuplicates(Trade::MeshData{data.primitive(), - {}, data.indexData(), Trade::MeshIndexData{data.indices()}, - {}, data.vertexData(), Trade::meshAttributeDataNonOwningArray(data.attributeData()), - data.vertexCount()}); -} - -Trade::MeshData removeDuplicates(Trade::MeshData&& data) { CORRADE_ASSERT(data.attributeCount(), "MeshTools::removeDuplicates(): can't remove duplicates in an attributeless mesh", (Trade::MeshData{MeshPrimitive::Points, 0})); @@ -418,12 +411,19 @@ Trade::MeshData removeDuplicates(Trade::MeshData&& data) { /* Turn the passed data into an interleaved owned mutable instance we can 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))); - + alone only makes the data interleaved (but the index data can stay + non-owned). Additionally we need to ensure the interleaved data are + tightly packed by removing InterleaveFlag::PreserveInterleavedAttributes + which is set by default, otherwise random padding bytes or filtered-out + attributes may contribute to the non-uniqueness of particular + elements. */ + Trade::MeshData ownedInterleaved = owned(interleave(data, {}, InterleaveFlags{})); + + /* Because the interleaved mesh was forced to be repacked, the vertex data + should span the whole stride -- this is relied on in the attribute + rerouting loop below */ const Containers::StridedArrayView2D vertexData = MeshTools::interleavedMutableData(ownedInterleaved); + CORRADE_INTERNAL_ASSERT(vertexData.size()[1] == ownedInterleaved.attributeStride(0)); UnsignedInt uniqueVertexCount; Containers::Array indexData; diff --git a/src/Magnum/MeshTools/RemoveDuplicates.h b/src/Magnum/MeshTools/RemoveDuplicates.h index 0c1ff9403..78f690183 100644 --- a/src/Magnum/MeshTools/RemoveDuplicates.h +++ b/src/Magnum/MeshTools/RemoveDuplicates.h @@ -302,24 +302,13 @@ interleaved and owned, if the input is already interleaved attribute offsets and paddings are preserved. All attributes are expected to not have an implementation-specific format. -This function unconditionally copies and interleaves passed vertex and index -data in order to operate on them in-place. If your data is interleaved and -owned by the instance and you don't need the original data after the process, -call @ref removeDuplicates(Trade::MeshData&&) instead to avoid the extra copy. +In order to remove random padding values from the input and make the vertices +suitable for fast in-place duplicate removal, this function unconditionally +copies and interleaves the input vertex and index data. @see @ref isVertexFormatImplementationSpecific() */ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData removeDuplicates(const Trade::MeshData& data); -/** -@brief Remove mesh data duplicates -@m_since{2020,06} - -Same as @ref removeDuplicates(const Trade::MeshData&), except that it operates -in-place on the passed instance, avoiding an extra copy of vertex and index -data. -*/ -MAGNUM_MESHTOOLS_EXPORT Trade::MeshData removeDuplicates(Trade::MeshData&& data); - /** @brief Remove mesh data duplicates with fuzzy comparison for floating-point attributes @m_since{2020,06}