Restructure Canvas/PropertyBasedElement

Nodes inside the osg scenegraph now hold a strong reference to
the according canvas element. Canvas elements in turn now only
hold a weak reference to the according node. This simplifies
for example the canvas::Group as it does not need to keep a
list of children on its own anymore. This is now stored inside
the scenegraph (as it was already before).

The restructuring will also allow to use a canvas::Group for
the canvas gui inside FlightGear and share for example the
handling of stacking based on z-index.

simgear/canvas/Canvas.cxx

@@ -75,9 +75,13 @@ namespace canvas
   }
 
   //----------------------------------------------------------------------------
-  Canvas::~Canvas()
+  void Canvas::onDestroy()
   {
-
+    if( _root_group )
+    {
+      _root_group->clearEventListener();
+      _root_group->onDestroy();
+    }
   }
 
   //----------------------------------------------------------------------------
@@ -111,19 +115,6 @@ namespace canvas
     return _texture.serviceable();
   }
 
-  //----------------------------------------------------------------------------
-  void Canvas::destroy()
-  {
-    if( _root_group )
-      _root_group->clearEventListener();
-
-    // TODO check if really not in use anymore
-    getProps()->getParent()
-              ->removeChild( getProps()->getName(),
-                             getProps()->getIndex(),
-                             false );
-  }
-
   //----------------------------------------------------------------------------
   void Canvas::addParentCanvas(const CanvasWeakPtr& canvas)
   {
@@ -133,7 +124,8 @@ namespace canvas
       (
         SG_GENERAL,
         SG_WARN,
-        "Canvas::addParentCanvas: got an expired parent: " << _node->getPath()
+        "Canvas::addParentCanvas(" << _node->getPath(true) << "): "
+        "got an expired parent!"
       );
       return;
     }
@@ -150,7 +142,8 @@ namespace canvas
       (
         SG_GENERAL,
         SG_WARN,
-        "Canvas::addChildCanvas: got an expired child: " << _node->getPath()
+        "Canvas::addChildCanvas(" << _node->getPath(true) << "): "
+        " got an expired child!"
       );
       return;
     }

simgear/canvas/Canvas.hxx

@@ -71,7 +71,7 @@ namespace canvas
       typedef osg::ref_ptr<CullCallback> CullCallbackPtr;
 
       Canvas(SGPropertyNode* node);
-      virtual ~Canvas();
+      virtual void onDestroy();
 
       void setSystemAdapter(const SystemAdapterPtr& system_adapter);
       SystemAdapterPtr getSystemAdapter() const;
@@ -80,7 +80,6 @@ namespace canvas
       CanvasMgr* getCanvasMgr() const;
 
       bool isInit() const;
-      void destroy();
 
       /**
        * Add a canvas which should be marked as dirty upon any change to this

simgear/canvas/elements/CanvasElement.cxx

@@ -43,19 +43,40 @@ namespace canvas
   const std::string NAME_TRANSFORM = "tf";
 
   //----------------------------------------------------------------------------
-  void Element::removeListener()
+  Element::OSGUserData::OSGUserData(ElementPtr element):
+    element(element)
   {
-    _node->removeChangeListener(this);
+
   }
 
   //----------------------------------------------------------------------------
   Element::~Element()
   {
-    removeListener();
 
+  }
+
+  //----------------------------------------------------------------------------
+  void Element::setSelf(const PropertyBasedElementPtr& self)
+  {
+    PropertyBasedElement::setSelf(self);
+
+    _transform->setUserData
+    (
+      new OSGUserData(boost::static_pointer_cast<Element>(self))
+    );
+  }
+
+  //----------------------------------------------------------------------------
+  void Element::onDestroy()
+  {
+    if( !_transform.valid() )
+      return;
+
+    // The transform node keeps a reference on this element, so ensure it is
+    // deleted.
     BOOST_FOREACH(osg::Group* parent, _transform->getParents())
     {
-      parent->removeChild(_transform);
+      parent->removeChild(_transform.get());
     }
   }
 
@@ -239,14 +260,15 @@ namespace canvas
   }
 
   //----------------------------------------------------------------------------
-  osg::ref_ptr<osg::MatrixTransform> Element::getMatrixTransform()
+  osg::MatrixTransform* Element::getMatrixTransform()
   {
-    return _transform;
+    return _transform.get();
   }
 
-  osg::ref_ptr<osg::MatrixTransform const> Element::getMatrixTransform() const
+  //----------------------------------------------------------------------------
+  osg::MatrixTransform const* Element::getMatrixTransform() const
   {
-    return _transform;
+    return _transform.get();
   }
 
   //----------------------------------------------------------------------------

simgear/canvas/elements/CanvasElement.hxx

@@ -45,6 +45,18 @@ namespace canvas
     public PropertyBasedElement
   {
     public:
+
+      /**
+       * Store pointer to window as user data
+       */
+      class OSGUserData:
+        public osg::Referenced
+      {
+        public:
+          ElementPtr element;
+          OSGUserData(ElementPtr element);
+      };
+
       typedef boost::function<bool(Element&, const SGPropertyNode*)>
               StyleSetterFunc;
       typedef boost::function<void(Element&, const SGPropertyNode*)>
@@ -61,20 +73,14 @@ namespace canvas
         std::string type;   ///!< Interpolation type
       };
 
-      /**
-       * Remove the property listener of the element.
-       *
-       * You will need to call the appropriate methods (#childAdded,
-       * #childRemoved, #valueChanged) yourself to ensure the element still
-       * receives the needed events.
-       */
-      void removeListener();
-
       /**
        *
        */
       virtual ~Element() = 0;
 
+      virtual void setSelf(const PropertyBasedElementPtr& self);
+      virtual void onDestroy();
+
       ElementWeakPtr getWeakPtr() const;
 
       /**
@@ -102,8 +108,8 @@ namespace canvas
        */
       bool isVisible() const;
 
-      osg::ref_ptr<osg::MatrixTransform> getMatrixTransform();
+      osg::MatrixTransform* getMatrixTransform();
-      osg::ref_ptr<osg::MatrixTransform const> getMatrixTransform() const;
+      osg::MatrixTransform const* getMatrixTransform() const;
 
       virtual void childAdded( SGPropertyNode * parent,
                                SGPropertyNode * child );
@@ -154,7 +160,7 @@ namespace canvas
       uint32_t _attributes_dirty;
 
       bool _transform_dirty;
-      osg::ref_ptr<osg::MatrixTransform>    _transform;
+      osg::observer_ptr<osg::MatrixTransform> _transform;
       std::vector<TransformType>              _transform_types;
 
       Style                             _style;

simgear/canvas/elements/CanvasGroup.cxx

@@ -96,25 +96,13 @@ namespace canvas
   //----------------------------------------------------------------------------
   ElementPtr Group::getChild(const SGPropertyNode* node)
   {
-    ChildList::iterator child = findChild(node);
+    return findChild(node, "");
-    if( child == _children.end() )
-      return ElementPtr();
-
-    return child->second;
   }
 
   //----------------------------------------------------------------------------
   ElementPtr Group::getChild(const std::string& id)
   {
-    for( ChildList::iterator child = _children.begin();
+    return findChild(0, id);
-                             child != _children.end();
-                           ++child )
-    {
-      if( child->second->get<std::string>("id") == id )
-        return child->second;
-    }
-
-    return ElementPtr();
   }
 
   //----------------------------------------------------------------------------
@@ -147,10 +135,10 @@ namespace canvas
   {
     std::vector<GroupPtr> groups;
 
-    BOOST_FOREACH( ChildList::value_type child, _children )
+    for(size_t i = 0; i < _transform->getNumChildren(); ++i)
     {
-      const ElementPtr& el = child.second;
+      const ElementPtr& el = getChildByIndex(i);
-      if( el->getProps()->getStringValue("id") == id )
+      if( el->get<std::string>("id") == id )
         return el;
 
       GroupPtr group = boost::dynamic_pointer_cast<Group>(el);
@@ -171,8 +159,9 @@ namespace canvas
   //----------------------------------------------------------------------------
   void Group::clearEventListener()
   {
-    BOOST_FOREACH( ChildList::value_type child, _children )
+
-      child.second->clearEventListener();
+    for(size_t i = 0; i < _transform->getNumChildren(); ++i)
+      getChildByIndex(i)->clearEventListener();
 
     Element::clearEventListener();
   }
@@ -180,8 +169,8 @@ namespace canvas
   //----------------------------------------------------------------------------
   void Group::update(double dt)
   {
-    BOOST_FOREACH( ChildList::value_type child, _children )
+    for(size_t i = 0; i < _transform->getNumChildren(); ++i)
-      child.second->update(dt);
+      getChildByIndex(i)->update(dt);
 
     Element::update(dt);
   }
@@ -190,9 +179,9 @@ namespace canvas
   bool Group::traverse(EventVisitor& visitor)
   {
     // Iterate in reverse order as last child is displayed on top
-    BOOST_REVERSE_FOREACH( ChildList::value_type child, _children )
+    for(size_t i = _transform->getNumChildren(); i --> 0;)
     {
-      if( child.second->accept(visitor) )
+      if( getChildByIndex(i)->accept(visitor) )
         return true;
     }
     return false;
@@ -210,9 +199,9 @@ namespace canvas
       return false;
 
     bool handled = false;
-    BOOST_FOREACH( ChildList::value_type child, _children )
+    for(size_t i = 0; i < _transform->getNumChildren(); ++i)
     {
-      if( child.second->setStyle(style) )
+      if( getChildByIndex(i)->setStyle(style) )
         handled = true;
     }
 
@@ -224,16 +213,17 @@ namespace canvas
   {
     osg::BoundingBox bb;
 
-    BOOST_FOREACH( ChildList::value_type child, _children )
+    for(size_t i = 0; i < _transform->getNumChildren(); ++i)
     {
-      if( !child.second->getMatrixTransform()->getNodeMask() )
+      const ElementPtr& child = getChildByIndex(i);
+      if( !child->getMatrixTransform()->getNodeMask() )
         continue;
 
       bb.expandBy
       (
-        child.second->getTransformedBounds
+        child->getTransformedBounds
         (
-          child.second->getMatrixTransform()->getMatrix() * m
+          child->getMatrixTransform()->getMatrix() * m
         )
       );
     }
@@ -255,10 +245,9 @@ namespace canvas
 
       // Add to osg scene graph...
       _transform->addChild( element->getMatrixTransform() );
-      _children.push_back( ChildList::value_type(child, element) );
 
       // ...and ensure correct ordering
-      handleZIndexChanged( --_children.end() );
+      handleZIndexChanged(element);
 
       return;
     }
@@ -279,8 +268,8 @@ namespace canvas
 
     if( _child_factories.find(node->getNameString()) != _child_factories.end() )
     {
-      ChildList::iterator child = findChild(node);
+      ElementPtr child = getChild(node);
-      if( child == _children.end() )
+      if( !child )
         SG_LOG
         (
           SG_GL,
@@ -289,8 +278,9 @@ namespace canvas
         );
       else
       {
-        _transform->removeChild( child->second->getMatrixTransform() );
+        // Remove child from the scenegraph (this automatically invalidates the
-        _children.erase(child);
+        // reference on the element hold by the MatrixTransform)
+        child->onDestroy();
       }
     }
     else
@@ -306,56 +296,52 @@ namespace canvas
   {
     if(    node->getParent()->getParent() == _node
         && node->getNameString() == "z-index" )
-      return handleZIndexChanged( findChild(node->getParent()),
+      return handleZIndexChanged( getChild(node->getParent()),
                                   node->getIntValue() );
   }
 
   //----------------------------------------------------------------------------
-  void Group::handleZIndexChanged(ChildList::iterator child, int z_index)
+  void Group::handleZIndexChanged(ElementPtr child, int z_index)
   {
-    if( child == _children.end() )
+    if( !child )
       return;
 
-    osg::Node* tf = child->second->getMatrixTransform();
+    osg::ref_ptr<osg::MatrixTransform> tf = child->getMatrixTransform();
-    int index = _transform->getChildIndex(tf),
+    size_t index = _transform->getChildIndex(tf),
            index_new = index;
 
-    ChildList::iterator next = child;
+    for(;; ++index_new)
-    ++next;
-
-    while(    next != _children.end()
-           && next->first->getIntValue("z-index", 0) <= z_index )
     {
-      ++index_new;
+      if( index_new + 1 == _transform->getNumChildren() )
-      ++next;
+        break;
+
+      // Move to end of block with same index (= move upwards until the next
+      // element has a higher index)
+      if( getChildByIndex(index_new + 1)->get<int>("z-index", 0) > z_index )
+        break;
     }
 
-    if( index_new != index )
+    if( index_new == index )
     {
-      _children.insert(next, *child);
+      // We were not able to move upwards so now try downwards
-    }
+      for(;; --index_new)
-    else
       {
-      ChildList::iterator prev = child,
+        if( index_new == 0 )
-                          check = child;
+          break;
-      while(    check != _children.begin()
+
-             && (--check)->first->getIntValue("z-index", 0) > z_index )
+        // Move to end of block with same index (= move downwards until the
-      {
+        // previous element has the same or a lower index)
-        --index_new;
+        if( getChildByIndex(index_new - 1)->get<int>("z-index", 0) <= z_index )
-        --prev;
+          break;
       }
 
       if( index == index_new )
         return;
-
-      _children.insert(prev, *child);
     }
 
     _transform->removeChild(index);
     _transform->insertChild(index_new, tf);
 
-    _children.erase(child);
-
     SG_LOG
     (
       SG_GENERAL,
@@ -365,14 +351,34 @@ namespace canvas
   }
 
   //----------------------------------------------------------------------------
-  Group::ChildList::iterator Group::findChild(const SGPropertyNode* node)
+  ElementPtr Group::getChildByIndex(size_t index) const
   {
-    return std::find_if
+    OSGUserData* ud =
-    (
+      static_cast<OSGUserData*>(_transform->getChild(index)->getUserData());
-      _children.begin(),
+    return ud->element;
-      _children.end(),
+  }
-      boost::bind(&ChildList::value_type::first, _1) == node
+
-    );
+  //----------------------------------------------------------------------------
+  ElementPtr Group::findChild( const SGPropertyNode* node,
+                               const std::string& id ) const
+  {
+    for(size_t i = 0; i < _transform->getNumChildren(); ++i)
+    {
+      ElementPtr el = getChildByIndex(i);
+
+      if( node )
+      {
+        if( el->getProps() == node )
+          return el;
+      }
+      else
+      {
+        if( el->get<std::string>("id") == id )
+          return el;
+      }
+    }
+
+    return ElementPtr();
   }
 
 } // namespace canvas

simgear/canvas/elements/CanvasGroup.hxx

@@ -99,15 +99,16 @@ namespace canvas
     protected:
 
       static ElementFactories   _child_factories;
-      ChildList                 _children;
 
       virtual void childAdded(SGPropertyNode * child);
       virtual void childRemoved(SGPropertyNode * child);
       virtual void childChanged(SGPropertyNode * child);
 
-      void handleZIndexChanged(ChildList::iterator child, int z_index = 0);
+      void handleZIndexChanged(ElementPtr child, int z_index = 0);
 
-      ChildList::iterator findChild(const SGPropertyNode* node);
+      ElementPtr getChildByIndex(size_t index) const;
+      ElementPtr findChild( const SGPropertyNode* node,
+                            const std::string& id ) const;
   };
 
 } // namespace canvas

simgear/canvas/elements/CanvasImage.cxx

@@ -350,22 +350,27 @@ namespace canvas
   //----------------------------------------------------------------------------
   void Image::setSrcCanvas(CanvasPtr canvas)
   {
-    if( !_src_canvas.expired() )
+    CanvasPtr src_canvas = _src_canvas.lock(),
-      _src_canvas.lock()->removeParentCanvas(_canvas);
+              self_canvas = _canvas.lock();
-    if( !_canvas.expired() )
-      _canvas.lock()->removeChildCanvas(_src_canvas);
 
-    _src_canvas = canvas;
+    if( src_canvas )
+      src_canvas->removeParentCanvas(self_canvas);
+    if( self_canvas )
+      self_canvas->removeChildCanvas(src_canvas);
+
+    _src_canvas = src_canvas = canvas;
     _attributes_dirty |= SRC_CANVAS;
     _geom->setCullCallback(canvas ? new CullCallback(canvas) : 0);
 
-    if( !_src_canvas.expired() )
+    if( src_canvas )
     {
       setupDefaultDimensions();
-      _src_canvas.lock()->addParentCanvas(_canvas);
 
-      if( !_canvas.expired() )
+      if( self_canvas )
-        _canvas.lock()->addChildCanvas(_src_canvas);
+      {
+        self_canvas->addChildCanvas(src_canvas);
+        src_canvas->addParentCanvas(self_canvas);
+      }
     }
   }
 

simgear/props/PropertyBasedElement.cxx

@@ -30,10 +30,29 @@ namespace simgear
 
   //------------------------------------------------------------------------------
   PropertyBasedElement::~PropertyBasedElement()
+  {
+    onDestroy();
+    removeListener();
+  }
+
+  //----------------------------------------------------------------------------
+  void PropertyBasedElement::removeListener()
   {
     _node->removeChangeListener(this);
   }
 
+  //----------------------------------------------------------------------------
+  void PropertyBasedElement::destroy()
+  {
+    if( !_node )
+      return;
+
+    // TODO check if really not in use anymore
+    if( _node->getParent() )
+      _node->getParent()
+           ->removeChild(_node->getName(), _node->getIndex(), false);
+  }
+
   //------------------------------------------------------------------------------
   SGConstPropertyNode_ptr PropertyBasedElement::getProps() const
   {

simgear/props/PropertyBasedElement.hxx

@@ -43,6 +43,20 @@ namespace simgear
       PropertyBasedElement(SGPropertyNode* node);
       virtual ~PropertyBasedElement();
 
+      /**
+       * Remove the property listener of the element.
+       *
+       * You will need to call the appropriate methods (#childAdded,
+       * #childRemoved, #valueChanged) yourself to ensure the element still
+       * receives the needed events.
+       */
+      void removeListener();
+
+      /**
+       * Destroys this element (removes node from property tree)
+       */
+      void destroy();
+
       virtual void update(double delta_time_sec) = 0;
 
       SGConstPropertyNode_ptr getProps() const;
@@ -67,6 +81,7 @@ namespace simgear
       }
 
       virtual void setSelf(const PropertyBasedElementPtr& self);
+      virtual void onDestroy() {};
 
     protected:
 

simgear/props/PropertyBasedMgr.cxx

@@ -126,6 +126,7 @@ namespace simgear
       _elements.resize(index + 1);
     }
     else if( _elements[index] )
+    {
       SG_LOG
       (
         SG_GENERAL,
@@ -133,6 +134,10 @@ namespace simgear
         _name_elements << "[" << index << "] already exists!"
       );
 
+      // Give anything holding a reference to this element to release it
+      _elements[index]->onDestroy();
+    }
+
     PropertyBasedElementPtr el = _element_factory(child);
     el->setSelf( el );
     _elements[index] = el;
@@ -158,8 +163,11 @@ namespace simgear
         "can't removed unknown " << _name_elements << "[" << index << "]!"
       );
     else
+    {
       // remove the element...
+      _elements[index]->onDestroy();
       _elements[index].reset();
     }
+  }
 
 } // namespace simgear