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

@@ -89,7 +89,8 @@ typedef SceneGraph::Object<SceneGraph::MatrixTransformation3D> 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

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
+Uses @ref Corrade::Containers::LinkedList for efficient feature management.
-through the list is done like in the following code. It is also possible to go
+Traversing through the feature list can be done using range-based for:
-in reverse order using @ref lastFeature() and @ref AbstractFeature::previousFeature().
 @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<UnsignedInt dimensions, class T> class AbstractObject
         explicit AbstractObject();
         virtual ~AbstractObject();
 
-        /** @brief Whether this object has features */
+        /**
-        bool hasFeatures() const {
+         * @brief Object features
-            return !Containers::LinkedList<AbstractFeature<dimensions, T>>::isEmpty();
+         *
-        }
+         * @see @ref AbstractFeature::object(),
-
+         *      @ref AbstractFeature::previousFeature(),
-        /** @brief First object feature or `nullptr`, if this object has no features */
+         *      @ref AbstractFeature::nextFeature()
-        FeatureType* firstFeature() {
+         */
-            return Containers::LinkedList<AbstractFeature<dimensions, T>>::first();
+        Containers::LinkedList<AbstractFeature<dimensions, T>>& features() {
+            return static_cast<Containers::LinkedList<AbstractFeature<dimensions, T>>&>(*this);
         }
 
         /** @overload */
-        const FeatureType* firstFeature() const {
+        const Containers::LinkedList<AbstractFeature<dimensions, T>>& features() const {
-            return Containers::LinkedList<AbstractFeature<dimensions, T>>::first();
+            return static_cast<const Containers::LinkedList<AbstractFeature<dimensions, T>>&>(*this);
         }
 
-        /** @brief Last object feature or `nullptr`, if this object has no features */
+        #ifdef MAGNUM_BUILD_DEPRECATED
-        FeatureType* lastFeature() {
+        /**
-            return Containers::LinkedList<AbstractFeature<dimensions, T>>::last();
+         * @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 {
+         * @brief First object feature or `nullptr`, if this object has no features
-            return Containers::LinkedList<AbstractFeature<dimensions, T>>::last();
+         * @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

src/Magnum/SceneGraph/Object.h

@@ -63,11 +63,20 @@ typedef SceneGraph::Scene<SceneGraph::MatrixTransformation3D> Scene3D;
 typedef SceneGraph::Object<SceneGraph::MatrixTransformation3D> Object3D;
 @endcode
 
-Uses @ref Corrade::Containers::LinkedList for parent/children relationship.
+Uses @ref Corrade::Containers::LinkedList for efficient hierarchy management.
-Traversing through the list is done like in the following code. It is also
+Traversing through the list of child objects can be done using range-based for:
-possible to go in reverse order using @ref lastChild() and @ref previousSibling().
 @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 Transformation> class Object: public AbstractObject<Transformatio
             return Containers::LinkedListItem<Object<Transformation>, Object<Transformation>>::next();
         }
 
-        /** @brief Whether this object has children */
+        /**
-        bool hasChildren() const {
+         * @brief Child objects
-            return !Containers::LinkedList<Object<Transformation>>::isEmpty();
+         *
-        }
+         * @see @ref parent(), @ref previousSibling(), @ref nextSibling()
-
+         */
-        /** @brief First child object or `nullptr`, if this object has no children */
+        Containers::LinkedList<Object<Transformation>>& children() {
-        Object<Transformation>* firstChild() {
+            return static_cast<Containers::LinkedList<Object<Transformation>>&>(*this);
-            return Containers::LinkedList<Object<Transformation>>::first();
         }
 
         /** @overload */
-        const Object<Transformation>* firstChild() const {
+        const Containers::LinkedList<Object<Transformation>>& children() const {
-            return Containers::LinkedList<Object<Transformation>>::first();
+            return static_cast<const Containers::LinkedList<Object<Transformation>>&>(*this);
         }
 
-        /** @brief Last child object or `nullptr`, if this object has no children */
+        #ifdef MAGNUM_BUILD_DEPRECATED
-        Object<Transformation>* lastChild() {
+        /**
-            return Containers::LinkedList<Object<Transformation>>::last();
+         * @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<Transformation>* lastChild() const {
+         * @brief First child object or `nullptr`, if this object has no children
-            return Containers::LinkedList<Object<Transformation>>::last();
+         * @deprecated Use `children().first()` instead.
-        }
+         */
+        CORRADE_DEPRECATED("use children().first()") Object<Transformation>* firstChild() { return children().first(); }
+
+        /** @overload
+         * @deprecated Use `children.first()` instead.
+         */
+        CORRADE_DEPRECATED("use children().first()") const Object<Transformation>* 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<Transformation>* lastChild() { return children().last(); }
+
+        /** @overload
+         * @deprecated Use `children().last()` instead.
+         */
+        CORRADE_DEPRECATED("use children().last()") const Object<Transformation>* lastChild() const { return children().last(); }
+        #endif
 
         /**
          * @brief Set parent object

src/Magnum/SceneGraph/Object.hpp

@@ -165,15 +165,13 @@ template<class Transformation> void Object<Transformation>::setDirty() {
        nothing to do */
     if(flags & Flag::Dirty) return;
 
-    Object<Transformation>* self = static_cast<Object<Transformation>*>(this);
-
     /* Make all features dirty */
-    for(AbstractFeature<Transformation::Dimensions, typename Transformation::Type>* i = self->firstFeature(); i; i = i->nextFeature())
+    for(AbstractFeature<Transformation::Dimensions, typename Transformation::Type>& feature: this->features())
-        i->markDirty();
+        feature.markDirty();
 
     /* Make all children dirty */
-    for(Object<Transformation>* i = self->firstChild(); i; i = i->nextSibling())
+    for(Object<Transformation>& child: children())
-        i->setDirty();
+        child.setDirty();
 
     /* Mark object as dirty */
     flags |= Flag::Dirty;
@@ -510,28 +508,28 @@ template<class Transformation> void Object<Transformation>::setCleanInternal(con
     MatrixType matrix, invertedMatrix;
 
     /* Clean all features */
-    for(AbstractFeature<Transformation::Dimensions, typename Transformation::Type>* i = this->firstFeature(); i; i = i->nextFeature()) {
+    for(AbstractFeature<Transformation::Dimensions, typename Transformation::Type>& 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<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<Transformation>::toMatrix(
                     Implementation::Transformation<Transformation>::inverted(absoluteTransformation));
             }
 
-            i->cleanInverted(invertedMatrix);
+            feature.cleanInverted(invertedMatrix);
         }
     }
 

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<SceneGraph::MatrixTransformation3D> 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.children().first() == childOne);
-    CORRADE_VERIFY(root.lastChild() == childTwo);
+    CORRADE_VERIFY(root.children().last() == childTwo);
-    CORRADE_VERIFY(root.firstChild()->nextSibling() == root.lastChild());
+    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(root.children().first() == childOne && root.children().first()->nextSibling() == nullptr);
-    CORRADE_VERIFY(childOne->firstChild() == childTwo && childOne->firstChild()->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<Object3D*> objects;
+    for(auto&& i: scene.children()) objects.push_back(&i);
+    CORRADE_COMPARE(objects, (std::vector<Object3D*>{&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<AbstractFeature3D*> features;
+    for(auto&& i: object.features()) features.push_back(&i);
+    CORRADE_COMPARE(features, (std::vector<AbstractFeature3D*>{&a, &b, &c}));
+}
+
 }}}
 
 CORRADE_TEST_MAIN(Magnum::SceneGraph::Test::ObjectTest)

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(scene.children().isEmpty());
-    CORRADE_VERIFY(!object.hasChildren());
+    CORRADE_VERIFY(object.children().isEmpty());
 }
 
 }}}