From ccf9580c330febc786821aba4bc7397cf64a539f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 12 Nov 2024 22:00:02 +0100 Subject: [PATCH] {Mesh,Scene}Tools: add r-value filter{Attributes,Fields}() overloads. When writing high-level documentation for those I realized that it's easier to just add the r-value variants than trying to explain why one has to be careful to not pass r-values there. --- src/Magnum/MeshTools/Filter.cpp | 61 ++++++-- src/Magnum/MeshTools/Filter.h | 54 +++++++ src/Magnum/MeshTools/Test/FilterTest.cpp | 178 ++++++++++++++++++++- src/Magnum/SceneTools/Filter.cpp | 39 ++++- src/Magnum/SceneTools/Filter.h | 48 ++++++ src/Magnum/SceneTools/Test/FilterTest.cpp | 180 +++++++++++++++++++++- 6 files changed, 534 insertions(+), 26 deletions(-) diff --git a/src/Magnum/MeshTools/Filter.cpp b/src/Magnum/MeshTools/Filter.cpp index c9f01754a..8a87e0976 100644 --- a/src/Magnum/MeshTools/Filter.cpp +++ b/src/Magnum/MeshTools/Filter.cpp @@ -30,6 +30,7 @@ #include #include +#include "Magnum/MeshTools/Copy.h" #include "Magnum/Trade/MeshData.h" #ifdef MAGNUM_BUILD_DEPRECATED @@ -39,7 +40,7 @@ namespace Magnum { namespace MeshTools { -Trade::MeshData filterAttributes(const Trade::MeshData& mesh, const Containers::BitArrayView attributesToKeep) { +Trade::MeshData filterAttributes(Trade::MeshData&& mesh, const Containers::BitArrayView attributesToKeep) { CORRADE_ASSERT(attributesToKeep.size() == mesh.attributeCount(), "MeshTools::filterAttributes(): expected" << mesh.attributeCount() << "bits but got" << attributesToKeep.size(), (Trade::MeshData{MeshPrimitive::Triangles, 0})); @@ -61,13 +62,39 @@ Trade::MeshData filterAttributes(const Trade::MeshData& mesh, const Containers:: mesh.indexCount(), mesh.indexStride()}}; - return Trade::MeshData{mesh.primitive(), - {}, mesh.indexData(), indices, - {}, mesh.vertexData(), Utility::move(filtered), - mesh.vertexCount()}; + /* If either the index or vertex buffer is owned, transfer the ownership to + the returned instance, otherwise reference the original. Save vertex + count first to use for the returned instance because releaseVertexData() + would clear it. */ + const UnsignedInt vertexCount = mesh.vertexCount(); + if(mesh.indexDataFlags() >= Trade::DataFlag::Owned && + mesh.vertexDataFlags() >= Trade::DataFlag::Owned) + return Trade::MeshData{mesh.primitive(), + mesh.releaseIndexData(), indices, + mesh.releaseVertexData(), Utility::move(filtered), + vertexCount}; + else if(mesh.indexDataFlags() >= Trade::DataFlag::Owned) + return Trade::MeshData{mesh.primitive(), + mesh.releaseIndexData(), indices, + {}, mesh.vertexData(), Utility::move(filtered), + vertexCount}; + else if(mesh.vertexDataFlags() >= Trade::DataFlag::Owned) + return Trade::MeshData{mesh.primitive(), + {}, mesh.indexData(), indices, + mesh.releaseVertexData(), Utility::move(filtered), + vertexCount}; + else + return Trade::MeshData{mesh.primitive(), + {}, mesh.indexData(), indices, + {}, mesh.vertexData(), Utility::move(filtered), + mesh.vertexCount()}; } -Trade::MeshData filterOnlyAttributes(const Trade::MeshData& mesh, const Containers::ArrayView attributes) { +Trade::MeshData filterAttributes(const Trade::MeshData& mesh, const Containers::BitArrayView attributesToKeep) { + return filterAttributes(reference(mesh), attributesToKeep); +} + +Trade::MeshData filterOnlyAttributes(Trade::MeshData&& mesh, const Containers::ArrayView attributes) { Containers::BitArray attributesToKeep{DirectInit, mesh.attributeCount(), false}; for(const Trade::MeshAttribute attribute: attributes) { /* Can't use findAttributeId() because all instances of the attribute @@ -77,13 +104,21 @@ Trade::MeshData filterOnlyAttributes(const Trade::MeshData& mesh, const Containe attributesToKeep.set(i); } } - return filterAttributes(mesh, attributesToKeep); + return filterAttributes(Utility::move(mesh), attributesToKeep); +} + +Trade::MeshData filterOnlyAttributes(const Trade::MeshData& mesh, const Containers::ArrayView attributes) { + return filterOnlyAttributes(reference(mesh), attributes); } Trade::MeshData filterOnlyAttributes(const Trade::MeshData& mesh, std::initializer_list attributes) { return filterOnlyAttributes(mesh, Containers::arrayView(attributes)); } +Trade::MeshData filterOnlyAttributes(Trade::MeshData&& mesh, std::initializer_list attributes) { + return filterOnlyAttributes(Utility::move(mesh), Containers::arrayView(attributes)); +} + #ifdef MAGNUM_BUILD_DEPRECATED Trade::MeshData filterOnlyAttributes(const Trade::MeshData& mesh, const Containers::ArrayView attributes) { Containers::BitArray attributesToKeep{DirectInit, mesh.attributeCount(), false}; @@ -104,7 +139,7 @@ Trade::MeshData filterOnlyAttributes(const Trade::MeshData& mesh, std::initializ } #endif -Trade::MeshData filterExceptAttributes(const Trade::MeshData& mesh, const Containers::ArrayView attributes) { +Trade::MeshData filterExceptAttributes(Trade::MeshData&& mesh, const Containers::ArrayView attributes) { Containers::BitArray attributesToKeep{DirectInit, mesh.attributeCount(), true}; for(const Trade::MeshAttribute attribute: attributes) { /* Can't use findAttributeId() because all instances of the attribute @@ -114,13 +149,21 @@ Trade::MeshData filterExceptAttributes(const Trade::MeshData& mesh, const Contai attributesToKeep.reset(i); } } - return filterAttributes(mesh, attributesToKeep); + return filterAttributes(Utility::move(mesh), attributesToKeep); +} + +Trade::MeshData filterExceptAttributes(const Trade::MeshData& mesh, const Containers::ArrayView attributes) { + return filterExceptAttributes(reference(mesh), attributes); } Trade::MeshData filterExceptAttributes(const Trade::MeshData& mesh, std::initializer_list attributes) { return filterExceptAttributes(mesh, Containers::arrayView(attributes)); } +Trade::MeshData filterExceptAttributes(Trade::MeshData&& mesh, std::initializer_list attributes) { + return filterExceptAttributes(Utility::move(mesh), Containers::arrayView(attributes)); +} + #ifdef MAGNUM_BUILD_DEPRECATED Trade::MeshData filterExceptAttributes(const Trade::MeshData& mesh, const Containers::ArrayView attributes) { Containers::BitArray attributesToKeep{DirectInit, mesh.attributeCount(), true}; diff --git a/src/Magnum/MeshTools/Filter.h b/src/Magnum/MeshTools/Filter.h index a34c893cb..2d0a962a7 100644 --- a/src/Magnum/MeshTools/Filter.h +++ b/src/Magnum/MeshTools/Filter.h @@ -56,6 +56,20 @@ without @ref InterleaveFlag::PreserveInterleavedAttributes set. */ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData filterAttributes(const Trade::MeshData& mesh, Containers::BitArrayView attributesToKeep); +/** +@brief Filter a mesh to contain only the selected subset of attributes +@m_since_latest + +Compared to @ref filterAttributes(const Trade::MeshData&, Containers::BitArrayView), +if the @p mesh index or vertex data is owned, the function transfers the data +ownership to the returned instance instead of returning a non-owning reference. +If neither the index nor the vertex data is owned, the two overloads behave the +same. +@see @ref Trade::MeshData::indexDataFlags(), + @ref Trade::MeshData::vertexDataFlags() +*/ +MAGNUM_MESHTOOLS_EXPORT Trade::MeshData filterAttributes(Trade::MeshData&& mesh, Containers::BitArrayView attributesToKeep); + /** @brief Filter a mesh to contain only the selected subset of named attributes @m_since_latest @@ -84,6 +98,26 @@ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData filterOnlyAttributes(const Trade::MeshDa */ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData filterOnlyAttributes(const Trade::MeshData& mesh, std::initializer_list attributes); +/** +@brief Filter a mesh to contain only the selected subset of named attributes +@m_since_latest + +Compared to @ref filterOnlyAttributes(const Trade::MeshData&, Containers::ArrayView), +if the @p mesh index or vertex data is owned, the function transfers the data +ownership to the returned instance instead of returning a non-owning reference. +If neither the index nor the vertex data is owned, the two overloads behave the +same. +@see @ref Trade::MeshData::indexDataFlags(), + @ref Trade::MeshData::vertexDataFlags() +*/ +MAGNUM_MESHTOOLS_EXPORT Trade::MeshData filterOnlyAttributes(Trade::MeshData&& mesh, Containers::ArrayView attributes); + +/** + * @overload + * @m_since_latest + */ +MAGNUM_MESHTOOLS_EXPORT Trade::MeshData filterOnlyAttributes(Trade::MeshData&& mesh, std::initializer_list attributes); + /** @brief Filter a mesh to contain everything except the selected subset of named attributes @m_since_latest @@ -111,6 +145,26 @@ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData filterExceptAttributes(const Trade::Mesh */ MAGNUM_MESHTOOLS_EXPORT Trade::MeshData filterExceptAttributes(const Trade::MeshData& mesh, std::initializer_list attributes); +/** +@brief Filter a mesh to contain everything except the selected subset of named attributes +@m_since_latest + +Compared to @ref filterExceptAttributes(const Trade::MeshData&, Containers::ArrayView), +if the @p mesh index or vertex data is owned, the function transfers the data +ownership to the returned instance instead of returning a non-owning reference. +If neither the index nor the vertex data is owned, the two overloads behave the +same. +@see @ref Trade::MeshData::indexDataFlags(), + @ref Trade::MeshData::vertexDataFlags() +*/ +MAGNUM_MESHTOOLS_EXPORT Trade::MeshData filterExceptAttributes(Trade::MeshData&& mesh, Containers::ArrayView attributes); + +/** + * @overload + * @m_since_latest + */ +MAGNUM_MESHTOOLS_EXPORT Trade::MeshData filterExceptAttributes(Trade::MeshData&& mesh, std::initializer_list attributes); + }} #endif diff --git a/src/Magnum/MeshTools/Test/FilterTest.cpp b/src/Magnum/MeshTools/Test/FilterTest.cpp index c083d9c22..fa8e29d88 100644 --- a/src/Magnum/MeshTools/Test/FilterTest.cpp +++ b/src/Magnum/MeshTools/Test/FilterTest.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include @@ -46,11 +47,13 @@ struct FilterTest: TestSuite::Tester { void attributes(); void attributesNoIndexData(); + void attributesRvalue(); void attributesWrongBitCount(); void onlyAttributes(); void onlyAttributesNoIndexData(); void onlyAttributesNoAttributeData(); + void onlyAttributesRvalue(); #ifdef MAGNUM_BUILD_DEPRECATED void onlyAttributeIds(); @@ -62,6 +65,7 @@ struct FilterTest: TestSuite::Tester { void exceptAttributes(); void exceptAttributesNoIndexData(); void exceptAttributesNoAttributeData(); + void exceptAttributesRvalue(); #ifdef MAGNUM_BUILD_DEPRECATED void exceptAttributeIds(); @@ -79,18 +83,44 @@ const struct { {"implementation-specific index type", meshIndexTypeWrap(0xcaca)} }; +const struct { + const char* name; + Trade::DataFlags indexDataFlags, vertexDataFlags; + Trade::DataFlags expectedIndexDataFlags, expectedVertexDataFlags; +} AttributesRvalueData[]{ + /* The Global or ExternallyOwned flags are not preserved, because + reference() doesn't preserve them either */ + {"neither owned", + {}, Trade::DataFlag::Global, + {}, {}}, + {"index data owned", + Trade::DataFlag::Owned, {}, + Trade::DataFlag::Owned|Trade::DataFlag::Mutable, {}}, + {"vertex data owned", + Trade::DataFlag::ExternallyOwned, Trade::DataFlag::Owned|Trade::DataFlag::Mutable, + {}, Trade::DataFlag::Owned|Trade::DataFlag::Mutable}, + {"both owned", + Trade::DataFlag::Owned, Trade::DataFlag::Owned, + Trade::DataFlag::Owned|Trade::DataFlag::Mutable, Trade::DataFlag::Owned|Trade::DataFlag::Mutable}, +}; + FilterTest::FilterTest() { addInstancedTests({&FilterTest::attributes}, Containers::arraySize(ImplementationSpecificIndexTypeData)); - addTests({&FilterTest::attributesNoIndexData, - &FilterTest::attributesWrongBitCount}); + addTests({&FilterTest::attributesNoIndexData}); + + addInstancedTests({&FilterTest::attributesRvalue}, + Containers::arraySize(AttributesRvalueData)); + + addTests({&FilterTest::attributesWrongBitCount}); addInstancedTests({&FilterTest::onlyAttributes}, Containers::arraySize(ImplementationSpecificIndexTypeData)); addTests({&FilterTest::onlyAttributesNoIndexData, - &FilterTest::onlyAttributesNoAttributeData}); + &FilterTest::onlyAttributesNoAttributeData, + &FilterTest::onlyAttributesRvalue}); #ifdef MAGNUM_BUILD_DEPRECATED addInstancedTests({&FilterTest::onlyAttributeIds}, @@ -105,7 +135,8 @@ FilterTest::FilterTest() { Containers::arraySize(ImplementationSpecificIndexTypeData)); addTests({&FilterTest::exceptAttributesNoIndexData, - &FilterTest::exceptAttributesNoAttributeData}); + &FilterTest::exceptAttributesNoAttributeData, + &FilterTest::exceptAttributesRvalue}); #ifdef MAGNUM_BUILD_DEPRECATED addInstancedTests({&FilterTest::exceptAttributeIds}, @@ -202,6 +233,66 @@ void FilterTest::attributesNoIndexData() { CORRADE_COMPARE(filtered.attributeOffset(0), offsetof(Vertex, textureCoordinates1)); } +void FilterTest::attributesRvalue() { + auto&& data = AttributesRvalueData[testCaseInstanceId()]; + setTestCaseDescription(data.name); + + /* Subset of attributes() verifying data ownership transfer behavior */ + + Containers::Array indexData{5*sizeof(UnsignedShort)}; + Containers::StridedArrayView1D indices = Containers::arrayCast(indexData); + Containers::Array vertexData{3*sizeof(Vertex)}; + Containers::StridedArrayView1D vertices = Containers::arrayCast(vertexData); + + Containers::Array attributes{InPlaceInit, { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, vertices.slice(&Vertex::position)}, + Trade::MeshAttributeData{Trade::MeshAttribute::Tangent, vertices.slice(&Vertex::tangent)}, + Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates, vertices.slice(&Vertex::textureCoordinates1)}, + Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates, vertices.slice(&Vertex::textureCoordinates2)}, + }}; + + Containers::Optional mesh; + if(data.indexDataFlags >= Trade::DataFlag::Owned && + data.vertexDataFlags >= Trade::DataFlag::Owned) + mesh = Trade::MeshData{MeshPrimitive::Triangles, + Utility::move(indexData), Trade::MeshIndexData{indices}, + Utility::move(vertexData), Utility::move(attributes)}; + else if(data.indexDataFlags >= Trade::DataFlag::Owned) + mesh = Trade::MeshData{MeshPrimitive::Triangles, + Utility::move(indexData), Trade::MeshIndexData{indices}, + data.vertexDataFlags, vertexData, Utility::move(attributes)}; + else if(data.vertexDataFlags >= Trade::DataFlag::Owned) + mesh = Trade::MeshData{MeshPrimitive::Triangles, + data.indexDataFlags, indexData, Trade::MeshIndexData{indices}, + Utility::move(vertexData), Utility::move(attributes)}; + else + mesh = Trade::MeshData{MeshPrimitive::Triangles, + data.indexDataFlags, indexData, Trade::MeshIndexData{indices}, + data.vertexDataFlags, vertexData, Utility::move(attributes)}; + + Containers::BitArray attributesToKeep{ValueInit, mesh->attributeCount()}; + attributesToKeep.set(1); + attributesToKeep.set(3); + + Trade::MeshData filtered = filterAttributes(Utility::move(*mesh), attributesToKeep); + + /* The data ownership should be transferred if possible */ + CORRADE_VERIFY(filtered.isIndexed()); + CORRADE_COMPARE(filtered.indexCount(), 5); + CORRADE_COMPARE(filtered.indexData().data(), indices.data()); + CORRADE_COMPARE(filtered.indexDataFlags(), data.expectedIndexDataFlags); + + CORRADE_COMPARE(filtered.vertexCount(), 3); + CORRADE_COMPARE(filtered.vertexData().data(), vertices.data()); + CORRADE_COMPARE(filtered.vertexDataFlags(), data.expectedVertexDataFlags); + + /* Just checking that the attributes get actually filtered instead of being + passed through verbatim, the actual verification is done in attributes() + above */ + CORRADE_COMPARE(filtered.attributeCount(), 2); + CORRADE_COMPARE(filtered.attributeName(0), Trade::MeshAttribute::Tangent); +} + void FilterTest::attributesWrongBitCount() { CORRADE_SKIP_IF_NO_ASSERT(); @@ -332,6 +423,45 @@ void FilterTest::onlyAttributesNoAttributeData() { CORRADE_COMPARE(filtered.attributeCount(), 0); } +void FilterTest::onlyAttributesRvalue() { + /* Subset of onlyAttributes() verifying data ownership transfer behavior. + All cases of ownership transfer are verified in attributesRvalue(), this + only checks that the r-value gets correctly passed through all overloads + to keep the index data owned and vertex data not. */ + + Containers::Array indexData{5*sizeof(UnsignedShort)}; + Containers::StridedArrayView1D indices = Containers::arrayCast(indexData); + Vertex vertexData[3]; + Containers::StridedArrayView1D vertices = vertexData; + + Trade::MeshData mesh{MeshPrimitive::TriangleStrip, + Utility::move(indexData), Trade::MeshIndexData{indices}, + {}, vertexData, { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, vertices.slice(&Vertex::position)}, + Trade::MeshAttributeData{Trade::MeshAttribute::Tangent, vertices.slice(&Vertex::tangent)}, + Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates, vertices.slice(&Vertex::textureCoordinates1)}, + Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates, vertices.slice(&Vertex::textureCoordinates2)}, + }}; + + Trade::MeshData filtered = filterOnlyAttributes(Utility::move(mesh), { + Trade::MeshAttribute::TextureCoordinates, + Trade::MeshAttribute::Position + }); + CORRADE_COMPARE(filtered.primitive(), MeshPrimitive::TriangleStrip); + + CORRADE_VERIFY(filtered.isIndexed()); + CORRADE_COMPARE(filtered.indexCount(), 5); + CORRADE_COMPARE(filtered.indexData().data(), indices.data()); + CORRADE_COMPARE(filtered.indexDataFlags(), Trade::DataFlag::Owned|Trade::DataFlag::Mutable); + + CORRADE_COMPARE(filtered.vertexCount(), 3); + CORRADE_COMPARE(filtered.vertexData().data(), vertices.data()); + CORRADE_COMPARE(filtered.vertexDataFlags(), Trade::DataFlags{}); + + CORRADE_COMPARE(filtered.attributeCount(), 3); + CORRADE_COMPARE(filtered.attributeName(0), Trade::MeshAttribute::Position); +} + #ifdef MAGNUM_BUILD_DEPRECATED void FilterTest::onlyAttributeIds() { auto&& data = ImplementationSpecificIndexTypeData[testCaseInstanceId()]; @@ -576,6 +706,46 @@ void FilterTest::exceptAttributesNoAttributeData() { CORRADE_COMPARE(filtered.attributeCount(), 0); } +void FilterTest::exceptAttributesRvalue() { + /* Subset of onlyAttributes() verifying data ownership transfer behavior. + All cases of ownership transfer are verified in attributesRvalue(), this + only checks that the r-value gets correctly passed through all overloads + to keep the vertex data owned and index data not. */ + + UnsignedShort indices[5]; + Containers::Array vertexData{3*sizeof(Vertex)}; + Containers::StridedArrayView1D vertices = Containers::arrayCast(vertexData); + + Trade::MeshData mesh{MeshPrimitive::TriangleStrip, + {}, indices, Trade::MeshIndexData{indices}, + Utility::move(vertexData), { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, vertices.slice(&Vertex::position)}, + Trade::MeshAttributeData{Trade::MeshAttribute::Tangent, vertices.slice(&Vertex::tangent)}, + Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates, vertices.slice(&Vertex::textureCoordinates1)}, + Trade::MeshAttributeData{Trade::MeshAttribute::TextureCoordinates, vertices.slice(&Vertex::textureCoordinates2)}, + /* Positions again, just under a different name. Should be kept. */ + Trade::MeshAttributeData{Trade::meshAttributeCustom(0xbaf), vertices.slice(&Vertex::position)}, + }}; + + Trade::MeshData filtered = filterExceptAttributes(Utility::move(mesh), { + Trade::MeshAttribute::Position, + Trade::MeshAttribute::TextureCoordinates + }); + CORRADE_COMPARE(filtered.primitive(), MeshPrimitive::TriangleStrip); + + CORRADE_VERIFY(filtered.isIndexed()); + CORRADE_COMPARE(filtered.indexCount(), 5); + CORRADE_COMPARE(filtered.indexData().data(), static_cast(indices)); + CORRADE_COMPARE(filtered.indexDataFlags(), Trade::DataFlags{}); + + CORRADE_COMPARE(filtered.vertexCount(), 3); + CORRADE_COMPARE(filtered.vertexData().data(), vertices.data()); + CORRADE_COMPARE(filtered.vertexDataFlags(), Trade::DataFlag::Owned|Trade::DataFlag::Mutable); + + CORRADE_COMPARE(filtered.attributeCount(), 2); + CORRADE_COMPARE(filtered.attributeName(0), Trade::MeshAttribute::Tangent); +} + #ifdef MAGNUM_BUILD_DEPRECATED void FilterTest::exceptAttributeIds() { auto&& data = ImplementationSpecificIndexTypeData[testCaseInstanceId()]; diff --git a/src/Magnum/SceneTools/Filter.cpp b/src/Magnum/SceneTools/Filter.cpp index 2048d7cf0..cb606ba6e 100644 --- a/src/Magnum/SceneTools/Filter.cpp +++ b/src/Magnum/SceneTools/Filter.cpp @@ -35,11 +35,12 @@ #include #include "Magnum/SceneTools/Combine.h" +#include "Magnum/SceneTools/Copy.h" #include "Magnum/Trade/SceneData.h" namespace Magnum { namespace SceneTools { -Trade::SceneData filterFields(const Trade::SceneData& scene, const Containers::BitArrayView fieldsToKeep) { +Trade::SceneData filterFields(Trade::SceneData&& scene, const Containers::BitArrayView fieldsToKeep) { CORRADE_ASSERT(fieldsToKeep.size() == scene.fieldCount(), "SceneTools::filterFields(): expected" << scene.fieldCount() << "bits but got" << fieldsToKeep.size(), (Trade::SceneData{Trade::SceneMappingType::UnsignedInt, 0, nullptr, {}})); @@ -48,36 +49,60 @@ Trade::SceneData filterFields(const Trade::SceneData& scene, const Containers::B Containers::Array filtered{ValueInit, fieldsToKeep.count()}; Utility::copyMasked(scene.fieldData(), fieldsToKeep, filtered); - return Trade::SceneData{scene.mappingType(), scene.mappingBound(), - {}, scene.data(), Utility::move(filtered)}; + if(scene.dataFlags() >= Trade::DataFlag::Owned) + return Trade::SceneData{scene.mappingType(), scene.mappingBound(), + scene.releaseData(), Utility::move(filtered)}; + else + return Trade::SceneData{scene.mappingType(), scene.mappingBound(), + {}, scene.data(), Utility::move(filtered)}; } -Trade::SceneData filterOnlyFields(const Trade::SceneData& scene, const Containers::ArrayView fields) { +Trade::SceneData filterFields(const Trade::SceneData& scene, const Containers::BitArrayView fieldsToKeep) { + return filterFields(reference(scene), fieldsToKeep); +} + +Trade::SceneData filterOnlyFields(Trade::SceneData&& scene, const Containers::ArrayView fields) { Containers::BitArray fieldsToKeep{DirectInit, scene.fieldCount(), false}; for(const Trade::SceneField field: fields) { if(const Containers::Optional fieldId = scene.findFieldId(field)) fieldsToKeep.set(*fieldId); } - return filterFields(scene, fieldsToKeep); + return filterFields(Utility::move(scene), fieldsToKeep); +} + +Trade::SceneData filterOnlyFields(const Trade::SceneData& scene, const Containers::ArrayView fields) { + return filterOnlyFields(reference(scene), fields); } Trade::SceneData filterOnlyFields(const Trade::SceneData& scene, std::initializer_list fields) { return filterOnlyFields(scene, Containers::arrayView(fields)); } -Trade::SceneData filterExceptFields(const Trade::SceneData& scene, const Containers::ArrayView fields) { +Trade::SceneData filterOnlyFields(Trade::SceneData&& scene, std::initializer_list fields) { + return filterOnlyFields(Utility::move(scene), Containers::arrayView(fields)); +} + +Trade::SceneData filterExceptFields(Trade::SceneData&& scene, const Containers::ArrayView fields) { Containers::BitArray fieldsToKeep{DirectInit, scene.fieldCount(), true}; for(const Trade::SceneField field: fields) { if(const Containers::Optional fieldId = scene.findFieldId(field)) fieldsToKeep.reset(*fieldId); } - return filterFields(scene, fieldsToKeep); + return filterFields(Utility::move(scene), fieldsToKeep); +} + +Trade::SceneData filterExceptFields(const Trade::SceneData& scene, const Containers::ArrayView fields) { + return filterExceptFields(reference(scene), fields); } Trade::SceneData filterExceptFields(const Trade::SceneData& scene, std::initializer_list fields) { return filterExceptFields(scene, Containers::arrayView(fields)); } +Trade::SceneData filterExceptFields(Trade::SceneData&& scene, std::initializer_list fields) { + return filterExceptFields(Utility::move(scene), Containers::arrayView(fields)); +} + Trade::SceneData filterFieldEntries(const Trade::SceneData& scene, const Containers::ArrayView> entriesToKeep) { /* Track unique mapping views (pointer, size, stride) so fields that shared a mapping before stay shared after as well -- if they're filtered, they diff --git a/src/Magnum/SceneTools/Filter.h b/src/Magnum/SceneTools/Filter.h index 73196995e..7c2ba23d7 100644 --- a/src/Magnum/SceneTools/Filter.h +++ b/src/Magnum/SceneTools/Filter.h @@ -52,6 +52,18 @@ the output to @ref combineFields(const Trade::SceneData&). */ MAGNUM_SCENETOOLS_EXPORT Trade::SceneData filterFields(const Trade::SceneData& scene, Containers::BitArrayView fieldsToKeep); +/** +@brief Filter a scene to contain only the selected subset of fields +@m_since_latest + +Compared to @ref filterFields(const Trade::SceneData&, Containers::BitArrayView), +if the @p scene data is owned, this function transfers the data ownership to +the returned instance instead of returning a non-owning reference. If the data +is not owned, the two overloads behave the same. +@see @ref Trade::SceneData::dataFlags() +*/ +MAGNUM_SCENETOOLS_EXPORT Trade::SceneData filterFields(Trade::SceneData&& scene, Containers::BitArrayView fieldsToKeep); + /** @brief Filter a scene to contain only the selected subset of named fields @m_since_latest @@ -73,6 +85,24 @@ MAGNUM_SCENETOOLS_EXPORT Trade::SceneData filterOnlyFields(const Trade::SceneDat */ MAGNUM_SCENETOOLS_EXPORT Trade::SceneData filterOnlyFields(const Trade::SceneData& scene, std::initializer_list fields); +/** +@brief Filter a scene to contain only the selected subset of named fields +@m_since_latest + +Compared to @ref filterOnlyFields(const Trade::SceneData&, Containers::ArrayView), +if the @p scene data is owned, this function transfers the data ownership to +the returned instance instead of returning a non-owning reference. If the data +is not owned, the two overloads behave the same. +@see @ref Trade::SceneData::dataFlags() +*/ +MAGNUM_SCENETOOLS_EXPORT Trade::SceneData filterOnlyFields(Trade::SceneData&& scene, Containers::ArrayView fields); + +/** +@overload +@m_since_latest +*/ +MAGNUM_SCENETOOLS_EXPORT Trade::SceneData filterOnlyFields(Trade::SceneData&& scene, std::initializer_list fields); + /** @brief Filter a scene to contain everything except the selected subset of named fields @m_since_latest @@ -94,6 +124,24 @@ MAGNUM_SCENETOOLS_EXPORT Trade::SceneData filterExceptFields(const Trade::SceneD */ MAGNUM_SCENETOOLS_EXPORT Trade::SceneData filterExceptFields(const Trade::SceneData& scene, std::initializer_list fields); +/** +@brief Filter a scene to contain everything except the selected subset of named fields +@m_since_latest + +Compared to @ref filterExceptFields(const Trade::SceneData&, Containers::ArrayView), +if the @p scene data is owned, this function transfers the data ownership to +the returned instance instead of returning a non-owning reference. If the data +is not owned, the two overloads behave the same. +@see @ref Trade::SceneData::dataFlags() +*/ +MAGNUM_SCENETOOLS_EXPORT Trade::SceneData filterExceptFields(Trade::SceneData&& scene, Containers::ArrayView fields); + +/** +@overload +@m_since_latest +*/ +MAGNUM_SCENETOOLS_EXPORT Trade::SceneData filterExceptFields(Trade::SceneData&& scene, std::initializer_list fields); + /** @brief Filter individual entries of fields in a scene @m_since_latest diff --git a/src/Magnum/SceneTools/Test/FilterTest.cpp b/src/Magnum/SceneTools/Test/FilterTest.cpp index cf2ae0c4b..d6feea996 100644 --- a/src/Magnum/SceneTools/Test/FilterTest.cpp +++ b/src/Magnum/SceneTools/Test/FilterTest.cpp @@ -26,6 +26,7 @@ #include #include +#include #include #include #include @@ -43,13 +44,16 @@ struct FilterTest: TestSuite::Tester { explicit FilterTest(); void fields(); + void fieldsRvalue(); void fieldsWrongBitCount(); void onlyFields(); void onlyFieldsNoFieldData(); + void onlyFieldsRvalue(); void exceptFields(); void exceptFieldsNoFieldData(); + void exceptFieldsRvalue(); void fieldEntries(); void fieldEntriesFieldNotFound(); @@ -70,6 +74,20 @@ struct FilterTest: TestSuite::Tester { using namespace Math::Literals; +const struct { + const char* name; + Trade::DataFlags dataFlags, expectedDataFlags; +} FieldsRvalueData[]{ + /* The Global or ExternallyOwned flags are not preserved, because + reference() doesn't preserve them either */ + {"not owned", + Trade::DataFlag::Global|Trade::DataFlag::ExternallyOwned, + {}}, + {"owned", + Trade::DataFlag::Owned, + Trade::DataFlag::Owned|Trade::DataFlag::Mutable} +}; + const struct { const char* name; bool byName; @@ -79,14 +97,20 @@ const struct { }; FilterTest::FilterTest() { - addTests({&FilterTest::fields, - &FilterTest::fieldsWrongBitCount, + addTests({&FilterTest::fields}); + + addInstancedTests({&FilterTest::fieldsRvalue}, + Containers::arraySize(FieldsRvalueData)); + + addTests({&FilterTest::fieldsWrongBitCount, &FilterTest::onlyFields, &FilterTest::onlyFieldsNoFieldData, + &FilterTest::onlyFieldsRvalue, &FilterTest::exceptFields, - &FilterTest::exceptFieldsNoFieldData}); + &FilterTest::exceptFieldsNoFieldData, + &FilterTest::exceptFieldsRvalue}); addInstancedTests({&FilterTest::fieldEntries}, Containers::arraySize(FieldEntriesData)); @@ -165,6 +189,62 @@ void FilterTest::fields() { CORRADE_VERIFY(!fieldData.deleter()); } +void FilterTest::fieldsRvalue() { + auto&& data = FieldsRvalueData[testCaseInstanceId()]; + setTestCaseDescription(data.name); + + /* Subset of attributes() verifying data ownership transfer behavior */ + + struct Data { + UnsignedShort meshMaterialMapping[5]; + UnsignedByte mesh[5]; + Byte meshMaterial[5]; + UnsignedShort lightVisibilityMapping[3]; + UnsignedInt light[3]; + bool visible[3]; + }; + Containers::Array sceneData{sizeof(Data)}; + Data& d = *reinterpret_cast(sceneData.data()); + Containers::Array fields{InPlaceInit, { + Trade::SceneFieldData{Trade::SceneField::Mesh, + Containers::arrayView(d.meshMaterialMapping), + Containers::arrayView(d.mesh)}, + Trade::SceneFieldData{Trade::SceneField::MeshMaterial, + Containers::arrayView(d.meshMaterialMapping), + Containers::arrayView(d.meshMaterial)}, + Trade::SceneFieldData{Trade::SceneField::Light, + Containers::arrayView(d.lightVisibilityMapping), + Containers::arrayView(d.light)}, + Trade::SceneFieldData{Trade::sceneFieldCustom(15), + Containers::arrayView(d.lightVisibilityMapping), + Containers::stridedArrayView(d.visible).sliceBit(0)}, + }}; + + Containers::Optional scene; + if(data.dataFlags >= Trade::DataFlag::Owned) + scene = Trade::SceneData{Trade::SceneMappingType::UnsignedShort, 76, Utility::move(sceneData), Utility::move(fields)}; + else + scene = Trade::SceneData{Trade::SceneMappingType::UnsignedShort, 76, data.dataFlags, sceneData, Utility::move(fields)}; + + Containers::BitArray attributesToKeep{ValueInit, scene->fieldCount()}; + attributesToKeep.set(0); + attributesToKeep.set(1); + attributesToKeep.set(3); + + /* The data ownership should be transferred if possible */ + Trade::SceneData filtered = filterFields(Utility::move(*scene), attributesToKeep); + CORRADE_COMPARE(filtered.mappingType(), Trade::SceneMappingType::UnsignedShort); + CORRADE_COMPARE(filtered.mappingBound(), 76); + CORRADE_COMPARE(filtered.data().data(), static_cast(&d)); + CORRADE_COMPARE(filtered.dataFlags(), data.expectedDataFlags); + + /* Just checking that the fields get actually filtered instead of being + passed through verbatim, the actual verification is done in fields() + above */ + CORRADE_COMPARE(filtered.fieldCount(), 3); + CORRADE_COMPARE(filtered.fieldName(0), Trade::SceneField::Mesh); +} + void FilterTest::fieldsWrongBitCount() { CORRADE_SKIP_IF_NO_ASSERT(); @@ -236,8 +316,9 @@ void FilterTest::onlyFields() { void FilterTest::onlyFieldsNoFieldData() { /* Just to verify it doesn't crash */ - Trade::SceneData filtered = filterOnlyFields(Trade::SceneData{Trade::SceneMappingType::UnsignedShort, 331, nullptr, {}}, { - Trade::SceneField::MeshMaterial, + Trade::SceneData scene{Trade::SceneMappingType::UnsignedShort, 331, nullptr, {}}; + Trade::SceneData filtered = filterOnlyFields(scene, { + Trade::SceneField::MeshMaterial }); CORRADE_COMPARE(filtered.mappingType(), Trade::SceneMappingType::UnsignedShort); CORRADE_COMPARE(filtered.mappingBound(), 331); @@ -245,6 +326,47 @@ void FilterTest::onlyFieldsNoFieldData() { CORRADE_COMPARE(filtered.dataFlags(), Trade::DataFlags{}); } +void FilterTest::onlyFieldsRvalue() { + /* Subset of onlyFields() verifying data ownership transfer behavior. All + cases of ownership transfer are verified in fieldsRvalue(), this only + checks that the r-value gets correctly passed through all overloads to + keep the data owned. */ + + struct Data { + UnsignedByte meshMaterialMapping[5]; + UnsignedByte mesh[5]; + Byte meshMaterial[5]; + UnsignedByte lightMapping[3]; + UnsignedInt light[3]; + }; + Containers::Array data{sizeof(Data)}; + Data& d = *reinterpret_cast(data.data()); + + Trade::SceneData scene{Trade::SceneMappingType::UnsignedByte, 133, Utility::move(data), { + Trade::SceneFieldData{Trade::SceneField::Mesh, + Containers::arrayView(d.meshMaterialMapping), + Containers::arrayView(d.mesh)}, + Trade::SceneFieldData{Trade::SceneField::MeshMaterial, + Containers::arrayView(d.meshMaterialMapping), + Containers::arrayView(d.meshMaterial)}, + Trade::SceneFieldData{Trade::SceneField::Light, + Containers::arrayView(d.lightMapping), + Containers::arrayView(d.light)}, + }}; + + Trade::SceneData filtered = filterOnlyFields(Utility::move(scene), { + Trade::SceneField::Light, + Trade::SceneField::MeshMaterial + }); + CORRADE_COMPARE(filtered.mappingType(), Trade::SceneMappingType::UnsignedByte); + CORRADE_COMPARE(filtered.mappingBound(), 133); + CORRADE_COMPARE(filtered.data().data(), static_cast(&d)); + CORRADE_COMPARE(filtered.dataFlags(), Trade::DataFlag::Owned|Trade::DataFlag::Mutable); + + CORRADE_COMPARE(filtered.fieldCount(), 2); + CORRADE_COMPARE(filtered.fieldName(0), Trade::SceneField::MeshMaterial); +} + void FilterTest::exceptFields() { const struct { UnsignedLong meshMaterialMapping[5]; @@ -302,7 +424,8 @@ void FilterTest::exceptFields() { void FilterTest::exceptFieldsNoFieldData() { /* Just to verify it doesn't crash */ - Trade::SceneData filtered = filterOnlyFields(Trade::SceneData{Trade::SceneMappingType::UnsignedShort, 331, nullptr, {}}, { + Trade::SceneData scene{Trade::SceneMappingType::UnsignedShort, 331, nullptr, {}}; + Trade::SceneData filtered = filterOnlyFields(scene, { Trade::SceneField::MeshMaterial, }); CORRADE_COMPARE(filtered.mappingType(), Trade::SceneMappingType::UnsignedShort); @@ -311,6 +434,51 @@ void FilterTest::exceptFieldsNoFieldData() { CORRADE_COMPARE(filtered.dataFlags(), Trade::DataFlags{}); } +void FilterTest::exceptFieldsRvalue() { + /* Subset of exceptFields() verifying data ownership transfer behavior. All + cases of ownership transfer are verified in fieldsRvalue(), this only + checks that the r-value gets correctly passed through all overloads to + keep the data owned. */ + + struct Data { + UnsignedLong meshMaterialMapping[5]; + UnsignedByte mesh[5]; + Byte meshMaterial[5]; + UnsignedLong lightVisibilityMapping[3]; + UnsignedInt light[3]; + bool visible[3]; + }; + Containers::Array data{sizeof(Data)}; + Data& d = *reinterpret_cast(data.data()); + + Trade::SceneData scene{Trade::SceneMappingType::UnsignedLong, 12, Utility::move(data), { + Trade::SceneFieldData{Trade::SceneField::Mesh, + Containers::arrayView(d.meshMaterialMapping), + Containers::arrayView(d.mesh)}, + Trade::SceneFieldData{Trade::SceneField::MeshMaterial, + Containers::arrayView(d.meshMaterialMapping), + Containers::arrayView(d.meshMaterial)}, + Trade::SceneFieldData{Trade::SceneField::Light, + Containers::arrayView(d.lightVisibilityMapping), + Containers::arrayView(d.light)}, + Trade::SceneFieldData{Trade::sceneFieldCustom(15), + Containers::arrayView(d.lightVisibilityMapping), + Containers::stridedArrayView(d.visible).sliceBit(0)}, + }}; + + Trade::SceneData filtered = filterExceptFields(Utility::move(scene), { + Trade::SceneField::Light, + Trade::SceneField::MeshMaterial + }); + CORRADE_COMPARE(filtered.mappingType(), Trade::SceneMappingType::UnsignedLong); + CORRADE_COMPARE(filtered.mappingBound(), 12); + CORRADE_COMPARE(filtered.data().data(), static_cast(&d)); + CORRADE_COMPARE(filtered.dataFlags(), Trade::DataFlag::Owned|Trade::DataFlag::Mutable); + + CORRADE_COMPARE(filtered.fieldCount(), 2); + CORRADE_COMPARE(filtered.fieldName(0), Trade::SceneField::Mesh); +} + void FilterTest::fieldEntries() { auto&& data = FieldEntriesData[testCaseInstanceId()]; setTestCaseDescription(data.name);