Browse Source

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]]
pull/525/head
Vladimír Vondruš 5 years ago
parent
commit
19c055d650
  1. 5
      src/Magnum/Trade/Implementation/sceneTools.h
  2. 20
      src/Magnum/Trade/SceneData.cpp
  3. 6
      src/Magnum/Trade/SceneData.h
  4. 44
      src/Magnum/Trade/Test/AbstractImporterTest.cpp
  5. 12
      src/Magnum/Trade/Test/SceneDataTest.cpp
  6. 16
      src/Magnum/Trade/Test/SceneToolsTest.cpp

5
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 attached, then attach the field to a new object and make
that new object a child of the previous one. */ that new object a child of the previous one. */
if(fieldObject < objectAttachmentCount.size() && objectAttachmentCount[fieldObject]) { if(fieldObject < objectAttachmentCount.size() && objectAttachmentCount[fieldObject]) {
/* Find an index of the old object and then use that index /* Use the old object as a parent of the new object */
to denote the parent of the new object */ newParents[newParentIndex] = fieldObject;
newParents[newParentIndex] = out.fieldObjectOffset(parentFieldId, fieldObject);
/* Assign the field to the new object */ /* Assign the field to the new object */
fieldObject = newParentObjects[newParentIndex]; fieldObject = newParentObjects[newParentIndex];
/* Move to the next reserved object */ /* Move to the next reserved object */

20
src/Magnum/Trade/SceneData.cpp

@ -2073,11 +2073,7 @@ Containers::Optional<Int> SceneData::parentFor(const UnsignedInt object) const {
Int index[1]; Int index[1];
parentsIntoInternal(fieldId, offset, index); parentsIntoInternal(fieldId, offset, index);
if(*index == -1) return -1; return *index;
UnsignedInt parent[1];
objectsIntoInternal(fieldId, *index, parent);
return Int(*parent);
} }
Containers::Array<UnsignedInt> SceneData::childrenFor(const Int object) const { Containers::Array<UnsignedInt> SceneData::childrenFor(const Int object) const {
@ -2089,22 +2085,12 @@ Containers::Array<UnsignedInt> SceneData::childrenFor(const Int object) const {
const SceneFieldData& parentField = _fields[parentFieldId]; const SceneFieldData& parentField = _fields[parentFieldId];
/* Figure out the parent object index to look for or -1 if we want /* Collect IDs of all objects that reference this object */
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 */
Containers::Array<UnsignedInt> out; Containers::Array<UnsignedInt> out;
for(std::size_t offset = 0; offset != parentField.size(); ++offset) { for(std::size_t offset = 0; offset != parentField.size(); ++offset) {
Int parentIndex[1]; Int parentIndex[1];
parentsIntoInternal(parentFieldId, offset, parentIndex); parentsIntoInternal(parentFieldId, offset, parentIndex);
if(*parentIndex == parentIndexToLookFor) { if(*parentIndex == object) {
UnsignedInt child[1]; UnsignedInt child[1];
/** @todo bleh slow, use the children <-> parent field proxying /** @todo bleh slow, use the children <-> parent field proxying
when implemented */ when implemented */

6
src/Magnum/Trade/SceneData.h

@ -98,17 +98,13 @@ enum class SceneField: UnsignedInt {
/* Zero used for an invalid value */ /* 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}, * any of @relativeref{SceneFieldType,Byte},
* @relativeref{SceneFieldType,Short} or a * @relativeref{SceneFieldType,Short} or a
* @relativeref{SceneFieldType,Long}. A value of @cpp -1 @ce means there's * @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 * 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 * enforced in any way, and which of the duplicate fields gets used is not
* defined. * 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(), * @see @ref SceneData::parentsAsArray(), @ref SceneData::parentFor(),
* @ref SceneData::childrenFor() * @ref SceneData::childrenFor()
*/ */

44
src/Magnum/Trade/Test/AbstractImporterTest.cpp

@ -1850,13 +1850,13 @@ void AbstractImporterTest::sceneDeprecatedFallback2D() {
cameras[0] = {3, 15}; cameras[0] = {3, 15};
importerState[0] = {3, &a}; importerState[0] = {3, &a};
/* Object 5 is a child of object 3 (which is at index 0), has a skin (which /* Object 5 is a child of object 3, has a skin (which gets ignored by the
gets ignored by the legacy interface) */ legacy interface) */
transformations[1] = {5, 0, Matrix3::rotation(-15.0_degf)}; transformations[1] = {5, 3, Matrix3::rotation(-15.0_degf)};
skins[0] = {5, 226}; skins[0] = {5, 226};
/* Object 1 is a child of object 2 (which will be at index 3) */ /* Object 1 is a child of object 2 */
transformations[2] = {1, 3, Matrix3::translation({1.0f, 0.5f})*Matrix3::rotation(15.0_degf)}; 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 */ /* Object 2 is in the root, has object 1 as a child but nothing else */
transformations[3] = {2, -1, {}}; transformations[3] = {2, -1, {}};
@ -1866,9 +1866,9 @@ void AbstractImporterTest::sceneDeprecatedFallback2D() {
meshes[0] = {0, 33, -1}; meshes[0] = {0, 33, -1};
/* Object 4 has TRS also, a mesh with a material and a skin and is a child /* 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 of object 3. The transformation gets ignored again. Has importer
again. Has importer state. */ state. */
transformations[5] = {4, 0, Matrix3::translation(Vector2::xAxis(5.0f))}; transformations[5] = {4, 3, Matrix3::translation(Vector2::xAxis(5.0f))};
trs[1] = {4, {}, {}, {1.5f, -0.5f}}; trs[1] = {4, {}, {}, {1.5f, -0.5f}};
meshes[1] = {4, 27, 46}; meshes[1] = {4, 27, 46};
skins[1] = {4, 72}; skins[1] = {4, 72};
@ -2121,13 +2121,13 @@ void AbstractImporterTest::sceneDeprecatedFallback3D() {
cameras[0] = {3, 15}; cameras[0] = {3, 15};
importerState[0] = {3, &a}; importerState[0] = {3, &a};
/* Object 5 is a child of object 3 (which is at index 0), has a skin (which /* Object 5 is a child of object 3, has a skin (which gets ignored by the
gets ignored by the legacy interface) */ legacy interface) */
transformations[1] = {5, 0, Matrix4::rotationY(-15.0_degf)}; transformations[1] = {5, 3, Matrix4::rotationY(-15.0_degf)};
skins[0] = {5, 226}; skins[0] = {5, 226};
/* Object 1 is a child of object 2 (which will be at index 3), has a light. */ /* Object 1 is a child of object 2, has a light. */
transformations[2] = {1, 3, Matrix4::translation({1.0f, 0.0f, 1.0f})*Matrix4::rotationZ(15.0_degf)}; transformations[2] = {1, 2, Matrix4::translation({1.0f, 0.0f, 1.0f})*Matrix4::rotationZ(15.0_degf)};
lights[0] = {1, 113}; lights[0] = {1, 113};
/* Object 2 is in the root, has object 1 as a child but nothing else */ /* 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}; meshes[0] = {0, 33, -1};
/* Object 4 has TRS also, a mesh with a material and a skin and is a child /* 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 of object 3. The transformation gets ignored again. Has importer
again. Has importer state. */ state. */
transformations[5] = {4, 0, Matrix4::translation(Vector3::xAxis(5.0f))}; transformations[5] = {4, 3, Matrix4::translation(Vector3::xAxis(5.0f))};
trs[1] = {4, {}, {}, {1.5f, 3.0f, -0.5f}}; trs[1] = {4, {}, {}, {1.5f, 3.0f, -0.5f}};
meshes[1] = {4, 27, 46}; meshes[1] = {4, 27, 46};
skins[1] = {4, 72}; skins[1] = {4, 72};
@ -2489,8 +2489,8 @@ void AbstractImporterTest::sceneDeprecatedFallbackTransformless2D() {
Int parent; Int parent;
} fields[]{ } fields[]{
{5, -1}, {5, -1},
{2, 0}, {2, 5},
{3, 0}, {3, 5},
{1, -1} {1, -1}
}; };
@ -2595,8 +2595,8 @@ void AbstractImporterTest::sceneDeprecatedFallbackTransformless3D() {
Int parent; Int parent;
} fields[]{ } fields[]{
{5, -1}, {5, -1},
{2, 0}, {2, 5},
{3, 0}, {3, 5},
{1, -1} {1, -1}
}; };
@ -2718,7 +2718,7 @@ void AbstractImporterTest::sceneDeprecatedFallbackMultiFunctionObjects2D() {
{NoInit, 7, meshes}, {NoInit, 7, meshes},
{NoInit, 2, cameras}, {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({ Utility::copy({
{15, 6, 4}, {15, 6, 4},
{23, 1, 0}, {23, 1, 0},
@ -2985,7 +2985,7 @@ void AbstractImporterTest::sceneDeprecatedFallbackMultiFunctionObjects3D() {
{NoInit, 7, meshes}, {NoInit, 7, meshes},
{NoInit, 2, cameras}, {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({ Utility::copy({
{15, 6, 4}, {15, 6, 4},
{23, 1, 0}, {23, 1, 0},

12
src/Magnum/Trade/Test/SceneDataTest.cpp

@ -4812,8 +4812,8 @@ void SceneDataTest::parentFor() {
Int parent; Int parent;
} fields[]{ } fields[]{
{3, -1}, {3, -1},
{4, 0}, {4, 3},
{2, 1}, {2, 4},
{4, 2} /* duplicate, ignored */ {4, 2} /* duplicate, ignored */
}; };
Containers::StridedArrayView1D<Field> view = fields; Containers::StridedArrayView1D<Field> view = fields;
@ -4865,10 +4865,10 @@ void SceneDataTest::childrenFor() {
Int parent; Int parent;
} fields[]{ } fields[]{
{4, -1}, {4, -1},
{3, 0}, {3, 4},
{2, 1}, {2, 3},
{1, 0}, {1, 4},
{5, 0}, {5, 4},
{0, -1}, {0, -1},
}; };
Containers::StridedArrayView1D<Field> view = fields; Containers::StridedArrayView1D<Field> view = fields;

16
src/Magnum/Trade/Test/SceneToolsTest.cpp

@ -90,7 +90,7 @@ void SceneToolsTest::combine() {
const UnsignedByte meshes[]{3, 5, 17}; const UnsignedByte meshes[]{3, 5, 17};
const UnsignedShort parentObjects[]{33, 25}; const UnsignedShort parentObjects[]{33, 25};
const Short parents[]{-1, 0}; const Short parents[]{-1, 33};
const UnsignedByte translationObjects[]{16}; const UnsignedByte translationObjects[]{16};
const Vector2d translations[]{{1.5, -0.5}}; const Vector2d translations[]{{1.5, -0.5}};
@ -341,7 +341,7 @@ void SceneToolsTest::convertToSingleFunctionObjects() {
state here, without having to mess with an ArrayTuple */ state here, without having to mess with an ArrayTuple */
const UnsignedShort parentObjects[]{15, 21, 22, 23, 1}; 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 /* Two objects have two and three mesh assignments respectively, meaning we
need three extra */ need three extra */
@ -426,13 +426,13 @@ void SceneToolsTest::convertToSingleFunctionObjects() {
CORRADE_COMPARE_AS(scene.parentsAsArray(), (Containers::arrayView<Containers::Pair<UnsignedInt, Int>>({ CORRADE_COMPARE_AS(scene.parentsAsArray(), (Containers::arrayView<Containers::Pair<UnsignedInt, Int>>({
{15, -1}, {15, -1},
{21, -1}, {21, -1},
{22, 1}, {22, 21},
{23, 2}, {23, 22},
{1, -1}, {1, -1},
{63, 3}, {63, 23},
{64, 3}, {64, 23},
{65, 0}, {65, 15},
{66, 4} {66, 1}
})), TestSuite::Compare::Container); })), TestSuite::Compare::Container);
/* Meshes / materials have certain objects reassigned, field data stay the /* Meshes / materials have certain objects reassigned, field data stay the

Loading…
Cancel
Save