From 59d6709007a81dde666391f7ec2c8b66c7e6676e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Thu, 9 Sep 2021 13:56:58 +0200 Subject: [PATCH] Trade: avoid double lookup in SceneData::fooAsArray(). These need to query field size for allocating the output array which means looking up the field by name, but the same lookup was then done again in the fooInto() implementation. --- src/Magnum/Trade/SceneData.cpp | 24 +++++++++++++----------- src/Magnum/Trade/SceneData.h | 3 ++- src/Magnum/Trade/Test/SceneDataTest.cpp | 14 ++++++++++++-- 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/src/Magnum/Trade/SceneData.cpp b/src/Magnum/Trade/SceneData.cpp index 13b90d0ce..1ae8f15b2 100644 --- a/src/Magnum/Trade/SceneData.cpp +++ b/src/Magnum/Trade/SceneData.cpp @@ -715,8 +715,7 @@ Containers::Array SceneData::objectsAsArray(const SceneField name) return objectsAsArray(fieldId); } -void SceneData::parentsInto(const Containers::StridedArrayView1D& destination) const { - const UnsignedInt fieldId = fieldFor(SceneField::Parent); +void SceneData::parentsIntoInternal(const UnsignedInt fieldId, const Containers::StridedArrayView1D& destination) const { CORRADE_ASSERT(fieldId != ~UnsignedInt{}, "Trade::SceneData::parentsInto(): field not found", ); const SceneFieldData& field = _fields[fieldId]; @@ -737,6 +736,10 @@ void SceneData::parentsInto(const Containers::StridedArrayView1D& destinati } else CORRADE_INTERNAL_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */ } +void SceneData::parentsInto(const Containers::StridedArrayView1D& destination) const { + parentsIntoInternal(fieldFor(SceneField::Parent), destination); +} + Containers::Array SceneData::parentsAsArray() const { const UnsignedInt fieldId = fieldFor(SceneField::Parent); CORRADE_ASSERT(fieldId != ~UnsignedInt{}, @@ -744,7 +747,7 @@ Containers::Array SceneData::parentsAsArray() const { strings in the binary */ "Trade::SceneData::parentsInto(): field not found", {}); Containers::Array out{NoInit, std::size_t(_fields[fieldId]._size)}; - parentsInto(out); + parentsIntoInternal(fieldId, out); return out; } @@ -1022,9 +1025,8 @@ void SceneData::indexFieldIntoInternal( #ifndef CORRADE_NO_ASSERT const char* const prefix, #endif - const SceneField name, const Containers::StridedArrayView1D& destination) const + const UnsignedInt fieldId, const Containers::StridedArrayView1D& destination) const { - const UnsignedInt fieldId = fieldFor(name); CORRADE_ASSERT(fieldId != ~UnsignedInt{}, prefix << "field not found", ); const SceneFieldData& field = _fields[fieldId]; @@ -1055,7 +1057,7 @@ Containers::Array SceneData::indexFieldAsArrayInternal( #ifndef CORRADE_NO_ASSERT prefix, #endif - name, out); + fieldId, out); return out; } @@ -1064,7 +1066,7 @@ void SceneData::meshesInto(const Containers::StridedArrayView1D& de #ifndef CORRADE_NO_ASSERT "Trade::SceneData::meshesInto():", #endif - SceneField::Mesh, destination); + fieldFor(SceneField::Mesh), destination); } Containers::Array SceneData::meshesAsArray() const { @@ -1082,7 +1084,7 @@ void SceneData::meshMaterialsInto(const Containers::StridedArrayView1D SceneData::meshMaterialsAsArray() const { @@ -1100,7 +1102,7 @@ void SceneData::lightsInto(const Containers::StridedArrayView1D& de #ifndef CORRADE_NO_ASSERT "Trade::SceneData::lightsInto():", #endif - SceneField::Light, destination); + fieldFor(SceneField::Light), destination); } Containers::Array SceneData::lightsAsArray() const { @@ -1118,7 +1120,7 @@ void SceneData::camerasInto(const Containers::StridedArrayView1D& d #ifndef CORRADE_NO_ASSERT "Trade::SceneData::camerasInto():", #endif - SceneField::Camera, destination); + fieldFor(SceneField::Camera), destination); } Containers::Array SceneData::camerasAsArray() const { @@ -1136,7 +1138,7 @@ void SceneData::skinsInto(const Containers::StridedArrayView1D& des #ifndef CORRADE_NO_ASSERT "Trade::SceneData::skinsInto():", #endif - SceneField::Skin, destination); + fieldFor(SceneField::Skin), destination); } Containers::Array SceneData::skinsAsArray() const { diff --git a/src/Magnum/Trade/SceneData.h b/src/Magnum/Trade/SceneData.h index 48569d955..04e4ca4e3 100644 --- a/src/Magnum/Trade/SceneData.h +++ b/src/Magnum/Trade/SceneData.h @@ -1550,6 +1550,7 @@ class MAGNUM_TRADE_EXPORT SceneData { template bool checkFieldTypeCompatibility(const SceneFieldData& attribute, const char* prefix) const; #endif + MAGNUM_TRADE_LOCAL void parentsIntoInternal(UnsignedInt fieldId, const Containers::StridedArrayView1D& destination) const; MAGNUM_TRADE_LOCAL std::size_t findTransformFields(UnsignedInt& transformationFieldId, UnsignedInt& translationFieldId, UnsignedInt& rotationFieldId, UnsignedInt& scalingFieldId) const; MAGNUM_TRADE_LOCAL void transformations2DIntoInternal(UnsignedInt transformationFieldId, UnsignedInt translationFieldId, UnsignedInt rotationFieldId, UnsignedInt scalingFieldId, const Containers::StridedArrayView1D& destination) const; MAGNUM_TRADE_LOCAL void transformations3DIntoInternal(UnsignedInt transformationFieldId, UnsignedInt translationFieldId, UnsignedInt rotationFieldId, UnsignedInt scalingFieldId, const Containers::StridedArrayView1D& destination) const; @@ -1557,7 +1558,7 @@ class MAGNUM_TRADE_EXPORT SceneData { #ifndef CORRADE_NO_ASSERT const char* const prefix, #endif - const SceneField name, const Containers::StridedArrayView1D& destination) const; + const UnsignedInt fieldId, const Containers::StridedArrayView1D& destination) const; MAGNUM_TRADE_LOCAL Containers::Array indexFieldAsArrayInternal( #ifndef CORRADE_NO_ASSERT const char* const prefix, diff --git a/src/Magnum/Trade/Test/SceneDataTest.cpp b/src/Magnum/Trade/Test/SceneDataTest.cpp index 1a91c6846..1cb1e6254 100644 --- a/src/Magnum/Trade/Test/SceneDataTest.cpp +++ b/src/Magnum/Trade/Test/SceneDataTest.cpp @@ -1786,8 +1786,18 @@ template void SceneDataTest::parentsAsArray() { SceneFieldData{SceneField::Mesh, SceneObjectType::UnsignedByte, nullptr, SceneFieldType::UnsignedInt, nullptr}, SceneFieldData{SceneField::Parent, view.slice(&Field::object), view.slice(&Field::parent)} }}; - CORRADE_COMPARE_AS(scene.parentsAsArray(), - Containers::arrayView({15, -1, 44}), + + Int expected[]{15, -1, 44}; + CORRADE_COMPARE_AS(arrayView(scene.parentsAsArray()), + Containers::arrayView(expected), + TestSuite::Compare::Container); + + /* Test Into() as well as it only shares a common helper with AsArray() but + has different top-level code paths */ + Int out[3]; + scene.parentsInto(out); + CORRADE_COMPARE_AS(Containers::arrayView(out), + Containers::arrayView(expected), TestSuite::Compare::Container); }