From 657725c4c0908446c183e96c1c9c1beb97101ea8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 17 Apr 2023 13:36:18 +0200 Subject: [PATCH] Trade: fix & improve SceneData checks for matching field mapping. The assertion message printed being/end range, which was extremely uninformative as it didn't show sizes and strides. That form made sense for reporting if the views weren't contained in the data arrays, but not here. Additionally, the existing assertion didn't check stride, which means that a mapping with 2 items and stride 8 was treated as being equal to a mapping with 4 items and stride 4. On the other hand, it didn't behave correctly for offset-only fields, those were always treated as different from pointer fields even if they were actually matching. --- src/Magnum/Trade/SceneData.cpp | 23 ++-- src/Magnum/Trade/Test/SceneDataTest.cpp | 148 +++++++++++++++++------- 2 files changed, 114 insertions(+), 57 deletions(-) diff --git a/src/Magnum/Trade/SceneData.cpp b/src/Magnum/Trade/SceneData.cpp index 2bea649bb..b1881c0ea 100644 --- a/src/Magnum/Trade/SceneData.cpp +++ b/src/Magnum/Trade/SceneData.cpp @@ -931,19 +931,16 @@ SceneData::SceneData(const SceneMappingType mappingType, const UnsignedLong mapp } #ifndef CORRADE_NO_ASSERT - /* Check that certain fields share the same object mapping. Printing as if - all would be pointers (and not offset-only), it's not worth the extra - effort just for an assert message. Also, compared to above, where - "begin" was always zero, here we're always comparing four values, so the - message for offset-only wouldn't be simpler either. */ - const auto checkFieldMappingDataMatch = [](const SceneFieldData& a, const SceneFieldData& b) { - const std::size_t mappingTypeSize = sceneMappingTypeSize(a.mappingType()); - const void* const aBegin = a._mappingData.pointer; - const void* const bBegin = b._mappingData.pointer; - const void* const aEnd = static_cast(a._mappingData.pointer) + a._size*mappingTypeSize; - const void* const bEnd = static_cast(b._mappingData.pointer) + b._size*mappingTypeSize; - CORRADE_ASSERT(aBegin == bBegin && aEnd == bEnd, - "Trade::SceneData:" << b._name << "mapping data [" << Debug::nospace << bBegin << Debug::nospace << ":" << Debug::nospace << bEnd << Debug::nospace << "] is different from" << a._name << "mapping data [" << Debug::nospace << aBegin << Debug::nospace << ":" << Debug::nospace << aEnd << Debug::nospace << "]", ); + /* Check that certain fields share the same object mapping, i.e. the same + begin, size and stride. Cases where one of the two is offset-only and + the other not are allowed if both have the same absolute begin pointer. */ + const auto checkFieldMappingDataMatch = [this](const SceneFieldData& a, const SceneFieldData& b) { + const void* const aBegin = a._flags & SceneFieldFlag::OffsetOnly ? + _data.begin() + a._mappingData.offset : a._mappingData.pointer; + const void* const bBegin = b._flags & SceneFieldFlag::OffsetOnly ? + _data.begin() + b._mappingData.offset : b._mappingData.pointer; + CORRADE_ASSERT(aBegin == bBegin && a._size == b._size && a._mappingStride == b._mappingStride, + "Trade::SceneData:" << b._name << "mapping data {" << Debug::nospace << bBegin << Debug::nospace << "," << b._size << Debug::nospace << "," << b._mappingStride << Debug::nospace << "} is different from" << a._name << "mapping data {" << Debug::nospace << aBegin << Debug::nospace << "," << a._size << Debug::nospace << "," << a._mappingStride << Debug::nospace << "}", ); }; /* All present TRS fields should share the same object mapping */ diff --git a/src/Magnum/Trade/Test/SceneDataTest.cpp b/src/Magnum/Trade/Test/SceneDataTest.cpp index e51e7de4e..3d86211e1 100644 --- a/src/Magnum/Trade/Test/SceneDataTest.cpp +++ b/src/Magnum/Trade/Test/SceneDataTest.cpp @@ -32,9 +32,11 @@ #include #include #include +#include /** @todo drop once Debug is stream-free */ #include #include #include +#include #include #include #include @@ -3894,43 +3896,102 @@ void SceneDataTest::constructMismatchedTRSViews() { CORRADE_SKIP_IF_NO_ASSERT(); Containers::ArrayView data{reinterpret_cast(0xcafe0000), - /* Three entries, each having a 2D TRS and 3 object IDs */ - 3*(24 + 12)}; - Containers::ArrayView translationMappingData{ - reinterpret_cast(data.data()), 3}; + 4 + 3*24 + 3*4}; + Containers::ArrayView mappingData{ + reinterpret_cast(data.data() + 4 + 3*24), 3}; + Containers::ArrayView mappingDifferentPointerData{ + reinterpret_cast(data.data() + 3*24), 3}; + Containers::StridedArrayView1D mappingDifferentStrideData{ + data, reinterpret_cast(data.data() + 4), 3, 8}; Containers::ArrayView translationFieldData{ - reinterpret_cast(data.data() + 0x0c), 3}; - Containers::ArrayView rotationMappingData{ - reinterpret_cast(data.data() + 0x24), 3}; + reinterpret_cast(data.data() + 4), 3}; Containers::ArrayView rotationFieldData{ - reinterpret_cast(data.data() + 0x30), 3}; - Containers::ArrayView scalingMappingData{ - reinterpret_cast(data.data() + 0x48), 3}; + reinterpret_cast(data.data() + 4 + 8), 3}; Containers::ArrayView scalingFieldData{ - reinterpret_cast(data.data() + 0x54), 3}; - - SceneFieldData translations{SceneField::Translation, translationMappingData, translationFieldData}; - SceneFieldData rotationsDifferent{SceneField::Rotation, rotationMappingData, rotationFieldData}; - SceneFieldData scalingsDifferent{SceneField::Scaling, scalingMappingData, scalingFieldData}; - SceneFieldData rotationsSameButLess{SceneField::Rotation, translationMappingData.exceptSuffix(1), rotationFieldData.exceptSuffix(1)}; - SceneFieldData scalingsSameButLess{SceneField::Scaling, translationMappingData.exceptSuffix(2), scalingFieldData.exceptSuffix(2)}; + reinterpret_cast(data.data() + 4 + 12), 3}; + + SceneFieldData translations{SceneField::Translation, mappingData, translationFieldData}; + SceneFieldData rotations{SceneField::Rotation, mappingData, rotationFieldData}; + SceneFieldData scalings{SceneField::Scaling, mappingData, scalingFieldData}; + SceneFieldData rotationsDifferentPointer{SceneField::Rotation, mappingDifferentPointerData, rotationFieldData}; + SceneFieldData scalingsDifferentPointer{SceneField::Scaling, mappingDifferentPointerData, scalingFieldData}; + SceneFieldData rotationsDifferentSize{SceneField::Rotation, mappingData.exceptSuffix(1), rotationFieldData.exceptSuffix(1)}; + SceneFieldData scalingsDifferentSize{SceneField::Scaling, mappingData.exceptSuffix(1), scalingFieldData.exceptSuffix(1)}; + SceneFieldData rotationsDifferentStride{SceneField::Rotation, mappingDifferentStrideData, rotationFieldData}; + SceneFieldData scalingsDifferentStride{SceneField::Scaling, mappingDifferentStrideData, scalingFieldData}; + + SceneFieldData translationsOffsetOnly{SceneField::Translation, 3, + SceneMappingType::UnsignedInt, 4 + 3*24, sizeof(UnsignedInt), + SceneFieldType::Vector2, 4, sizeof(Vector2)}; + SceneFieldData rotationsOffsetOnly{SceneField::Rotation, 3, + SceneMappingType::UnsignedInt, 4 + 3*24, sizeof(UnsignedInt), + SceneFieldType::Complex, 4, sizeof(Complex)}; + SceneFieldData scalingsOffsetOnly{SceneField::Scaling, 3, + SceneMappingType::UnsignedInt, 4 + 3*24, sizeof(UnsignedInt), + SceneFieldType::Vector2, 4, sizeof(Vector2)}; + SceneFieldData rotationsDifferentPointerOffsetOnly{SceneField::Rotation, 3, + SceneMappingType::UnsignedInt, 3*24, sizeof(UnsignedInt), + SceneFieldType::Complex, 4, sizeof(Complex)}; + SceneFieldData scalingsDifferentPointerOffsetOnly{SceneField::Scaling, 3, + SceneMappingType::UnsignedInt, 3*24, sizeof(UnsignedInt), + SceneFieldType::Vector2, 4, sizeof(Vector2)}; + + /* The matching offset-only variants should give no assert */ + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translations, rotationsOffsetOnly}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translationsOffsetOnly, rotations}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translations, scalingsOffsetOnly}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translationsOffsetOnly, scalings}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {rotations, scalingsOffsetOnly}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {rotationsOffsetOnly, scalings}}; /* Test that all pairs get checked */ std::ostringstream out; Error redirectError{&out}; - SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translations, rotationsDifferent}}; - SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translations, scalingsDifferent}}; - SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {rotationsDifferent, scalingsDifferent}}; - SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translations, rotationsSameButLess}}; - SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translations, scalingsSameButLess}}; - SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {rotationsSameButLess, scalingsSameButLess}}; - CORRADE_COMPARE(out.str(), - "Trade::SceneData: Trade::SceneField::Rotation mapping data [0xcafe0024:0xcafe0030] is different from Trade::SceneField::Translation mapping data [0xcafe0000:0xcafe000c]\n" - "Trade::SceneData: Trade::SceneField::Scaling mapping data [0xcafe0048:0xcafe0054] is different from Trade::SceneField::Translation mapping data [0xcafe0000:0xcafe000c]\n" - "Trade::SceneData: Trade::SceneField::Scaling mapping data [0xcafe0048:0xcafe0054] is different from Trade::SceneField::Rotation mapping data [0xcafe0024:0xcafe0030]\n" - "Trade::SceneData: Trade::SceneField::Rotation mapping data [0xcafe0000:0xcafe0008] is different from Trade::SceneField::Translation mapping data [0xcafe0000:0xcafe000c]\n" - "Trade::SceneData: Trade::SceneField::Scaling mapping data [0xcafe0000:0xcafe0004] is different from Trade::SceneField::Translation mapping data [0xcafe0000:0xcafe000c]\n" - "Trade::SceneData: Trade::SceneField::Scaling mapping data [0xcafe0000:0xcafe0004] is different from Trade::SceneField::Rotation mapping data [0xcafe0000:0xcafe0008]\n"); + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translations, rotationsDifferentPointer}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translations, rotationsDifferentPointerOffsetOnly}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translationsOffsetOnly, rotationsDifferentPointer}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translations, rotationsDifferentSize}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translations, rotationsDifferentStride}}; + + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translations, scalingsDifferentPointer}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translations, scalingsDifferentPointerOffsetOnly}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translationsOffsetOnly, scalingsDifferentPointer}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translations, scalingsDifferentSize}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {translations, scalingsDifferentStride}}; + + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {rotations, scalingsDifferentPointer}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {rotations, scalingsDifferentPointerOffsetOnly}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {rotationsOffsetOnly, scalingsDifferentPointer}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {rotations, scalingsDifferentSize}}; + SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {rotations, scalingsDifferentStride}}; + CORRADE_COMPARE_AS(out.str(), + /* Different pointer, three variants with offset-only */ + "Trade::SceneData: Trade::SceneField::Rotation mapping data {0xcafe0048, 3, 4} is different from Trade::SceneField::Translation mapping data {0xcafe004c, 3, 4}\n" + "Trade::SceneData: Trade::SceneField::Rotation mapping data {0xcafe0048, 3, 4} is different from Trade::SceneField::Translation mapping data {0xcafe004c, 3, 4}\n" + "Trade::SceneData: Trade::SceneField::Rotation mapping data {0xcafe0048, 3, 4} is different from Trade::SceneField::Translation mapping data {0xcafe004c, 3, 4}\n" + /* Different size */ + "Trade::SceneData: Trade::SceneField::Rotation mapping data {0xcafe004c, 2, 4} is different from Trade::SceneField::Translation mapping data {0xcafe004c, 3, 4}\n" + /* Different stride */ + "Trade::SceneData: Trade::SceneField::Rotation mapping data {0xcafe0004, 3, 8} is different from Trade::SceneField::Translation mapping data {0xcafe004c, 3, 4}\n" + + /* Different pointer, three variants with offset-only */ + "Trade::SceneData: Trade::SceneField::Scaling mapping data {0xcafe0048, 3, 4} is different from Trade::SceneField::Translation mapping data {0xcafe004c, 3, 4}\n" + "Trade::SceneData: Trade::SceneField::Scaling mapping data {0xcafe0048, 3, 4} is different from Trade::SceneField::Translation mapping data {0xcafe004c, 3, 4}\n" + "Trade::SceneData: Trade::SceneField::Scaling mapping data {0xcafe0048, 3, 4} is different from Trade::SceneField::Translation mapping data {0xcafe004c, 3, 4}\n" + /* Different size */ + "Trade::SceneData: Trade::SceneField::Scaling mapping data {0xcafe004c, 2, 4} is different from Trade::SceneField::Translation mapping data {0xcafe004c, 3, 4}\n" + /* Different stride */ + "Trade::SceneData: Trade::SceneField::Scaling mapping data {0xcafe0004, 3, 8} is different from Trade::SceneField::Translation mapping data {0xcafe004c, 3, 4}\n" + + /* Different pointer, three variants with offset-only */ + "Trade::SceneData: Trade::SceneField::Scaling mapping data {0xcafe0048, 3, 4} is different from Trade::SceneField::Rotation mapping data {0xcafe004c, 3, 4}\n" + "Trade::SceneData: Trade::SceneField::Scaling mapping data {0xcafe0048, 3, 4} is different from Trade::SceneField::Rotation mapping data {0xcafe004c, 3, 4}\n" + "Trade::SceneData: Trade::SceneField::Scaling mapping data {0xcafe0048, 3, 4} is different from Trade::SceneField::Rotation mapping data {0xcafe004c, 3, 4}\n" + /* Different size */ + "Trade::SceneData: Trade::SceneField::Scaling mapping data {0xcafe004c, 2, 4} is different from Trade::SceneField::Rotation mapping data {0xcafe004c, 3, 4}\n" + /* Different stride */ + "Trade::SceneData: Trade::SceneField::Scaling mapping data {0xcafe0004, 3, 8} is different from Trade::SceneField::Rotation mapping data {0xcafe004c, 3, 4}\n", + TestSuite::Compare::String); } template struct NameTraits; @@ -4060,29 +4121,28 @@ template void SceneDataTest::constructMismatchedTRSDimensionality() { void SceneDataTest::constructMismatchedMeshMaterialView() { CORRADE_SKIP_IF_NO_ASSERT(); + /* Different sizes, strides and offset-only field handling tested + thoroughly in constructMismatchedTRSViews() already */ + Containers::ArrayView data{reinterpret_cast(0xcafe0000), - /* Three entries, each having mesh/material ID and 2 object IDs */ - 3*(8 + 8)}; - Containers::ArrayView meshMappingData{ - reinterpret_cast(data.data()), 3}; + 4 + 3*8 + 3*4}; + Containers::ArrayView mappingData{ + reinterpret_cast(data.data() + 4 + 3*8), 3}; + Containers::ArrayView mappingDifferentPointerData{ + reinterpret_cast(data.data() + 3*8), 3}; Containers::ArrayView meshFieldData{ - reinterpret_cast(data.data() + 0x0c), 3}; - Containers::ArrayView meshMaterialMappingData{ - reinterpret_cast(data.data() + 0x18), 3}; + reinterpret_cast(data.data() + 4), 3}; Containers::ArrayView meshMaterialFieldData{ - reinterpret_cast(data.data() + 0x24), 3}; + reinterpret_cast(data.data() + 4 + 3*4), 3}; - SceneFieldData meshes{SceneField::Mesh, meshMappingData, meshFieldData}; - SceneFieldData meshMaterialsDifferent{SceneField::MeshMaterial, meshMaterialMappingData, meshMaterialFieldData}; - SceneFieldData meshMaterialsSameButLess{SceneField::MeshMaterial, meshMappingData.exceptSuffix(1), meshMaterialFieldData.exceptSuffix(1)}; + SceneFieldData meshes{SceneField::Mesh, mappingData, meshFieldData}; + SceneFieldData meshMaterialsDifferent{SceneField::MeshMaterial, mappingDifferentPointerData, meshMaterialFieldData}; std::ostringstream out; Error redirectError{&out}; SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {meshes, meshMaterialsDifferent}}; - SceneData{SceneMappingType::UnsignedInt, 3, {}, data, {meshes, meshMaterialsSameButLess}}; CORRADE_COMPARE(out.str(), - "Trade::SceneData: Trade::SceneField::MeshMaterial mapping data [0xcafe0018:0xcafe0024] is different from Trade::SceneField::Mesh mapping data [0xcafe0000:0xcafe000c]\n" - "Trade::SceneData: Trade::SceneField::MeshMaterial mapping data [0xcafe0000:0xcafe0008] is different from Trade::SceneField::Mesh mapping data [0xcafe0000:0xcafe000c]\n"); + "Trade::SceneData: Trade::SceneField::MeshMaterial mapping data {0xcafe0018, 3, 4} is different from Trade::SceneField::Mesh mapping data {0xcafe001c, 3, 4}\n"); } void SceneDataTest::constructAmbiguousSkinDimensions() {