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}