From 1254c8c72ae03cee41fb0162004dc51aadf931fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 6 Dec 2021 23:39:47 +0100 Subject: [PATCH 1/3] Trade: allow omitting implicit SceneFieldData object mapping. --- doc/snippets/MagnumTrade.cpp | 11 ++ src/Magnum/Trade/SceneData.cpp | 43 +++++-- src/Magnum/Trade/SceneData.h | 62 +++++++++- src/Magnum/Trade/Test/SceneDataTest.cpp | 150 ++++++++++++++++++++++++ 4 files changed, 257 insertions(+), 9 deletions(-) diff --git a/doc/snippets/MagnumTrade.cpp b/doc/snippets/MagnumTrade.cpp index 75ac34c70..3ff68fdc2 100644 --- a/doc/snippets/MagnumTrade.cpp +++ b/doc/snippets/MagnumTrade.cpp @@ -915,6 +915,17 @@ Trade::SceneData{Trade::SceneMappingType::UnsignedInt, 120, std::move(data), /* [SceneFieldData-usage-offset-only] */ } +{ +/* [SceneFieldData-usage-implicit-mapping] */ +Containers::ArrayView transformations = DOXYGEN_ELLIPSIS({}); + +Trade::SceneFieldData field{Trade::SceneField::Transformation, + Containers::ArrayView{nullptr, transformations.size()}, + transformations, + Trade::SceneFieldFlag::ImplicitMapping}; +/* [SceneFieldData-usage-implicit-mapping] */ +} + { typedef SceneGraph::Scene Scene3D; typedef SceneGraph::Object Object3D; diff --git a/src/Magnum/Trade/SceneData.cpp b/src/Magnum/Trade/SceneData.cpp index 6af41487f..fe543a5d5 100644 --- a/src/Magnum/Trade/SceneData.cpp +++ b/src/Magnum/Trade/SceneData.cpp @@ -594,19 +594,30 @@ SceneData::SceneData(const SceneMappingType mappingType, const UnsignedLong mapp const UnsignedInt fieldTypeSize = sceneFieldTypeSize(field._fieldType)* (field._fieldArraySize ? field._fieldArraySize : 1); if(field._flags & SceneFieldFlag::OffsetOnly) { - const std::size_t mappingSize = field._mappingData.offset + (field._size - 1)*field._mappingStride + mappingTypeSize; + /* If an offset-only field has an implicit mapping, we ignore + the offset / size completely */ + if(!(field._flags >= SceneFieldFlag::ImplicitMapping)) { + const std::size_t mappingSize = field._mappingData.offset + (field._size - 1)*field._mappingStride + mappingTypeSize; + CORRADE_ASSERT(mappingSize <= _data.size(), + "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(mappingSize <= _data.size(), - "Trade::SceneData: offset-only mapping data of field" << i << "span" << mappingSize << "bytes but passed data array has only" << _data.size(), ); 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 { - const void* const mappingBegin = field._mappingData.pointer; + /* If a field has an implicit mapping, we allow it to be + nullptr */ + if(!(field._flags >= SceneFieldFlag::ImplicitMapping && !field._mappingData.pointer)) { + const void* const mappingBegin = field._mappingData.pointer; + const void* const mappingEnd = static_cast(field._mappingData.pointer) + (field._size - 1)*field._mappingStride + mappingTypeSize; + CORRADE_ASSERT(mappingBegin >= _data.begin() && mappingEnd <= _data.end(), + "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 mappingEnd = static_cast(field._mappingData.pointer) + (field._size - 1)*field._mappingStride + mappingTypeSize; const void* const fieldEnd = static_cast(field._fieldData.pointer) + (field._size - 1)*field._fieldStride + fieldTypeSize; - CORRADE_ASSERT(mappingBegin >= _data.begin() && mappingEnd <= _data.end(), - "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 << "]", ); 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 << "]", ); } @@ -806,6 +817,12 @@ Containers::ArrayView SceneData::mutableData() & { Containers::StridedArrayView1D SceneData::fieldDataMappingViewInternal(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 with implicit mapping, ignore the + offset/stride and always assume it's not present */ + if(field._flags >= (SceneFieldFlag::OffsetOnly|SceneFieldFlag::ImplicitMapping)) + return {{nullptr, ~std::size_t{}}, size, field._mappingStride}; + return Containers::StridedArrayView1D{ /* We're *sure* the view is correct, so faking the view size */ {static_cast(field._flags & SceneFieldFlag::OffsetOnly ? @@ -1140,6 +1157,18 @@ void SceneData::mappingIntoInternal(const UnsignedInt fieldId, const std::size_t checked by the callers */ const SceneFieldData& field = _fields[fieldId]; + + /* If we don't have any data for an implicit mapping or the implicit + mapping is offset-only (where we always assume there's no data), + generate the sequence */ + if((field._flags >= SceneFieldFlag::ImplicitMapping && !field._mappingData.pointer) || + (field._flags >= (SceneFieldFlag::ImplicitMapping|SceneFieldFlag::OffsetOnly))) + { + for(std::size_t i = 0; i != destination.size(); ++i) + destination[i] = offset + i; + return; + } + const Containers::StridedArrayView1D mappingData = fieldDataMappingViewInternal(field, offset, destination.size()); const auto destination1ui = Containers::arrayCast<2, UnsignedInt>(destination); diff --git a/src/Magnum/Trade/SceneData.h b/src/Magnum/Trade/SceneData.h index f65e29aa7..2b060aa7e 100644 --- a/src/Magnum/Trade/SceneData.h +++ b/src/Magnum/Trade/SceneData.h @@ -566,7 +566,9 @@ enum class SceneFieldFlag: UnsignedByte { * from 0 up to size of the field. A superset of * @ref SceneFieldFlag::OrderedMapping. Object IDs in fields marked with * this flag can be looked up with an @f$ \mathcal{O}(1) @f$ complexity, - * but the field is restricted to exactly one value for each object. + * but the field is restricted to exactly one value for each object. If + * this flag is set, the object mapping view is allowed to be + * @cpp nullptr @ce. * * Note that validity of the object mapping data isn't checked in any way * and if the data doesn't correspond to rules of the flag, queries such @@ -647,7 +649,18 @@ In some cases the object mapping is even implicit, i.e. the first entry of the field specifying data for object @cpp 0 @ce, second entry for object @cpp 1 @ce, third for object @cpp 2 @ce and so on. You can annotate such fields with @ref SceneFieldFlag::ImplicitMapping, which is a superset of -@relativeref{SceneFieldFlag,OrderedMapping}. +@relativeref{SceneFieldFlag,OrderedMapping}. Furthermore, to avoid having to +generate such mapping data, the mapping view can be @cpp nullptr @ce if this +flag is present. The view however still needs to have a size matching the field +data size and the same @ref SceneMappingType as other fields passed to the +@link SceneData @endlink: + +@snippet MagnumTrade.cpp SceneFieldData-usage-implicit-mapping + +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. */ class MAGNUM_TRADE_EXPORT SceneFieldData { public: @@ -675,6 +688,14 @@ class MAGNUM_TRADE_EXPORT SceneFieldData { * Expects that @p mappingData and @p fieldData have the same size, * @p fieldType corresponds to @p name and @p fieldArraySize is zero * for builtin fields. + * + * If @p flags contain @ref SceneFieldFlag::ImplicitMapping, the + * @p mappingData can be a @cpp nullptr @ce view (although it still has + * to follow other constraints regarding size and type). While + * @ref SceneData::mapping() will return it as-is, + * @relativeref{SceneData,mappingAsArray()} and + * @relativeref{SceneData,mappingInto()} functions will generate its + * contents on-the-fly. */ constexpr explicit SceneFieldData(SceneField name, SceneMappingType mappingType, const Containers::StridedArrayView1D& mappingData, SceneFieldType fieldType, const Containers::StridedArrayView1D& fieldData, UnsignedShort fieldArraySize = 0, SceneFieldFlags flags = {}) noexcept; @@ -699,6 +720,14 @@ class MAGNUM_TRADE_EXPORT SceneFieldData { * @p fieldData is contiguous and its size matches @p fieldType and * @p fieldArraySize and that @p fieldType corresponds to @p name and * @p fieldArraySize is zero for builtin attributes. + * + * If @p flags contain @ref SceneFieldFlag::ImplicitMapping, the + * @p mappingData can be a @cpp nullptr @ce view (although it still has + * to follow other constraints regarding size and type). While + * @ref SceneData::mapping() will return it as-is, + * @relativeref{SceneData,mappingAsArray()} and + * @relativeref{SceneData,mappingInto()} functions will generate its + * contents on-the-fly. */ explicit SceneFieldData(SceneField name, const Containers::StridedArrayView2D& mappingData, SceneFieldType fieldType, const Containers::StridedArrayView2D& fieldData, UnsignedShort fieldArraySize = 0, SceneFieldFlags flags = {}) noexcept; @@ -779,6 +808,15 @@ class MAGNUM_TRADE_EXPORT SceneFieldData { * @p fieldType / @p fieldArraySize checks against @p fieldStride can * be done. You're encouraged to use the @ref SceneFieldData(SceneField, SceneMappingType, const Containers::StridedArrayView1D&, SceneFieldType, const Containers::StridedArrayView1D&, UnsignedShort, SceneFieldFlags) * constructor if you want additional safeguards. + * + * If @p flags contain @ref SceneFieldFlag::ImplicitMapping, the + * @p mappingOffset and @p mappingStride fields are ignored and the + * object mapping is assumed to not be present (however you still have + * to follow constraints regarding its type). The + * @ref SceneData::mapping() will then return a @cpp nullptr @ce view, + * and the @relativeref{SceneData,mappingAsArray()} and + * @relativeref{SceneData,mappingInto()} functions will generate its + * contents on-the-fly. * @see @ref flags(), @ref fieldArraySize(), * @ref mappingData(Containers::ArrayView) const, * @ref fieldData(Containers::ArrayView) const @@ -1621,6 +1659,10 @@ class MAGNUM_TRADE_EXPORT SceneData { * to @ref SceneMappingType size) and is guaranteed to be contiguous. * Use the templated overload below to get the mapping in a concrete * type. + * + * If the field has @ref SceneFieldFlag::ImplicitMapping set and no + * data was supplied for it or it's @ref SceneFieldFlag::OffsetOnly, + * the returned view will be correctly sized but @cpp nullptr @ce. * @see @ref mutableMapping(UnsignedInt), * @ref Corrade::Containers::StridedArrayView::isContiguous(), * @ref sceneMappingTypeSize() @@ -1644,6 +1686,10 @@ class MAGNUM_TRADE_EXPORT SceneData { * The @p fieldId is expected to be smaller than @ref fieldCount() and * @p T is expected to correspond to @ref mappingType(). * + * If the field has @ref SceneFieldFlag::ImplicitMapping set and either + * no data was supplied for it or it's @ref SceneFieldFlag::OffsetOnly, + * the returned view will be correctly sized but @cpp nullptr @ce. + * * You can also use the non-templated @ref mappingAsArray() accessor * (or the combined @ref parentsAsArray(), * @ref transformations2DAsArray(), @ref transformations3DAsArray(), @@ -1677,6 +1723,10 @@ class MAGNUM_TRADE_EXPORT SceneData { * @ref SceneMappingType size) and is guaranteed to be contiguous. Use * the templated overload below to get the object mapping in a concrete * type. + * + * If the field has @ref SceneFieldFlag::ImplicitMapping set and either + * no data was supplied for it or it's @ref SceneFieldFlag::OffsetOnly, + * the returned view will be correctly sized but @cpp nullptr @ce. * @see @ref hasField(), @ref mapping(UnsignedInt) const, * @ref mutableMapping(SceneField), * @ref Corrade::Containers::StridedArrayView::isContiguous() @@ -1700,6 +1750,10 @@ class MAGNUM_TRADE_EXPORT SceneData { * The @p fieldName is expected to exist and @p T is expected to * correspond to @ref mappingType(). * + * If the field has @ref SceneFieldFlag::ImplicitMapping set and either + * no data was supplied for it or it's @ref SceneFieldFlag::OffsetOnly, + * the returned view will be correctly sized but @cpp nullptr @ce. + * * You can also use the non-templated @ref mappingAsArray() accessor * (or the combined @ref parentsAsArray(), * @ref transformations2DAsArray(), @ref transformations3DAsArray(), @@ -1896,6 +1950,10 @@ class MAGNUM_TRADE_EXPORT SceneData { * arbitrary underlying type and returns it in a newly-allocated array. * The @p fieldId is expected to be smaller than @ref fieldCount(). * + * If the field has @ref SceneFieldFlag::ImplicitMapping set and either + * no data was supplied for it or it's @ref SceneFieldFlag::OffsetOnly, + * the data will be generated on-the-fly. + * * Note that, for common fields, you can also use the * @ref parentsAsArray(), @ref transformations2DAsArray(), * @ref transformations3DAsArray(), diff --git a/src/Magnum/Trade/Test/SceneDataTest.cpp b/src/Magnum/Trade/Test/SceneDataTest.cpp index b6bcbc7ce..92a70e11d 100644 --- a/src/Magnum/Trade/Test/SceneDataTest.cpp +++ b/src/Magnum/Trade/Test/SceneDataTest.cpp @@ -196,6 +196,8 @@ struct SceneDataTest: TestSuite::Tester { void fieldForFieldMissing(); void findFieldObjectOffsetInvalidObject(); + template void implicitNullMapping(); + void releaseFieldData(); void releaseData(); }; @@ -562,6 +564,11 @@ SceneDataTest::SceneDataTest() { addTests({&SceneDataTest::fieldForFieldMissing, &SceneDataTest::findFieldObjectOffsetInvalidObject, + &SceneDataTest::implicitNullMapping, + &SceneDataTest::implicitNullMapping, + &SceneDataTest::implicitNullMapping, + &SceneDataTest::implicitNullMapping, + &SceneDataTest::releaseFieldData, &SceneDataTest::releaseData}); } @@ -5552,6 +5559,149 @@ void SceneDataTest::findFieldObjectOffsetInvalidObject() { "Trade::SceneData::skinsFor(): object 7 out of bounds for 7 objects\n"); } +template void SceneDataTest::implicitNullMapping() { + setTestCaseTemplateName(NameTraits::name()); + + struct Field { + UnsignedByte id; + /* Mapping second so it isn't at offset 0, implying something weird */ + T mapping; + } data[]{ + {1, 0}, + {7, 1}, + {22, 2}, + {15, 3}, + {3, 5}, /* this is to know whether we got our or generated data */ + }; + + Containers::StridedArrayView1D view = data; + + SceneData scene{Implementation::sceneMappingTypeFor(), 6, {}, data, { + /* Implicit mapping, with data supplied */ + SceneFieldData{SceneField::Mesh, view.slice(&Field::mapping), view.slice(&Field::id), SceneFieldFlag::ImplicitMapping}, + /* Implicit mapping, with no data */ + SceneFieldData{SceneField::Camera, Containers::ArrayView{nullptr, Containers::arraySize(data)}, view.slice(&Field::id), SceneFieldFlag::ImplicitMapping}, + /* Implicit mapping offset-only, pointing to the data. This gets + ignored because there's no non-shitty non-magic-constants way to + know if the offset is valid. */ + SceneFieldData{SceneField::Light, Containers::arraySize(data), Implementation::sceneMappingTypeFor(), offsetof(Field, mapping), sizeof(Field), SceneFieldType::UnsignedByte, offsetof(Field, id), sizeof(Field), SceneFieldFlag::ImplicitMapping}, + /* Implicit mapping offset-only, pointing to wherever. Abusing a Parent + field and faking it to be signed because Skin needs some + transformation field as well */ + SceneFieldData{SceneField::Parent, Containers::arraySize(data), Implementation::sceneMappingTypeFor(), 666666666, 0, SceneFieldType::Byte, offsetof(Field, id), sizeof(Field), SceneFieldFlag::ImplicitMapping} + }}; + + /* Only the non-null view will be non-null, the offset-only will all be + null also */ + CORRADE_COMPARE(scene.mapping(0).data(), &data[0].mapping); + CORRADE_COMPARE(scene.mapping(1).data(), nullptr); + CORRADE_COMPARE(scene.mapping(2).data(), nullptr); + CORRADE_COMPARE(scene.mapping(3).data(), nullptr); + + /* If the view is not nullptr, it'll use the data and won't generate */ + { + CORRADE_COMPARE_AS(scene.meshesMaterialsAsArray(), (Containers::arrayView>>({ + {0, {1, -1}}, + {1, {7, -1}}, + {2, {22, -1}}, + {3, {15, -1}}, + {5, {3, -1}}, /* this would be 4 if generated */ + })), TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.meshesMaterialsFor(3), (Containers::arrayView>({ + {15, -1} + })), TestSuite::Compare::Container); + } { + UnsignedInt mapping[5]; + scene.meshesMaterialsInto(mapping, nullptr, nullptr); + CORRADE_COMPARE_AS(Containers::arrayView(mapping), Containers::arrayView({ + 0, 1, 2, 3, 5 /* this would be 4 if generated */ + }), TestSuite::Compare::Container); + } { + UnsignedInt mapping[3]; + scene.meshesMaterialsInto(2, mapping, nullptr, nullptr); + CORRADE_COMPARE_AS(Containers::arrayView(mapping), Containers::arrayView({ + 2, 3, 5 /* this would be 4 if generated */ + }), TestSuite::Compare::Container); + } + + /* If the view is nullptr, it'll generate the data */ + { + CORRADE_COMPARE_AS(scene.camerasAsArray(), (Containers::arrayView>({ + {0, 1}, + {1, 7}, + {2, 22}, + {3, 15}, + {4, 3}, + })), TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.camerasFor(3), Containers::arrayView({ + 15 + }), TestSuite::Compare::Container); + } { + UnsignedInt mapping[5]; + scene.camerasInto(mapping, nullptr); + CORRADE_COMPARE_AS(Containers::arrayView(mapping), Containers::arrayView({ + 0, 1, 2, 3, 4 + }), TestSuite::Compare::Container); + } { + UnsignedInt mapping[3]; + scene.camerasInto(2, mapping, nullptr); + CORRADE_COMPARE_AS(Containers::arrayView(mapping), Containers::arrayView({ + 2, 3, 4 + }), TestSuite::Compare::Container); + } + + /* For an offset-only implicit mapping it'll generate the data always, even + if the mapping is seemingly there */ + { + CORRADE_COMPARE_AS(scene.lightsAsArray(), (Containers::arrayView>({ + {0, 1}, + {1, 7}, + {2, 22}, + {3, 15}, + {4, 3}, + })), TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.lightsFor(3), Containers::arrayView({ + 15 + }), TestSuite::Compare::Container); + } { + UnsignedInt mapping[5]; + scene.lightsInto(mapping, nullptr); + CORRADE_COMPARE_AS(Containers::arrayView(mapping), Containers::arrayView({ + 0, 1, 2, 3, 4 + }), TestSuite::Compare::Container); + } { + UnsignedInt mapping[3]; + scene.lightsInto(2, mapping, nullptr); + CORRADE_COMPARE_AS(Containers::arrayView(mapping), Containers::arrayView({ + 2, 3, 4 + }), TestSuite::Compare::Container); + } + + /* And if the offset is weirdly off it won't blow up on that */ + { + CORRADE_COMPARE_AS(scene.parentsAsArray(), (Containers::arrayView>({ + {0, 1}, + {1, 7}, + {2, 22}, + {3, 15}, + {4, 3}, + })), TestSuite::Compare::Container); + CORRADE_COMPARE(scene.parentFor(3), 15); + } { + UnsignedInt mapping[5]; + scene.parentsInto(mapping, nullptr); + CORRADE_COMPARE_AS(Containers::arrayView(mapping), Containers::arrayView({ + 0, 1, 2, 3, 4 + }), TestSuite::Compare::Container); + } { + UnsignedInt mapping[3]; + scene.parentsInto(2, mapping, nullptr); + CORRADE_COMPARE_AS(Containers::arrayView(mapping), Containers::arrayView({ + 2, 3, 4 + }), TestSuite::Compare::Container); + } +} + void SceneDataTest::releaseFieldData() { struct Field { UnsignedByte object; From 3e28d8f532049ad56a8f20afae7254117b496032 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 2/3] 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 3ff68fdc2..40649653c 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 fe543a5d5..b9102f4e5 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 2b060aa7e..ee2913781 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: @@ -3113,6 +3135,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: @@ -3121,7 +3147,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()))}, @@ -3150,7 +3179,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 92a70e11d..91d1c7564 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"); @@ -5702,6 +5727,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; From da27aaca3ba30731cbf1ac43d0c60be7648570ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 7 Dec 2021 21:29:21 +0100 Subject: [PATCH 3/3] [wip] the null fields/mappings are a PAIN --- .../convertToSingleFunctionObjects.h | 2 + src/Magnum/SceneTools/Test/CombineTest.cpp | 49 ++++++++++++++++++- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/Magnum/SceneTools/Implementation/convertToSingleFunctionObjects.h b/src/Magnum/SceneTools/Implementation/convertToSingleFunctionObjects.h index 854152f2f..a17c5a505 100644 --- a/src/Magnum/SceneTools/Implementation/convertToSingleFunctionObjects.h +++ b/src/Magnum/SceneTools/Implementation/convertToSingleFunctionObjects.h @@ -154,6 +154,8 @@ inline Trade::SceneData convertToSingleFunctionObjects(const Trade::SceneData& s - fields that don't actually get their object mapping touched during the process (and then all fields that share object mapping with them) */ +#warning removing implicit mapping from here will mean the null will get treated as a placeholder by copy(), not wanted +#warning it needs to restore the field instead } else fields[i] = Trade::SceneFieldData{field.name(), field.mappingType(), field.mappingData(), field.fieldType(), field.fieldData(), field.fieldArraySize(), field.flags() & ~Trade::SceneFieldFlag::ImplicitMapping}; } diff --git a/src/Magnum/SceneTools/Test/CombineTest.cpp b/src/Magnum/SceneTools/Test/CombineTest.cpp index 544cc5b45..2107da2f5 100644 --- a/src/Magnum/SceneTools/Test/CombineTest.cpp +++ b/src/Magnum/SceneTools/Test/CombineTest.cpp @@ -41,6 +41,9 @@ struct CombineTest: TestSuite::Tester { void objectsShared(); void objectsPlaceholderFieldPlaceholder(); void objectSharedFieldPlaceholder(); + + void implicitNullMapping(); + void trivialNullParent(); }; struct { @@ -60,7 +63,10 @@ CombineTest::CombineTest() { addTests({&CombineTest::alignment, &CombineTest::objectsShared, &CombineTest::objectsPlaceholderFieldPlaceholder, - &CombineTest::objectSharedFieldPlaceholder}); + &CombineTest::objectSharedFieldPlaceholder, + + &CombineTest::implicitNullMapping, + &CombineTest::trivialNullParent}); } using namespace Math::Literals; @@ -324,6 +330,47 @@ void CombineTest::objectSharedFieldPlaceholder() { CORRADE_COMPARE(scene.field(Trade::SceneField::MeshMaterial).stride()[0], 4); } +void CombineTest::implicitNullMapping() { + const Short parentFieldData[]{-1, 0, 0}; + const UnsignedByte meshFieldData[]{3, 5}; + + Trade::SceneData scene = Implementation::combine(Trade::SceneMappingType::UnsignedShort, 167, Containers::arrayView({ + /* If the field has any flags, it shouldn't be treated as a + placeholder */ +#warning or maybe it should be preserved? yeah + Trade::SceneFieldData{Trade::SceneField::Mesh, Containers::ArrayView{nullptr, Containers::arraySize(meshFieldData)}, Containers::arrayView(meshFieldData), Trade::SceneFieldFlag::ImplicitMapping}, + Trade::SceneFieldData{Trade::SceneField::Parent, Containers::ArrayView{nullptr, Containers::arraySize(parentFieldData)}, Containers::arrayView(parentFieldData), Trade::SceneFieldFlag::ImplicitMapping} + })); + + CORRADE_COMPARE(scene.mappingBound(), 167); + CORRADE_COMPARE(scene.fieldCount(), 2); + + CORRADE_COMPARE(scene.fieldName(0), Trade::SceneField::Mesh); + CORRADE_COMPARE(scene.fieldFlags(0), Trade::SceneFieldFlag::ImplicitMapping); + CORRADE_COMPARE(scene.fieldType(0), Trade::SceneFieldType::UnsignedByte); + CORRADE_COMPARE(scene.fieldArraySize(0), 0); + CORRADE_COMPARE_AS(scene.mapping(0), Containers::arrayView({ + 0, 1, 2 + }), TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.field(0), + Containers::arrayView(meshFieldData), + TestSuite::Compare::Container); + + CORRADE_COMPARE(scene.fieldName(1), Trade::SceneField::Parent); + CORRADE_COMPARE(scene.fieldFlags(1), Trade::SceneFieldFlag::ImplicitMapping); + CORRADE_COMPARE(scene.fieldType(1), Trade::SceneFieldType::Short); + CORRADE_COMPARE(scene.fieldArraySize(1), 0); + CORRADE_COMPARE_AS(scene.mapping(1), Containers::arrayView({ + 0, 1 + }), TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.field(1), + Containers::arrayView(meshFieldData), + TestSuite::Compare::Container); +} + +void CombineTest::trivialNullParent() { +} + }}}} CORRADE_TEST_MAIN(Magnum::SceneTools::Test::CombineTest)