From c34e03dc347118207bae54f8ce765f9d96bbd4f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 7 Dec 2021 01:22:51 +0100 Subject: [PATCH] Trade: allow omitting trivial SceneField::Parent data. --- doc/snippets/MagnumTrade.cpp | 10 ++ src/Magnum/Trade/SceneData.cpp | 45 +++++-- src/Magnum/Trade/SceneData.h | 36 +++++- src/Magnum/Trade/Test/SceneDataTest.cpp | 160 ++++++++++++++++++++++++ 4 files changed, 241 insertions(+), 10 deletions(-) diff --git a/doc/snippets/MagnumTrade.cpp b/doc/snippets/MagnumTrade.cpp index 3ff68fdc24..40649653c0 100644 --- a/doc/snippets/MagnumTrade.cpp +++ b/doc/snippets/MagnumTrade.cpp @@ -926,6 +926,16 @@ Trade::SceneFieldData field{Trade::SceneField::Transformation, /* [SceneFieldData-usage-implicit-mapping] */ } +{ +std::size_t objectCount{}; +/* [SceneFieldData-usage-trivial-parent] */ +Trade::SceneFieldData field{Trade::SceneField::Parent, + Containers::ArrayView{nullptr, objectCount}, + Containers::ArrayView{nullptr, objectCount}, + Trade::SceneFieldFlag::ImplicitMapping|Trade::SceneFieldFlag::TrivialField}; +/* [SceneFieldData-usage-trivial-parent] */ +} + { typedef SceneGraph::Scene Scene3D; typedef SceneGraph::Object Object3D; diff --git a/src/Magnum/Trade/SceneData.cpp b/src/Magnum/Trade/SceneData.cpp index e8aeb271a0..53e0d77f0c 100644 --- a/src/Magnum/Trade/SceneData.cpp +++ b/src/Magnum/Trade/SceneData.cpp @@ -485,6 +485,7 @@ Debug& operator<<(Debug& debug, const SceneFieldFlag value) { _c(OffsetOnly) _c(ImplicitMapping) _c(OrderedMapping) + _c(TrivialField) #undef _c /* LCOV_EXCL_STOP */ } @@ -497,7 +498,8 @@ Debug& operator<<(Debug& debug, const SceneFieldFlags value) { SceneFieldFlag::OffsetOnly, SceneFieldFlag::ImplicitMapping, /* This one is implied by ImplicitMapping, so has to be after */ - SceneFieldFlag::OrderedMapping + SceneFieldFlag::OrderedMapping, + SceneFieldFlag::TrivialField }); } @@ -602,9 +604,14 @@ SceneData::SceneData(const SceneMappingType mappingType, const UnsignedLong mapp "Trade::SceneData: offset-only mapping data of field" << i << "span" << mappingSize << "bytes but passed data array has only" << _data.size(), ); } - const std::size_t fieldSize = field._fieldData.offset + (field._size - 1)*field._fieldStride + fieldTypeSize; - CORRADE_ASSERT(fieldSize <= _data.size(), - "Trade::SceneData: offset-only field data of field" << i << "span" << fieldSize << "bytes but passed data array has only" << _data.size(), ); + /* If an offset-only field is trivial, we ignore the offset / + size completely. Trivial fields are whitelisted, which was + already checked in SceneFieldData constructor. */ + if(!(field._flags >= SceneFieldFlag::TrivialField)) { + const std::size_t fieldSize = field._fieldData.offset + (field._size - 1)*field._fieldStride + fieldTypeSize; + CORRADE_ASSERT(fieldSize <= _data.size(), + "Trade::SceneData: offset-only field data of field" << i << "span" << fieldSize << "bytes but passed data array has only" << _data.size(), ); + } } else { /* If a field has an implicit mapping, we allow it to be @@ -616,10 +623,15 @@ SceneData::SceneData(const SceneMappingType mappingType, const UnsignedLong mapp "Trade::SceneData: mapping data [" << Debug::nospace << mappingBegin << Debug::nospace << ":" << Debug::nospace << mappingEnd << Debug::nospace << "] of field" << i << "are not contained in passed data array [" << Debug::nospace << static_cast(_data.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast(_data.end()) << Debug::nospace << "]", ); } - const void* const fieldBegin = field._fieldData.pointer; - const void* const fieldEnd = static_cast(field._fieldData.pointer) + (field._size - 1)*field._fieldStride + fieldTypeSize; - CORRADE_ASSERT(fieldBegin >= _data.begin() && fieldEnd <= _data.end(), - "Trade::SceneData: field data [" << Debug::nospace << fieldBegin << Debug::nospace << ":" << Debug::nospace << fieldEnd << Debug::nospace << "] of field" << i << "are not contained in passed data array [" << Debug::nospace << static_cast(_data.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast(_data.end()) << Debug::nospace << "]", ); + /* Trivial fields are allowed to be nullptr. Trivial fields are + whitelisted, which was already checked in SceneFieldData + constructor. */ + if(!(field._flags >= SceneFieldFlag::TrivialField && !field._fieldData.pointer)) { + const void* const fieldBegin = field._fieldData.pointer; + const void* const fieldEnd = static_cast(field._fieldData.pointer) + (field._size - 1)*field._fieldStride + fieldTypeSize; + CORRADE_ASSERT(fieldBegin >= _data.begin() && fieldEnd <= _data.end(), + "Trade::SceneData: field data [" << Debug::nospace << fieldBegin << Debug::nospace << ":" << Debug::nospace << fieldEnd << Debug::nospace << "] of field" << i << "are not contained in passed data array [" << Debug::nospace << static_cast(_data.begin()) << Debug::nospace << ":" << Debug::nospace << static_cast(_data.end()) << Debug::nospace << "]", ); + } } } #endif @@ -838,6 +850,12 @@ Containers::StridedArrayView1D SceneData::fieldDataMappingViewIntern Containers::StridedArrayView1D SceneData::fieldDataFieldViewInternal(const SceneFieldData& field, const std::size_t offset, const std::size_t size) const { CORRADE_INTERNAL_ASSERT(offset + size <= field._size); + + /* If this is a offset-only field that's trivial, ignore the offset/stride + and always assume it's not present */ + if(field._flags >= (SceneFieldFlag::OffsetOnly|SceneFieldFlag::TrivialField)) + return {{nullptr, ~std::size_t{}}, size, field._fieldStride}; + return Containers::StridedArrayView1D{ /* We're *sure* the view is correct, so faking the view size */ {static_cast(field._flags & SceneFieldFlag::OffsetOnly ? @@ -1239,6 +1257,17 @@ void SceneData::parentsIntoInternal(const UnsignedInt fieldId, const std::size_t checked by the callers */ const SceneFieldData& field = _fields[fieldId]; + + /* If we don't have any data for a trivial parent or the trivial parent is + offset-only (where we always assume there's no data), generate the data */ + if((field._flags >= SceneFieldFlag::TrivialField && !field._fieldData.pointer) || + (field._flags >= (SceneFieldFlag::TrivialField|SceneFieldFlag::OffsetOnly))) + { + for(std::size_t i = 0; i != destination.size(); ++i) + destination[i] = -1; + return; + } + const Containers::StridedArrayView1D fieldData = fieldDataFieldViewInternal(field, offset, destination.size()); const auto destination1i = Containers::arrayCast<2, Int>(destination); diff --git a/src/Magnum/Trade/SceneData.h b/src/Magnum/Trade/SceneData.h index 6c9893c736..8057e3b697 100644 --- a/src/Magnum/Trade/SceneData.h +++ b/src/Magnum/Trade/SceneData.h @@ -105,6 +105,9 @@ enum class SceneField: UnsignedInt { * no parent. An object should have only one parent, altough this isn't * enforced in any way, and which of the duplicate fields gets used is not * defined. + * + * This field is allowed to have @ref SceneFieldFlag::TrivialField set, + * which implies it has @cpp -1 @ce for all values. * @see @ref SceneData::parentsAsArray(), @ref SceneData::parentFor(), * @ref SceneData::childrenFor() */ @@ -579,6 +582,13 @@ enum class SceneFieldFlag: UnsignedByte { * @f$ \mathcal{O}(n) @f$ lookup complexity. */ ImplicitMapping = (1 << 2)|OrderedMapping, + + /** + * The field has a trivial content. Currently allowed only for + * @ref SceneField::Parent, indicating all entries are @cpp -1 @ce. If this + * flag is set, the field view is allowed to be @cpp nullptr @ce. + */ + TrivialField = 1 << 3 }; /** @@ -661,6 +671,18 @@ Fields that are both @ref SceneFieldFlag::OffsetOnly and @ref SceneFieldFlag::ImplicitMapping have their object mapping data always ignored as it's not possible to know whether the offset points to actual data or not. + +@subsection Trade-SceneFieldData-usage-trivial-field Trivial fields + +The @ref SceneField::Parent can be annotated with +@ref SceneFieldFlag::TrivialField, which implies that all nodes are in scene +root. While similar effect could be achieved by repeating a @cpp -1 @ce using +zero stride, the main purpose of this flag is in combination with +@ref SceneFieldFlag::ImplicitMapping --- that way you can indicate that all +objects in the scene are top-level without having to explicitly supply any +field data: + +@snippet MagnumTrade.cpp SceneFieldData-usage-trivial-parent */ class MAGNUM_TRADE_EXPORT SceneFieldData { public: @@ -3101,6 +3123,10 @@ namespace Implementation { constexpr bool isSceneFieldArrayAllowed(SceneField name) { return isSceneFieldCustom(name); } + + constexpr bool isSceneFieldAllowedTrivial(SceneField name) { + return name == SceneField::Parent; + } } constexpr SceneFieldData::SceneFieldData(const SceneField name, const SceneMappingType mappingType, const Containers::StridedArrayView1D& mappingData, const SceneFieldType fieldType, const Containers::StridedArrayView1D& fieldData, const UnsignedShort fieldArraySize, const SceneFieldFlags flags) noexcept: @@ -3109,7 +3135,10 @@ constexpr SceneFieldData::SceneFieldData(const SceneField name, const SceneMappi _name{(CORRADE_CONSTEXPR_ASSERT(Implementation::isSceneFieldTypeCompatibleWithField(name, fieldType), "Trade::SceneFieldData:" << fieldType << "is not a valid type for" << name), name)}, _flags{(CORRADE_CONSTEXPR_ASSERT(!(flags & SceneFieldFlag::OffsetOnly), - "Trade::SceneFieldData: can't pass Trade::SceneFieldFlag::OffsetOnly for a view"), flags)}, + "Trade::SceneFieldData: can't pass Trade::SceneFieldFlag::OffsetOnly for a view"), + CORRADE_CONSTEXPR_ASSERT(!(flags & SceneFieldFlag::TrivialField) || Implementation::isSceneFieldAllowedTrivial(name), + "Trade::SceneFieldData: can't pass Trade::SceneFieldFlag::TrivialField for" << name), + flags)}, _mappingType{mappingType}, _mappingStride{(CORRADE_CONSTEXPR_ASSERT(mappingData.stride() >= -32768 && mappingData.stride() <= 32767, "Trade::SceneFieldData: expected mapping view stride to fit into 16 bits, but got" << mappingData.stride()), Short(mappingData.stride()))}, @@ -3138,7 +3167,10 @@ constexpr SceneFieldData::SceneFieldData(const SceneField name, const std::size_ _size{size}, _name{(CORRADE_CONSTEXPR_ASSERT(Implementation::isSceneFieldTypeCompatibleWithField(name, fieldType), "Trade::SceneFieldData:" << fieldType << "is not a valid type for" << name), name)}, - _flags{flags|SceneFieldFlag::OffsetOnly}, + _flags{( + CORRADE_CONSTEXPR_ASSERT(!(flags & SceneFieldFlag::TrivialField) || Implementation::isSceneFieldAllowedTrivial(name), + "Trade::SceneFieldData: can't pass Trade::SceneFieldFlag::TrivialField for" << name), + flags|SceneFieldFlag::OffsetOnly)}, _mappingType{mappingType}, _mappingStride{(CORRADE_CONSTEXPR_ASSERT(mappingStride >= -32768 && mappingStride <= 32767, "Trade::SceneFieldData: expected mapping view stride to fit into 16 bits, but got" << mappingStride), Short(mappingStride))}, diff --git a/src/Magnum/Trade/Test/SceneDataTest.cpp b/src/Magnum/Trade/Test/SceneDataTest.cpp index 8eead14551..e1bc07b4d1 100644 --- a/src/Magnum/Trade/Test/SceneDataTest.cpp +++ b/src/Magnum/Trade/Test/SceneDataTest.cpp @@ -84,6 +84,7 @@ struct SceneDataTest: TestSuite::Tester { void constructFieldTooLargeMappingStride(); void constructFieldTooLargeFieldStride(); void constructFieldOffsetOnlyNotAllowed(); + void constructFieldTrivialNotAllowed(); void constructFieldWrongDataAccess(); void constructField2DWrongSize(); void constructField2DNonContiguous(); @@ -197,6 +198,7 @@ struct SceneDataTest: TestSuite::Tester { void findFieldObjectOffsetInvalidObject(); template void implicitNullMapping(); + template void trivialNullParent(); void releaseFieldData(); void releaseData(); @@ -377,6 +379,7 @@ SceneDataTest::SceneDataTest() { &SceneDataTest::constructFieldTooLargeMappingStride, &SceneDataTest::constructFieldTooLargeFieldStride, &SceneDataTest::constructFieldOffsetOnlyNotAllowed, + &SceneDataTest::constructFieldTrivialNotAllowed, &SceneDataTest::constructFieldWrongDataAccess, &SceneDataTest::constructField2DWrongSize, &SceneDataTest::constructField2DNonContiguous, @@ -569,6 +572,11 @@ SceneDataTest::SceneDataTest() { &SceneDataTest::implicitNullMapping, &SceneDataTest::implicitNullMapping, + &SceneDataTest::trivialNullParent, + &SceneDataTest::trivialNullParent, + &SceneDataTest::trivialNullParent, + &SceneDataTest::trivialNullParent, + &SceneDataTest::releaseFieldData, &SceneDataTest::releaseData}); } @@ -1111,6 +1119,23 @@ void SceneDataTest::constructFieldOffsetOnlyNotAllowed() { "Trade::SceneFieldData: can't pass Trade::SceneFieldFlag::OffsetOnly for a view\n"); } +void SceneDataTest::constructFieldTrivialNotAllowed() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + const UnsignedShort rotationMappingData[3]{}; + const Quaternion rotationFieldData[3]; + + std::ostringstream out; + Error redirectError{&out}; + SceneFieldData{SceneField::Rotation, 3, SceneMappingType::UnsignedShort, 0, sizeof(UnsignedShort), SceneFieldType::Quaternion, 0, sizeof(Quaternion), SceneFieldFlag::TrivialField}; + SceneFieldData{sceneFieldCustom(666), Containers::arrayView(rotationMappingData), Containers::arrayView(rotationFieldData), SceneFieldFlag::TrivialField}; + CORRADE_COMPARE(out.str(), + "Trade::SceneFieldData: can't pass Trade::SceneFieldFlag::TrivialField for Trade::SceneField::Rotation\n" + "Trade::SceneFieldData: can't pass Trade::SceneFieldFlag::TrivialField for Trade::SceneField::Custom(666)\n"); +} + void SceneDataTest::constructFieldWrongDataAccess() { #ifdef CORRADE_NO_ASSERT CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); @@ -5679,6 +5704,141 @@ template void SceneDataTest::implicitNullMapping() { } } +template void SceneDataTest::trivialNullParent() { + setTestCaseTemplateName(NameTraits::name()); + + struct Field { + UnsignedInt mapping; + T parent; + } data[]{ + {0, -1}, + {1, -1}, + {2, -1}, + {3, -2}, /* this is to know whether we got our or generated data */ + {5, -1}, /* this is to know whether we got our or generated data */ + }; + + Containers::StridedArrayView1D view = data; + + /* If the view is not nullptr, it'll use the data and won't generate */ + { + SceneData scene{SceneMappingType::UnsignedInt, 6, {}, data, { + SceneFieldData{SceneField::Parent, view.slice(&Field::mapping), view.slice(&Field::parent), SceneFieldFlag::TrivialField}, + }}; + CORRADE_COMPARE(scene.field(0).data(), &data[0].parent); + CORRADE_COMPARE_AS(scene.parentsAsArray(), (Containers::arrayView>({ + {0, -1}, + {1, -1}, + {2, -1}, + {3, -2}, /* this would be -1 if generated */ + {5, -1}, /* this would be 4 if generated */ + })), TestSuite::Compare::Container); + CORRADE_COMPARE(scene.parentFor(3), -2); + + Int parents[5]; + scene.parentsInto(nullptr, parents); + CORRADE_COMPARE_AS(Containers::arrayView(parents), Containers::arrayView({ + -1, -1, -1, -2, /* this would be 4 if generated */ -1 + }), TestSuite::Compare::Container); + } + + /* If the view is nullptr, it'll generate the data */ + { + SceneData scene{SceneMappingType::UnsignedInt, 6, {}, data, { + SceneFieldData{SceneField::Parent, view.slice(&Field::mapping), Containers::ArrayView{nullptr, view.size()}, SceneFieldFlag::TrivialField}, + }}; + CORRADE_COMPARE(scene.field(0).data(), nullptr); + CORRADE_COMPARE_AS(scene.parentsAsArray(), (Containers::arrayView>({ + {0, -1}, + {1, -1}, + {2, -1}, + {3, -1}, + {5, -1}, /* this would be 4 if generated */ + })), TestSuite::Compare::Container); + CORRADE_COMPARE(scene.parentFor(3), -1); + + Int parents[5]; + scene.parentsInto(nullptr, parents); + CORRADE_COMPARE_AS(Containers::arrayView(parents), Containers::arrayView({ + -1, -1, -1, -1, -1 + }), TestSuite::Compare::Container); + } + + /* For an offset-only trivial field it'll generate the data always, even + if the field is seemingly there */ + { + SceneData scene{SceneMappingType::UnsignedInt, 6, {}, data, { + SceneFieldData{SceneField::Parent, view.size(), SceneMappingType::UnsignedInt, offsetof(Field, mapping), sizeof(Field), Implementation::SceneFieldTypeFor::type(), offsetof(Field, parent), sizeof(Field), SceneFieldFlag::TrivialField}, + }}; + CORRADE_COMPARE(scene.field(0).data(), nullptr); + CORRADE_COMPARE_AS(scene.parentsAsArray(), (Containers::arrayView>({ + {0, -1}, + {1, -1}, + {2, -1}, + {3, -1}, + {5, -1}, /* this would be 4 if generated */ + })), TestSuite::Compare::Container); + CORRADE_COMPARE(scene.parentFor(3), -1); + + Int parents[5]; + scene.parentsInto(nullptr, parents); + CORRADE_COMPARE_AS(Containers::arrayView(parents), Containers::arrayView({ + -1, -1, -1, -1, -1 + }), TestSuite::Compare::Container); + } + + /* And if the offset is weirdly off it won't blow up on that */ + { + SceneData scene{SceneMappingType::UnsignedInt, 6, {}, data, { + SceneFieldData{SceneField::Parent, view.size(), SceneMappingType::UnsignedInt, offsetof(Field, mapping), sizeof(Field), Implementation::SceneFieldTypeFor::type(), 666666666, 0, SceneFieldFlag::TrivialField}, + }}; + CORRADE_COMPARE(scene.field(0).data(), nullptr); + CORRADE_COMPARE_AS(scene.parentsAsArray(), (Containers::arrayView>({ + {0, -1}, + {1, -1}, + {2, -1}, + {3, -1}, + {5, -1}, /* this would be 4 if generated */ + })), TestSuite::Compare::Container); + CORRADE_COMPARE(scene.parentFor(3), -1); + + Int parents[5]; + scene.parentsInto(nullptr, parents); + CORRADE_COMPARE_AS(Containers::arrayView(parents), Containers::arrayView({ + -1, -1, -1, -1, -1 + }), TestSuite::Compare::Container); + } + + /* Finally, neither the mapping nor the field is specified */ + { + SceneData scene{SceneMappingType::UnsignedInt, 6, {}, data, { + SceneFieldData{SceneField::Parent, + Containers::ArrayView{nullptr, view.size()}, + Containers::ArrayView{nullptr, view.size()}, + SceneFieldFlag::ImplicitMapping|SceneFieldFlag::TrivialField}, + }}; + CORRADE_COMPARE(scene.field(0).data(), nullptr); + CORRADE_COMPARE_AS(scene.parentsAsArray(), (Containers::arrayView>({ + {0, -1}, + {1, -1}, + {2, -1}, + {3, -1}, + {4, -1}, + })), TestSuite::Compare::Container); + CORRADE_COMPARE(scene.parentFor(3), -1); + + UnsignedInt mapping[5]; + Int parents[5]; + scene.parentsInto(mapping, parents); + CORRADE_COMPARE_AS(Containers::arrayView(mapping), Containers::arrayView({ + 0, 1, 2, 3, 4 + }), TestSuite::Compare::Container); + CORRADE_COMPARE_AS(Containers::arrayView(parents), Containers::arrayView({ + -1, -1, -1, -1, -1 + }), TestSuite::Compare::Container); + } +} + void SceneDataTest::releaseFieldData() { struct Field { UnsignedByte object;