From 67831511cf078cbc916a5c4dbb90d7fa01e0c966 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Thu, 20 Apr 2023 19:21:08 +0200 Subject: [PATCH] SceneTools: handle enforced shared mapping in combineFields(). Without the asserts, it'd blow up only subsequently in the SceneData constructor, printing addresses & strides wildly different from what the input had, causing great confusion. There also needs to be dedicated handling for placeholder mapping views in TRS or mesh/material fields, as simply allocating a new mapping view for each would again trigger an assert in SceneData. --- .../SceneTools/Implementation/combine.h | 79 ++++- src/Magnum/SceneTools/Test/CombineTest.cpp | 275 +++++++++++++++++- .../checkSharedSceneFieldMapping.h | 2 +- 3 files changed, 344 insertions(+), 12 deletions(-) diff --git a/src/Magnum/SceneTools/Implementation/combine.h b/src/Magnum/SceneTools/Implementation/combine.h index 5b2953cf2..2f6d7d52b 100644 --- a/src/Magnum/SceneTools/Implementation/combine.h +++ b/src/Magnum/SceneTools/Implementation/combine.h @@ -37,6 +37,7 @@ #include "Magnum/Math/Functions.h" #include "Magnum/Math/PackingBatch.h" #include "Magnum/Trade/SceneData.h" +#include "Magnum/Trade/Implementation/checkSharedSceneFieldMapping.h" namespace Magnum { namespace SceneTools { namespace Implementation { @@ -111,15 +112,27 @@ template std::size_t stringRangeNullTerminatedFieldSize(const char* str } inline Trade::SceneData combineFields(const Trade::SceneMappingType mappingType, const UnsignedLong mappingBound, const Containers::ArrayView fields) { - const std::size_t mappingTypeSize = sceneMappingTypeSize(mappingType); - const std::size_t mappingTypeAlignment = sceneMappingTypeAlignment(mappingType); + #ifndef CORRADE_NO_ASSERT + /* Offset-only fields are not allowed as there's no data to refer them to. + This has to be checked before shared scene field mapping, otherwise it'd + assert there first, leading to confusion. */ + for(std::size_t i = 0; i != fields.size(); ++i) { + CORRADE_ASSERT(!(fields[i].flags() & Trade::SceneFieldFlag::OffsetOnly), + "SceneTools::combineFields(): field" << i << "is offset-only", (Trade::SceneData{Trade::SceneMappingType::UnsignedInt, 0, nullptr, {}})); + } + #endif + + /* Find fields that have to share the mapping views */ + const Trade::Implementation::SharedSceneFieldIds sharedSceneFieldIds = Trade::Implementation::findSharedSceneFields(fields); + + /* Check that they actually share the same object mapping, i.e. the same + begin, size and stride. As offset-only fields are disallowed, the data + pointer can be whatever, just needs to be large enough. */ + #ifndef CORRADE_NO_ASSERT + if(!checkSharedSceneFieldMapping("SceneTools::combineFields():", sharedSceneFieldIds, {nullptr, ~std::size_t{}}, fields)) + return Trade::SceneData{Trade::SceneMappingType::UnsignedInt, 0, nullptr, {}}; + #endif - /* Track unique mapping views (pointer, size, stride) so fields that shared - a mapping before stay shared after as well. A map is used because - it has conveniently implemented ordering, an unordered_map couldn't be - used without manually implementing a std::tuple hash because STL DOES - NOT HAVE IT, UGH. */ - std::map, UnsignedInt> uniqueMappings; Containers::Array items; Containers::Array> itemViewMappings{NoInit, fields.size()}; @@ -135,11 +148,45 @@ inline Trade::SceneData combineFields(const Trade::SceneMappingType mappingType, Containers::Array itemViews{fields.size()*3}; std::size_t itemViewOffset = 0; + const std::size_t mappingTypeSize = sceneMappingTypeSize(mappingType); + const std::size_t mappingTypeAlignment = sceneMappingTypeAlignment(mappingType); + + /* If any share group has a placeholder view (which thanks to the above + check implies that all present fields in that group), add a mapping view + for it -- it'll get picked up below */ + Containers::Optional sharedTrsMapping; + if(sharedSceneFieldIds.trs[0] != ~UnsignedInt{} && !fields[sharedSceneFieldIds.trs[0]].mappingData().data()) { + sharedTrsMapping = itemViewOffset; + arrayAppend(items, InPlaceInit, + NoInit, + std::size_t(fields[sharedSceneFieldIds.trs[0]].size()), + mappingTypeSize, + mappingTypeAlignment, + itemViews[itemViewOffset].types); + ++itemViewOffset; + } + Containers::Optional sharedMeshMaterialMapping; + if(sharedSceneFieldIds.meshMaterial[0] != ~UnsignedInt{} && !fields[sharedSceneFieldIds.meshMaterial[0]].mappingData().data()) { + sharedMeshMaterialMapping = itemViewOffset; + arrayAppend(items, InPlaceInit, + NoInit, + std::size_t(fields[sharedSceneFieldIds.meshMaterial[0]].size()), + mappingTypeSize, + mappingTypeAlignment, + itemViews[itemViewOffset].types); + ++itemViewOffset; + } + + /* Track unique mapping views (pointer, size, stride) so fields that shared + a mapping before stay shared after as well. A map is used because + it has conveniently implemented ordering, an unordered_map couldn't be + used without manually implementing a std::tuple hash because STL DOES + NOT HAVE IT, UGH. */ + std::map, UnsignedInt> uniqueMappings; + /* Go through all fields and collect ArrayTuple allocations for these */ for(std::size_t i = 0; i != fields.size(); ++i) { const Trade::SceneFieldData& field = fields[i]; - CORRADE_ASSERT(!(field.flags() & Trade::SceneFieldFlag::OffsetOnly), - "SceneTools::combineFields(): field" << i << "is offset-only", (Trade::SceneData{Trade::SceneMappingType::UnsignedInt, 0, nullptr, {}})); /* Mapping data. If the view isn't a placeholder, check if it is shared with an existing view already, and insert it if not. */ @@ -154,6 +201,18 @@ inline Trade::SceneData combineFields(const Trade::SceneMappingType mappingType, if(field.mappingData().data() && !inserted.second) { itemViewMappings[i].first() = inserted.first->second; + /* If it's a placeholder in one of the required-to-be-shared groups, + add a view that was preallocated above */ + } else if(!field.mappingData().data() && + (field.name() == Trade::SceneField::Translation || + field.name() == Trade::SceneField::Rotation || + field.name() == Trade::SceneField::Scaling)) { + itemViewMappings[i].first() = *sharedTrsMapping; + } else if(!field.mappingData().data() && + (field.name() == Trade::SceneField::Mesh || + field.name() == Trade::SceneField::MeshMaterial)) { + itemViewMappings[i].first() = *sharedMeshMaterialMapping; + /* If it's not shared or it's a placeholder, allocate a new mapping view of given size by adding a new item to the list of views to allocate by an ArrayTuple. */ diff --git a/src/Magnum/SceneTools/Test/CombineTest.cpp b/src/Magnum/SceneTools/Test/CombineTest.cpp index 9483f6342..b71559c7d 100644 --- a/src/Magnum/SceneTools/Test/CombineTest.cpp +++ b/src/Magnum/SceneTools/Test/CombineTest.cpp @@ -24,6 +24,8 @@ */ #include +#include +#include #include #include #include @@ -50,7 +52,10 @@ struct CombineTest: TestSuite::Tester { void fieldsMappingSharedPartial(); void fieldsMappingPlaceholderFieldPlaceholder(); void fieldsMappingSharedFieldPlaceholder(); + void fieldsMappingSharedTRSPlaceholder(); + void fieldsMappingSharedMeshMaterialPlaceholder(); + void fieldsSharedMappingExpected(); void fieldsStringPlaceholder(); void fieldsOffsetOnly(); void fieldsFromDataOffsetOnly(); @@ -68,6 +73,82 @@ const struct { {"UnsignedLong output", Trade::SceneMappingType::UnsignedLong}, }; +const struct { + const char* name; + bool translationPresent, rotationPresent, scalingPresent; + /* Either all or none can be placeholders */ + bool placeholders; +} FieldsMappingSharedTRSPlaceholderData[]{ + {"all", + true, true, true, + false}, + {"all, placeholders", + true, true, true, + true}, + {"rotation & scaling", + false, true, true, + false}, + {"rotation & scaling, placeholders", + false, true, true, + true}, + {"translation & scaling", + true, false, true, + false}, + {"translation & scaling, placeholders", + true, false, true, + true}, + {"translation & rotation", + true, true, false, + false}, + {"translation & rotation, placeholders", + true, true, false, + true}, + {"translation", + true, false, false, + false}, + {"translation, placeholder", + true, false, false, + true}, + {"rotation", + false, true, false, + false}, + {"rotation, placeholder", + false, true, false, + true}, + {"scaling", + false, false, true, + false}, + {"scaling, placeholder", + false, false, true, + true}, +}; + +const struct { + const char* name; + bool meshPresent, meshMaterialPresent; + /* Either all or none can be placeholders */ + bool placeholders; +} FieldsMappingSharedMeshMaterialPlaceholderData[]{ + {"both placeholders", + true, true, + false}, + {"no placeholders", + true, true, + true}, + {"just mesh present, placeholder", + true, false, + false}, + {"just mesh present, not a placeholder", + true, false, + true}, + {"just mesh material present, placeholder", + false, true, + false}, + {"just mesh material present, not a placeholder", + false, true, + true}, +}; + CombineTest::CombineTest() { addInstancedTests({&CombineTest::fields}, Containers::arraySize(FieldsData)); @@ -81,8 +162,15 @@ CombineTest::CombineTest() { &CombineTest::fieldsMappingShared, &CombineTest::fieldsMappingSharedPartial, &CombineTest::fieldsMappingPlaceholderFieldPlaceholder, - &CombineTest::fieldsMappingSharedFieldPlaceholder, + &CombineTest::fieldsMappingSharedFieldPlaceholder}); + + addInstancedTests({&CombineTest::fieldsMappingSharedTRSPlaceholder}, + Containers::arraySize(FieldsMappingSharedTRSPlaceholderData)); + addInstancedTests({&CombineTest::fieldsMappingSharedMeshMaterialPlaceholder}, + Containers::arraySize(FieldsMappingSharedMeshMaterialPlaceholderData)); + + addTests({&CombineTest::fieldsSharedMappingExpected, &CombineTest::fieldsStringPlaceholder, &CombineTest::fieldsOffsetOnly, &CombineTest::fieldsFromDataOffsetOnly}); @@ -747,6 +835,191 @@ void CombineTest::fieldsMappingSharedFieldPlaceholder() { CORRADE_COMPARE(scene.field(Trade::SceneField::MeshMaterial).stride()[0], 4); } +void CombineTest::fieldsMappingSharedTRSPlaceholder() { + auto&& data = FieldsMappingSharedTRSPlaceholderData[testCaseInstanceId()]; + setTestCaseDescription(data.name); + + const UnsignedShort mapping[]{13, 31, 67}; + + const Vector2 translationData[]{{1.0f, 2.0f}, {3.0f, 4.0f}, {5.0f, 6.0f}}; + const Complex rotationData[]{Complex::rotation(30.0_degf), Complex::rotation(60.0_degf), Complex::rotation(90.0_degf)}; + const Vector2 scalingData[]{{7.0f, -1.0f}, {8.0f, -2.0f}, {9.0f, -3.0f}}; + const UnsignedByte meshData[]{5, 7, 119}; + + Containers::Array fields; + if(data.translationPresent) arrayAppend(fields, InPlaceInit, + Trade::SceneField::Translation, + data.placeholders ? + Containers::ArrayView{nullptr, 3} : + Containers::arrayView(mapping), + Containers::arrayView(translationData)); + if(data.rotationPresent) arrayAppend(fields, InPlaceInit, + Trade::SceneField::Rotation, + data.placeholders ? + Containers::ArrayView{nullptr, 3} : + Containers::arrayView(mapping), + Containers::arrayView(rotationData)); + + /* Add a placeholder mapping field from another share group with the same + pointer/size/stride to verify they don't get shared by accident; add it + among the other fields to avoid them accidentally being treated as always + together */ + arrayAppend(fields, InPlaceInit, + Trade::SceneField::Mesh, + Containers::ArrayView{nullptr, 3}, + Containers::arrayView(meshData)); + + if(data.scalingPresent) arrayAppend(fields, InPlaceInit, + Trade::SceneField::Scaling, + data.placeholders ? + Containers::ArrayView{nullptr, 3} : + Containers::arrayView(mapping), + Containers::arrayView(scalingData)); + + /* The assertions inside SceneData will verify that the mapping is + shared */ + Trade::SceneData scene = combineFields(Trade::SceneMappingType::UnsignedInt, 173, fields); + + Containers::StridedArrayView1D mappingData; + if(data.translationPresent) { + CORRADE_VERIFY(scene.hasField(Trade::SceneField::Translation)); + CORRADE_COMPARE_AS(scene.field(Trade::SceneField::Translation), + Containers::arrayView(translationData), + TestSuite::Compare::Container); + mappingData = scene.mapping(Trade::SceneField::Translation); + } + if(data.rotationPresent) { + CORRADE_VERIFY(scene.hasField(Trade::SceneField::Rotation)); + CORRADE_COMPARE_AS(scene.field(Trade::SceneField::Rotation), + Containers::arrayView(rotationData), + TestSuite::Compare::Container); + mappingData = scene.mapping(Trade::SceneField::Rotation); + } + if(data.scalingPresent) { + CORRADE_VERIFY(scene.hasField(Trade::SceneField::Scaling)); + CORRADE_COMPARE_AS(scene.field(Trade::SceneField::Scaling), + Containers::arrayView(scalingData), + TestSuite::Compare::Container); + mappingData = scene.mapping(Trade::SceneField::Scaling); + } + + if(!data.placeholders) + CORRADE_COMPARE_AS(mappingData, + Containers::arrayView({13u, 31u, 67u}), + TestSuite::Compare::Container); + + /* The other field should be copied as well, but with its own mapping + data */ + CORRADE_VERIFY(scene.hasField(Trade::SceneField::Mesh)); + CORRADE_VERIFY(scene.mapping(Trade::SceneField::Mesh).data() != mappingData.data()); + CORRADE_COMPARE_AS(scene.field(Trade::SceneField::Mesh), + Containers::arrayView(meshData), + TestSuite::Compare::Container); +} + +void CombineTest::fieldsMappingSharedMeshMaterialPlaceholder() { + auto&& data = FieldsMappingSharedMeshMaterialPlaceholderData[testCaseInstanceId()]; + setTestCaseDescription(data.name); + + const UnsignedShort mapping[]{13, 31, 67}; + + const UnsignedByte meshData[]{5, 7, 119}; + const Int meshMaterialData[]{25, -1, 23}; + + const Vector2 translationData[]{{1.0f, 2.0f}, {3.0f, 4.0f}, {5.0f, 6.0f}}; + + Containers::Array fields; + if(data.meshPresent) arrayAppend(fields, InPlaceInit, + Trade::SceneField::Mesh, + data.placeholders ? + Containers::ArrayView{nullptr, 3} : + Containers::arrayView(mapping), + Containers::arrayView(meshData)); + + /* Add a placeholder mapping field from another share group to verify + they're not shared by accident; add it among the other fields to avoid + them accidentally being treated as always together */ + arrayAppend(fields, InPlaceInit, + Trade::SceneField::Translation, + Containers::ArrayView{nullptr, 3}, + Containers::arrayView(translationData)); + + if(data.meshMaterialPresent) arrayAppend(fields, InPlaceInit, + Trade::SceneField::MeshMaterial, + data.placeholders ? + Containers::ArrayView{nullptr, 3} : + Containers::arrayView(mapping), + Containers::arrayView(meshMaterialData)); + + /* The assertions inside SceneData will verify that the mapping is + shared */ + Trade::SceneData scene = combineFields(Trade::SceneMappingType::UnsignedInt, 173, fields); + + Containers::StridedArrayView1D mappingData; + if(data.meshPresent) { + CORRADE_VERIFY(scene.hasField(Trade::SceneField::Mesh)); + CORRADE_COMPARE_AS(scene.field(Trade::SceneField::Mesh), + Containers::arrayView(meshData), + TestSuite::Compare::Container); + mappingData = scene.mapping(Trade::SceneField::Mesh); + } + if(data.meshMaterialPresent) { + CORRADE_VERIFY(scene.hasField(Trade::SceneField::MeshMaterial)); + CORRADE_COMPARE_AS(scene.field(Trade::SceneField::MeshMaterial), + Containers::arrayView(meshMaterialData), + TestSuite::Compare::Container); + mappingData = scene.mapping(Trade::SceneField::MeshMaterial); + } + + if(!data.placeholders) + CORRADE_COMPARE_AS(mappingData, + Containers::arrayView({13u, 31u, 67u}), + TestSuite::Compare::Container); + + /* The other field should be copied as well, but with its own mapping + data */ + CORRADE_VERIFY(scene.hasField(Trade::SceneField::Translation)); + CORRADE_VERIFY(scene.mapping(Trade::SceneField::Translation).data() != mappingData.data()); + CORRADE_COMPARE_AS(scene.field(Trade::SceneField::Translation), + Containers::arrayView(translationData), + TestSuite::Compare::Container); +} + +void CombineTest::fieldsSharedMappingExpected() { + CORRADE_SKIP_IF_NO_ASSERT(); + + /* Tested thoroughly in SceneDataTest::constructMismatchedTRSViews() and + constructMismatchedMeshMaterialViews(), here it uses the same internal + utility so test just that it's correctly called */ + + UnsignedInt meshes[3]{}; + Int materials[3]{}; + + std::ostringstream out; + Error redirectError{&out}; + combineFields(Trade::SceneMappingType::UnsignedInt, 3, { + Trade::SceneFieldData{Trade::SceneField::Mesh, + Containers::ArrayView{reinterpret_cast(0xdead), 3}, + Containers::arrayView(meshes)}, + Trade::SceneFieldData{Trade::SceneField::MeshMaterial, + Containers::ArrayView{reinterpret_cast(0xbeef), 2}, + Containers::arrayView(materials).prefix(2)}, + }); + combineFields(Trade::SceneMappingType::UnsignedInt, 3, { + Trade::SceneFieldData{Trade::SceneField::Mesh, + Containers::ArrayView{reinterpret_cast(0xdead), 3}, + Containers::arrayView(meshes)}, + Trade::SceneFieldData{Trade::SceneField::MeshMaterial, + Containers::ArrayView{nullptr, 3}, + Containers::arrayView(materials)}, + }); + CORRADE_COMPARE(out.str(), + "SceneTools::combineFields(): Trade::SceneField::MeshMaterial mapping data {0xbeef, 2, 4} is different from Trade::SceneField::Mesh mapping data {0xdead, 3, 4}\n" + /* Placeholder mapping is also disallowed right now -- it has to be + either all placeholders or none */ + "SceneTools::combineFields(): Trade::SceneField::MeshMaterial mapping data {0x0, 3, 4} is different from Trade::SceneField::Mesh mapping data {0xdead, 3, 4}\n"); +} + void CombineTest::fieldsStringPlaceholder() { CORRADE_SKIP_IF_NO_ASSERT(); diff --git a/src/Magnum/Trade/Implementation/checkSharedSceneFieldMapping.h b/src/Magnum/Trade/Implementation/checkSharedSceneFieldMapping.h index 3e199056d..205f30a4c 100644 --- a/src/Magnum/Trade/Implementation/checkSharedSceneFieldMapping.h +++ b/src/Magnum/Trade/Implementation/checkSharedSceneFieldMapping.h @@ -29,7 +29,7 @@ #include "Magnum/Trade/SceneData.h" -/* Used by SceneData internals */ +/* Used by SceneData and SceneTools::combineFields() internals */ namespace Magnum { namespace Trade { namespace Implementation {