From a01c3f404deb6f7615f4d74183f30af8a4bb463a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Thu, 1 Jan 2015 22:35:22 +0100 Subject: [PATCH] SceneGraph: better way to traverse object hierarchies and features. Direct access to list of children is now provided through Object::children(), list of features is provided in AbstractObject::features(). In most cases the range-based-for is good enough, the previousSibling()/nextSibling() and previousFeature()/nextFeature() functions are for the cases where user needs more flexibility. Because everything that was previously done using firstChild() etc. can be now done also with children().first() etc., there would be more than one way to do the same thing. Thus the old functions are now marked as deprecated and will be removed in some future release. --- doc/scenegraph.dox | 30 ++------- src/Magnum/SceneGraph/AbstractObject.h | 74 ++++++++++++++++------- src/Magnum/SceneGraph/Object.h | 72 +++++++++++++++------- src/Magnum/SceneGraph/Object.hpp | 20 +++--- src/Magnum/SceneGraph/Test/ObjectTest.cpp | 46 +++++++++++--- src/Magnum/SceneGraph/Test/SceneTest.cpp | 4 +- 6 files changed, 156 insertions(+), 90 deletions(-) diff --git a/doc/scenegraph.dox b/doc/scenegraph.dox index 7a36d9e39..7a0ed5288 100644 --- a/doc/scenegraph.dox +++ b/doc/scenegraph.dox @@ -89,7 +89,8 @@ typedef SceneGraph::Object Object3D; Then you can start building the hierarchy by *parenting* one object to another. Parent object can be either passed in constructor or using @ref SceneGraph::Object::setParent(). Scene is always root object, so it -naturally cannot have parent object. +naturally cannot have parent object. List of object children can be accessed +through @ref SceneGraph::Object::children(). @code Scene3D scene; @@ -97,18 +98,6 @@ auto first = new Object3D(&scene); auto second = new Object3D(first); @endcode -Object children can be accessed using @ref SceneGraph::Object::firstChild() and -@ref SceneGraph::Object::lastChild(), then you can traverse siblings (objects -with the same parent) with @ref SceneGraph::Object::previousSibling() and -@ref SceneGraph::Object::nextSibling(). For example all children of an object -can be traversed the following way: -@code -Object3D* o; -for(Object3D* child = o->firstChild(); child; child = child->nextSibling()) { - // ... -} -@endcode - The hierarchy takes care of memory management - when an object is destroyed, all its children are destroyed too. See detailed explanation of @ref scenegraph-object-construction-order "construction and destruction order" @@ -133,24 +122,13 @@ have to add a *feature* to it. Each feature takes reference to holder object in constructor, so adding a feature to an object might look just like this, as in some cases you don't even -need to keep the pointer to it: +need to keep the pointer to it. List of object features is accessible through +@ref SceneGraph::Object::features(). @code Object3D* o; new MyFeature(o); @endcode -Features of an object can be accessed using @ref SceneGraph::Object::firstFeature() -and @ref SceneGraph::Object::lastFeature(), then you can traverse the features -using @ref SceneGraph::AbstractFeature::previousFeature() and -@ref SceneGraph::AbstractFeature::nextFeature(), similarly to traversing object -children: -@code -Object3D* o; -for(Object3D::FeatureType feature = o->firstFeature(); feature; feature = feature->nextFeature()) { - // ... -} -@endcode - Some features are passive, some active. Passive features can be just added to an object like above, without any additional work (for example collision shape). Active features require the user to implement some virtual function diff --git a/src/Magnum/SceneGraph/AbstractObject.h b/src/Magnum/SceneGraph/AbstractObject.h index e733f5ec4..637bfaed4 100644 --- a/src/Magnum/SceneGraph/AbstractObject.h +++ b/src/Magnum/SceneGraph/AbstractObject.h @@ -46,11 +46,20 @@ Provides minimal interface for features, not depending on object transformation implementation. This class is not directly instantiatable, use @ref Object subclass instead. See also @ref scenegraph for more information. -Uses @ref Corrade::Containers::LinkedList for storing features. Traversing -through the list is done like in the following code. It is also possible to go -in reverse order using @ref lastFeature() and @ref AbstractFeature::previousFeature(). +Uses @ref Corrade::Containers::LinkedList for efficient feature management. +Traversing through the feature list can be done using range-based for: @code -for(AbstractFeature* feature = o->firstFeature(); feature; feature = feature->nextFeature()) { +AbstractObject3D object; +for(AbstractFeature3D& feature: object.features()) { + // ... +} +@endcode + +Or, if you need more flexibility, like in the following code. It is also +possible to go in reverse order using @ref Corrade::Containers::LinkedList::last() +and @ref AbstractFeature::previousFeature(). +@code +for(AbstractFeature3D* feature = object.features().first(); feature; feature = feature->nextFeature()) { // ... } @endcode @@ -92,30 +101,51 @@ template class AbstractObject explicit AbstractObject(); virtual ~AbstractObject(); - /** @brief Whether this object has features */ - bool hasFeatures() const { - return !Containers::LinkedList>::isEmpty(); - } - - /** @brief First object feature or `nullptr`, if this object has no features */ - FeatureType* firstFeature() { - return Containers::LinkedList>::first(); + /** + * @brief Object features + * + * @see @ref AbstractFeature::object(), + * @ref AbstractFeature::previousFeature(), + * @ref AbstractFeature::nextFeature() + */ + Containers::LinkedList>& features() { + return static_cast>&>(*this); } /** @overload */ - const FeatureType* firstFeature() const { - return Containers::LinkedList>::first(); + const Containers::LinkedList>& features() const { + return static_cast>&>(*this); } - /** @brief Last object feature or `nullptr`, if this object has no features */ - FeatureType* lastFeature() { - return Containers::LinkedList>::last(); - } + #ifdef MAGNUM_BUILD_DEPRECATED + /** + * @brief Whether this object has features + * @deprecated Use `features().isEmpty()` instead. + */ + CORRADE_DEPRECATED("use features().isEmpty() instead") bool hasFeatures() const { return !features().isEmpty(); } - /** @overload */ - const FeatureType* lastFeature() const { - return Containers::LinkedList>::last(); - } + /** + * @brief First object feature or `nullptr`, if this object has no features + * @deprecated Use `features().first()` instead. + */ + CORRADE_DEPRECATED("use features().first() instead") FeatureType* firstFeature() { return features().first(); } + + /** @overload + * @deprecated Use `features().first()` instead. + */ + CORRADE_DEPRECATED("use features().first() instead") const FeatureType* firstFeature() const { return features().first(); } + + /** + * @brief Last object feature or `nullptr`, if this object has no features + * @deprecated Use `features().last()` instead.` + */ + CORRADE_DEPRECATED("use features().last() instead") FeatureType* lastFeature() { return features().last(); } + + /** @overload + * @deprecated Use `features().last()` instead. + */ + CORRADE_DEPRECATED("use features().last() instead") const FeatureType* lastFeature() const { return features().last(); } + #endif /** * @brief Scene diff --git a/src/Magnum/SceneGraph/Object.h b/src/Magnum/SceneGraph/Object.h index d1d962c1b..f9b348d10 100644 --- a/src/Magnum/SceneGraph/Object.h +++ b/src/Magnum/SceneGraph/Object.h @@ -63,11 +63,20 @@ typedef SceneGraph::Scene Scene3D; typedef SceneGraph::Object Object3D; @endcode -Uses @ref Corrade::Containers::LinkedList for parent/children relationship. -Traversing through the list is done like in the following code. It is also -possible to go in reverse order using @ref lastChild() and @ref previousSibling(). +Uses @ref Corrade::Containers::LinkedList for efficient hierarchy management. +Traversing through the list of child objects can be done using range-based for: @code -for(Object* child = o->firstChild(); child; child = child->nextSibling()) { +Object3D o; +for(AbstractFeature3D& feature: o.features()) { + // ... +} +@endcode + +Or, if you need more flexibility, like in the following code. It is also +possible to go in reverse order using @ref Corrade::Containers::LinkedList::last() +and @ref previousSibling(). +@code +for(Object3D* child = o->children().first(); child; child = child->nextSibling()) { // ... } @endcode @@ -175,30 +184,49 @@ template class Object: public AbstractObject, Object>::next(); } - /** @brief Whether this object has children */ - bool hasChildren() const { - return !Containers::LinkedList>::isEmpty(); - } - - /** @brief First child object or `nullptr`, if this object has no children */ - Object* firstChild() { - return Containers::LinkedList>::first(); + /** + * @brief Child objects + * + * @see @ref parent(), @ref previousSibling(), @ref nextSibling() + */ + Containers::LinkedList>& children() { + return static_cast>&>(*this); } /** @overload */ - const Object* firstChild() const { - return Containers::LinkedList>::first(); + const Containers::LinkedList>& children() const { + return static_cast>&>(*this); } - /** @brief Last child object or `nullptr`, if this object has no children */ - Object* lastChild() { - return Containers::LinkedList>::last(); - } + #ifdef MAGNUM_BUILD_DEPRECATED + /** + * @brief Whether this object has children + * @deprecated Use `children().isEmpty()` instead. + */ + CORRADE_DEPRECATED("use children().isEmpty()") bool hasChildren() const { return !children().isEmpty(); } - /** @overload */ - const Object* lastChild() const { - return Containers::LinkedList>::last(); - } + /** + * @brief First child object or `nullptr`, if this object has no children + * @deprecated Use `children().first()` instead. + */ + CORRADE_DEPRECATED("use children().first()") Object* firstChild() { return children().first(); } + + /** @overload + * @deprecated Use `children.first()` instead. + */ + CORRADE_DEPRECATED("use children().first()") const Object* firstChild() const { return children().first(); } + + /** + * @brief Last child object or `nullptr`, if this object has no children + * @deprecated Use `children().last()` instead. + */ + CORRADE_DEPRECATED("use children().last()") Object* lastChild() { return children().last(); } + + /** @overload + * @deprecated Use `children().last()` instead. + */ + CORRADE_DEPRECATED("use children().last()") const Object* lastChild() const { return children().last(); } + #endif /** * @brief Set parent object diff --git a/src/Magnum/SceneGraph/Object.hpp b/src/Magnum/SceneGraph/Object.hpp index a08a0c8d4..b78aa8eba 100644 --- a/src/Magnum/SceneGraph/Object.hpp +++ b/src/Magnum/SceneGraph/Object.hpp @@ -165,15 +165,13 @@ template void Object::setDirty() { nothing to do */ if(flags & Flag::Dirty) return; - Object* self = static_cast*>(this); - /* Make all features dirty */ - for(AbstractFeature* i = self->firstFeature(); i; i = i->nextFeature()) - i->markDirty(); + for(AbstractFeature& feature: this->features()) + feature.markDirty(); /* Make all children dirty */ - for(Object* i = self->firstChild(); i; i = i->nextSibling()) - i->setDirty(); + for(Object& child: children()) + child.setDirty(); /* Mark object as dirty */ flags |= Flag::Dirty; @@ -510,28 +508,28 @@ template void Object::setCleanInternal(con MatrixType matrix, invertedMatrix; /* Clean all features */ - for(AbstractFeature* i = this->firstFeature(); i; i = i->nextFeature()) { + for(AbstractFeature& feature: this->features()) { /* Cached absolute transformation, compute it if it wasn't computed already */ - if(i->cachedTransformations() & CachedTransformation::Absolute) { + if(feature.cachedTransformations() & CachedTransformation::Absolute) { if(!(cached & CachedTransformation::Absolute)) { cached |= CachedTransformation::Absolute; matrix = Implementation::Transformation::toMatrix(absoluteTransformation); } - i->clean(matrix); + feature.clean(matrix); } /* Cached inverse absolute transformation, compute it if it wasn't computed already */ - if(i->cachedTransformations() & CachedTransformation::InvertedAbsolute) { + if(feature.cachedTransformations() & CachedTransformation::InvertedAbsolute) { if(!(cached & CachedTransformation::InvertedAbsolute)) { cached |= CachedTransformation::InvertedAbsolute; invertedMatrix = Implementation::Transformation::toMatrix( Implementation::Transformation::inverted(absoluteTransformation)); } - i->cleanInverted(invertedMatrix); + feature.cleanInverted(invertedMatrix); } } diff --git a/src/Magnum/SceneGraph/Test/ObjectTest.cpp b/src/Magnum/SceneGraph/Test/ObjectTest.cpp index cb5aefe57..32cb403ef 100644 --- a/src/Magnum/SceneGraph/Test/ObjectTest.cpp +++ b/src/Magnum/SceneGraph/Test/ObjectTest.cpp @@ -46,6 +46,9 @@ class ObjectTest: public TestSuite::Tester { void setClean(); void setCleanListHierarchy(); void setCleanListBulk(); + + void rangeBasedForChildren(); + void rangeBasedForFeatures(); }; typedef SceneGraph::Object Object3D; @@ -76,7 +79,10 @@ ObjectTest::ObjectTest() { &ObjectTest::transformationsDuplicate, &ObjectTest::setClean, &ObjectTest::setCleanListHierarchy, - &ObjectTest::setCleanListBulk}); + &ObjectTest::setCleanListBulk, + + &ObjectTest::rangeBasedForChildren, + &ObjectTest::rangeBasedForFeatures}); } void ObjectTest::parenting() { @@ -87,9 +93,9 @@ void ObjectTest::parenting() { CORRADE_VERIFY(childOne->parent() == &root); CORRADE_VERIFY(childTwo->parent() == &root); - CORRADE_VERIFY(root.firstChild() == childOne); - CORRADE_VERIFY(root.lastChild() == childTwo); - CORRADE_VERIFY(root.firstChild()->nextSibling() == root.lastChild()); + CORRADE_VERIFY(root.children().first() == childOne); + CORRADE_VERIFY(root.children().last() == childTwo); + CORRADE_VERIFY(root.children().first()->nextSibling() == root.children().last()); /* A object cannot be parent of itself */ childOne->setParent(childOne); @@ -101,12 +107,12 @@ void ObjectTest::parenting() { /* Reparent to another */ childTwo->setParent(childOne); - CORRADE_VERIFY(root.firstChild() == childOne && root.firstChild()->nextSibling() == nullptr); - CORRADE_VERIFY(childOne->firstChild() == childTwo && childOne->firstChild()->nextSibling() == nullptr); + CORRADE_VERIFY(root.children().first() == childOne && root.children().first()->nextSibling() == nullptr); + CORRADE_VERIFY(childOne->children().first() == childTwo && childOne->children().first()->nextSibling() == nullptr); /* Delete child */ delete childTwo; - CORRADE_VERIFY(!childOne->hasChildren()); + CORRADE_VERIFY(childOne->children().isEmpty()); } void ObjectTest::scene() { @@ -455,6 +461,32 @@ void ObjectTest::setCleanListBulk() { CORRADE_COMPARE(d.cleanedAbsoluteTransformation, Matrix4::translation(Vector3::zAxis(3.0f))*Matrix4::scaling(Vector3(-2.0f))); } +void ObjectTest::rangeBasedForChildren() { + Scene3D scene; + Object3D a(&scene); + Object3D b(&scene); + Object3D c(&scene); + + std::vector objects; + for(auto&& i: scene.children()) objects.push_back(&i); + CORRADE_COMPARE(objects, (std::vector{&a, &b, &c})); +} + +void ObjectTest::rangeBasedForFeatures() { + struct Feature: AbstractFeature3D { + Feature(AbstractObject3D& object): AbstractFeature3D{object} {} + }; + + Object3D object; + Feature a(object); + Feature b(object); + Feature c(object); + + std::vector features; + for(auto&& i: object.features()) features.push_back(&i); + CORRADE_COMPARE(features, (std::vector{&a, &b, &c})); +} + }}} CORRADE_TEST_MAIN(Magnum::SceneGraph::Test::ObjectTest) diff --git a/src/Magnum/SceneGraph/Test/SceneTest.cpp b/src/Magnum/SceneGraph/Test/SceneTest.cpp index 25d9b789b..48196c8b7 100644 --- a/src/Magnum/SceneGraph/Test/SceneTest.cpp +++ b/src/Magnum/SceneGraph/Test/SceneTest.cpp @@ -62,8 +62,8 @@ void SceneTest::parent() { Object3D object; scenePointer->setParent(&object); CORRADE_VERIFY(scene.parent() == nullptr); - CORRADE_VERIFY(!scene.hasChildren()); - CORRADE_VERIFY(!object.hasChildren()); + CORRADE_VERIFY(scene.children().isEmpty()); + CORRADE_VERIFY(object.children().isEmpty()); } }}}