From 2b409ee0bad4205bb1227196eabcad510f4554f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 22 Nov 2021 12:47:16 +0100 Subject: [PATCH] Trade: drop assertions for 64-bit object/parent values from SceneData. This has to be solved on a more systematic level, perhaps even by switching all types to be 64-bit. In the following commit all *AsArray() and *Int() functions will output the object IDs as well, meaning this would need to be handled in each and every API, which is a huge maintenance burden. As it's very unlikely that there actually will *ever* be >4G objects, one possible option would be to introduce some "object ID hash" field that would provide (contiguous?) remapping of the object ID to 32-bit values, and the Into() and AsArray() accessors would return this remapping instead of the original. But then again it'd cause issues with for example animation references that would still reference the original 64-bit value. --- src/Magnum/Trade/SceneData.cpp | 2 - src/Magnum/Trade/SceneData.h | 28 +--------- src/Magnum/Trade/Test/SceneDataTest.cpp | 68 +------------------------ 3 files changed, 4 insertions(+), 94 deletions(-) diff --git a/src/Magnum/Trade/SceneData.cpp b/src/Magnum/Trade/SceneData.cpp index de9d247c3..4b1c6b34e 100644 --- a/src/Magnum/Trade/SceneData.cpp +++ b/src/Magnum/Trade/SceneData.cpp @@ -1088,7 +1088,6 @@ void SceneData::objectsIntoInternal(const UnsignedInt fieldId, const std::size_t else if(field._objectType == SceneObjectType::UnsignedByte) Math::castInto(Containers::arrayCast<2, const UnsignedByte>(objectData, 1), destination1ui); else if(field._objectType == SceneObjectType::UnsignedLong) { - CORRADE_ASSERT(_objectCount <= 0xffffffffull, "Trade::SceneData::objectsInto(): indices for up to" << _objectCount << "objects can't fit into a 32-bit type, access them directly via objects() instead", ); Math::castInto(Containers::arrayCast<2, const UnsignedLong>(objectData, 1), destination1ui); } else CORRADE_INTERNAL_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */ } @@ -1159,7 +1158,6 @@ void SceneData::parentsIntoInternal(const UnsignedInt fieldId, const std::size_t else if(field._fieldType == SceneFieldType::Byte) Math::castInto(Containers::arrayCast<2, const Byte>(fieldData, 1), destination1i); else if(field._fieldType == SceneFieldType::Long) { - CORRADE_ASSERT(field._size <= 0xffffffffull, "Trade::SceneData::parentsInto(): parent indices for up to" << field._size << "objects can't fit into a 32-bit type, access them directly via field() instead", ); Math::castInto(Containers::arrayCast<2, const Long>(fieldData, 1), destination1i); } else CORRADE_INTERNAL_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */ } diff --git a/src/Magnum/Trade/SceneData.h b/src/Magnum/Trade/SceneData.h index 9bb087d17..8e077cb54 100644 --- a/src/Magnum/Trade/SceneData.h +++ b/src/Magnum/Trade/SceneData.h @@ -60,16 +60,7 @@ enum class SceneObjectType: UnsignedByte { UnsignedByte = 1, /**< @relativeref{Magnum,UnsignedByte} */ UnsignedShort, /**< @relativeref{Magnum,UnsignedShort} */ UnsignedInt, /**< @relativeref{Magnum,UnsignedInt} */ - - /** - * @relativeref{Magnum,UnsignedLong}. Meant to be used only in rare cases - * for *really huge* scenes. If this type is used and - * @ref SceneData::objectCount() is larger than the max representable - * 32-bit value, the indices can't be retrieved using - * @ref SceneData::objectsAsArray(), but only using appropriately typed - * @ref SceneData::objects(). - */ - UnsignedLong + UnsignedLong /**< @relativeref{Magnum,UnsignedLong} */ }; /** @@ -109,7 +100,7 @@ enum class SceneField: UnsignedInt { /** * Parent index. Type is usually @ref SceneFieldType::Int, but can be also * any of @relativeref{SceneFieldType,Byte}, - * @relativeref{SceneFieldType,Short} or, rarely, a + * @relativeref{SceneFieldType,Short} or a * @relativeref{SceneFieldType,Long}. A value of @cpp -1 @ce means there's * 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 @@ -1483,11 +1474,6 @@ class MAGNUM_TRADE_EXPORT SceneData { * @ref objects(UnsignedInt) const that converts the field from an * arbitrary underlying type and returns it in a newly-allocated array. * The @p fieldId is expected to be smaller than @ref fieldCount(). - * @attention In the rare case when @ref objectType() is - * @ref SceneObjectType::UnsignedLong and @ref objectCount() is - * larger than the max representable 32-bit value, this function - * can't be used, only an appropriately typed - * @ref objects(UnsignedInt) const. * @see @ref objectsInto(UnsignedInt, const Containers::StridedArrayView1D&) const */ Containers::Array objectsAsArray(UnsignedInt fieldId) const; @@ -1525,11 +1511,6 @@ class MAGNUM_TRADE_EXPORT SceneData { * @ref objects(SceneField) const that converts the field from an * arbitrary underlying type and returns it in a newly-allocated array. * The @p fieldName is expected to exist. - * @attention In the rare case when @ref objectType() is - * @ref SceneObjectType::UnsignedLong and @ref objectCount() is - * larger than the max representable 32-bit value, this function - * can't be used, only an appropriately typed - * @ref objects(SceneField) const. * @see @ref objectsInto(SceneField, const Containers::StridedArrayView1D&) const, * @ref hasField() */ @@ -1568,11 +1549,6 @@ class MAGNUM_TRADE_EXPORT SceneData { * @ref SceneField::Parent as the argument that converts the field from * an arbitrary underlying type and returns it in a newly-allocated * array. The field is expected to exist. - * @attention In the rare case when @ref fieldType(SceneField) const is - * @ref SceneFieldType::Long and @ref fieldSize(SceneField) const - * is larger than the max representable 32-bit value, this - * function can't be used, only an appropriately typed - * @ref field(SceneField) const. * @see @ref parentsInto(), @ref hasField(), @ref parentFor(), * @ref childrenFor() */ diff --git a/src/Magnum/Trade/Test/SceneDataTest.cpp b/src/Magnum/Trade/Test/SceneDataTest.cpp index ef0e6a78e..f0af111d2 100644 --- a/src/Magnum/Trade/Test/SceneDataTest.cpp +++ b/src/Magnum/Trade/Test/SceneDataTest.cpp @@ -119,14 +119,10 @@ struct SceneDataTest: TestSuite::Tester { template void objectsAsArrayByIndex(); template void objectsAsArrayByName(); - void objectsAsArrayLongType(); void objectsIntoArrayByIndex(); void objectsIntoArrayByName(); void objectsIntoArrayInvalidSizeOrOffset(); template void parentsAsArray(); - #ifndef CORRADE_TARGET_32BIT - void parentsAsArrayLongType(); - #endif void parentsIntoArray(); void parentsIntoArrayInvalidSizeOrOffset(); template void transformations2DAsArray(); @@ -316,8 +312,7 @@ SceneDataTest::SceneDataTest() { &SceneDataTest::objectsAsArrayByName, &SceneDataTest::objectsAsArrayByName, &SceneDataTest::objectsAsArrayByName, - &SceneDataTest::objectsAsArrayByName, - &SceneDataTest::objectsAsArrayLongType}); + &SceneDataTest::objectsAsArrayByName}); addInstancedTests({&SceneDataTest::objectsIntoArrayByIndex, &SceneDataTest::objectsIntoArrayByName}, @@ -327,11 +322,7 @@ SceneDataTest::SceneDataTest() { &SceneDataTest::parentsAsArray, &SceneDataTest::parentsAsArray, &SceneDataTest::parentsAsArray, - &SceneDataTest::parentsAsArray, - #ifndef CORRADE_TARGET_32BIT - &SceneDataTest::parentsAsArrayLongType, - #endif - }); + &SceneDataTest::parentsAsArray}); addInstancedTests({&SceneDataTest::parentsIntoArray}, Containers::arraySize(IntoArrayOffsetData)); @@ -2233,33 +2224,6 @@ template void SceneDataTest::objectsAsArrayByName() { TestSuite::Compare::Container); } -void SceneDataTest::objectsAsArrayLongType() { - #ifdef CORRADE_NO_ASSERT - CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); - #endif - - struct Field { - UnsignedLong object; - UnsignedByte mesh; - } fields[3]{}; - - Containers::StridedArrayView1D view = fields; - - SceneData scene{SceneObjectType::UnsignedLong, 0x100000000ull, {}, fields, { - SceneFieldData{SceneField::Mesh, view.slice(&Field::object), view.slice(&Field::mesh)} - }}; - - /* AsArray calls into IntoArray, which then has the assert, so this tests - both */ - std::ostringstream out; - Error redirectError{&out}; - scene.objectsAsArray(0); - scene.objectsAsArray(SceneField::Mesh); - CORRADE_COMPARE(out.str(), - "Trade::SceneData::objectsInto(): indices for up to 4294967296 objects can't fit into a 32-bit type, access them directly via objects() instead\n" - "Trade::SceneData::objectsInto(): indices for up to 4294967296 objects can't fit into a 32-bit type, access them directly via objects() instead\n"); -} - void SceneDataTest::objectsIntoArrayByIndex() { auto&& data = IntoArrayOffsetData[testCaseInstanceId()]; setTestCaseDescription(data.name); @@ -2407,34 +2371,6 @@ template void SceneDataTest::parentsAsArray() { TestSuite::Compare::Container); } -#ifndef CORRADE_TARGET_32BIT -void SceneDataTest::parentsAsArrayLongType() { - #ifdef CORRADE_NO_ASSERT - CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); - #endif - - struct Field { - UnsignedLong object; - Long parent; - }; - - Containers::Array data{nullptr, 0x100000000ull*sizeof(Field), [](char*, std::size_t) {}}; - Containers::StridedArrayView1D view = Containers::arrayCast(data); - - SceneData scene{SceneObjectType::UnsignedLong, 0x100000000ull, std::move(data), { - SceneFieldData{SceneField::Parent, view.slice(&Field::object), view.slice(&Field::parent)} - }}; - - /* AsArray calls into IntoArray, which then has the assert, so this tests - both */ - std::ostringstream out; - Error redirectError{&out}; - scene.parentsAsArray(); - CORRADE_COMPARE(out.str(), - "Trade::SceneData::parentsInto(): parent indices for up to 4294967296 objects can't fit into a 32-bit type, access them directly via field() instead\n"); -} -#endif - void SceneDataTest::parentsIntoArray() { auto&& data = IntoArrayOffsetData[testCaseInstanceId()]; setTestCaseDescription(data.name);