From 01dfcc3a6869425973fa8844e87371ad97d7694a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 19 Jan 2022 18:57:46 +0100 Subject: [PATCH] MeshTools: fail isInterleaved() for zero and negative strides. I tried, I really did, but the downsides eventually just outweighed the potential of this feature and I gave up. May try to tackle this again in the future, but not now. --- src/Magnum/MeshTools/Interleave.cpp | 10 ++++-- src/Magnum/MeshTools/Interleave.h | 22 +++++++----- src/Magnum/MeshTools/Test/InterleaveTest.cpp | 38 ++++++++++++++++++++ 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/src/Magnum/MeshTools/Interleave.cpp b/src/Magnum/MeshTools/Interleave.cpp index eabe44119..55d7ce29a 100644 --- a/src/Magnum/MeshTools/Interleave.cpp +++ b/src/Magnum/MeshTools/Interleave.cpp @@ -47,7 +47,13 @@ Containers::Optional> interleavedData if(!data.attributeCount()) return Containers::StridedArrayView2D{data.vertexData(), {data.vertexCount(), 0}}; - const UnsignedInt stride = data.attributeStride(0); + /* Technically zero and negative strides *may* also be categorized as + interleaved if they are all the same, but it causes way too many + problems especially when used within interleavedLayout() etc. May + tackle properly later. */ + const Int stride = data.attributeStride(0); + if(stride <= 0) return Containers::NullOpt; + std::size_t minOffset = ~std::size_t{}; std::size_t maxOffset = 0; bool hasImplementationSpecificVertexFormat = false; @@ -77,7 +83,7 @@ Containers::Optional> interleavedData maxOffset = Math::max(maxOffset, minOffset + stride); /* The offsets can't fit into the stride, report failure */ - if(maxOffset - minOffset > stride) return Containers::NullOpt; + if(maxOffset - minOffset > UnsignedInt(stride)) return Containers::NullOpt; return Containers::StridedArrayView2D{ data.vertexData(), data.vertexData().data() + minOffset, diff --git a/src/Magnum/MeshTools/Interleave.h b/src/Magnum/MeshTools/Interleave.h index 86a3ea09f..60c2e13f7 100644 --- a/src/Magnum/MeshTools/Interleave.h +++ b/src/Magnum/MeshTools/Interleave.h @@ -228,10 +228,14 @@ CORRADE_ENUMSET_OPERATORS(InterleaveFlags) @brief If the mesh data is interleaved @m_since{2020,06} -Returns @cpp true @ce if all attributes have the same stride and the difference -between minimal and maximal offset is not larger than the stride, @cpp false @ce -otherwise. In particular, returns @cpp true @ce also if the mesh has just one -or no attributes. +Returns @cpp true @ce if all attributes have the same *positive* stride and the +difference between minimal and maximal offset is not larger than the stride, +@cpp false @ce otherwise. In particular, returns @cpp true @ce also if the mesh +has just one or no attributes. + +While interleaved layouts technically may also have zero or negative strides, +this case is currently not implemented and such layouts are treated as +non-interleaved. @see @ref Trade::MeshData::attributeStride(), @ref Trade::MeshData::attributeOffset(), @ref interleavedData() */ @@ -269,11 +273,11 @@ MAGNUM_MESHTOOLS_EXPORT Containers::StridedArrayView2D interleavedMutableD Returns a @ref Trade::MeshData instance with its vertex data allocated for @p vertexCount vertices containing attributes from both @p data and @p extra interleaved together. No data is actually copied, only an interleaved layout is -created. If @p data is already interleaved and -@ref InterleaveFlag::PreserveInterleavedAttributes is set in @p flags, keeps -the attributes in the same layout, potentially extending them with @p extra. -The @p extra attributes, if any, are interleaved together with existing -attributes. Returned instance vertex data flags have both +created. If @p data is already interleaved according to @ref isInterleaved() +and @ref InterleaveFlag::PreserveInterleavedAttributes is set in @p flags, +keeps the attributes in the same layout, potentially extending them with +@p extra. The @p extra attributes, if any, are interleaved together with +existing attributes. Returned instance vertex data flags have both @ref Trade::DataFlag::Mutable and @ref Trade::DataFlag::Owned, so mutable attribute access is guaranteed. diff --git a/src/Magnum/MeshTools/Test/InterleaveTest.cpp b/src/Magnum/MeshTools/Test/InterleaveTest.cpp index c57181f50..fe22891bd 100644 --- a/src/Magnum/MeshTools/Test/InterleaveTest.cpp +++ b/src/Magnum/MeshTools/Test/InterleaveTest.cpp @@ -64,6 +64,8 @@ struct InterleaveTest: Corrade::TestSuite::Tester { void interleavedDataNoVertices(); void interleavedDataNotInterleaved(); void interleavedDataAttributeAcrossStride(); + void interleavedDataZeroStride(); + void interleavedDataNegativeStride(); void interleavedDataVertexDataWholeMemory(); void interleavedMutableDataNotMutable(); void interleavedDataImplementationSpecificVertexFormat(); @@ -129,6 +131,8 @@ InterleaveTest::InterleaveTest() { &InterleaveTest::interleavedDataNoVertices, &InterleaveTest::interleavedDataNotInterleaved, &InterleaveTest::interleavedDataAttributeAcrossStride, + &InterleaveTest::interleavedDataZeroStride, + &InterleaveTest::interleavedDataNegativeStride, &InterleaveTest::interleavedDataVertexDataWholeMemory, &InterleaveTest::interleavedMutableDataNotMutable, &InterleaveTest::interleavedDataImplementationSpecificVertexFormat, @@ -422,6 +426,40 @@ void InterleaveTest::interleavedDataArrayAttributes() { CORRADE_COMPARE(interleaved.stride()[1], 1); } +void InterleaveTest::interleavedDataZeroStride() { + Containers::Array vertexData{100 + 20}; + Containers::StridedArrayView1D positions{vertexData, + reinterpret_cast(vertexData.data() + 100), 3, 0}; + Containers::StridedArrayView1D normals{vertexData, + reinterpret_cast(vertexData.data() + 100 + 8), 3, 0}; + + Trade::MeshData data{MeshPrimitive::Triangles, std::move(vertexData), { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, positions}, + Trade::MeshAttributeData{Trade::MeshAttribute::Normal, normals} + }}; + + /* Technically they *are*, but it causes way too many problems especially + when used within interleavedLayout() etc. May tackle properly later. */ + CORRADE_VERIFY(!MeshTools::isInterleaved(data)); +} + +void InterleaveTest::interleavedDataNegativeStride() { + Containers::Array vertexData{100 + 3*20}; + Containers::StridedArrayView1D positions{vertexData, + reinterpret_cast(vertexData.data() + 100), 3, 20}; + Containers::StridedArrayView1D normals{vertexData, + reinterpret_cast(vertexData.data() + 100 + 8), 3, 20}; + + Trade::MeshData data{MeshPrimitive::Triangles, std::move(vertexData), { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, positions.flipped<0>()}, + Trade::MeshAttributeData{Trade::MeshAttribute::Normal, normals.flipped<0>()} + }}; + + /* Technically they *are*, but it causes way too many problems especially + when used within interleavedLayout() etc. May tackle properly later. */ + CORRADE_VERIFY(!MeshTools::isInterleaved(data)); +} + void InterleaveTest::interleavedDataEmpty() { Trade::MeshData data{MeshPrimitive::Triangles, 5}; CORRADE_VERIFY(MeshTools::isInterleaved(data));