From 19c055d6508586ea847fe305bdea6b0752205513 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Fri, 26 Nov 2021 20:43:51 +0100 Subject: [PATCH] Trade: remove the useless indirection from SceneField::Parent. This got originally added as some sort of a kludge to make it easy to go to the parent transformation, assuming Parent and Transformation share the same object mapping: parentTransformation = transformations[parents[i]] But after some ACTUAL REAL WORLD use, I realized that there's often a set of objects that have a Parent defined, and then another, completely disjoint, set of objects that have a transformation (for example certain nodes having no transformation at all because it's an identity). And so this parent indirection is not only useless, but in fact an additional complication. Let's say we make a map of the transformations, where transformationMap[i] is a transformation for object i: transformationMap = {} for j in range(len(transformations)): transformationMap[transformationObject[j]] = transformation[j] Then, with *no* assumptions about shared object mapping, the indirection would cause parent transformation retrieval to look like this: parentTransformation = transformationMap[parentObjects[parents[i]] While *without* the indirection, it'd be just parentTransformation = transformationMap[parents[i]] --- src/Magnum/Trade/Implementation/sceneTools.h | 5 +-- src/Magnum/Trade/SceneData.cpp | 20 ++------- src/Magnum/Trade/SceneData.h | 6 +-- .../Trade/Test/AbstractImporterTest.cpp | 44 +++++++++---------- src/Magnum/Trade/Test/SceneDataTest.cpp | 12 ++--- src/Magnum/Trade/Test/SceneToolsTest.cpp | 16 +++---- 6 files changed, 42 insertions(+), 61 deletions(-) diff --git a/src/Magnum/Trade/Implementation/sceneTools.h b/src/Magnum/Trade/Implementation/sceneTools.h index fb032db54..45d451d5f 100644 --- a/src/Magnum/Trade/Implementation/sceneTools.h +++ b/src/Magnum/Trade/Implementation/sceneTools.h @@ -285,9 +285,8 @@ inline SceneData sceneConvertToSingleFunctionObjects(const SceneData& scene, Con attached, then attach the field to a new object and make that new object a child of the previous one. */ if(fieldObject < objectAttachmentCount.size() && objectAttachmentCount[fieldObject]) { - /* Find an index of the old object and then use that index - to denote the parent of the new object */ - newParents[newParentIndex] = out.fieldObjectOffset(parentFieldId, fieldObject); + /* Use the old object as a parent of the new object */ + newParents[newParentIndex] = fieldObject; /* Assign the field to the new object */ fieldObject = newParentObjects[newParentIndex]; /* Move to the next reserved object */ diff --git a/src/Magnum/Trade/SceneData.cpp b/src/Magnum/Trade/SceneData.cpp index 88cc20aac..28a482d83 100644 --- a/src/Magnum/Trade/SceneData.cpp +++ b/src/Magnum/Trade/SceneData.cpp @@ -2073,11 +2073,7 @@ Containers::Optional SceneData::parentFor(const UnsignedInt object) const { Int index[1]; parentsIntoInternal(fieldId, offset, index); - if(*index == -1) return -1; - - UnsignedInt parent[1]; - objectsIntoInternal(fieldId, *index, parent); - return Int(*parent); + return *index; } Containers::Array SceneData::childrenFor(const Int object) const { @@ -2089,22 +2085,12 @@ Containers::Array SceneData::childrenFor(const Int object) const { const SceneFieldData& parentField = _fields[parentFieldId]; - /* Figure out the parent object index to look for or -1 if we want - top-level objects */ - Int parentIndexToLookFor; - if(object == -1) parentIndexToLookFor = -1; - else { - const std::size_t parentObjectIndex = findFieldObjectOffsetInternal(parentField, object, 0); - if(parentObjectIndex == parentField._size) return {}; - parentIndexToLookFor = parentObjectIndex; - } - - /* Collect IDs of all objects that reference this index */ + /* Collect IDs of all objects that reference this object */ Containers::Array out; for(std::size_t offset = 0; offset != parentField.size(); ++offset) { Int parentIndex[1]; parentsIntoInternal(parentFieldId, offset, parentIndex); - if(*parentIndex == parentIndexToLookFor) { + if(*parentIndex == object) { UnsignedInt child[1]; /** @todo bleh slow, use the children <-> parent field proxying when implemented */ diff --git a/src/Magnum/Trade/SceneData.h b/src/Magnum/Trade/SceneData.h index b9b6d02be..6af7dc26c 100644 --- a/src/Magnum/Trade/SceneData.h +++ b/src/Magnum/Trade/SceneData.h @@ -98,17 +98,13 @@ enum class SceneField: UnsignedInt { /* Zero used for an invalid value */ /** - * Parent index. Type is usually @ref SceneFieldType::Int, but can be also + * Parent object. Type is usually @ref SceneFieldType::Int, but can be also * any of @relativeref{SceneFieldType,Byte}, * @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 * defined. - * - * Note that the index points to the parent array itself and isn't the - * actual object index --- the object mapping is stored in the - * corresponding @ref SceneData::objects() array. * @see @ref SceneData::parentsAsArray(), @ref SceneData::parentFor(), * @ref SceneData::childrenFor() */ diff --git a/src/Magnum/Trade/Test/AbstractImporterTest.cpp b/src/Magnum/Trade/Test/AbstractImporterTest.cpp index d549cf7db..3c6bfc948 100644 --- a/src/Magnum/Trade/Test/AbstractImporterTest.cpp +++ b/src/Magnum/Trade/Test/AbstractImporterTest.cpp @@ -1850,13 +1850,13 @@ void AbstractImporterTest::sceneDeprecatedFallback2D() { cameras[0] = {3, 15}; importerState[0] = {3, &a}; - /* Object 5 is a child of object 3 (which is at index 0), has a skin (which - gets ignored by the legacy interface) */ - transformations[1] = {5, 0, Matrix3::rotation(-15.0_degf)}; + /* Object 5 is a child of object 3, has a skin (which gets ignored by the + legacy interface) */ + transformations[1] = {5, 3, Matrix3::rotation(-15.0_degf)}; skins[0] = {5, 226}; - /* Object 1 is a child of object 2 (which will be at index 3) */ - transformations[2] = {1, 3, Matrix3::translation({1.0f, 0.5f})*Matrix3::rotation(15.0_degf)}; + /* Object 1 is a child of object 2 */ + transformations[2] = {1, 2, Matrix3::translation({1.0f, 0.5f})*Matrix3::rotation(15.0_degf)}; /* Object 2 is in the root, has object 1 as a child but nothing else */ transformations[3] = {2, -1, {}}; @@ -1866,9 +1866,9 @@ void AbstractImporterTest::sceneDeprecatedFallback2D() { meshes[0] = {0, 33, -1}; /* Object 4 has TRS also, a mesh with a material and a skin and is a child - of object 3 (which is at index 0). The transformation gets ignored - again. Has importer state. */ - transformations[5] = {4, 0, Matrix3::translation(Vector2::xAxis(5.0f))}; + of object 3. The transformation gets ignored again. Has importer + state. */ + transformations[5] = {4, 3, Matrix3::translation(Vector2::xAxis(5.0f))}; trs[1] = {4, {}, {}, {1.5f, -0.5f}}; meshes[1] = {4, 27, 46}; skins[1] = {4, 72}; @@ -2121,13 +2121,13 @@ void AbstractImporterTest::sceneDeprecatedFallback3D() { cameras[0] = {3, 15}; importerState[0] = {3, &a}; - /* Object 5 is a child of object 3 (which is at index 0), has a skin (which - gets ignored by the legacy interface) */ - transformations[1] = {5, 0, Matrix4::rotationY(-15.0_degf)}; + /* Object 5 is a child of object 3, has a skin (which gets ignored by the + legacy interface) */ + transformations[1] = {5, 3, Matrix4::rotationY(-15.0_degf)}; skins[0] = {5, 226}; - /* Object 1 is a child of object 2 (which will be at index 3), has a light. */ - transformations[2] = {1, 3, Matrix4::translation({1.0f, 0.0f, 1.0f})*Matrix4::rotationZ(15.0_degf)}; + /* Object 1 is a child of object 2, has a light. */ + transformations[2] = {1, 2, Matrix4::translation({1.0f, 0.0f, 1.0f})*Matrix4::rotationZ(15.0_degf)}; lights[0] = {1, 113}; /* Object 2 is in the root, has object 1 as a child but nothing else */ @@ -2138,9 +2138,9 @@ void AbstractImporterTest::sceneDeprecatedFallback3D() { meshes[0] = {0, 33, -1}; /* Object 4 has TRS also, a mesh with a material and a skin and is a child - of object 3 (which is at index 0). The transformation gets ignored - again. Has importer state. */ - transformations[5] = {4, 0, Matrix4::translation(Vector3::xAxis(5.0f))}; + of object 3. The transformation gets ignored again. Has importer + state. */ + transformations[5] = {4, 3, Matrix4::translation(Vector3::xAxis(5.0f))}; trs[1] = {4, {}, {}, {1.5f, 3.0f, -0.5f}}; meshes[1] = {4, 27, 46}; skins[1] = {4, 72}; @@ -2489,8 +2489,8 @@ void AbstractImporterTest::sceneDeprecatedFallbackTransformless2D() { Int parent; } fields[]{ {5, -1}, - {2, 0}, - {3, 0}, + {2, 5}, + {3, 5}, {1, -1} }; @@ -2595,8 +2595,8 @@ void AbstractImporterTest::sceneDeprecatedFallbackTransformless3D() { Int parent; } fields[]{ {5, -1}, - {2, 0}, - {3, 0}, + {2, 5}, + {3, 5}, {1, -1} }; @@ -2718,7 +2718,7 @@ void AbstractImporterTest::sceneDeprecatedFallbackMultiFunctionObjects2D() { {NoInit, 7, meshes}, {NoInit, 2, cameras}, }; - Utility::copy({{15, -1}, {21, -1}, {22, 1}, {23, 2}, {1, -1}}, parents); + Utility::copy({{15, -1}, {21, -1}, {22, 21}, {23, 22}, {1, -1}}, parents); Utility::copy({ {15, 6, 4}, {23, 1, 0}, @@ -2985,7 +2985,7 @@ void AbstractImporterTest::sceneDeprecatedFallbackMultiFunctionObjects3D() { {NoInit, 7, meshes}, {NoInit, 2, cameras}, }; - Utility::copy({{15, -1}, {21, -1}, {22, 1}, {23, 2}, {1, -1}}, parents); + Utility::copy({{15, -1}, {21, -1}, {22, 21}, {23, 22}, {1, -1}}, parents); Utility::copy({ {15, 6, 4}, {23, 1, 0}, diff --git a/src/Magnum/Trade/Test/SceneDataTest.cpp b/src/Magnum/Trade/Test/SceneDataTest.cpp index 57f4da2e4..20c8d190d 100644 --- a/src/Magnum/Trade/Test/SceneDataTest.cpp +++ b/src/Magnum/Trade/Test/SceneDataTest.cpp @@ -4812,8 +4812,8 @@ void SceneDataTest::parentFor() { Int parent; } fields[]{ {3, -1}, - {4, 0}, - {2, 1}, + {4, 3}, + {2, 4}, {4, 2} /* duplicate, ignored */ }; Containers::StridedArrayView1D view = fields; @@ -4865,10 +4865,10 @@ void SceneDataTest::childrenFor() { Int parent; } fields[]{ {4, -1}, - {3, 0}, - {2, 1}, - {1, 0}, - {5, 0}, + {3, 4}, + {2, 3}, + {1, 4}, + {5, 4}, {0, -1}, }; Containers::StridedArrayView1D view = fields; diff --git a/src/Magnum/Trade/Test/SceneToolsTest.cpp b/src/Magnum/Trade/Test/SceneToolsTest.cpp index 5fed43146..fabb93855 100644 --- a/src/Magnum/Trade/Test/SceneToolsTest.cpp +++ b/src/Magnum/Trade/Test/SceneToolsTest.cpp @@ -90,7 +90,7 @@ void SceneToolsTest::combine() { const UnsignedByte meshes[]{3, 5, 17}; const UnsignedShort parentObjects[]{33, 25}; - const Short parents[]{-1, 0}; + const Short parents[]{-1, 33}; const UnsignedByte translationObjects[]{16}; const Vector2d translations[]{{1.5, -0.5}}; @@ -341,7 +341,7 @@ void SceneToolsTest::convertToSingleFunctionObjects() { state here, without having to mess with an ArrayTuple */ const UnsignedShort parentObjects[]{15, 21, 22, 23, 1}; - const Byte parents[]{-1, -1, 1, 2, -1}; + const Byte parents[]{-1, -1, 21, 22, -1}; /* Two objects have two and three mesh assignments respectively, meaning we need three extra */ @@ -426,13 +426,13 @@ void SceneToolsTest::convertToSingleFunctionObjects() { CORRADE_COMPARE_AS(scene.parentsAsArray(), (Containers::arrayView>({ {15, -1}, {21, -1}, - {22, 1}, - {23, 2}, + {22, 21}, + {23, 22}, {1, -1}, - {63, 3}, - {64, 3}, - {65, 0}, - {66, 4} + {63, 23}, + {64, 23}, + {65, 15}, + {66, 1} })), TestSuite::Compare::Container); /* Meshes / materials have certain objects reassigned, field data stay the