From b8c2f8b317a46ed523431f51065cc7a777be7cc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 22 Feb 2023 19:35:51 +0100 Subject: [PATCH] MeshTools: allow smaller-to-larger array copying in concatenate(). THe remaining elements are zero-filled, consistently with the behavior with zeroing out attributes that are missing in subsequent meshes. One use case is concatenating a mesh with 4 JointIds / Weights into a mesh that has 8 instead. Right now, copying larger arrays to smaller arrays is still disallowed to avoid data loss. That is inconsistent with behavior for attributes (which are silently left out when not present in the first mesh), I'll need to think about this more and solve the inconsistency somehow. Either by allowing array slicing or by failing for unhandled attributes as well, and the latter makes more sense from the perspective of avoiding data loss. --- doc/changelog.dox | 6 + src/Magnum/MeshTools/Concatenate.cpp | 21 +++- src/Magnum/MeshTools/Concatenate.h | 13 +- src/Magnum/MeshTools/Test/ConcatenateTest.cpp | 111 +++++++++++++----- 4 files changed, 114 insertions(+), 37 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index bc50ffa16..83b36c041 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -516,6 +516,12 @@ See also: @ref MeshTools::concatenate(const Containers::Iterable&, InterleaveFlags) optionally take a @ref MeshTools::InterleaveFlags parameter affecting the output, in particular whether to preserve the original interleaved layout. +- @ref MeshTools::concatenate() now allows concatenating an array attribute + into an array attribute with more elements. The remaining elements are + zero-filled, similarly to how missing attributes are handled. To avoid + accidents and data loss, concatenating a bigger array into a smaller one + isn't allowed. There's also a restriction that non-array attributes can't + be concatenated into arrays, even though they would fit. - Support for new @ref Trade::MeshAttribute::JointIds and @ref Trade::MeshAttribute::Weights in @ref MeshTools::compile() as well as a new @ref MeshTools::compiledPerVertexJointCount() helper utility (see diff --git a/src/Magnum/MeshTools/Concatenate.cpp b/src/Magnum/MeshTools/Concatenate.cpp index 36e913091..b965dc5a6 100644 --- a/src/Magnum/MeshTools/Concatenate.cpp +++ b/src/Magnum/MeshTools/Concatenate.cpp @@ -169,14 +169,25 @@ Trade::MeshData concatenate(Containers::Array&& indexData, const UnsignedI CORRADE_ASSERT(out.attributeFormat(dst) == mesh.attributeFormat(src), assertPrefix << "expected" << out.attributeFormat(dst) << "for attribute" << dst << "(" << Debug::nospace << out.attributeName(dst) << Debug::nospace << ") but got" << mesh.attributeFormat(src) << "in mesh" << i << "attribute" << src, (Trade::MeshData{MeshPrimitive{}, 0})); - CORRADE_ASSERT(out.attributeArraySize(dst) == mesh.attributeArraySize(src), - assertPrefix << "expected array size" << out.attributeArraySize(dst) << "for attribute" << dst << "(" << Debug::nospace << out.attributeName(dst) << Debug::nospace << ") but got" << mesh.attributeArraySize(src) << "in mesh" << i << "attribute" << src, + CORRADE_ASSERT(!out.attributeArraySize(dst) == !mesh.attributeArraySize(src), + assertPrefix << "attribute" << dst << "(" << Debug::nospace << out.attributeName(dst) << Debug::nospace << ")" << (out.attributeArraySize(dst) ? "is" : "isn't") << "an array but attribute" << src << "in mesh" << i << (mesh.attributeArraySize(src) ? "is" : "isn't"), (Trade::MeshData{MeshPrimitive{}, 0})); + CORRADE_ASSERT(out.attributeArraySize(dst) >= mesh.attributeArraySize(src), + assertPrefix << "expected array size" << out.attributeArraySize(dst) << "or less for attribute" << dst << "(" << Debug::nospace << out.attributeName(dst) << Debug::nospace << ") but got" << mesh.attributeArraySize(src) << "in mesh" << i << "attribute" << src, + (Trade::MeshData{MeshPrimitive{}, 0})); + + const Containers::StridedArrayView2D srcAttribute = mesh.attribute(src); + const Containers::StridedArrayView2D dstAttribute = out.mutableAttribute(dst); /* Copy the data to a slice of the output, mark the attribute as - copied */ - Utility::copy(mesh.attribute(src), out.mutableAttribute(dst) - .slice(vertexOffset, vertexOffset + mesh.vertexCount())); + copied. For non-array attributes the second dimension should be + matching (because the format is matching), for array attributes + we may be copying to just a prefix of the elements in + dstAttribute. */ + CORRADE_INTERNAL_ASSERT(out.attributeArraySize(dst) || srcAttribute.size()[1] == dstAttribute.size()[1]); + Utility::copy(srcAttribute, dstAttribute.sliceSize( + {vertexOffset, 0}, + {mesh.vertexCount(), srcAttribute.size()[1]})); found->second.second = true; } diff --git a/src/Magnum/MeshTools/Concatenate.h b/src/Magnum/MeshTools/Concatenate.h index dac8b01d8..84e131040 100644 --- a/src/Magnum/MeshTools/Concatenate.h +++ b/src/Magnum/MeshTools/Concatenate.h @@ -64,11 +64,14 @@ All attributes from the first mesh are taken, expected to not have an implementation-specific format. For each following mesh attributes present in the first are copied, superfluous attributes ignored and missing attributes zeroed out. Matching attributes are expected to have the same type, all meshes -are expected to have the same primitive. The vertex data are concatenated in -the same order as passed, with no duplicate removal. Returned instance vertex -and index data flags always have both @ref Trade::DataFlag::Owned and -@ref Trade::DataFlag::Mutable to guarante mutable access to particular parts of -the concatenated mesh --- for example for applying transformations. +are expected to have the same primitive. In case of array attributes, +attributes in subsequent meshes are expected to be arrays as well and have the +same or smaller array size. Unused components at the end are zeroed out. The +vertex data are concatenated in the same order as passed, with no duplicate +removal. Returned instance vertex and index data flags always have both +@ref Trade::DataFlag::Owned and @ref Trade::DataFlag::Mutable to guarante +mutable access to particular parts of the concatenated mesh --- for example for +applying transformations. The data layouting is done by @ref interleavedLayout() with the @p flags parameter propagated to it, see its documentation for detailed behavior diff --git a/src/Magnum/MeshTools/Test/ConcatenateTest.cpp b/src/Magnum/MeshTools/Test/ConcatenateTest.cpp index 16cbb0bb0..7253e435a 100644 --- a/src/Magnum/MeshTools/Test/ConcatenateTest.cpp +++ b/src/Magnum/MeshTools/Test/ConcatenateTest.cpp @@ -25,8 +25,10 @@ #include #include +#include /** @todo remove when Debug is stream-free */ #include #include +#include #include #include "Magnum/Math/Color.h" @@ -50,7 +52,8 @@ struct ConcatenateTest: TestSuite::Tester { void concatenateUnsupportedPrimitive(); void concatenateInconsistentPrimitive(); void concatenateInconsistentAttributeFormat(); - void concatenateInconsistentAttributeArraySize(); + void concatenateInconsistentArrayAttribute(); + void concatenateTooLargeAttributeArraySize(); void concatenateImplementationSpecificIndexType(); void concatenateImplementationSpecificVertexFormat(); void concatenateIntoNoMeshes(); @@ -81,7 +84,8 @@ ConcatenateTest::ConcatenateTest() { &ConcatenateTest::concatenateUnsupportedPrimitive, &ConcatenateTest::concatenateInconsistentPrimitive, &ConcatenateTest::concatenateInconsistentAttributeFormat, - &ConcatenateTest::concatenateInconsistentAttributeArraySize, + &ConcatenateTest::concatenateInconsistentArrayAttribute, + &ConcatenateTest::concatenateTooLargeAttributeArraySize, &ConcatenateTest::concatenateImplementationSpecificIndexType, &ConcatenateTest::concatenateImplementationSpecificVertexFormat, &ConcatenateTest::concatenateIntoNoMeshes}); @@ -94,7 +98,7 @@ struct VertexDataA { Vector2 texcoords2; Int:32; Vector3 position; - Short data[2]; + Short data[3]; }; void ConcatenateTest::concatenate() { @@ -106,8 +110,8 @@ void ConcatenateTest::concatenate() { /* First is non-indexed, this layout (including the gap) will be preserved */ const VertexDataA vertexDataA[]{ - {{0.1f, 0.2f}, {0.5f, 0.6f}, {1.0f, 2.0f, 3.0f}, {15, 3}}, - {{0.3f, 0.4f}, {0.7f, 0.8f}, {4.0f, 5.0f, 6.0f}, {14, 2}} + {{0.1f, 0.2f}, {0.5f, 0.6f}, {1.0f, 2.0f, 3.0f}, {15, 3, -1}}, + {{0.3f, 0.4f}, {0.7f, 0.8f}, {4.0f, 5.0f, 6.0f}, {14, 2, -4}} }; Containers::StridedArrayView1D verticesA = vertexDataA; Trade::MeshData a{MeshPrimitive::Points, {}, vertexDataA, { @@ -119,7 +123,7 @@ void ConcatenateTest::concatenate() { verticesA.slice(&VertexDataA::position)}, /* Array attribute to verify it's correctly propagated */ Trade::MeshAttributeData{Trade::meshAttributeCustom(42), - VertexFormat::Short, verticesA.slice(&VertexDataA::data), 2} + VertexFormat::Short, verticesA.slice(&VertexDataA::data), 3} }}; /* Second is indexed, has only one texture coordinate of the two, an extra @@ -127,13 +131,13 @@ void ConcatenateTest::concatenate() { zero-filled) */ const struct VertexDataB { Color4 color; - Short data[2]; + Short data[3]; Vector2 texcoords1; } vertexDataB[]{ - {0x112233_rgbf, {28, -15}, {0.15f, 0.25f}}, - {0x445566_rgbf, {29, -16}, {0.35f, 0.45f}}, - {0x778899_rgbf, {30, -17}, {0.55f, 0.65f}}, - {0xaabbcc_rgbf, {40, -18}, {0.75f, 0.85f}} + {0x112233_rgbf, {28, -15, 0}, {0.15f, 0.25f}}, + {0x445566_rgbf, {29, -16, 1}, {0.35f, 0.45f}}, + {0x778899_rgbf, {30, -17, 2}, {0.55f, 0.65f}}, + {0xaabbcc_rgbf, {40, -18, 3}, {0.75f, 0.85f}} }; Containers::StridedArrayView1D verticesB = vertexDataB; const UnsignedShort indicesB[]{0, 2, 1, 0, 3, 2}; @@ -143,7 +147,7 @@ void ConcatenateTest::concatenate() { verticesB.slice(&VertexDataB::color)}, /* Array attribute to verify it's correctly propagated */ Trade::MeshAttributeData{Trade::meshAttributeCustom(42), - VertexFormat::Short, verticesB.slice(&VertexDataB::data), 2}, + VertexFormat::Short, verticesB.slice(&VertexDataB::data), 3}, Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates, verticesB.slice(&VertexDataB::texcoords1)} }}; @@ -157,10 +161,11 @@ void ConcatenateTest::concatenate() { Vector3 position; Vector2 texcoords3; Vector2 texcoords1; + Short data[1]; } vertexDataC[]{ - {{0.425f, 0.475f}, {1.5f, 2.5f, 3.5f}, {0.725f, 0.775f}, {0.125f, 0.175f}}, - {{0.525f, 0.575f}, {4.5f, 5.5f, 6.5f}, {0.825f, 0.875f}, {0.225f, 0.275f}}, - {{0.625f, 0.675f}, {7.5f, 8.5f, 9.5f}, {0.925f, 0.975f}, {0.325f, 0.375f}}, + {{0.425f, 0.475f}, {1.5f, 2.5f, 3.5f}, {0.725f, 0.775f}, {0.125f, 0.175f}, {320}}, + {{0.525f, 0.575f}, {4.5f, 5.5f, 6.5f}, {0.825f, 0.875f}, {0.225f, 0.275f}, {3200}}, + {{0.625f, 0.675f}, {7.5f, 8.5f, 9.5f}, {0.925f, 0.975f}, {0.325f, 0.375f}, {32000}}, }; Containers::StridedArrayView1D verticesC = vertexDataC; Trade::MeshData c{MeshPrimitive::Points, {}, vertexDataC, { @@ -172,6 +177,9 @@ void ConcatenateTest::concatenate() { verticesC.slice(&VertexDataC::texcoords2)}, Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates, verticesC.slice(&VertexDataC::texcoords3)}, + /* Array attribute with less elements. The rest will be zero-filled. */ + Trade::MeshAttributeData{Trade::meshAttributeCustom(42), + VertexFormat::Short, verticesC.slice(&VertexDataC::data), 1}, }}; /* To catch when the default argument becomes different */ @@ -213,12 +221,13 @@ void ConcatenateTest::concatenate() { }), TestSuite::Compare::Container); CORRADE_COMPARE(dst.attributeName(3), Trade::meshAttributeCustom(42)); CORRADE_COMPARE(dst.attributeFormat(3), VertexFormat::Short); - CORRADE_COMPARE(dst.attributeArraySize(3), 2); - CORRADE_COMPARE_AS((Containers::arrayCast<1, const Vector2s>(dst.attribute(3))), - Containers::arrayView({ - {15, 3}, {14, 2}, - {28, -15}, {29, -16}, {30, -17}, {40, -18}, - {}, {}, {}, /* Missing in the third mesh */ + CORRADE_COMPARE(dst.attributeArraySize(3), 3); + CORRADE_COMPARE_AS((Containers::arrayCast<1, const Vector3s>(dst.attribute(3))), + Containers::arrayView({ + {15, 3, -1}, {14, 2, -4}, + {28, -15, 0}, {29, -16, 1}, {30, -17, 2}, {40, -18, 3}, + /* Last two components missing in the third mesh, kept at zeros */ + {320, 0, 0}, {3200, 0, 0}, {32000, 0, 0}, }), TestSuite::Compare::Container); CORRADE_VERIFY(dst.isIndexed()); CORRADE_COMPARE(dst.indexType(), MeshIndexType::UnsignedInt); @@ -239,7 +248,7 @@ void ConcatenateTest::concatenate() { CORRADE_COMPARE(dst.attributeOffset(3), 2*sizeof(Vector2) + 4 + sizeof(Vector3)); } else { /* Everything gets tightly packed */ - CORRADE_COMPARE(dst.attributeStride(0), 2*sizeof(Vector2) + sizeof(Vector3) + 2*sizeof(Short)); + CORRADE_COMPARE(dst.attributeStride(0), 2*sizeof(Vector2) + sizeof(Vector3) + 3*sizeof(Short)); CORRADE_COMPARE(dst.attributeOffset(0), 0); CORRADE_COMPARE(dst.attributeOffset(1), sizeof(Vector2)); CORRADE_COMPARE(dst.attributeOffset(2), 2*sizeof(Vector2)); @@ -622,7 +631,7 @@ void ConcatenateTest::concatenateInconsistentAttributeFormat() { "MeshTools::concatenateInto(): expected VertexFormat::Vector3ubNormalized for attribute 2 (Trade::MeshAttribute::Color) but got VertexFormat::Vector3usNormalized in mesh 3 attribute 1\n"); } -void ConcatenateTest::concatenateInconsistentAttributeArraySize() { +void ConcatenateTest::concatenateInconsistentArrayAttribute() { CORRADE_SKIP_IF_NO_ASSERT(); /* Things are a bit duplicated to test correct numbering */ @@ -632,22 +641,70 @@ void ConcatenateTest::concatenateInconsistentAttributeArraySize() { Trade::MeshAttributeData{Trade::MeshAttribute::Position, VertexFormat::Vector3, nullptr}, Trade::MeshAttributeData{Trade::meshAttributeCustom(42), - VertexFormat::ByteNormalized, nullptr, 5} + VertexFormat::ByteNormalized, nullptr, 4} + }}; + /* Copy of `a` so we can (destructively) concatenateInto() it and then use + `a` again in another assert */ + Trade::MeshData a2{MeshPrimitive::Lines, nullptr, { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, + VertexFormat::Vector3, nullptr}, + Trade::MeshAttributeData{Trade::MeshAttribute::Position, + VertexFormat::Vector3, nullptr}, + Trade::MeshAttributeData{Trade::meshAttributeCustom(42), + VertexFormat::ByteNormalized, nullptr, 4} }}; Trade::MeshData b{MeshPrimitive::Lines, nullptr, { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, + VertexFormat::Vector3, nullptr}, + Trade::MeshAttributeData{Trade::meshAttributeCustom(42), + VertexFormat::ByteNormalized, nullptr} + }}; + + std::ostringstream out; + Error redirectError{&out}; + MeshTools::concatenate({a, a, a, a, b}); + MeshTools::concatenate({b, b, b, b, a}); + MeshTools::concatenateInto(a2, {a, a, a, b}); + MeshTools::concatenateInto(b, {b, b, b, a}); + CORRADE_COMPARE_AS(out.str(), + "MeshTools::concatenate(): attribute 2 (Trade::MeshAttribute::Custom(42)) is an array but attribute 1 in mesh 4 isn't\n" + "MeshTools::concatenate(): attribute 1 (Trade::MeshAttribute::Custom(42)) isn't an array but attribute 2 in mesh 4 is\n" + "MeshTools::concatenateInto(): attribute 2 (Trade::MeshAttribute::Custom(42)) is an array but attribute 1 in mesh 3 isn't\n" + "MeshTools::concatenateInto(): attribute 1 (Trade::MeshAttribute::Custom(42)) isn't an array but attribute 2 in mesh 3 is\n", + TestSuite::Compare::String); +} + +void ConcatenateTest::concatenateTooLargeAttributeArraySize() { + CORRADE_SKIP_IF_NO_ASSERT(); + + /* Things are a bit duplicated to test correct numbering */ + Trade::MeshData a{MeshPrimitive::Lines, nullptr, { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, + VertexFormat::Vector3, nullptr}, Trade::MeshAttributeData{Trade::MeshAttribute::Position, VertexFormat::Vector3, nullptr}, Trade::MeshAttributeData{Trade::meshAttributeCustom(42), VertexFormat::ByteNormalized, nullptr, 4} }}; + Trade::MeshData b{MeshPrimitive::Lines, nullptr, { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, + VertexFormat::Vector3, nullptr}, + Trade::MeshAttributeData{Trade::meshAttributeCustom(42), + VertexFormat::ByteNormalized, nullptr, 5} + }}; + + /* Using a lower array size in subsequent meshes is fine (tested in + concatenate() above) */ + MeshTools::concatenate({b, a}); std::ostringstream out; Error redirectError{&out}; MeshTools::concatenate({a, a, a, a, b}); MeshTools::concatenateInto(a, {a, a, a, b}); - CORRADE_COMPARE(out.str(), - "MeshTools::concatenate(): expected array size 5 for attribute 2 (Trade::MeshAttribute::Custom(42)) but got 4 in mesh 4 attribute 1\n" - "MeshTools::concatenateInto(): expected array size 5 for attribute 2 (Trade::MeshAttribute::Custom(42)) but got 4 in mesh 3 attribute 1\n"); + CORRADE_COMPARE_AS(out.str(), + "MeshTools::concatenate(): expected array size 4 or less for attribute 2 (Trade::MeshAttribute::Custom(42)) but got 5 in mesh 4 attribute 1\n" + "MeshTools::concatenateInto(): expected array size 4 or less for attribute 2 (Trade::MeshAttribute::Custom(42)) but got 5 in mesh 3 attribute 1\n", + TestSuite::Compare::String); } void ConcatenateTest::concatenateImplementationSpecificIndexType() {