From a3c5c0052db0cd9dedb1c8be1b8800b8032c43a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 19 Feb 2020 19:02:53 +0100 Subject: [PATCH] Trade: make MeshData::release*() less brutal, add releaseAttributeData(). It only resets count of released thing to zero, not going all nuclear. Otherwise it wouldn't be possible to release attribute data and then vertex data as releasing one would wipe the other. --- src/Magnum/Trade/MeshData.cpp | 29 +++++++++---- src/Magnum/Trade/MeshData.h | 40 +++++++++++++---- src/Magnum/Trade/Test/MeshDataTest.cpp | 59 ++++++++++++++++++++++---- 3 files changed, 103 insertions(+), 25 deletions(-) diff --git a/src/Magnum/Trade/MeshData.cpp b/src/Magnum/Trade/MeshData.cpp index 75e816293..9c149db99 100644 --- a/src/Magnum/Trade/MeshData.cpp +++ b/src/Magnum/Trade/MeshData.cpp @@ -282,9 +282,11 @@ UnsignedInt MeshData::attributeStride(MeshAttribute name, UnsignedInt id) const Containers::StridedArrayView2D MeshData::attribute(UnsignedInt id) const { CORRADE_ASSERT(id < _attributes.size(), "Trade::MeshData::attribute(): index" << id << "out of range for" << _attributes.size() << "attributes", nullptr); - /* Build a 2D view using information about attribute type size */ + /* Build a 2D view using information about attribute type size, return only + a prefix of the actual vertex count (which is zero in case vertex data + is released) */ return Containers::arrayCast<2, const char>(_attributes[id]._data, - vertexFormatSize(_attributes[id]._format)); + vertexFormatSize(_attributes[id]._format)).prefix(_vertexCount); } Containers::StridedArrayView2D MeshData::mutableAttribute(UnsignedInt id) { @@ -292,9 +294,11 @@ Containers::StridedArrayView2D MeshData::mutableAttribute(UnsignedInt id) "Trade::MeshData::mutableAttribute(): vertex data not mutable", {}); CORRADE_ASSERT(id < _attributes.size(), "Trade::MeshData::mutableAttribute(): index" << id << "out of range for" << _attributes.size() << "attributes", nullptr); - /* Build a 2D view using information about attribute type size */ + /* Build a 2D view using information about attribute type size, return only + a prefix of the actual vertex count (which is zero in case vertex data + is released) */ auto out = Containers::arrayCast<2, const char>(_attributes[id]._data, - vertexFormatSize(_attributes[id]._format)); + vertexFormatSize(_attributes[id]._format)).prefix(_vertexCount); /** @todo some arrayConstCast? UGH */ return Containers::StridedArrayView2D{ /* The view size is there only for a size assert, we're pretty sure the @@ -458,14 +462,21 @@ Containers::Array MeshData::colorsAsArray(const UnsignedInt id) const { } Containers::Array MeshData::releaseIndexData() { - _indexType = MeshIndexType{}; /* so isIndexed() returns false */ - _indices = nullptr; - return std::move(_indexData); + _indices = {_indices.data(), 0}; + Containers::Array out = std::move(_indexData); + _indexData = Containers::Array{out.data(), 0, Implementation::nonOwnedArrayDeleter}; + return out; +} + +Containers::Array MeshData::releaseAttributeData() { + return std::move(_attributes); } Containers::Array MeshData::releaseVertexData() { - _attributes = nullptr; - return std::move(_vertexData); + _vertexCount = 0; + Containers::Array out = std::move(_vertexData); + _vertexData = Containers::Array{out.data(), 0, Implementation::nonOwnedArrayDeleter}; + return out; } Debug& operator<<(Debug& debug, const MeshAttribute value) { diff --git a/src/Magnum/Trade/MeshData.h b/src/Magnum/Trade/MeshData.h index cb3070760..bba3ce6ab 100644 --- a/src/Magnum/Trade/MeshData.h +++ b/src/Magnum/Trade/MeshData.h @@ -1073,21 +1073,45 @@ class MAGNUM_TRADE_EXPORT MeshData { * * Releases the ownership of the index data array and resets internal * index-related state to default. The mesh then behaves like - * non-indexed. Note that the returned array has a custom no-op deleter - * when the data are not owned by the mesh, and while the returned - * array type is mutable, the actual memory might be not. + * it has zero indices (but it can still have a non-zero vertex count), + * however @ref indexData() still return a zero-sized non-null array so + * index offset calculation continues to work as expected. + * + * Note that the returned array has a custom no-op deleter when the + * data are not owned by the mesh, and while the returned array type is + * mutable, the actual memory might be not. * @see @ref indexData(), @ref indexDataFlags() */ Containers::Array releaseIndexData(); + /** + * @brief Release attribute data storage + * + * Releases the ownership of the attribute data array and resets + * internal attribute-related state to default. The mesh then behaves + * like if it has no attributes (but it can still have a non-zero + * vertex count). Note that the returned array has a custom no-op + * deleter when the data are not owned by the mesh, and while the + * returned array type is mutable, the actual memory might be not --- + * use this function only if you are sure about the origin of the + * array. + * @see @ref attributeData() + */ + Containers::Array releaseAttributeData(); + /** * @brief Release vertex data storage * - * Releases the ownership of the index data array and resets internal - * attribute-related state to default. The mesh then behaves like if - * it has no attributes. Note that the returned array has a custom - * no-op deleter when the data are not owned by the mesh, and while the - * returned array type is mutable, the actual memory might be not. + * Releases the ownership of the vertex data array and resets internal + * attribute-related state to default. The mesh then behaves like it + * has zero vertices (but it can still have a non-zero amount of + * attributes), however @ref vertexData() will still return a zero- + * sized non-null array so attribute offset calculation continues to + * work as expected. + * + * Note that the returned array has a custom no-op deleter when the + * data are not owned by the mesh, and while the returned array type is + * mutable, the actual memory might be not. * @see @ref vertexData(), @ref vertexDataFlags() */ Containers::Array releaseVertexData(); diff --git a/src/Magnum/Trade/Test/MeshDataTest.cpp b/src/Magnum/Trade/Test/MeshDataTest.cpp index 45e51067d..72f267860 100644 --- a/src/Magnum/Trade/Test/MeshDataTest.cpp +++ b/src/Magnum/Trade/Test/MeshDataTest.cpp @@ -115,6 +115,7 @@ struct MeshDataTest: TestSuite::Tester { void attributeWrongType(); void releaseIndexData(); + void releaseAttributeData(); void releaseVertexData(); }; @@ -223,6 +224,7 @@ MeshDataTest::MeshDataTest() { &MeshDataTest::attributeWrongType, &MeshDataTest::releaseIndexData, + &MeshDataTest::releaseAttributeData, &MeshDataTest::releaseVertexData}); } @@ -1581,19 +1583,26 @@ void MeshDataTest::attributeWrongType() { } void MeshDataTest::releaseIndexData() { - Containers::Array indexData{6}; - auto indexView = Containers::arrayCast(indexData); + Containers::Array indexData{23}; + auto indexView = Containers::arrayCast(indexData.slice(6, 12)); MeshData data{MeshPrimitive::TriangleStrip, std::move(indexData), MeshIndexData{indexView}}; CORRADE_VERIFY(data.isIndexed()); + CORRADE_COMPARE(data.indexCount(), 3); + CORRADE_COMPARE(data.indexOffset(), 6); Containers::Array released = data.releaseIndexData(); - CORRADE_COMPARE(static_cast(released.data()), indexView.data()); - CORRADE_COMPARE(data.indexData(), nullptr); - CORRADE_VERIFY(!data.isIndexed()); + CORRADE_COMPARE(static_cast(released.data() + 6), indexView.data()); + /* This is not null as we still need the value for calculating offsets */ + CORRADE_COMPARE(static_cast(data.indexData()), released.data()); + CORRADE_COMPARE(data.indexData().size(), 0); + CORRADE_VERIFY(data.isIndexed()); + CORRADE_COMPARE(data.indexCount(), 0); + CORRADE_COMPARE(data.indexType(), MeshIndexType::UnsignedShort); + CORRADE_COMPARE(data.indexOffset(), 6); } -void MeshDataTest::releaseVertexData() { +void MeshDataTest::releaseAttributeData() { Containers::Array vertexData{16}; auto vertexView = Containers::arrayCast(vertexData); @@ -1601,9 +1610,43 @@ void MeshDataTest::releaseVertexData() { MeshData data{MeshPrimitive::LineLoop, std::move(vertexData), {positions, positions}}; CORRADE_COMPARE(data.attributeCount(), 2); - Containers::Array released = data.releaseVertexData(); - CORRADE_COMPARE(data.vertexData(), nullptr); + Containers::Array released = data.releaseAttributeData(); + CORRADE_COMPARE(released.size(), 2); + CORRADE_COMPARE(static_cast(released[0].data().data()), vertexView.data()); + CORRADE_COMPARE(released[0].data().size(), 2); + /* Unlike the other two, this is null as we don't need the value for + calculating anything */ + CORRADE_COMPARE(static_cast(data.attributeData()), nullptr); CORRADE_COMPARE(data.attributeCount(), 0); + CORRADE_COMPARE(static_cast(data.vertexData()), vertexView); + CORRADE_COMPARE(data.vertexCount(), 2); +} + +void MeshDataTest::releaseVertexData() { + Containers::Array vertexData{80}; + auto vertexView = Containers::arrayCast(vertexData.slice(48, 72)); + + MeshAttributeData positions{MeshAttribute::Position, vertexView}; + MeshData data{MeshPrimitive::LineLoop, std::move(vertexData), {positions, positions}}; + CORRADE_COMPARE(data.attributeCount(), 2); + CORRADE_COMPARE(data.vertexCount(), 3); + CORRADE_COMPARE(data.attributeOffset(0), 48); + + Containers::Array released = data.releaseVertexData(); + CORRADE_VERIFY(data.attributeData()); + CORRADE_COMPARE(data.attributeCount(), 2); + CORRADE_COMPARE(static_cast(static_cast(data.attribute(0).data())), vertexView.data()); + CORRADE_COMPARE(static_cast(static_cast(data.mutableAttribute(0).data())), vertexView.data()); + /* Returned views should be patched to have zero size (but not the direct + access, there it stays as it's an internal API really) */ + CORRADE_COMPARE(data.attribute(0).size()[0], 0); + CORRADE_COMPARE(data.mutableAttribute(0).size()[0], 0); + CORRADE_COMPARE(data.attributeData()[0].data().size(), 3); + CORRADE_COMPARE(static_cast(released.data() + 48), vertexView.data()); + /* This is not null as we still need the value for calculating offsets */ + CORRADE_COMPARE(static_cast(data.vertexData()), released.data()); + CORRADE_COMPARE(data.vertexCount(), 0); + CORRADE_COMPARE(data.attributeOffset(0), 48); } }}}}