From 08a205f725cd9d48cbf49b93c7737cb75f73fd68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 17 Jan 2022 17:53:39 +0100 Subject: [PATCH] Trade: support implementation-specific index types in MeshData. This mirrors what's done already for implementation-specific vertex formats, thus: * Ability to construct the classes without tripping up when trying to check for type size in various asserts * Providing a zero-size type-erased access in indices() and mutableIndices() * Disallowing typed and convenience access --- src/Magnum/Trade/MeshData.cpp | 16 ++- src/Magnum/Trade/MeshData.h | 22 +++- src/Magnum/Trade/Test/MeshDataTest.cpp | 175 ++++++++++++++++++++++++- 3 files changed, 202 insertions(+), 11 deletions(-) diff --git a/src/Magnum/Trade/MeshData.cpp b/src/Magnum/Trade/MeshData.cpp index 0ff363ff7..a7eeabb02 100644 --- a/src/Magnum/Trade/MeshData.cpp +++ b/src/Magnum/Trade/MeshData.cpp @@ -128,6 +128,10 @@ MeshData::MeshData(const MeshPrimitive primitive, Containers::Array&& inde CORRADE_ASSERT(_indexCount || _indexData.empty(), "Trade::MeshData: indexData passed for a non-indexed mesh", ); if(_indexCount) { + const UnsignedInt typeSize = + isMeshIndexTypeImplementationSpecific(_indexType) ? 0 : + meshIndexTypeSize(_indexType); + const void* begin = _indices; /* C integer promotion rules are weird, without the Int the result is an unsigned 32-bit value that messes things up on 64bit */ @@ -135,7 +139,7 @@ MeshData::MeshData(const MeshPrimitive primitive, Containers::Array&& inde /* Flip for negative stride */ if(begin > end) std::swap(begin, end); /* Add the last element size to the higher address */ - end = static_cast(end) + meshIndexTypeSize(_indexType); + end = static_cast(end) + typeSize; CORRADE_ASSERT(begin >= _indexData.begin() && end <= _indexData.end(), "Trade::MeshData: indices [" << Debug::nospace << begin << Debug::nospace << ":" << Debug::nospace << end << Debug::nospace << "] are not contained in passed indexData array [" << Debug::nospace << static_cast(_indexData.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast(_indexData.end()) << Debug::nospace << "]", ); @@ -283,7 +287,9 @@ Containers::StridedArrayView2D MeshData::indices() const { /* For a non-indexed mesh returning zero size in both dimensions, indexed mesh with zero indices still has the second dimension non-zero */ if(!isIndexed()) return {}; - const std::size_t indexTypeSize = meshIndexTypeSize(_indexType); + const std::size_t indexTypeSize = + isMeshIndexTypeImplementationSpecific(_indexType) ? + Math::abs(_indexStride) : meshIndexTypeSize(_indexType); /* Build a 2D view using information about index type size and stride. We're *sure* the view is correct, so faking the view size */ return {{_indices, ~std::size_t{}}, {_indexCount, indexTypeSize}, {_indexStride, 1}}; @@ -295,7 +301,9 @@ Containers::StridedArrayView2D MeshData::mutableIndices() { /* For a non-indexed mesh returning zero size in both dimensions, indexed mesh with zero indices still has the second dimension non-zero */ if(!isIndexed()) return {}; - const std::size_t indexTypeSize = meshIndexTypeSize(_indexType); + const std::size_t indexTypeSize = + isMeshIndexTypeImplementationSpecific(_indexType) ? + Math::abs(_indexStride) : meshIndexTypeSize(_indexType); /* Build a 2D view using information about index type size and stride. We're *sure* the view is correct, so faking the view size */ Containers::StridedArrayView2D out{{_indices, ~std::size_t{}}, {_indexCount, indexTypeSize}, {_indexStride, 1}}; @@ -458,6 +466,8 @@ void MeshData::indicesInto(const Containers::StridedArrayView1D& de CORRADE_ASSERT(isIndexed(), "Trade::MeshData::indicesInto(): the mesh is not indexed", ); CORRADE_ASSERT(destination.size() == indexCount(), "Trade::MeshData::indicesInto(): expected a view with" << indexCount() << "elements but got" << destination.size(), ); + CORRADE_ASSERT(!isMeshIndexTypeImplementationSpecific(_indexType), + "Trade::MeshData::indicesInto(): can't extract data out of an implementation-specific index type" << reinterpret_cast(meshIndexTypeUnwrap(_indexType)), ); const auto destination1ui = Containers::arrayCast<2, UnsignedInt>(destination); const Containers::StridedArrayView2D indexData = indices(); diff --git a/src/Magnum/Trade/MeshData.h b/src/Magnum/Trade/MeshData.h index cc72deaf2..49e8d2bcf 100644 --- a/src/Magnum/Trade/MeshData.h +++ b/src/Magnum/Trade/MeshData.h @@ -1180,13 +1180,15 @@ class MAGNUM_TRADE_EXPORT MeshData { /** * @brief Mesh indices * - * For an indexed mesh, the second dimension of the view is guaranteed - * to be contiguous and its size is equal to type size, even in case - * there's zero indices. For a non-indexed mesh, the returned view has - * a zero size in both dimensions. In rare cases the first dimension - * stride may be different from the index type size and even be zero or - * negative, such data layouts are however not commonly supported by - * GPU APIs. + * For an indexed mesh, the second dimension represent the actual data + * type (its size is equal to type size for known @ref MeshIndexType + * values, and to *absolute* @ref indexStride() for + * implementation-specific values), even in case there's zero indices, + * and is guaranteed to be contiguous. For a non-indexed mesh, the + * returned view has a zero size in both dimensions. In rare cases the + * first dimension stride may be different from the index type size and + * even be zero or negative, such data layouts are however not commonly + * supported by GPU APIs. * * Use the templated overload below to get the indices in a concrete * type. @@ -1956,6 +1958,8 @@ namespace Implementation { #ifndef DOXYGEN_GENERATING_OUTPUT template MeshIndexData::MeshIndexData(const MeshIndexType type, T&& data) noexcept: _type{type} { const Containers::ArrayView erased = data; + CORRADE_ASSERT(!isMeshIndexTypeImplementationSpecific(type), + "Trade::MeshIndexData: can't create index data from a contiguous view and an implementation-specific type" << reinterpret_cast(meshIndexTypeUnwrap(type)) << Debug::nospace << ", pass a strided view instead", ); const std::size_t typeSize = meshIndexTypeSize(type); CORRADE_ASSERT(erased.size()%typeSize == 0, "Trade::MeshIndexData: view size" << erased.size() << "does not correspond to" << type, ); @@ -2274,6 +2278,8 @@ template Containers::StridedArrayView1D MeshData::indices() co #ifdef CORRADE_GRACEFUL_ASSERT /* Sigh. Brittle. Better idea? */ if(!data.stride()[1]) return {}; #endif + CORRADE_ASSERT(!isMeshIndexTypeImplementationSpecific(_indexType), + "Trade::MeshData::indices(): can't cast data from an implementation-specific index type" << reinterpret_cast(meshIndexTypeUnwrap(_indexType)), {}); CORRADE_ASSERT(Implementation::meshIndexTypeFor() == _indexType, "Trade::MeshData::indices(): indices are" << _indexType << "but requested" << Implementation::meshIndexTypeFor(), {}); return Containers::arrayCast<1, const T>(data); @@ -2286,6 +2292,8 @@ template Containers::StridedArrayView1D MeshData::mutableIndices() { #ifdef CORRADE_GRACEFUL_ASSERT /* Sigh. Brittle. Better idea? */ if(!data.stride()[1]) return {}; #endif + CORRADE_ASSERT(!isMeshIndexTypeImplementationSpecific(_indexType), + "Trade::MeshData::mutableIndices(): can't cast data from an implementation-specific index type" << reinterpret_cast(meshIndexTypeUnwrap(_indexType)), {}); CORRADE_ASSERT(Implementation::meshIndexTypeFor() == _indexType, "Trade::MeshData::mutableIndices(): indices are" << _indexType << "but requested" << Implementation::meshIndexTypeFor(), {}); return Containers::arrayCast<1, T>(data); diff --git a/src/Magnum/Trade/Test/MeshDataTest.cpp b/src/Magnum/Trade/Test/MeshDataTest.cpp index 09d469f24..e3523f445 100644 --- a/src/Magnum/Trade/Test/MeshDataTest.cpp +++ b/src/Magnum/Trade/Test/MeshDataTest.cpp @@ -48,8 +48,10 @@ struct MeshDataTest: TestSuite::Tester { void constructIndexStrided(); void constructIndexStridedWrongStride(); void constructIndexTypeErasedContiguous(); + void constructIndexTypeErasedContiguousImplementationSpecificFormat(); void constructIndexTypeErasedContiguousWrongSize(); void constructIndexTypeErasedStrided(); + void constructIndexTypeErasedStridedImplementationSpecificFormat(); void constructIndexTypeErasedStridedWrongStride(); void constructIndex2D(); void constructIndex2DNotIndexed(); @@ -95,8 +97,10 @@ struct MeshDataTest: TestSuite::Tester { void constructIndexlessAttributeless(); void constructIndexlessAttributelessZeroVertices(); + void constructImplementationSpecificIndexType(); void constructImplementationSpecificVertexFormat(); void constructSpecialIndexStrides(); + void constructSpecialIndexStridesImplementationSpecificIndexType(); void constructSpecialAttributeStrides(); void constructSpecialAttributeStridesImplementationSpecificVertexFormat(); @@ -161,6 +165,7 @@ struct MeshDataTest: TestSuite::Tester { template void objectIdsAsArray(); void objectIdsIntoArrayInvalidSize(); + void implementationSpecificIndexTypeWrongAccess(); void implementationSpecificVertexFormatWrongAccess(); void mutableAccessNotAllowed(); @@ -215,8 +220,10 @@ MeshDataTest::MeshDataTest() { &MeshDataTest::constructIndexStrided, &MeshDataTest::constructIndexStridedWrongStride, &MeshDataTest::constructIndexTypeErasedContiguous, + &MeshDataTest::constructIndexTypeErasedContiguousImplementationSpecificFormat, &MeshDataTest::constructIndexTypeErasedContiguousWrongSize, &MeshDataTest::constructIndexTypeErasedStrided, + &MeshDataTest::constructIndexTypeErasedStridedImplementationSpecificFormat, &MeshDataTest::constructIndexTypeErasedStridedWrongStride, &MeshDataTest::constructIndex2D, &MeshDataTest::constructIndex2DNotIndexed, @@ -263,8 +270,10 @@ MeshDataTest::MeshDataTest() { &MeshDataTest::constructIndexlessAttributeless, &MeshDataTest::constructIndexlessAttributelessZeroVertices, + &MeshDataTest::constructImplementationSpecificIndexType, &MeshDataTest::constructImplementationSpecificVertexFormat, &MeshDataTest::constructSpecialIndexStrides, + &MeshDataTest::constructSpecialIndexStridesImplementationSpecificIndexType, &MeshDataTest::constructSpecialAttributeStrides, &MeshDataTest::constructSpecialAttributeStridesImplementationSpecificVertexFormat}); @@ -388,6 +397,7 @@ MeshDataTest::MeshDataTest() { &MeshDataTest::objectIdsAsArray, &MeshDataTest::objectIdsIntoArrayInvalidSize, + &MeshDataTest::implementationSpecificIndexTypeWrongAccess, &MeshDataTest::implementationSpecificVertexFormatWrongAccess, &MeshDataTest::mutableAccessNotAllowed, @@ -601,6 +611,19 @@ void MeshDataTest::constructIndexTypeErasedContiguous() { CORRADE_COMPARE(indices.data().stride(), 2); } +void MeshDataTest::constructIndexTypeErasedContiguousImplementationSpecificFormat() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + const char indexData[3*2]{}; + + std::ostringstream out; + Error redirectError{&out}; + MeshIndexData{meshIndexTypeWrap(0xcaca), indexData}; + CORRADE_COMPARE(out.str(), "Trade::MeshIndexData: can't create index data from a contiguous view and an implementation-specific type 0xcaca, pass a strided view instead\n"); +} + void MeshDataTest::constructIndexTypeErasedContiguousWrongSize() { #ifdef CORRADE_NO_ASSERT CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); @@ -633,6 +656,16 @@ void MeshDataTest::constructIndexTypeErasedStrided() { CORRADE_COMPARE(data.stride(), 4); } +void MeshDataTest::constructIndexTypeErasedStridedImplementationSpecificFormat() { + const char indexData[3*2]{}; + + MeshIndexData indices{meshIndexTypeWrap(0xcaca), Containers::StridedArrayView1D{indexData, 3, 2}}; + CORRADE_COMPARE(indices.type(), meshIndexTypeWrap(0xcaca)); + CORRADE_COMPARE(indices.data().data(), indexData); + CORRADE_COMPARE(indices.data().size(), 3); + CORRADE_COMPARE(indices.data().stride(), 2); +} + void MeshDataTest::constructIndexTypeErasedStridedWrongStride() { #ifdef CORRADE_NO_ASSERT CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); @@ -1832,9 +1865,41 @@ void MeshDataTest::constructIndexlessAttributelessZeroVertices() { declaration outside */ struct VertexWithImplementationSpecificData { Long:64; + /* Using some definitely not a vertex format to test there's no weird + compile-time assertion preventing this */ long double thing; }; +void MeshDataTest::constructImplementationSpecificIndexType() { + /* Using some definitely not an index type to test there's no weird + compile-time assertion preventing this. Also using a strided view to + have the same case as with implementation-specific vertex formats + below -- for an implementation-specific type it's always strided, + anyway. */ + VertexWithImplementationSpecificData indexData[]{{12.3l}, {34.5l}, {45.6l}}; + + /* Constructing should work w/o asserts */ + Containers::StridedArrayView1D indices{indexData, + &indexData[0].thing, 3, sizeof(VertexWithImplementationSpecificData)}; + MeshData data{MeshPrimitive::Triangles, DataFlag::Mutable, indexData, + MeshIndexData{meshIndexTypeWrap(0xcaca), indices}, 1}; + + /* Getting typeless indices should work also */ + CORRADE_COMPARE(data.indexType(), meshIndexTypeWrap(0xcaca)); + CORRADE_COMPARE(data.indexCount(), 3); + CORRADE_COMPARE(data.indexStride(), sizeof(VertexWithImplementationSpecificData)); + + /* The actual type size is unknown, so this will use the full stride */ + CORRADE_COMPARE(data.indices().size()[1], sizeof(VertexWithImplementationSpecificData)); + + CORRADE_COMPARE_AS((Containers::arrayCast<1, const long double>( + data.indices().prefix({3, sizeof(long double)}))), + indices, TestSuite::Compare::Container); + CORRADE_COMPARE_AS((Containers::arrayCast<1, const long double>( + data.mutableIndices().prefix({3, sizeof(long double)}))), + indices, TestSuite::Compare::Container); +} + void MeshDataTest::constructImplementationSpecificVertexFormat() { VertexWithImplementationSpecificData vertexData[] { {456.0l}, @@ -1971,6 +2036,75 @@ void MeshDataTest::constructSpecialIndexStrides() { } } +void MeshDataTest::constructSpecialIndexStridesImplementationSpecificIndexType() { + /* Same as constructSpecialIndexStrides() except for custom index types, + which causes the indices() to return the full stride in second + dimension */ + + /* Every second index */ + { + Containers::Array indexData{sizeof(UnsignedShort)*8}; + Containers::StridedArrayView1D indices = Containers::arrayCast(indexData); + Utility::copy({1, 0, 2, 0, 3, 0, 4, 0}, indices); + MeshData mesh{MeshPrimitive::Points, std::move(indexData), MeshIndexData{meshIndexTypeWrap(0xcaca), indices.every(2)}, 1}; + + CORRADE_COMPARE(mesh.indexStride(), 4); + + /* Type-erased access with a cast later. The size is the whole stride, + so we need to take just the prefix we want. */ + CORRADE_COMPARE_AS((Containers::arrayCast<1, const UnsignedShort>(mesh.indices().prefix({mesh.indexCount(), 2}))), + Containers::arrayView({1, 2, 3, 4}), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS((Containers::arrayCast<1, UnsignedShort>(mesh.mutableIndices().prefix({mesh.indexCount(), 2}))), + Containers::stridedArrayView({1, 2, 3, 4}), + TestSuite::Compare::Container); + + /* Typed access and convenience accessors won't work here due to the + implementation-specific format */ + + /* Zero stride. The element size is zero as well, meaning there's no way to + access anything except for directly interpreting the data pointer. Which + is actually as desired for implementation-specific index types. */ + } { + Containers::Array indexData{sizeof(UnsignedShort)}; + Containers::StridedArrayView1D indices = Containers::arrayCast(indexData); + indices[0] = 15; + MeshData mesh{MeshPrimitive::Points, std::move(indexData), MeshIndexData{meshIndexTypeWrap(0xcaca), indices.broadcasted<0>(4)}, 1}; + + CORRADE_COMPARE(mesh.indexStride(), 0); + + CORRADE_COMPARE(mesh.indices().size(), (Containers::StridedDimensions<2, std::size_t>{4, 0})); + CORRADE_COMPARE(mesh.mutableIndices().size(), (Containers::StridedDimensions<2, std::size_t>{4, 0})); + CORRADE_COMPARE(mesh.indices().stride(), (Containers::StridedDimensions<2, std::ptrdiff_t>{0, 1})); + CORRADE_COMPARE(mesh.mutableIndices().stride(), (Containers::StridedDimensions<2, std::ptrdiff_t>{0, 1})); + CORRADE_COMPARE(*reinterpret_cast(mesh.indices().data()), 15); + CORRADE_COMPARE(*reinterpret_cast(mesh.mutableIndices().data()), 15); + + /* Typed access and convenience accessors won't work here due to the + implementation-specific format */ + + /* Negative stride */ + } { + Containers::Array indexData{sizeof(UnsignedShort)*4}; + Containers::StridedArrayView1D indices = Containers::arrayCast(indexData); + Utility::copy({1, 2, 3, 4}, indices); + MeshData mesh{MeshPrimitive::Points, std::move(indexData), MeshIndexData{meshIndexTypeWrap(0xcaca), indices.flipped<0>()}, 1}; + + CORRADE_COMPARE(mesh.indexStride(), -2); + + /* Type-erased access with a cast later */ + CORRADE_COMPARE_AS((Containers::arrayCast<1, const UnsignedShort>(mesh.indices())), + Containers::arrayView({4, 3, 2, 1}), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS((Containers::arrayCast<1, UnsignedShort>(mesh.mutableIndices())), + Containers::stridedArrayView({4, 3, 2, 1}), + TestSuite::Compare::Container); + + /* Typed access and convenience accessors won't work here due to the + implementation-specific format */ + } +} + void MeshDataTest::constructSpecialAttributeStrides() { Containers::Array vertexData{sizeof(UnsignedShort)*5}; Containers::StridedArrayView1D vertices = Containers::arrayCast(vertexData); @@ -2099,6 +2233,12 @@ void MeshDataTest::constructIndicesNotContained() { /* "Obviously good" case */ MeshData{MeshPrimitive::Triangles, {}, indexData, MeshIndexData{Containers::arrayCast(indexData)}, 1}; + /* An implementation-specific index type has a size assumed to be 0, so + even though the last element starts at 0xbaddaf it's fine */ + MeshData{MeshPrimitive::Triangles, {}, indexData, MeshIndexData{meshIndexTypeWrap(0xcaca), Containers::StridedArrayView1D{reinterpret_cast(0xbadda9 + sizeof(UnsignedShort)), 3}}, 1}; + /* This has both stride and size zero, so it's treated as both starting and + ending at 0xbaddaf */ + MeshData{MeshPrimitive::Triangles, {}, indexData, MeshIndexData{meshIndexTypeWrap(0xcaca), Containers::StridedArrayView1D{{reinterpret_cast(0xbaddaf), 1}, 1}.broadcasted<0>(3)}, 1}; std::ostringstream out; Error redirectError{&out}; @@ -2114,9 +2254,16 @@ void MeshDataTest::constructIndicesNotContained() { /* If we have no data at all, it doesn't try to dereference them but still checks properly */ MeshData{MeshPrimitive::Triangles, nullptr, MeshIndexData{indexDataOut}, 1}; + /* An implementation-specific index type has a size assumed to be 0, but + even then this exceeds the data by one byte */ + MeshData{MeshPrimitive::Triangles, {}, indexData, MeshIndexData{meshIndexTypeWrap(0xcaca), Containers::StridedArrayView1D{reinterpret_cast(0xbadda9 + sizeof(UnsignedShort) + 1), 3}}, 1}; /* And the final boss, negative strides. Only caught if the element size gets properly added to the larger offset, not just the "end". */ MeshData{MeshPrimitive::Triangles, {}, indexData, MeshIndexData{stridedArrayView(indexDataSlightlyOut).flipped<0>()}, 1}; + /* In this case the implementation-specific type is treated as having a + zero size, and the stride is zero as well, but since it starts one byte + after, it's wrong */ + MeshData{MeshPrimitive::Triangles, {}, indexData, MeshIndexData{meshIndexTypeWrap(0xcaca), Containers::StridedArrayView1D{{reinterpret_cast(0xbaddaf + 1), 1}, 1}.broadcasted<0>(3)}, 1}; CORRADE_COMPARE(out.str(), "Trade::MeshData: indices [0xdead:0xdeb3] are not contained in passed indexData array [0xbadda9:0xbaddaf]\n" "Trade::MeshData: indices [0xbaddaa:0xbaddb0] are not contained in passed indexData array [0xbadda9:0xbaddaf]\n" @@ -2125,7 +2272,10 @@ void MeshDataTest::constructIndicesNotContained() { "Trade::MeshData: indexData passed for a non-indexed mesh\n" "Trade::MeshData: indices [0xdead:0xdeb3] are not contained in passed indexData array [0x0:0x0]\n" - "Trade::MeshData: indices [0xbaddaa:0xbaddb0] are not contained in passed indexData array [0xbadda9:0xbaddaf]\n"); + "Trade::MeshData: indices [0xbaddac:0xbaddb0] are not contained in passed indexData array [0xbadda9:0xbaddaf]\n" + + "Trade::MeshData: indices [0xbaddaa:0xbaddb0] are not contained in passed indexData array [0xbadda9:0xbaddaf]\n" + "Trade::MeshData: indices [0xbaddb0:0xbaddb0] are not contained in passed indexData array [0xbadda9:0xbaddaf]\n"); } void MeshDataTest::constructAttributeNotContained() { @@ -3123,6 +3273,29 @@ void MeshDataTest::objectIdsIntoArrayInvalidSize() { "Trade::MeshData::objectIdsInto(): expected a view with 3 elements but got 2\n"); } +void MeshDataTest::implementationSpecificIndexTypeWrongAccess() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + VertexWithImplementationSpecificData indexData[3]; + + Containers::StridedArrayView1D indices{indexData, + &indexData[0].thing, 3, sizeof(VertexWithImplementationSpecificData)}; + MeshData data{MeshPrimitive::Triangles, DataFlag::Mutable, indexData, + MeshIndexData{meshIndexTypeWrap(0xcaca), indices}, 1}; + + std::ostringstream out; + Error redirectError{&out}; + data.indices(); + data.mutableIndices(); + data.indicesAsArray(); + CORRADE_COMPARE(out.str(), + "Trade::MeshData::indices(): can't cast data from an implementation-specific index type 0xcaca\n" + "Trade::MeshData::mutableIndices(): can't cast data from an implementation-specific index type 0xcaca\n" + "Trade::MeshData::indicesInto(): can't extract data out of an implementation-specific index type 0xcaca\n"); +} + void MeshDataTest::implementationSpecificVertexFormatWrongAccess() { #ifdef CORRADE_NO_ASSERT CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions");