From 8dddda9c77ea282811baa540b4a6c8217cc465ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 17 Jan 2022 17:41:06 +0100 Subject: [PATCH] Trade: support zero and negative strides in mesh attribute data. This is not something the classic GPU vertex pipeline can handle (except maybe Vulkan, which can handle zero strides for instanced attributes?), but useful for other scenarios. This means existing code needs to be aware of and handle the new corner case. --- doc/changelog.dox | 7 + doc/snippets/MagnumTrade.cpp | 11 ++ src/Magnum/MeshTools/RemoveDuplicates.cpp | 2 +- src/Magnum/SceneTools/sceneconverter.cpp | 3 +- src/Magnum/Trade/MeshData.cpp | 36 +++-- src/Magnum/Trade/MeshData.h | 90 +++++++----- src/Magnum/Trade/Test/MeshDataTest.cpp | 160 +++++++++++++++++++--- 7 files changed, 238 insertions(+), 71 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 9de79b7be..4119ac766 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -452,6 +452,9 @@ See also: @ref Trade::PhongMaterialData::commonTextureCoordinates() exposing a common texture coordinate set as a complement to a per-texture property added in 2020.06 +- @ref Trade::MeshData now allows zero and negative strides for better + data layout flexibility. This is however not commonly supported in GPU + APIs. - Added @ref Trade::MeshData::findAttributeId() for an ability to check that an attribute exists and retrieve its ID in a single step, avoiding a double lookup compared to @relativeref{Trade::MeshData,hasAttribute()} + @@ -875,6 +878,10 @@ See also: @cpp Trade::AbstractMaterialData @ce aliases to, doesn't have a @cpp virtual @ce destructor as subclasses with extra data members aren't a desired use case anymore. +- @ref Trade::MeshData now allows zero and negative strides for better + data layout flexibility, however as this is not commonly supported by GPU + APIs, it implies the user is now expected to validate this value when + passing it there. - @ref Trade::SceneData constructor taking a @ref std::vector of 2D and 3D children that got deprecated in favor of @ref Trade::SceneData::SceneData(SceneMappingType, UnsignedLong, Containers::Array&&, Containers::Array&&, const void*) diff --git a/doc/snippets/MagnumTrade.cpp b/doc/snippets/MagnumTrade.cpp index 75ac34c70..b508baad1 100644 --- a/doc/snippets/MagnumTrade.cpp +++ b/doc/snippets/MagnumTrade.cpp @@ -732,6 +732,17 @@ MeshTools::transformPointsInPlace(Matrix4::scaling(Vector3{2.0f}), /* [MeshData-usage-mutable] */ } +{ +Trade::MeshData data{MeshPrimitive::Points, 0}; +/* [MeshData-usage-special-layouts] */ +if(data.attributeStride(Trade::MeshAttribute::Position) <= 0 || + data.attributeStride(Trade::MeshAttribute::Normal) <= 0) + Fatal{} << "Uh oh"; + +// Now it's safe to use the Position and Normal attributes in a GPU mesh +/* [MeshData-usage-special-layouts] */ +} + { std::size_t vertexCount{}, indexCount{}; /* [MeshData-populating] */ diff --git a/src/Magnum/MeshTools/RemoveDuplicates.cpp b/src/Magnum/MeshTools/RemoveDuplicates.cpp index 101691e80..d0ee34bc1 100644 --- a/src/Magnum/MeshTools/RemoveDuplicates.cpp +++ b/src/Magnum/MeshTools/RemoveDuplicates.cpp @@ -423,7 +423,7 @@ Trade::MeshData removeDuplicates(const Trade::MeshData& 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)); + CORRADE_INTERNAL_ASSERT(vertexData.size()[1] == std::size_t(ownedInterleaved.attributeStride(0))); UnsignedInt uniqueVertexCount; Containers::Array indexData; diff --git a/src/Magnum/SceneTools/sceneconverter.cpp b/src/Magnum/SceneTools/sceneconverter.cpp index b2d7fb4f7..b0cbe0a7f 100644 --- a/src/Magnum/SceneTools/sceneconverter.cpp +++ b/src/Magnum/SceneTools/sceneconverter.cpp @@ -366,7 +366,8 @@ used.)") struct MeshAttributeInfo { std::size_t offset; - UnsignedInt stride, arraySize; + Int stride; + UnsignedInt arraySize; Trade::MeshAttribute name; std::string customName; VertexFormat format; diff --git a/src/Magnum/Trade/MeshData.cpp b/src/Magnum/Trade/MeshData.cpp index 9a98d6550..343d79df4 100644 --- a/src/Magnum/Trade/MeshData.cpp +++ b/src/Magnum/Trade/MeshData.cpp @@ -62,9 +62,6 @@ MeshAttributeData::MeshAttributeData(const MeshAttribute name, const VertexForma /* Yes, this calls into a constexpr function defined in the header -- because I feel that makes more sense than duplicating the full assert logic */ - /** @todo support zero / negative stride? would be hard to transfer to GL */ - CORRADE_ASSERT(data.empty() || isVertexFormatImplementationSpecific(format) || std::ptrdiff_t(vertexFormatSize(format))*(arraySize ? arraySize : 1) <= data.stride(), - "Trade::MeshAttributeData: expected stride to be positive and enough to fit" << format << Debug::nospace << (arraySize ? Utility::format("[{}]", arraySize).data() : "") << Debug::nospace << ", got" << data.stride(), ); } MeshAttributeData::MeshAttributeData(const MeshAttribute name, const VertexFormat format, const Containers::StridedArrayView2D& data, UnsignedShort arraySize) noexcept: MeshAttributeData{nullptr, name, format, Containers::StridedArrayView1D{{data.data(), ~std::size_t{}}, data.size()[0], data.stride()[0]}, arraySize} { @@ -140,15 +137,26 @@ MeshData::MeshData(const MeshPrimitive primitive, Containers::Array&& inde isVertexFormatImplementationSpecific(attribute._format) ? 0 : (vertexFormatSize(attribute._format)* (attribute._arraySize ? attribute._arraySize : 1)); + + /* Both pointer and offset-only rely on basically same calculation, + do it with offsets in a single place and only interpret as + pointers in the non-offset-only check. Yes, yes, this may read + the `pointer` union member through `offset`. */ + std::size_t begin = attribute._data.offset; + /* C integer promotion rules are weird, without the Int the result + is an unsigned 32-bit value that messes things up on 64bit */ + std::size_t end = begin + Int(_vertexCount - 1)*attribute._stride; + /* Flip for negative stride */ + if(begin > end) std::swap(begin, end); + /* Add the last element size to the higher address */ + end += typeSize; + if(attribute._isOffsetOnly) { - const std::size_t size = attribute._data.offset + (_vertexCount - 1)*attribute._stride + typeSize; - CORRADE_ASSERT(size <= _vertexData.size(), - "Trade::MeshData: offset-only attribute" << i << "spans" << size << "bytes but passed vertexData array has only" << _vertexData.size(), ); + CORRADE_ASSERT(end <= _vertexData.size(), + "Trade::MeshData: offset-only attribute" << i << "spans" << end << "bytes but passed vertexData array has only" << _vertexData.size(), ); } else { - const void* const begin = static_cast(attribute._data.pointer); - const void* const end = static_cast(attribute._data.pointer) + (_vertexCount - 1)*attribute._stride + typeSize; - CORRADE_ASSERT(begin >= _vertexData.begin() && end <= _vertexData.end(), - "Trade::MeshData: attribute" << i << "[" << Debug::nospace << begin << Debug::nospace << ":" << Debug::nospace << end << Debug::nospace << "] is not contained in passed vertexData array [" << Debug::nospace << static_cast(_vertexData.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast(_vertexData.end()) << Debug::nospace << "]", ); + CORRADE_ASSERT(reinterpret_cast(begin) >= _vertexData.begin() && reinterpret_cast(end) <= _vertexData.end(), + "Trade::MeshData: attribute" << i << "[" << Debug::nospace << reinterpret_cast(begin) << Debug::nospace << ":" << Debug::nospace << reinterpret_cast(end) << Debug::nospace << "] is not contained in passed vertexData array [" << Debug::nospace << static_cast(_vertexData.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast(_vertexData.end()) << Debug::nospace << "]", ); } } } @@ -306,7 +314,7 @@ std::size_t MeshData::attributeOffset(const UnsignedInt id) const { static_cast(_attributes[id]._data.pointer) - _vertexData.data(); } -UnsignedInt MeshData::attributeStride(const UnsignedInt id) const { +Short MeshData::attributeStride(const UnsignedInt id) const { CORRADE_ASSERT(id < _attributes.size(), "Trade::MeshData::attributeStride(): index" << id << "out of range for" << _attributes.size() << "attributes", {}); return _attributes[id]._stride; @@ -357,7 +365,7 @@ std::size_t MeshData::attributeOffset(const MeshAttribute name, const UnsignedIn return attributeOffset(attributeId); } -UnsignedInt MeshData::attributeStride(const MeshAttribute name, const UnsignedInt id) const { +Short MeshData::attributeStride(const MeshAttribute name, const UnsignedInt id) const { const UnsignedInt attributeId = findAttributeIdInternal(name, id); CORRADE_ASSERT(attributeId != ~UnsignedInt{}, "Trade::MeshData::attributeStride(): index" << id << "out of range for" << attributeCount(name) << name << "attributes", {}); return _attributes[attributeId]._stride; @@ -377,7 +385,7 @@ Containers::StridedArrayView2D MeshData::attribute(const UnsignedInt return Containers::arrayCast<2, const char>( attributeDataViewInternal(attribute), isVertexFormatImplementationSpecific(attribute._format) ? - attribute._stride : vertexFormatSize(attribute._format)* + Math::abs(attribute._stride) : vertexFormatSize(attribute._format)* (attribute._arraySize ? attribute._arraySize : 1)); } @@ -391,7 +399,7 @@ Containers::StridedArrayView2D MeshData::mutableAttribute(const UnsignedIn const auto out = Containers::arrayCast<2, const char>( attributeDataViewInternal(attribute), isVertexFormatImplementationSpecific(attribute._format) ? - attribute._stride : vertexFormatSize(attribute._format)* + Math::abs(attribute._stride) : vertexFormatSize(attribute._format)* (attribute._arraySize ? attribute._arraySize : 1)); /** @todo some arrayConstCast? UGH */ return Containers::StridedArrayView2D{ diff --git a/src/Magnum/Trade/MeshData.h b/src/Magnum/Trade/MeshData.h index 1f411b578..c66600311 100644 --- a/src/Magnum/Trade/MeshData.h +++ b/src/Magnum/Trade/MeshData.h @@ -350,9 +350,10 @@ class MAGNUM_TRADE_EXPORT MeshAttributeData { * @param arraySize Array size. Use @cpp 0 @ce for non-array * attributes. * - * Expects that @p data stride is large enough to fit all @p arraySize - * items of @p format, @p format corresponds to @p name and - * @p arraySize is zero for builtin attributes. + * Expects that @p data stride fits into a signed 16-bit value, + * @p format corresponds to @p name and @p arraySize is zero for + * builtin attributes. The stride can be zero or negative, but note + * that such data layouts are not commonly supported by GPU APIs. */ explicit MeshAttributeData(MeshAttribute name, VertexFormat format, const Containers::StridedArrayView1D& data, UnsignedShort arraySize = 0) noexcept; @@ -366,7 +367,10 @@ class MAGNUM_TRADE_EXPORT MeshAttributeData { * * Expects that the second dimension of @p data is contiguous and its * size matches @p format and @p arraySize, that @p format corresponds - * to @p name and @p arraySize is zero for builtin attributes. + * to @p name and @p arraySize is zero for builtin attributes. The + * stride is expected to fit into a signed 16-bit value. It can be zero + * or negative, but note that such data layouts are not commonly + * supported by GPU APIs. */ explicit MeshAttributeData(MeshAttribute name, VertexFormat format, const Containers::StridedArrayView2D& data, UnsignedShort arraySize = 0) noexcept; @@ -443,18 +447,18 @@ class MAGNUM_TRADE_EXPORT MeshAttributeData { * Instances created this way refer to an offset in unspecified * external vertex data instead of containing the data view directly. * Useful when the location of the vertex data array is not known at - * attribute construction time. Expects that @p arraySize is zero for - * builtin attributes. Note that instances created this way can't be - * used in most @ref MeshTools algorithms. + * attribute construction time. Note that instances created this way + * can't be used in most @ref MeshTools algorithms. + * + * Expects that @p arraySize is zero for builtin attributes and + * @p stride fits into a signed 16-bit value. The stride can be zero or + * negative, but note that such data layouts are not commonly supported + * by GPU APIs. * * Additionally, for even more flexibility, the @p vertexCount can be * overridden at @ref MeshData construction time, however all attributes * are still required to have the same vertex count to catch accidents. * - * Note that due to the @cpp constexpr @ce nature of this constructor, - * no @p format / @p arraySize checks against @p stride can be done. - * You're encouraged to use the @ref MeshAttributeData(MeshAttribute, VertexFormat, const Containers::StridedArrayView1D&, UnsignedShort) - * constructor if you want additional safeguards. * @see @ref isOffsetOnly(), @ref arraySize(), * @ref data(Containers::ArrayView) const */ @@ -471,7 +475,7 @@ class MAGNUM_TRADE_EXPORT MeshAttributeData { */ constexpr explicit MeshAttributeData(Int padding): _format{}, _name{}, _isOffsetOnly{false}, _vertexCount{0}, _stride{ (CORRADE_CONSTEXPR_ASSERT(padding >= -32768 && padding <= 32767, - "Trade::MeshAttributeData: at most 32k padding supported, got" << padding), Short(padding)) + "Trade::MeshAttributeData: expected padding to fit into 16 bits but got" << padding), Short(padding)) }, _arraySize{}, _data{nullptr} {} /** @@ -504,7 +508,8 @@ class MAGNUM_TRADE_EXPORT MeshAttributeData { /** * @brief Attribute stride * - * Can be negative for pad values, never negative for real attributes. + * In rare cases the stride may be zero or negative, such data layouts + * are however not commonly supported by GPU APIs. * @see @ref MeshAttributeData(Int) */ constexpr Short stride() const { return _stride; } @@ -517,6 +522,9 @@ class MAGNUM_TRADE_EXPORT MeshAttributeData { * * Expects that the attribute is not offset-only, in that case use the * @ref data(Containers::ArrayView) const overload instead. + * In rare cases the stride of the returned view may be zero or + * negative, such data layouts are however not commonly supported by + * GPU APIs. * @see @ref isOffsetOnly() */ constexpr Containers::StridedArrayView1D data() const { @@ -531,7 +539,9 @@ class MAGNUM_TRADE_EXPORT MeshAttributeData { * @brief Type-erased attribute data for an offset-only attribute * * If the attribute is not offset-only, the @p vertexData parameter is - * ignored. + * ignored. In rare cases the stride of the returned view may be zero + * or negative, such data layouts are however not commonly supported by + * GPU APIs. * @see @ref isOffsetOnly(), @ref data() const */ Containers::StridedArrayView1D data(Containers::ArrayView vertexData) const { @@ -1229,13 +1239,15 @@ class MAGNUM_TRADE_EXPORT MeshData { * @brief Attribute stride * * Stride between consecutive elements of given attribute in the - * @ref vertexData() array. The @p id is expected to be smaller - * than @ref attributeCount() const. You can also use - * @ref attributeStride(MeshAttribute, UnsignedInt) const to - * directly get a stride of given named attribute. + * @ref vertexData() array. In rare cases the stride may be zero or + * negative, such data layouts are however not commonly supported by + * GPU APIs. The @p id is expected to be smaller than + * @ref attributeCount() const. You can also use + * @ref attributeStride(MeshAttribute, UnsignedInt) const to directly + * get a stride of given named attribute. * @see @ref MeshTools::isInterleaved() */ - UnsignedInt attributeStride(UnsignedInt id) const; + Short attributeStride(UnsignedInt id) const; /** * @brief Attribute array size @@ -1320,11 +1332,13 @@ class MAGNUM_TRADE_EXPORT MeshData { * @brief Stride of a named attribute * * Stride between consecutive elements of given named attribute in the - * @ref vertexData() array. The @p id is expected to be smaller than + * @ref vertexData() array. In rare cases the stride may be zero or + * negative, such data layouts are however not commonly supported by + * GPU APIs. The @p id is expected to be smaller than * @ref attributeCount(MeshAttribute) const. * @see @ref attributeStride(UnsignedInt) const */ - UnsignedInt attributeStride(MeshAttribute name, UnsignedInt id = 0) const; + Short attributeStride(MeshAttribute name, UnsignedInt id = 0) const; /** * @brief Array size of a named attribute @@ -1343,8 +1357,11 @@ class MAGNUM_TRADE_EXPORT MeshData { * The @p id is expected to be smaller than @ref attributeCount() const. * The second dimension represents the actual data type (its size is * equal to format size for known @ref VertexFormat values, possibly - * multiplied by array size, and to attribute stride for + * multiplied by array size, and to *absolute* attribute stride for * implementation-specific values) and is guaranteed to be contiguous. + * In rare cases the first dimension stride may be zero or negative, + * such data layouts are however not commonly supported by GPU APIs. + * * Use the templated overload below to get the attribute in a concrete * type. * @see @relativeref{Corrade,Containers::StridedArrayView::isContiguous()}, @@ -1372,14 +1389,16 @@ class MAGNUM_TRADE_EXPORT MeshData { * access the attribute via the typeless @ref attribute(UnsignedInt) const * above. The attribute is also expected to not be an array, in that * case you need to use the overload below by using @cpp T[] @ce - * instead of @cpp T @ce. You can also use the non-templated + * instead of @cpp T @ce. In rare cases the stride of the returned view + * may be zero or negative, such data layouts are however not commonly + * supported by GPU APIs. You can also use the non-templated * @ref positions2DAsArray(), @ref positions3DAsArray(), * @ref tangentsAsArray(), @ref bitangentSignsAsArray(), * @ref bitangentsAsArray(), @ref normalsAsArray(), * @ref textureCoordinates2DAsArray(), @ref colorsAsArray() and * @ref objectIdsAsArray() accessors to get common attributes converted - * to usual types, but note that these operations involve extra - * allocation and data conversion. + * to usual types in contiguous arrays, but note that these operations + * involve extra allocation and data conversion. * @see @ref attribute(MeshAttribute, UnsignedInt) const, * @ref mutableAttribute(UnsignedInt), * @ref isVertexFormatImplementationSpecific(), @@ -1426,6 +1445,9 @@ class MAGNUM_TRADE_EXPORT MeshData { * represents the actual data type (its size is equal to format size * for known @ref VertexFormat values and to attribute stride for * implementation-specific values) and is guaranteed to be contiguous. + * In rare cases the first dimension stride may be zero or negative, + * such data layouts are however not commonly supported by GPU APIs. + * * Use the templated overload below to get the attribute in a concrete * type. * @see @ref attribute(UnsignedInt) const, @@ -1455,14 +1477,16 @@ class MAGNUM_TRADE_EXPORT MeshData { * @ref attribute(MeshAttribute, UnsignedInt) const above. The * attribute is also expected to not be an array, in that case you need * to use the overload below by using @cpp T[] @ce instead of - * @cpp T @ce. You can also use the non-templated + * @cpp T @ce. In rare cases the stride of the returned view may be + * zero or negative, such data layouts are however not commonly + * supported by GPU APIs. You can also use the non-templated * @ref positions2DAsArray(), @ref positions3DAsArray(), * @ref tangentsAsArray(), @ref bitangentSignsAsArray(), * @ref bitangentsAsArray(), @ref normalsAsArray(), * @ref textureCoordinates2DAsArray(), @ref colorsAsArray() and * @ref objectIdsAsArray() accessors to get common attributes converted - * to usual types, but note that these operations involve extra data - * conversion and an allocation. + * to usual types in contiguous arrays, but note that these operations + * involve extra data conversion and an allocation. * @see @ref attribute(UnsignedInt) const, * @ref mutableAttribute(MeshAttribute, UnsignedInt), * @ref isVertexFormatImplementationSpecific() @@ -2118,9 +2142,8 @@ constexpr MeshAttributeData::MeshAttributeData(std::nullptr_t, const MeshAttribu _name{(CORRADE_CONSTEXPR_ASSERT(Implementation::isVertexFormatCompatibleWithAttribute(name, format), "Trade::MeshAttributeData:" << format << "is not a valid format for" << name), name)}, _isOffsetOnly{false}, _vertexCount{UnsignedInt(data.size())}, - /** @todo support zero / negative stride? would be hard to transfer to GL */ - _stride{(CORRADE_CONSTEXPR_ASSERT(!(UnsignedInt(data.stride()) & 0xffff8000), - "Trade::MeshAttributeData: expected stride to be positive and at most 32k, got" << data.stride()), + _stride{(CORRADE_CONSTEXPR_ASSERT(data.stride() >= -32768 && data.stride() <= 32767, + "Trade::MeshAttributeData: expected stride to fit into 16 bits but got" << data.stride()), Short(data.stride()))}, _arraySize{(CORRADE_CONSTEXPR_ASSERT(!arraySize || Implementation::isAttributeArrayAllowed(name), "Trade::MeshAttributeData:" << name << "can't be an array attribute"), arraySize)}, @@ -2132,9 +2155,8 @@ constexpr MeshAttributeData::MeshAttributeData(const MeshAttribute name, const V _name{(CORRADE_CONSTEXPR_ASSERT(Implementation::isVertexFormatCompatibleWithAttribute(name, format), "Trade::MeshAttributeData:" << format << "is not a valid format for" << name), name)}, _isOffsetOnly{true}, _vertexCount{vertexCount}, - /** @todo support zero / negative stride? would be hard to transfer to GL */ - _stride{(CORRADE_CONSTEXPR_ASSERT(!(UnsignedInt(stride) & 0xffff8000), - "Trade::MeshAttributeData: expected stride to be positive and at most 32k, got" << stride), + _stride{(CORRADE_CONSTEXPR_ASSERT(stride >= -32768 && stride <= 32767, + "Trade::MeshAttributeData: expected stride to fit into 16 bits but got" << stride), Short(stride))}, _arraySize{(CORRADE_CONSTEXPR_ASSERT(!arraySize || Implementation::isAttributeArrayAllowed(name), "Trade::MeshAttributeData:" << name << "can't be an array attribute"), arraySize)}, diff --git a/src/Magnum/Trade/Test/MeshDataTest.cpp b/src/Magnum/Trade/Test/MeshDataTest.cpp index 0509fc924..25fc09bf6 100644 --- a/src/Magnum/Trade/Test/MeshDataTest.cpp +++ b/src/Magnum/Trade/Test/MeshDataTest.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include "Magnum/Math/Color.h" @@ -89,6 +90,9 @@ struct MeshDataTest: TestSuite::Tester { void constructIndexlessAttributeless(); void constructIndexlessAttributelessZeroVertices(); + void constructSpecialAttributeStrides(); + void constructSpecialAttributeStridesImplementationSpecificVertexFormat(); + void constructNotOwned(); void constructIndicesNotOwned(); void constructVerticesNotOwned(); @@ -247,7 +251,10 @@ MeshDataTest::MeshDataTest() { &MeshDataTest::constructIndexlessZeroVertices, &MeshDataTest::constructAttributeless, &MeshDataTest::constructIndexlessAttributeless, - &MeshDataTest::constructIndexlessAttributelessZeroVertices}); + &MeshDataTest::constructIndexlessAttributelessZeroVertices, + + &MeshDataTest::constructSpecialAttributeStrides, + &MeshDataTest::constructSpecialAttributeStridesImplementationSpecificVertexFormat}); addInstancedTests({&MeshDataTest::constructNotOwned}, Containers::arraySize(NotOwnedData)); @@ -760,25 +767,31 @@ void MeshDataTest::constructAttributeWrongStride() { CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); #endif - char positionData[3*sizeof(Vector3)]{}; + char toomuch[2*(32768 + sizeof(Vector2))]; + + /* These should be fine */ + MeshAttributeData{MeshAttribute::Position, Containers::StridedArrayView1D{Containers::arrayCast(toomuch), 2, 32767}}; + MeshAttributeData{MeshAttribute::Position, Containers::StridedArrayView1D{Containers::arrayCast(toomuch), 2, 32768}.flipped<0>()}; + MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector2, 0, 1, 32767}; + MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector2, 65536, 1, -32768}; + MeshAttributeData{32767}; + MeshAttributeData{-32768}; std::ostringstream out; Error redirectError{&out}; - MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector3, Containers::arrayCast(positionData)}; - MeshAttributeData{meshAttributeCustom(1), VertexFormat::Float, Containers::arrayCast(positionData), 4}; - /* We need this one to be constexpr, which means there can't be a warning - about stride not matching the size */ - MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector3, 0, 3*sizeof(Vector3), 1}; - MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector3, Containers::StridedArrayView1D{positionData, 0, -16}}; - MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector3, Containers::StridedArrayView1D{positionData, 0, 65000}}; - MeshAttributeData{65000}; + MeshAttributeData{MeshAttribute::Position, Containers::StridedArrayView1D{Containers::arrayCast(toomuch), 2, 32768}}; + MeshAttributeData{MeshAttribute::Position, Containers::StridedArrayView1D{Containers::arrayCast(toomuch), 2, 32769}.flipped<0>()}; + MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector2, 0, 1, 32768}; + MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector2, 65536, 1, -32769}; + MeshAttributeData{32768}; + MeshAttributeData{-32769}; CORRADE_COMPARE(out.str(), - "Trade::MeshAttributeData: expected stride to be positive and enough to fit VertexFormat::Vector3, got 1\n" - "Trade::MeshAttributeData: expected stride to be positive and enough to fit VertexFormat::Float[4], got 12\n" - "Trade::MeshAttributeData: expected stride to be positive and at most 32k, got -16\n" - "Trade::MeshAttributeData: expected stride to be positive and at most 32k, got 65000\n" - "Trade::MeshAttributeData: at most 32k padding supported, got 65000\n" - ); + "Trade::MeshAttributeData: expected stride to fit into 16 bits but got 32768\n" + "Trade::MeshAttributeData: expected stride to fit into 16 bits but got -32769\n" + "Trade::MeshAttributeData: expected stride to fit into 16 bits but got 32768\n" + "Trade::MeshAttributeData: expected stride to fit into 16 bits but got -32769\n" + "Trade::MeshAttributeData: expected padding to fit into 16 bits but got 32768\n" + "Trade::MeshAttributeData: expected padding to fit into 16 bits but got -32769\n"); } void MeshDataTest::constructAttributeWrongDataAccess() { @@ -1636,6 +1649,96 @@ void MeshDataTest::constructIndexlessAttributelessZeroVertices() { CORRADE_COMPARE(data.attributeCount(), 0); } +void MeshDataTest::constructSpecialAttributeStrides() { + Containers::Array vertexData{sizeof(UnsignedShort)*5}; + Containers::StridedArrayView1D vertices = Containers::arrayCast(vertexData); + Utility::copy({15, 1, 2, 3, 4}, vertices); + + MeshData mesh{MeshPrimitive::Points, std::move(vertexData), { + MeshAttributeData{MeshAttribute::ObjectId, vertices.prefix(1).broadcasted<0>(4)}, + MeshAttributeData{MeshAttribute::ObjectId, vertices.suffix(1).flipped<0>()}, + }}; + + CORRADE_COMPARE(mesh.attributeStride(0), 0); + CORRADE_COMPARE(mesh.attributeStride(1), -2); + + /* Type-erased access with a cast later */ + CORRADE_COMPARE_AS((Containers::arrayCast<1, const UnsignedShort>(mesh.attribute(0))), + Containers::arrayView({15, 15, 15, 15}), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS((Containers::arrayCast<1, UnsignedShort>(mesh.mutableAttribute(0))), + Containers::stridedArrayView({15, 15, 15, 15}), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS((Containers::arrayCast<1, const UnsignedShort>(mesh.attribute(1))), + Containers::arrayView({4, 3, 2, 1}), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS((Containers::arrayCast<1, UnsignedShort>(mesh.mutableAttribute(1))), + Containers::stridedArrayView({4, 3, 2, 1}), + TestSuite::Compare::Container); + + /* Typed access */ + CORRADE_COMPARE_AS(mesh.attribute(0), + Containers::arrayView({15, 15, 15, 15}), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS(mesh.mutableAttribute(0), + Containers::stridedArrayView({15, 15, 15, 15}), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS(mesh.attribute(1), + Containers::arrayView({4, 3, 2, 1}), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS(mesh.mutableAttribute(1), + Containers::stridedArrayView({4, 3, 2, 1}), + TestSuite::Compare::Container); + + /* All convenience accessors should work well also as they consume the + output of the type-erased one. But just to be sure, test at least + one. */ + CORRADE_COMPARE_AS(mesh.objectIdsAsArray(0), + Containers::arrayView({15, 15, 15, 15}), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS(mesh.objectIdsAsArray(1), + Containers::arrayView({4, 3, 2, 1}), + TestSuite::Compare::Container); +} + +void MeshDataTest::constructSpecialAttributeStridesImplementationSpecificVertexFormat() { + /* Same as constructSpecialAttributeStrides() except for custom vertex + formats, which causes the attribute() to return the full stride in + second dimension */ + + Containers::Array vertexData{sizeof(UnsignedShort)*5}; + Containers::StridedArrayView1D vertices = Containers::arrayCast(vertexData); + Utility::copy({15, 1, 2, 3, 4}, vertices); + + MeshData mesh{MeshPrimitive::Points, std::move(vertexData), { + MeshAttributeData{MeshAttribute::ObjectId, vertexFormatWrap(0xdead), vertices.prefix(1).broadcasted<0>(4)}, + MeshAttributeData{MeshAttribute::ObjectId, vertexFormatWrap(0xdead), vertices.suffix(1).flipped<0>()} + }}; + + CORRADE_COMPARE(mesh.attributeStride(0), 0); + CORRADE_COMPARE(mesh.attributeStride(1), -2); + + /* Type-erased access with a cast later. For the zero-stride attribute 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 vertex formats. */ + CORRADE_COMPARE(mesh.attribute(0).size(), (Containers::StridedDimensions<2, std::size_t>{4, 0})); + CORRADE_COMPARE(mesh.mutableAttribute(0).size(), (Containers::StridedDimensions<2, std::size_t>{4, 0})); + CORRADE_COMPARE(mesh.attribute(0).stride(), (Containers::StridedDimensions<2, std::ptrdiff_t>{0, 1})); + CORRADE_COMPARE(mesh.mutableAttribute(0).stride(), (Containers::StridedDimensions<2, std::ptrdiff_t>{0, 1})); + CORRADE_COMPARE(*reinterpret_cast(mesh.attribute(0).data()), 15); + CORRADE_COMPARE(*reinterpret_cast(mesh.mutableAttribute(0).data()), 15); + CORRADE_COMPARE_AS((Containers::arrayCast<1, const UnsignedShort>(mesh.attribute(1))), + Containers::arrayView({4, 3, 2, 1}), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS((Containers::arrayCast<1, UnsignedShort>(mesh.mutableAttribute(1))), + 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::constructIndexDataButNotIndexed() { #ifdef CORRADE_NO_ASSERT CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); @@ -1700,8 +1803,10 @@ void MeshDataTest::constructAttributeNotContained() { /* See implementationSpecificVertexFormatNotContained() below for implementation-specific formats */ - Containers::Array vertexData{reinterpret_cast(0xbadda9), 24, [](char*, std::size_t){}}; + const Containers::Array vertexData{reinterpret_cast(0xbadda9), 24, [](char*, std::size_t){}}; + Containers::Array sameVertexDataButMovable{reinterpret_cast(0xbadda9), 24, [](char*, std::size_t){}}; Containers::ArrayView vertexDataIn{reinterpret_cast(0xbadda9), 3}; + Containers::ArrayView vertexDataSlightlyOut{reinterpret_cast(0xbaddaa), 3}; Containers::ArrayView vertexDataOut{reinterpret_cast(0xdead), 3}; MeshAttributeData{MeshAttribute::Position, Containers::arrayCast(vertexData)}; @@ -1724,10 +1829,10 @@ void MeshDataTest::constructAttributeNotContained() { array size wouldn't be taken into account, it would span only 16 / 21, which fits into the vertex data size and thus wouldn't fail */ MeshData{MeshPrimitive::Triangles, {}, vertexData, { - MeshAttributeData{meshAttributeCustom(37), Containers::StridedArrayView2D{Containers::arrayCast(vertexData), {4, 5}, {5, 1}}} + MeshAttributeData{meshAttributeCustom(37), Containers::StridedArrayView2D{Containers::arrayCast(vertexData), {4, 5}, {5, 1}}} }, 5}; /* Verify the owning constructor does the same check */ - MeshData{MeshPrimitive::Triangles, std::move(vertexData), { + MeshData{MeshPrimitive::Triangles, std::move(sameVertexDataButMovable), { MeshAttributeData{MeshAttribute::Position, vertexDataIn}, MeshAttributeData{MeshAttribute::Position, Containers::arrayView(vertexDataOut)} }}; @@ -1736,7 +1841,7 @@ void MeshDataTest::constructAttributeNotContained() { MeshData{MeshPrimitive::Triangles, nullptr, { MeshAttributeData{MeshAttribute::Position, vertexDataIn} }}; - /* Finally, offset-only attributes with a different message */ + /* Offset-only attributes with a different message */ MeshData{MeshPrimitive::Triangles, Containers::Array{24}, { MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector2, 1, 3, 8} }}; @@ -1745,13 +1850,26 @@ void MeshDataTest::constructAttributeNotContained() { MeshData{MeshPrimitive::Triangles, Containers::Array{24}, { MeshAttributeData{meshAttributeCustom(37), VertexFormat::UnsignedByte, 0, 5, 5, 5} }}; + /* And the final boss, negative strides. Both only caught if the element + size gets properly added to the larger offset, not just the "end". */ + MeshData{MeshPrimitive::Triangles, {}, vertexData, { + MeshAttributeData{MeshAttribute::Position, stridedArrayView(vertexDataSlightlyOut).flipped<0>()} + }}; + MeshData{MeshPrimitive::Triangles, Containers::Array{24}, { + MeshAttributeData{meshAttributeCustom(37), VertexFormat::UnsignedByte, 24, 3, -8} + }}; CORRADE_COMPARE(out.str(), "Trade::MeshData: attribute 0 [0xbadda9:0xbaddc9] is not contained in passed vertexData array [0xbadda9:0xbaddc1]\n" "Trade::MeshData: attribute 0 [0xbadda9:0xbaddc2] is not contained in passed vertexData array [0xbadda9:0xbaddc1]\n" "Trade::MeshData: attribute 1 [0xdead:0xdec5] is not contained in passed vertexData array [0xbadda9:0xbaddc1]\n" "Trade::MeshData: attribute 0 [0xbadda9:0xbaddc1] is not contained in passed vertexData array [0x0:0x0]\n" + + "Trade::MeshData: offset-only attribute 0 spans 25 bytes but passed vertexData array has only 24\n" "Trade::MeshData: offset-only attribute 0 spans 25 bytes but passed vertexData array has only 24\n" - "Trade::MeshData: offset-only attribute 0 spans 25 bytes but passed vertexData array has only 24\n"); + + "Trade::MeshData: attribute 0 [0xbaddaa:0xbaddc2] is not contained in passed vertexData array [0xbadda9:0xbaddc1]\n" + "Trade::MeshData: offset-only attribute 0 spans 25 bytes but passed vertexData array has only 24\n" + ); } void MeshDataTest::constructInconsitentVertexCount() {