From a67effb57a658f05d2be13b7524169c280a8a660 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 16 Nov 2022 12:18:08 +0100 Subject: [PATCH] Trade: allow array attributes to have implementation-specific formats. I'm not sure why this restriction was there as nothing was preventing them from being used. The attribute is only accessible through the typeless attribute(), which gives back `Containers::StridedArrayView2D` with second dimension size being set to the full stride. And there it doesn't matter if the format is an array or not. This will be useful for joint IDs and weights, for example doing crazy things like packing the IDs into an array of 8 4-bit numbers, saving half the memory compared to the smallest builtin representation using UnsignedByte[8]. --- doc/changelog.dox | 3 +++ src/Magnum/Trade/MeshData.h | 6 ++---- src/Magnum/Trade/Test/MeshDataTest.cpp | 30 ++++++++++++++++++++------ 3 files changed, 28 insertions(+), 11 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 1b41e7223..f252b5bcf 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -627,6 +627,9 @@ See also: attribute strides for better data layout flexibility. This is however not commonly supported by GPU APIs and tools like @ref MeshTools::compile() assert in that case. +- @ref Trade::MeshData now allows array attributes to have + implementation-specific vertex formats as well. The restriction didn't make + sense, as there was nothing in the design preventing them from being used. - 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()} + diff --git a/src/Magnum/Trade/MeshData.h b/src/Magnum/Trade/MeshData.h index f5d0254d9..8930cb51d 100644 --- a/src/Magnum/Trade/MeshData.h +++ b/src/Magnum/Trade/MeshData.h @@ -2287,8 +2287,7 @@ namespace Implementation { } constexpr MeshAttributeData::MeshAttributeData(std::nullptr_t, const MeshAttribute name, const VertexFormat format, const Containers::StridedArrayView1D& data, const UnsignedShort arraySize) noexcept: - _format{(CORRADE_CONSTEXPR_ASSERT(!arraySize || !isVertexFormatImplementationSpecific(format), - "Trade::MeshAttributeData: array attributes can't have an implementation-specific format"), format)}, + _format{format}, _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())}, @@ -2300,8 +2299,7 @@ constexpr MeshAttributeData::MeshAttributeData(std::nullptr_t, const MeshAttribu _data{data.data()} {} constexpr MeshAttributeData::MeshAttributeData(const MeshAttribute name, const VertexFormat format, const std::size_t offset, const UnsignedInt vertexCount, const std::ptrdiff_t stride, UnsignedShort arraySize) noexcept: - _format{(CORRADE_CONSTEXPR_ASSERT(!arraySize || !isVertexFormatImplementationSpecific(format), - "Trade::MeshAttributeData: array attributes can't have an implementation-specific format"), format)}, + _format{format}, _name{(CORRADE_CONSTEXPR_ASSERT(Implementation::isVertexFormatCompatibleWithAttribute(name, format), "Trade::MeshAttributeData:" << format << "is not a valid format for" << name), name)}, _isOffsetOnly{true}, _vertexCount{vertexCount}, diff --git a/src/Magnum/Trade/Test/MeshDataTest.cpp b/src/Magnum/Trade/Test/MeshDataTest.cpp index 43d46f187..4917faea0 100644 --- a/src/Magnum/Trade/Test/MeshDataTest.cpp +++ b/src/Magnum/Trade/Test/MeshDataTest.cpp @@ -85,6 +85,7 @@ struct MeshDataTest: TestSuite::Tester { void constructArrayAttributeTypeErased(); void constructArrayAttributeNullptr(); void constructArrayAttributeOffsetOnly(); + void constructArrayAttributeImplementationSpecificFormat(); void constructArrayAttributeNotAllowed(); void construct(); @@ -258,6 +259,7 @@ MeshDataTest::MeshDataTest() { &MeshDataTest::constructArrayAttributeTypeErased, &MeshDataTest::constructArrayAttributeNullptr, &MeshDataTest::constructArrayAttributeOffsetOnly, + &MeshDataTest::constructArrayAttributeImplementationSpecificFormat, &MeshDataTest::constructArrayAttributeNotAllowed}); addInstancedTests({&MeshDataTest::construct}, @@ -1129,6 +1131,19 @@ void MeshDataTest::constructArrayAttributeOffsetOnly() { CORRADE_COMPARE(cdata.arraySize(), 4); } +void MeshDataTest::constructArrayAttributeImplementationSpecificFormat() { + Vector2 positions[]{{1.0f, 0.3f}, {0.5f, 0.7f}}; + + /* This should not fire any asserts */ + MeshAttributeData a{meshAttributeCustom(35), vertexFormatWrap(0x3a), positions, 2}; + CORRADE_COMPARE(a.name(), meshAttributeCustom(35)); + CORRADE_COMPARE(a.format(), vertexFormatWrap(0x3a)); + CORRADE_COMPARE(a.arraySize(), 2); + CORRADE_COMPARE_AS(Containers::arrayCast(a.data()), + Containers::arrayView({{1.0f, 0.3f}, {0.5f, 0.7f}}), + TestSuite::Compare::Container); +} + void MeshDataTest::constructArrayAttributeNotAllowed() { CORRADE_SKIP_IF_NO_ASSERT(); @@ -1144,23 +1159,21 @@ void MeshDataTest::constructArrayAttributeNotAllowed() { MeshAttributeData{meshAttributeCustom(35), positions2D}; MeshAttributeData{meshAttributeCustom(35), VertexFormat::Vector2, positions2Dchar, 3}; MeshAttributeData{meshAttributeCustom(35), VertexFormat::Vector2, 0, 3, 6*sizeof(Vector2), 3}; + MeshAttributeData{meshAttributeCustom(35), vertexFormatWrap(0xdead), positions, 3}; + MeshAttributeData{meshAttributeCustom(35), vertexFormatWrap(0xdead), 0, 3, 6*sizeof(Vector2), 3}; /* This is not */ std::ostringstream out; Error redirectError{&out}; MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector2b, positions, 3}; - MeshAttributeData{meshAttributeCustom(35), vertexFormatWrap(0xdead), positions, 3}; MeshAttributeData{MeshAttribute::Position, positions2D}; MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector2, positions2Dchar, 3}; MeshAttributeData{MeshAttribute::Position, VertexFormat::Vector2, 0, 3, 6*sizeof(Vector2), 3}; - MeshAttributeData{meshAttributeCustom(35), vertexFormatWrap(0xdead), 0, 3, 6*sizeof(Vector2), 3}; CORRADE_COMPARE(out.str(), - "Trade::MeshAttributeData: Trade::MeshAttribute::Position can't be an array attribute\n" - "Trade::MeshAttributeData: array attributes can't have an implementation-specific format\n" "Trade::MeshAttributeData: Trade::MeshAttribute::Position can't be an array attribute\n" "Trade::MeshAttributeData: Trade::MeshAttribute::Position can't be an array attribute\n" "Trade::MeshAttributeData: Trade::MeshAttribute::Position can't be an array attribute\n" - "Trade::MeshAttributeData: array attributes can't have an implementation-specific format\n"); + "Trade::MeshAttributeData: Trade::MeshAttribute::Position can't be an array attribute\n"); } void MeshDataTest::construct() { @@ -1895,14 +1908,17 @@ void MeshDataTest::constructImplementationSpecificVertexFormat() { MeshAttributeData{MeshAttribute::TextureCoordinates, vertexFormatWrap(0xdead3), attribute}, MeshAttributeData{MeshAttribute::Color, - vertexFormatWrap(0xdead4), attribute}}}; + vertexFormatWrap(0xdead4), attribute}, + MeshAttributeData{meshAttributeCustom(35), + vertexFormatWrap(0xdead5), attribute, 27}}}; /* Getting typeless attribute should work also */ UnsignedInt format = 0xdead1; for(MeshAttribute name: {MeshAttribute::Position, MeshAttribute::Normal, MeshAttribute::TextureCoordinates, - MeshAttribute::Color}) { + MeshAttribute::Color, + meshAttributeCustom(35)}) { CORRADE_ITERATION(name); CORRADE_COMPARE(data.attributeFormat(name), vertexFormatWrap(format++));