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() {