Dropping scene nodes

[Darktib]

Hello,

Currently, when you want to remove a scene node, you call remove(). If you want to be a bit more flexible, then you have to use grab() / drop(), and the problem is that the drop() deleting the node will not update the node parent, since the destructor of ISceneNode does not update it.

Why is this ? It is a lot better to have things like:

Code: Select all

 
node->grab();
node->drop();
node->grab();
node->drop();
node->drop(); // delete
 

Also, I could try to make a patch fixing this bug.

PS: I searched the forum and didn't found subjects like this.

[hybrid]

Well, the remove call basically only removes the node from the scene graph and does some internal reference handling (which starts with add* and is then closed by remove). You always have to deal with grab/drop if you want to work with the scene node on your own. But if you don't want to deal with the node at all, as it is maybe just some automatically managed thing which exists the whole time, you can forget about that node. So after you added it to the graph, you are allowed to hand that node over to the engine completely.
If you don't want that, you have to deal with grab and drop, and you have to do that symmetrically. That's how this stuff works. Calling drop once more is illogical and unintuitive in this case.

[Darktib]

Well, I know that, but I find it unconvenient.

My point is that the scene node destructor should inform its parent that it will no longer be available.

[CuteAlien]

No, because If the parent still has a reference to the SceneNode at the point where the node is deleted then your code is already wrong probably (it should always first release the parent and then delete or the parent has invalid children pointers at some point). Also calling setParent in the destuctor would be wrong because you should never call virtual functions in a destructor. And calling it on each drop() would be even more wrong - because you have to be able to call drop() _without_ removing it from the SceneGraph (such things are more or less the reason to even have reference-counting).

edit: As an example - think about what would happen in the current ISceneNode::removeAll() when the child nodes would start deleting themselves from their parent in their destructor. That's why it wouldn't get safer or easier - just lead to another categorie of bugs (and slower code in this case to avoid the troubles).

[Darktib]

And calling it on each drop() would be even more wrong - because you have to be able to call drop() _without_ removing it from the SceneGraph (such things are more or less the reason to even have reference-counting).

I agree with it, but that is not the point.

Also calling setParent in the destuctor would be wrong because you should never call virtual functions in a destructor

edit: As an example - think about what would happen in the current ISceneNode::removeAll() when the child nodes would start deleting themselves from their parent in their destructor. That's why it wouldn't get safer or easier - just lead to another categorie of bugs (and slower code in this case to avoid the troubles).

These are not real problems. I agree that the code will be a bit slower, but since you don't add/delete scene nodes every frame, it is not a concern. What is possible is adding the following protected method to ISceneNode:

Code: Select all

 
protected:
    virtual void removeChildrenFromDestructor(ISceneNode* c)
    {
        ISceneNodeList::Iterator it = Children.begin();
        for (; it != Children.end(); ++it)
            if ((*it) == child)
            {
                Children.erase(it);
                return;
            }
    }
 

Then, in the destructor:

Code: Select all

 
ISceneNode::~ISceneNode()
{
    if(Parent)
        Parent->removeChildrenFromDestructor(this);
    Parent = 0;
    // usual stuff
}
 

The parent will be informed that one of his children is not available anymore:
- if the parent is not being destroyed, then the virtual function call will call the (maybe) overriden function.
- if the parent is being destroyed, then it is a call to the ISceneNode::removeChildFromDestructor. That means the derived type is already deleted and all variables pointing to children are that were defined in the derived class are not present anymore. In short, there are no references to children from derived parent object, since this layer was already deleted. The remaining references are in ISceneNode, and the called function can deal with them.

A call from removeAll() or removechild() will not call removeChildFromDestructor(), since:
- parent call removeAll() or removeChild()
- node parent is set to 0 (current behavior)
- node get dropped
- node is deleted
- condition for calling removeChildFromDestructor() is not met : parent == 0...

I've done a simple example here: http://codepad.org/MhVObi60
(with inheritance, I hope it is a bit readable. You can see the current Irrlicht implementation behavior here: http://codepad.org/LJDpmXG0).

edit: forgot to say... since removeChildFromDestructor() is virtual, it is possible to reimplement it if your derived object stores additionnal references to children.

[CuteAlien]

Yeah, it would be possible to kinda get this work. Thought in the above solution you would have to remember to always reset an internal member (the parent) when deleting in a loop.

But even ignoring that - I think an object should never try to figure out where it is used and then clean up those parts in it's destructor. A class should clean up the pointers it is using - not the pointer which are using it. That the object is in this case an ISceneNode which is used by another ISceneNode confuses that point a little bit. But I still think the current way it's less bug prone.

The class would probably have been designed better if setParent, addChild and remove would be in ISceneManager instead. Because all those functions do modify the scenegraph and are not so much about the scenenode itself.

I mean - I get your point and I suppose you can argue in both directions here. But right now I prefer the current way. Also changing it would break applications out there which are expecting the current behaviour and that is always very bad.

[Darktib]

I mean - I get your point and I suppose you can argue in both directions here. But right now I prefer the current way. Also changing it would break applications out there which are expecting the current behaviour and that is always very bad.

Fair enough (even though it would not change anything for existing applications as dropping then removing will still work the same as before).