From 69b6ebd2f542f0796eaa771e3145a6761a0c348b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 11 Jan 2022 23:11:52 +0100 Subject: [PATCH] MeshTools: assert on impl-specific vert format in transform*(). --- src/Magnum/MeshTools/Test/TransformTest.cpp | 78 ++++++++++++++++++++- src/Magnum/MeshTools/Transform.cpp | 57 +++++++++++---- src/Magnum/MeshTools/Transform.h | 37 ++++++---- 3 files changed, 142 insertions(+), 30 deletions(-) diff --git a/src/Magnum/MeshTools/Test/TransformTest.cpp b/src/Magnum/MeshTools/Test/TransformTest.cpp index d440aeea3..fc197f99c 100644 --- a/src/Magnum/MeshTools/Test/TransformTest.cpp +++ b/src/Magnum/MeshTools/Test/TransformTest.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include "Magnum/Math/Half.h" #include "Magnum/Math/Matrix3.h" @@ -52,6 +53,7 @@ struct TransformTest: TestSuite::Tester { template void meshData2D(); void meshData2DNoPosition(); void meshData2DNot2D(); + void meshData2DImplementationSpecificVertexFormat(); void meshData2DRvaluePassthrough(); void meshData2DRvaluePassthroughIndexDataNotOwned(); void meshData2DRvaluePassthroughVertexDataNotOwned(); @@ -66,6 +68,7 @@ struct TransformTest: TestSuite::Tester { template void meshData3D(); void meshData3DNoPosition(); void meshData3DNot3D(); + void meshData3DImplementationSpecificVertexFormat(); void meshData3DRvaluePassthrough(); void meshData3DRvaluePassthroughIndexDataNotOwned(); void meshData3DRvaluePassthroughVertexDataNotOwned(); @@ -79,6 +82,7 @@ struct TransformTest: TestSuite::Tester { template void meshDataTextureCoordinates2D(); void meshDataTextureCoordinates2DNoCoordinates(); + void meshDataTextureCoordinates2DImplementationSpecificVertexFormat(); void meshDataTextureCoordinates2DRvaluePassthrough(); void meshDataTextureCoordinates2DRvaluePassthroughIndexDataNotOwned(); void meshDataTextureCoordinates2DRvaluePassthroughVertexDataNotOwned(); @@ -183,6 +187,18 @@ const struct { /** @todo negative scaling that flips face winding */ }; +const struct { + const char* name; + VertexFormat positionFormat; + Trade::MeshAttribute otherAttribute; + VertexFormat otherAttributeFormat; +} MeshData3DIMplementationSpecificVertexFormatData[]{ + {"positions", vertexFormatWrap(0xcaca), Trade::MeshAttribute::Position, VertexFormat::Vector3}, + {"normals", VertexFormat::Vector3, Trade::MeshAttribute::Normal, vertexFormatWrap(0xcaca)}, + {"tangents", VertexFormat::Vector3, Trade::MeshAttribute::Tangent, vertexFormatWrap(0xcaca)}, + {"bitangents", VertexFormat::Vector3, Trade::MeshAttribute::Bitangent, vertexFormatWrap(0xcaca)}, +}; + const struct { const char* name; bool indexed; @@ -269,7 +285,8 @@ TransformTest::TransformTest() { }, Containers::arraySize(MeshData2DData)); addTests({&TransformTest::meshData2DNoPosition, - &TransformTest::meshData2DNot2D}); + &TransformTest::meshData2DNot2D, + &TransformTest::meshData2DImplementationSpecificVertexFormat}); addInstancedTests({&TransformTest::meshData2DRvaluePassthrough}, Containers::arraySize(MeshData2DRvaluePassthroughData)); @@ -292,6 +309,9 @@ TransformTest::TransformTest() { addTests({&TransformTest::meshData3DNoPosition, &TransformTest::meshData3DNot3D}); + addInstancedTests({&TransformTest::meshData3DImplementationSpecificVertexFormat}, + Containers::arraySize(MeshData3DIMplementationSpecificVertexFormatData)); + addInstancedTests({&TransformTest::meshData3DRvaluePassthrough}, Containers::arraySize(MeshData3DRvaluePassthroughData)); @@ -313,7 +333,8 @@ TransformTest::TransformTest() { &TransformTest::meshDataTextureCoordinates2D }, Containers::arraySize(MeshDataTextureCoordinatesData)); - addTests({&TransformTest::meshDataTextureCoordinates2DNoCoordinates}); + addTests({&TransformTest::meshDataTextureCoordinates2DNoCoordinates, + &TransformTest::meshDataTextureCoordinates2DImplementationSpecificVertexFormat}); addInstancedTests({&TransformTest::meshDataTextureCoordinates2DRvaluePassthrough}, Containers::arraySize(MeshDataTextureCoordinatesRvaluePassthroughData)); @@ -483,6 +504,22 @@ void TransformTest::meshData2DNot2D() { CORRADE_COMPARE(out.str(), "MeshTools::transform2D(): expected 2D positions but got VertexFormat::Vector3\n"); } +void TransformTest::meshData2DImplementationSpecificVertexFormat() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + Trade::MeshData mesh{MeshPrimitive::Points, nullptr, { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, VertexFormat::Vector2, nullptr}, + Trade::MeshAttributeData{Trade::MeshAttribute::Position, vertexFormatWrap(0xcaca), nullptr}, + }}; + + std::ostringstream out; + Error redirectError{&out}; + transform2D(mesh, {}, 1); + CORRADE_COMPARE(out.str(), "MeshTools::transform2D(): positions have an implementation-specific format 0xcaca\n"); +} + void TransformTest::meshData2DRvaluePassthrough() { auto&& data = MeshData2DRvaluePassthroughData[testCaseInstanceId()]; setTestCaseDescription(data.name); @@ -881,6 +918,27 @@ void TransformTest::meshData3DNot3D() { CORRADE_COMPARE(out.str(), "MeshTools::transform3D(): expected 3D positions but got VertexFormat::Vector2\n"); } +void TransformTest::meshData3DImplementationSpecificVertexFormat() { + auto&& data = MeshData3DIMplementationSpecificVertexFormatData[testCaseInstanceId()]; + setTestCaseDescription(data.name); + + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + Trade::MeshData mesh{MeshPrimitive::Points, nullptr, { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, VertexFormat::Vector3, nullptr}, + Trade::MeshAttributeData{Trade::MeshAttribute::Position, data.positionFormat, nullptr}, + Trade::MeshAttributeData{data.otherAttribute, VertexFormat::Vector3, nullptr}, + Trade::MeshAttributeData{data.otherAttribute, data.otherAttributeFormat, nullptr}, + }}; + + std::ostringstream out; + Error redirectError{&out}; + transform3D(mesh, {}, 1); + CORRADE_COMPARE(out.str(), Utility::formatString("MeshTools::transform3D(): {} have an implementation-specific format 0xcaca\n", data.name)); +} + void TransformTest::meshData3DRvaluePassthrough() { auto&& data = MeshData3DRvaluePassthroughData[testCaseInstanceId()]; setTestCaseDescription(data.name); @@ -1325,6 +1383,22 @@ void TransformTest::meshDataTextureCoordinates2DNoCoordinates() { CORRADE_COMPARE(out.str(), "MeshTools::transformTextureCoordinates2D(): the mesh has no texture coordinates with index 1\n"); } +void TransformTest::meshDataTextureCoordinates2DImplementationSpecificVertexFormat() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + Trade::MeshData mesh{MeshPrimitive::Points, nullptr, { + Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates, VertexFormat::Vector2, nullptr}, + Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates, vertexFormatWrap(0xcaca), nullptr}, + }}; + + std::ostringstream out; + Error redirectError{&out}; + transformTextureCoordinates2D(mesh, {}, 1); + CORRADE_COMPARE(out.str(), "MeshTools::transformTextureCoordinates2D(): texture coordinates have an implementation-specific format 0xcaca\n"); +} + void TransformTest::meshDataTextureCoordinates2DRvaluePassthrough() { auto&& data = MeshDataTextureCoordinatesRvaluePassthroughData[testCaseInstanceId()]; setTestCaseDescription(data.name); diff --git a/src/Magnum/MeshTools/Transform.cpp b/src/Magnum/MeshTools/Transform.cpp index c5ec783c7..d6d3d9fbd 100644 --- a/src/Magnum/MeshTools/Transform.cpp +++ b/src/Magnum/MeshTools/Transform.cpp @@ -38,6 +38,9 @@ Trade::MeshData transform2D(const Trade::MeshData& data, const Matrix3& transfor "MeshTools::transform2D(): the mesh has no positions with index" << id, (Trade::MeshData{MeshPrimitive::Triangles, 0})); const VertexFormat positionAttributeFormat = data.attributeFormat(*positionAttributeId); + CORRADE_ASSERT(!isVertexFormatImplementationSpecific(positionAttributeFormat), + "MeshTools::transform2D(): positions have an implementation-specific format" << reinterpret_cast(vertexFormatUnwrap(positionAttributeFormat)), + (Trade::MeshData{MeshPrimitive::Points, 0})); CORRADE_ASSERT(vertexFormatComponentCount(positionAttributeFormat) == 2, "MeshTools::transform2D(): expected 2D positions but got" << positionAttributeFormat, (Trade::MeshData{MeshPrimitive::Triangles, 0})); @@ -110,13 +113,13 @@ Trade::MeshData transform3D(const Trade::MeshData& data, const Matrix4& transfor "MeshTools::transform3D(): the mesh has no positions with index" << id, (Trade::MeshData{MeshPrimitive::Triangles, 0})); const VertexFormat positionAttributeFormat = data.attributeFormat(*positionAttributeId); + CORRADE_ASSERT(!isVertexFormatImplementationSpecific(positionAttributeFormat), + "MeshTools::transform3D(): positions have an implementation-specific format" << reinterpret_cast(vertexFormatUnwrap(positionAttributeFormat)), + (Trade::MeshData{MeshPrimitive::Points, 0})); CORRADE_ASSERT(vertexFormatComponentCount(positionAttributeFormat) == 3, "MeshTools::transform3D(): expected 3D positions but got" << positionAttributeFormat, (Trade::MeshData{MeshPrimitive::Triangles, 0})); const Containers::Optional tangentAttributeId = data.findAttributeId(Trade::MeshAttribute::Tangent, id); - const VertexFormat desiredTangentVertexFormat = tangentAttributeId ? - (vertexFormatComponentCount(data.attributeFormat(*tangentAttributeId)) == 4 ? - VertexFormat::Vector4 : VertexFormat::Vector3) : VertexFormat{}; const Containers::Optional bitangentAttributeId = data.findAttributeId(Trade::MeshAttribute::Bitangent, id); const Containers::Optional normalAttributeId = data.findAttributeId(Trade::MeshAttribute::Normal, id); @@ -132,12 +135,36 @@ Trade::MeshData transform3D(const Trade::MeshData& data, const Matrix4& transfor with an empty placeholder that we'll unpack the data into */ if(positionAttributeFormat != VertexFormat::Vector3) attributes[*positionAttributeId] = Trade::MeshAttributeData{Trade::MeshAttribute::Position, VertexFormat::Vector3, nullptr}; - if(tangentAttributeId && data.attributeFormat(*tangentAttributeId) != desiredTangentVertexFormat) - attributes[*tangentAttributeId] = Trade::MeshAttributeData{Trade::MeshAttribute::Tangent, desiredTangentVertexFormat, nullptr}; - if(bitangentAttributeId && data.attributeFormat(*bitangentAttributeId) != VertexFormat::Vector3) - attributes[*bitangentAttributeId] = Trade::MeshAttributeData{Trade::MeshAttribute::Bitangent, VertexFormat::Vector3, nullptr}; - if(normalAttributeId && data.attributeFormat(*normalAttributeId) != VertexFormat::Vector3) - attributes[*normalAttributeId] = Trade::MeshAttributeData{Trade::MeshAttribute::Normal, VertexFormat::Vector3, nullptr}; + VertexFormat tangentAttributeFormat{}; + VertexFormat desiredTangentVertexFormat{}; + if(tangentAttributeId) { + tangentAttributeFormat = data.attributeFormat(*tangentAttributeId); + CORRADE_ASSERT(!isVertexFormatImplementationSpecific(tangentAttributeFormat), + "MeshTools::transform3D(): tangents have an implementation-specific format" << reinterpret_cast(vertexFormatUnwrap(tangentAttributeFormat)), + (Trade::MeshData{MeshPrimitive::Points, 0})); + desiredTangentVertexFormat = vertexFormatComponentCount(data.attributeFormat(*tangentAttributeId)) == 4 ? + VertexFormat::Vector4 : VertexFormat::Vector3; + if(tangentAttributeFormat != desiredTangentVertexFormat) + attributes[*tangentAttributeId] = Trade::MeshAttributeData{Trade::MeshAttribute::Tangent, desiredTangentVertexFormat, nullptr}; + } + VertexFormat bitangentAttributeFormat{}; + if(bitangentAttributeId) { + bitangentAttributeFormat = data.attributeFormat(*bitangentAttributeId); + CORRADE_ASSERT(!isVertexFormatImplementationSpecific(bitangentAttributeFormat), + "MeshTools::transform3D(): bitangents have an implementation-specific format" << reinterpret_cast(vertexFormatUnwrap(bitangentAttributeFormat)), + (Trade::MeshData{MeshPrimitive::Points, 0})); + if(bitangentAttributeFormat != VertexFormat::Vector3) + attributes[*bitangentAttributeId] = Trade::MeshAttributeData{Trade::MeshAttribute::Bitangent, VertexFormat::Vector3, nullptr}; + } + VertexFormat normalAttributeFormat{}; + if(normalAttributeId) { + normalAttributeFormat = data.attributeFormat(*normalAttributeId); + CORRADE_ASSERT(!isVertexFormatImplementationSpecific(normalAttributeFormat), + "MeshTools::transform3D(): normals have an implementation-specific format" << reinterpret_cast(vertexFormatUnwrap(normalAttributeFormat)), + (Trade::MeshData{MeshPrimitive::Points, 0})); + if(normalAttributeFormat != VertexFormat::Vector3) + attributes[*normalAttributeId] = Trade::MeshAttributeData{Trade::MeshAttribute::Normal, VertexFormat::Vector3, nullptr}; + } /* Create the output mesh, making more room for the full formats if necessary */ @@ -149,7 +176,7 @@ Trade::MeshData transform3D(const Trade::MeshData& data, const Matrix4& transfor /* If the position/TBN attributes weren't in a desired format, unpack them */ if(positionAttributeFormat != VertexFormat::Vector3) data.positions3DInto(out.mutableAttribute(*positionAttributeId), id); - if(tangentAttributeId && data.attributeFormat(*tangentAttributeId) != desiredTangentVertexFormat) { + if(tangentAttributeId && tangentAttributeFormat != desiredTangentVertexFormat) { if(desiredTangentVertexFormat == VertexFormat::Vector4) { data.tangentsInto(out.mutableAttribute(*tangentAttributeId).slice(&Vector4::xyz), id); data.bitangentSignsInto(out.mutableAttribute(*tangentAttributeId).slice(&Vector4::w), id); @@ -157,9 +184,9 @@ Trade::MeshData transform3D(const Trade::MeshData& data, const Matrix4& transfor data.tangentsInto(out.mutableAttribute(*tangentAttributeId), id); } } - if(bitangentAttributeId && data.attributeFormat(*bitangentAttributeId) != VertexFormat::Vector3) + if(bitangentAttributeId && bitangentAttributeFormat != VertexFormat::Vector3) data.bitangentsInto(out.mutableAttribute(*bitangentAttributeId), id); - if(normalAttributeId && data.attributeFormat(*normalAttributeId) != VertexFormat::Vector3) + if(normalAttributeId && normalAttributeFormat != VertexFormat::Vector3) data.normalsInto(out.mutableAttribute(*normalAttributeId), id); /* Delegate to the in-place implementation and return */ @@ -245,6 +272,10 @@ Trade::MeshData transformTextureCoordinates2D(const Trade::MeshData& data, const CORRADE_ASSERT(textureCoordinateAttributeId, "MeshTools::transformTextureCoordinates2D(): the mesh has no texture coordinates with index" << id, (Trade::MeshData{MeshPrimitive::Triangles, 0})); + const VertexFormat textureCoordinateAttributeFormat = data.attributeFormat(*textureCoordinateAttributeId); + CORRADE_ASSERT(!isVertexFormatImplementationSpecific(textureCoordinateAttributeFormat), + "MeshTools::transformTextureCoordinates2D(): texture coordinates have an implementation-specific format" << reinterpret_cast(vertexFormatUnwrap(textureCoordinateAttributeFormat)), + (Trade::MeshData{MeshPrimitive::Points, 0})); /* Copy original attributes to a mutable array so we can update the position attribute format, if needed. Not using Utility::copy() here as @@ -256,7 +287,7 @@ Trade::MeshData transformTextureCoordinates2D(const Trade::MeshData& data, const /* If the position attribute isn't in a desired format, replace it with an empty placeholder that we'll unpack the data into */ - if(data.attributeFormat(*textureCoordinateAttributeId) != VertexFormat::Vector2) + if(textureCoordinateAttributeFormat != VertexFormat::Vector2) attributes[*textureCoordinateAttributeId] = Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates, VertexFormat::Vector2, nullptr}; /* Create the output mesh, making more room for the full formats if diff --git a/src/Magnum/MeshTools/Transform.h b/src/Magnum/MeshTools/Transform.h index 9a830ba33..3c0dc4a33 100644 --- a/src/Magnum/MeshTools/Transform.h +++ b/src/Magnum/MeshTools/Transform.h @@ -144,17 +144,19 @@ template U transformPoints(const T& transformation, U vectors) @m_since_latest Expects that the mesh contains a two-dimensional -@ref Trade::MeshAttribute::Position with index @p id. To avoid data loss with -packed types, the positions are always converted to @ref VertexFormat::Vector2. -Other attributes, position attributes other than @p id, and indices (if any) -are passed through untouched. +@ref Trade::MeshAttribute::Position with index @p id and that the attribute +does not have an implementation-specific format. To avoid data loss with packed +types, the positions are always converted to @ref VertexFormat::Vector2. Other +attributes, position attributes other than @p id, and indices (if any) are +passed through untouched. See also @ref transform2D(Trade::MeshData&&, const Matrix3&, UnsignedInt) for a potentially more efficient operation instead of always performing a full copy, you can also do an in-place transformation using @ref transform2DInPlace(). @see @ref transform3D(), @ref transformTextureCoordinates2D(), @ref Trade::MeshData::attributeCount(MeshAttribute) const, - @ref Trade::MeshData::attributeFormat(MeshAttribute, UnsignedInt) const + @ref Trade::MeshData::attributeFormat(MeshAttribute, UnsignedInt) const, + @ref isVertexFormatImplementationSpecific() */ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData transform2D(const Trade::MeshData& data, const Matrix3& transformation, UnsignedInt id = 0); @@ -196,18 +198,21 @@ Expects that the mesh contains a three-dimensional @ref Trade::MeshAttribute::Normal, @ref Trade::MeshAttribute::Tangent or @ref Trade::MeshAttribute::Bitangent with index @p id are present as well, those get transformed with @ref Matrix4::normalMatrix() extracted out of -@p transformation. To avoid data loss with packed types, the positions, normals -and bitangents are always converted to @ref VertexFormat::Vector3, tangents to -either @ref VertexFormat::Vector3 or @ref VertexFormat::Vector4. Other -attributes, additional position/TBN attributes other than @p id, and indices -(if any) are passed through untouched. +@p transformation. All these attributes are expected to not have an +implementation-specific format. To avoid data loss with packed types, the +positions, normals and bitangents are always converted to +@ref VertexFormat::Vector3, tangents to either @ref VertexFormat::Vector3 or +@ref VertexFormat::Vector4. Other attributes, additional +position/TBN attributes other than @p id, and indices (if any) are passed +through untouched. See also @ref transform3D(Trade::MeshData&&, const Matrix4&, UnsignedInt) for a potentially more efficient operation instead of always performing a full copy, you can also do an in-place transformation using @ref transform3DInPlace(). @see @ref transform2D(), @ref transformTextureCoordinates2D(), @ref Trade::MeshData::attributeCount(MeshAttribute) const, - @ref Trade::MeshData::attributeFormat(MeshAttribute, UnsignedInt) const + @ref Trade::MeshData::attributeFormat(MeshAttribute, UnsignedInt) const, + @ref isVertexFormatImplementationSpecific() */ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData transform3D(const Trade::MeshData& data, const Matrix4& transformation, UnsignedInt id = 0); @@ -261,7 +266,8 @@ for a potentially more efficient operation instead of always performing a full copy, you can also do an in-place transformation using @ref transformTextureCoordinates2DInPlace(). @see @ref transform2D(), @ref transform3D(), - @ref Trade::MeshData::attributeCount(MeshAttribute) const + @ref Trade::MeshData::attributeCount(MeshAttribute) const, + @ref isVertexFormatImplementationSpecific() */ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData transformTextureCoordinates2D(const Trade::MeshData& data, const Matrix3& transformation, UnsignedInt id = 0); @@ -282,9 +288,10 @@ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData transformTextureCoordinates2D(Trade::Mes @m_since_latest Expects that the mesh has mutable vertex data and contains a -@ref Trade::MeshAttribute::TextureCoordinates with index @p id. To avoid data -loss with packed types, the in-place operation requires the coordinate type to -be @ref VertexFormat::Vector2 --- if you can't guarantee that, use +@ref Trade::MeshAttribute::TextureCoordinates with index @p id and that the +attribute does not have an implementation-specific format. To avoid data loss +with packed types, the in-place operation requires the coordinate type to be +@ref VertexFormat::Vector2 --- if you can't guarantee that, use @ref transformTextureCoordinates2D() instead. Other attributes, texture coordinate attributes other than @p id, and indices (if any) are passed through untouched.