From 78b71e1859167cadaac971fa2c24cc85911a6773 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 6 Dec 2021 13:18:53 +0100 Subject: [PATCH] Trade: preserve field flags in the scene combining tools. Not really important right now as the SceneData from these are used only in internal deprecated APIs, but at least it might speed up the children2D() / children3D() queries. Mainly done so I don't forget to do this later when these APIs are published in the SceneTools library. What's not done is the rather complex logic in the single-function conversion utility, where a field could retain the implicit/ordered flags in *some* scenarios. There's too many corner cases so better be conservative and don't preserve anything rather than mark something as ordered while it's no longer the case. The corner cases are hopefully all checled for (and XFAIL'd) in the test. --- src/Magnum/Trade/Implementation/sceneTools.h | 29 +++- src/Magnum/Trade/Test/SceneToolsTest.cpp | 166 ++++++++++++++++--- 2 files changed, 166 insertions(+), 29 deletions(-) diff --git a/src/Magnum/Trade/Implementation/sceneTools.h b/src/Magnum/Trade/Implementation/sceneTools.h index a4242f6c3..9c1e05605 100644 --- a/src/Magnum/Trade/Implementation/sceneTools.h +++ b/src/Magnum/Trade/Implementation/sceneTools.h @@ -185,7 +185,7 @@ inline SceneData sceneCombine(const SceneMappingType mappingType, const Unsigned /* Map the fields to the new data */ Containers::Array outFields{fields.size()}; for(std::size_t i = 0; i != fields.size(); ++i) { - outFields[i] = SceneFieldData{fields[i].name(), itemViews[itemViewMappings[i].first()], fields[i].fieldType(), itemViews[itemViewMappings[i].second()], fields[i].fieldArraySize()}; + outFields[i] = SceneFieldData{fields[i].name(), itemViews[itemViewMappings[i].first()], fields[i].fieldType(), itemViews[itemViewMappings[i].second()], fields[i].fieldArraySize(), fields[i].flags()}; } return SceneData{mappingType, mappingBound, std::move(outData), std::move(outFields)}; @@ -239,14 +239,26 @@ inline SceneData sceneConvertToSingleFunctionObjects(const SceneData& scene, Con for(std::size_t i = 0; i != scene.fieldCount(); ++i) { const SceneFieldData& field = scene.fieldData(i); - /* If this is a parent, enlarge it for the newly added objects */ + /* If this is a parent, enlarge it for the newly added objects, and if + it was implicit make it ordered */ if(field.name() == SceneField::Parent) { /** @todo some nicer constructor for placeholders once this is in public interest */ - fields[i] = SceneFieldData{SceneField::Parent, Containers::ArrayView{nullptr, std::size_t(field.size() + objectsToAdd)}, Containers::ArrayView{nullptr, std::size_t(field.size() + objectsToAdd)}}; - - /* All other fields are copied as-is */ - } else fields[i] = field; + fields[i] = SceneFieldData{SceneField::Parent, Containers::ArrayView{nullptr, std::size_t(field.size() + objectsToAdd)}, Containers::ArrayView{nullptr, std::size_t(field.size() + objectsToAdd)}, + /* If the field is ordered, we preserve that. But if it's + implicit, we can't. */ + field.flags() & ~(SceneFieldFlag::ImplicitMapping & ~SceneFieldFlag::OrderedMapping) + }; + + /* All other fields are copied as-is, but lose the implicit/ordered + flags */ + /** @todo the flags could get preserved for + - fields that don't share their object mapping with any fields in + the fieldsToConvert list + - fields that don't actually get their object mapping touched + during the process (and then all fields that share object + mapping with them) */ + } else fields[i] = SceneFieldData{field.name(), field.mappingType(), field.mappingData(), field.fieldType(), field.fieldData(), field.fieldArraySize(), field.flags() & ~SceneFieldFlag::ImplicitMapping}; } /* Combine the fields into a new SceneData */ @@ -292,6 +304,11 @@ inline SceneData sceneConvertToSingleFunctionObjects(const SceneData& scene, Con fieldObject = newParentMapping[newParentIndex]; /* Move to the next reserved object */ ++newParentIndex; + + /** @todo mark this field as touched here and the restore + the original flags for all fields that didn't have + their object mapping touched */ + } else ++objectAttachmentCount[fieldObject]; } } diff --git a/src/Magnum/Trade/Test/SceneToolsTest.cpp b/src/Magnum/Trade/Test/SceneToolsTest.cpp index 2cb3ae373..807b1839e 100644 --- a/src/Magnum/Trade/Test/SceneToolsTest.cpp +++ b/src/Magnum/Trade/Test/SceneToolsTest.cpp @@ -59,9 +59,17 @@ struct { const char* name; UnsignedLong originalObjectCount; UnsignedLong expectedObjectCount; + SceneFieldFlags parentFieldFlagsInput; + SceneFieldFlags parentFieldFlagsExpected; } ConvertToSingleFunctionObjectsData[]{ - {"original object count smaller than new", 64, 67}, - {"original object count larger than new", 96, 96} + {"original object count smaller than new", 64, 70, {}, {}}, + {"original object count larger than new", 96, 96, {}, {}}, + {"parent field with ordered mapping", 64, 70, + SceneFieldFlag::OrderedMapping, SceneFieldFlag::OrderedMapping}, + {"parent field with implicit mapping", 64, 70, + /* The mapping is *not* implicit but we're not using the flag for + anything so this should work */ + SceneFieldFlag::ImplicitMapping, SceneFieldFlag::OrderedMapping} }; SceneToolsTest::SceneToolsTest() { @@ -89,8 +97,8 @@ void SceneToolsTest::combine() { const UnsignedInt meshMappingData[]{45, 78, 23}; const UnsignedByte meshFieldData[]{3, 5, 17}; - const UnsignedShort parentMappingData[]{33, 25}; - const Short parentData[]{-1, 33}; + const UnsignedShort parentMappingData[]{0, 1}; + const Short parentData[]{-1, 0}; const UnsignedByte translationMappingData[]{16}; const Vector2d translationFieldData[]{{1.5, -0.5}}; @@ -100,10 +108,10 @@ void SceneToolsTest::combine() { SceneData scene = Implementation::sceneCombine(data.objectType, 167, Containers::arrayView({ SceneFieldData{SceneField::Mesh, Containers::arrayView(meshMappingData), Containers::arrayView(meshFieldData)}, - SceneFieldData{SceneField::Parent, Containers::arrayView(parentMappingData), Containers::arrayView(parentData)}, + SceneFieldData{SceneField::Parent, Containers::arrayView(parentMappingData), Containers::arrayView(parentData), SceneFieldFlag::ImplicitMapping}, SceneFieldData{SceneField::Translation, Containers::arrayView(translationMappingData), Containers::arrayView(translationFieldData)}, /* Array field */ - SceneFieldData{sceneFieldCustom(15), Containers::arrayView(fooMappingData), Containers::StridedArrayView2D{fooFieldData, {2, 2}}}, + SceneFieldData{sceneFieldCustom(15), Containers::arrayView(fooMappingData), Containers::StridedArrayView2D{fooFieldData, {2, 2}}, SceneFieldFlag::OrderedMapping}, /* Empty field */ SceneFieldData{SceneField::Camera, Containers::ArrayView{}, Containers::ArrayView{}} })); @@ -114,6 +122,7 @@ void SceneToolsTest::combine() { CORRADE_COMPARE(scene.fieldCount(), 5); CORRADE_COMPARE(scene.fieldName(0), SceneField::Mesh); + CORRADE_COMPARE(scene.fieldFlags(0), SceneFieldFlags{}); CORRADE_COMPARE(scene.fieldType(0), SceneFieldType::UnsignedByte); CORRADE_COMPARE(scene.fieldArraySize(0), 0); CORRADE_COMPARE_AS(scene.mappingAsArray(0), Containers::arrayView({ @@ -124,16 +133,18 @@ void SceneToolsTest::combine() { TestSuite::Compare::Container); CORRADE_COMPARE(scene.fieldName(1), SceneField::Parent); + CORRADE_COMPARE(scene.fieldFlags(1), SceneFieldFlag::ImplicitMapping); CORRADE_COMPARE(scene.fieldType(1), SceneFieldType::Short); CORRADE_COMPARE(scene.fieldArraySize(1), 0); CORRADE_COMPARE_AS(scene.mappingAsArray(1), Containers::arrayView({ - 33, 25 + 0, 1 }), TestSuite::Compare::Container); CORRADE_COMPARE_AS(scene.field(1), Containers::arrayView(parentData), TestSuite::Compare::Container); CORRADE_COMPARE(scene.fieldName(2), SceneField::Translation); + CORRADE_COMPARE(scene.fieldFlags(2), SceneFieldFlags{}); CORRADE_COMPARE(scene.fieldType(2), SceneFieldType::Vector2d); CORRADE_COMPARE(scene.fieldArraySize(2), 0); CORRADE_COMPARE_AS(scene.mappingAsArray(2), @@ -144,6 +155,7 @@ void SceneToolsTest::combine() { TestSuite::Compare::Container); CORRADE_COMPARE(scene.fieldName(3), sceneFieldCustom(15)); + CORRADE_COMPARE(scene.fieldFlags(3), SceneFieldFlag::OrderedMapping); CORRADE_COMPARE(scene.fieldType(3), SceneFieldType::Int); CORRADE_COMPARE(scene.fieldArraySize(3), 2); CORRADE_COMPARE_AS(scene.mappingAsArray(3), @@ -159,6 +171,7 @@ void SceneToolsTest::combine() { TestSuite::Compare::Container); CORRADE_COMPARE(scene.fieldName(4), SceneField::Camera); + CORRADE_COMPARE(scene.fieldFlags(4), SceneFieldFlags{}); CORRADE_COMPARE(scene.fieldType(4), SceneFieldType::UnsignedShort); CORRADE_COMPARE(scene.fieldSize(4), 0); CORRADE_COMPARE(scene.fieldArraySize(4), 0); @@ -340,12 +353,12 @@ void SceneToolsTest::convertToSingleFunctionObjects() { /* Haha now I can use sceneCombine() to conveniently prepare the initial state here, without having to mess with an ArrayTuple */ - const UnsignedShort parentMappingData[]{15, 21, 22, 23, 1}; - const Byte parentFieldData[]{-1, -1, 21, 22, -1}; + const UnsignedShort parentMappingData[]{2, 15, 21, 22, 23}; + const Byte parentFieldData[]{-1, -1, -1, 21, 22}; /* Two objects have two and three mesh assignments respectively, meaning we need three extra */ - const UnsignedShort meshMappingData[]{15, 23, 23, 23, 1, 15, 21}; + const UnsignedShort meshMappingData[]{15, 23, 23, 23, 2, 15, 21}; const Containers::Pair meshMaterialFieldData[]{ {6, 4}, {1, 0}, @@ -357,14 +370,39 @@ void SceneToolsTest::convertToSingleFunctionObjects() { }; /* One camera is attached to an object that already has a mesh, meaning we - need a third extra object */ - const UnsignedShort cameraMappingData[]{22, 1}; + need a fourth extra object */ + const UnsignedShort cameraMappingData[]{22, 2}; const UnsignedInt cameraFieldData[]{1, 5}; + + /* Lights don't conflict with anything so they *could* retain the + ImplicitMapping flag */ + const UnsignedShort lightMappingData[]{0, 1}; + const UnsignedByte lightFieldData[]{15, 23}; + + /* Object 0 and 1 has a light, 2 a mesh already, meaning we need a fifth, + sixth and seventh extra object and we lose the ImplicitMapping flag. */ + const UnsignedShort fooMappingData[]{0, 1, 2, 3}; + const Float fooFieldData[]{1.0f, 2.0f, 3.0f, 4.0f}; + + /* This field is not among the fields to convert so it should preserve the + ImplicitMapping flag */ + const UnsignedShort foo2MappingData[]{0, 1}; + const Byte foo2FieldData[]{-5, -7}; + + /* This field shares mapping with foo (and thus has the ImplicitMapping + flag), but it's not among the fields to convert. Since the mapping gets + changed, it should not retain the ImplicitMapping flag. */ + const Byte foo3FieldData[]{-1, -2, 7, 2}; + SceneData original = Implementation::sceneCombine(SceneMappingType::UnsignedShort, data.originalObjectCount, Containers::arrayView({ - SceneFieldData{SceneField::Parent, Containers::arrayView(parentMappingData), Containers::arrayView(parentFieldData)}, + SceneFieldData{SceneField::Parent, Containers::arrayView(parentMappingData), Containers::arrayView(parentFieldData), data.parentFieldFlagsInput}, SceneFieldData{SceneField::Mesh, Containers::arrayView(meshMappingData), Containers::StridedArrayView1D{meshMaterialFieldData, &meshMaterialFieldData[0].first(), Containers::arraySize(meshMaterialFieldData), sizeof(meshMaterialFieldData[0])}}, SceneFieldData{SceneField::MeshMaterial, Containers::arrayView(meshMappingData), Containers::StridedArrayView1D{meshMaterialFieldData, &meshMaterialFieldData[0].second(), Containers::arraySize(meshMaterialFieldData), sizeof(meshMaterialFieldData[0])}}, SceneFieldData{SceneField::Camera, Containers::arrayView(cameraMappingData), Containers::arrayView(cameraFieldData)}, + SceneFieldData{SceneField::Light, Containers::arrayView(lightMappingData), Containers::arrayView(lightFieldData), SceneFieldFlag::ImplicitMapping}, + SceneFieldData{sceneFieldCustom(15), Containers::arrayView(fooMappingData), Containers::arrayView(fooFieldData), SceneFieldFlag::ImplicitMapping}, + SceneFieldData{sceneFieldCustom(16), Containers::arrayView(foo2MappingData), Containers::arrayView(foo2FieldData), SceneFieldFlag::ImplicitMapping}, + SceneFieldData{sceneFieldCustom(17), Containers::arrayView(fooMappingData), Containers::arrayView(foo3FieldData), SceneFieldFlag::ImplicitMapping} })); SceneData scene = Implementation::sceneConvertToSingleFunctionObjects(original, Containers::arrayView({ @@ -373,6 +411,12 @@ void SceneToolsTest::convertToSingleFunctionObjects() { get automatically updated as they share the same object mapping. OTOH including them would break the output. */ SceneField::Camera, + /* A field with implicit mapping that doesn't conflict with anything so + it *could* retain the flag */ + SceneField::Light, + /* A field with implicit mapping, which loses the flag because entries + get reassigned */ + sceneFieldCustom(15), /* Include also a field that's not present -- it should get skipped */ SceneField::ImporterState }), 63); @@ -381,14 +425,31 @@ void SceneToolsTest::convertToSingleFunctionObjects() { it's large enough */ CORRADE_COMPARE(scene.mappingBound(), data.expectedObjectCount); - /* Object 1 should have a new child that has the camera, as it has a mesh */ + /* Object 0 should have new children with "foo", as it has a light */ + CORRADE_COMPARE_AS(scene.childrenFor(0), + Containers::arrayView({67}), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.lightsFor(0), + Containers::arrayView({15}), + TestSuite::Compare::Container); + + /* Object 1 should have a new child with "foo", as it has a light */ CORRADE_COMPARE_AS(scene.childrenFor(1), - Containers::arrayView({66}), + Containers::arrayView({68}), + TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.lightsFor(1), + Containers::arrayView({23}), + TestSuite::Compare::Container); + + /* Object 2 should have a new child with the camera and "foo", as it has a + mesh */ + CORRADE_COMPARE_AS(scene.childrenFor(2), + Containers::arrayView({66, 69}), TestSuite::Compare::Container); - CORRADE_COMPARE_AS(scene.meshesMaterialsFor(1), + CORRADE_COMPARE_AS(scene.meshesMaterialsFor(2), (Containers::arrayView>({{7, 2}})), TestSuite::Compare::Container); - CORRADE_COMPARE_AS(scene.camerasFor(1), + CORRADE_COMPARE_AS(scene.camerasFor(2), Containers::arrayView({}), TestSuite::Compare::Container); CORRADE_COMPARE_AS(scene.camerasFor(66), @@ -422,36 +483,95 @@ void SceneToolsTest::convertToSingleFunctionObjects() { TestSuite::Compare::Container); /* To be extra sure, verify the actual data. Parents have a few objects - added, the rest is the same */ + added, the rest is the same. Because new objects are added at the end, + the ordered flag is preserved if present. */ CORRADE_COMPARE_AS(scene.parentsAsArray(), (Containers::arrayView>({ + {2, -1}, {15, -1}, {21, -1}, {22, 21}, {23, 22}, - {1, -1}, {63, 23}, {64, 23}, {65, 15}, - {66, 1} + {66, 2}, + {67, 0}, + {68, 1}, + {69, 2}, })), TestSuite::Compare::Container); + CORRADE_COMPARE(scene.fieldFlags(SceneField::Parent), data.parentFieldFlagsExpected); /* Meshes / materials have certain objects reassigned, field data stay the - same */ + same. There was no flag before so neither is after. */ CORRADE_COMPARE_AS(scene.meshesMaterialsAsArray(), (Containers::arrayView>>({ {15, {6, 4}}, {23, {1, 0}}, {63, {2, 3}}, {64, {4, 2}}, - {1, {7, 2}}, + {2, {7, 2}}, {65, {3, 1}}, {21, {5, -1}} })), TestSuite::Compare::Container); + CORRADE_COMPARE(scene.fieldFlags(SceneField::Mesh), SceneFieldFlags{}); + CORRADE_COMPARE(scene.fieldFlags(SceneField::MeshMaterial), SceneFieldFlags{}); - /* Cameras have certain objects reassigned, field data stay the same */ + /* Cameras have certain objects reassigned, field data stay the same. There + was no flag before so neither is after. */ CORRADE_COMPARE_AS(scene.camerasAsArray(), (Containers::arrayView>({ {22, 1}, {66, 5} })), TestSuite::Compare::Container); + CORRADE_COMPARE(scene.fieldFlags(SceneField::Camera), SceneFieldFlags{}); + + /* Lights stay the same, thus the implicit flag could be preserved. It's + not currently, though. */ + CORRADE_COMPARE_AS(scene.lightsAsArray(), (Containers::arrayView>({ + {0, 15}, + {1, 23} + })), TestSuite::Compare::Container); + { + CORRADE_EXPECT_FAIL("Logic for preserving flags of untouched fields is rather complex and thus not implemented yet."); + CORRADE_COMPARE(scene.fieldFlags(SceneField::Light), SceneFieldFlag::ImplicitMapping); + } { + CORRADE_COMPARE(scene.fieldFlags(SceneField::Light), SceneFieldFlags{}); + } + + /* A custom field gets the last object reassigned, field data stay the + same. The implicit flag gets turned to nothing after that. */ + CORRADE_COMPARE_AS(scene.mappingAsArray(sceneFieldCustom(15)), (Containers::arrayView({ + 67, 68, 69, 3 + })), TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.field(sceneFieldCustom(15)), + Containers::arrayView(fooFieldData), + TestSuite::Compare::Container); + CORRADE_COMPARE(scene.fieldFlags(sceneFieldCustom(15)), SceneFieldFlags{}); + + /* A custom field that is not among fields to convert so it preserves the + flag */ + CORRADE_COMPARE_AS(scene.mappingAsArray(sceneFieldCustom(16)), (Containers::arrayView({ + 0, 1 + })), TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.field(sceneFieldCustom(16)), + Containers::arrayView(foo2FieldData), + TestSuite::Compare::Container); + { + CORRADE_EXPECT_FAIL("Logic for preserving flags of untouched fields is rather complex and thus not implemented yet."); + CORRADE_COMPARE(scene.fieldFlags(sceneFieldCustom(16)), SceneFieldFlag::ImplicitMapping); + } { + CORRADE_COMPARE(scene.fieldFlags(sceneFieldCustom(16)), SceneFieldFlags{}); + } + + /* A custom field that is not among fields to convert but it shares the + mapping with a field that is and that gets changed. The implicit flag + should thus get removed here as well. */ + CORRADE_COMPARE_AS(scene.mappingAsArray(sceneFieldCustom(17)), (Containers::arrayView({ + 67, 68, 69, 3 + })), TestSuite::Compare::Container); + CORRADE_COMPARE_AS(scene.field(sceneFieldCustom(17)), + Containers::arrayView(foo3FieldData), + TestSuite::Compare::Container); + CORRADE_COMPARE(scene.fieldFlags(sceneFieldCustom(17)), SceneFieldFlags{}); + } }}}}