[fixed]ISceneNode reference count

[Ulf]

Just a quick question about ISceneNode.
I'm using some of the ideas in ISceneNode in my 2D engine.

When creating an ISceneNode you can give it a parent parameter.
If you give it a parent, it's reference counter will increase to 2.
So if you tell the parent to remove the child, it will still exist.

However when creating a clone of the ISceneNode, it drops the extra grab.

Code: Select all

ISceneNode* CBillboardSceneNode::clone(ISceneNode* newParent, ISceneManager* newManager)
{
	if (!newParent)
		newParent = Parent;
	if (!newManager)
		newManager = SceneManager;

	CBillboardSceneNode* nb = new CBillboardSceneNode(newParent, 
		newManager, ID, RelativeTranslation, Size);

	nb->cloneMembers(this, newManager);
	nb->Material = Material;

	nb->drop();
	return nb;
}

So a cloned ISceneNode will be deleted if its parent removes it, or if it is dropped once.

1. Firstly, am I correct in my assumptions?
2. Secondly just for my understanding, if I am correct, why is it set up like this?

Also, when using clone(), if you don't give the cloned ISceneNode a parent and assuming the node being cloned also does not have a parent, the newly "cloned" ISceneNode will be deleted before it is returned. As it didn't have a parent to grab it an extra time.

I'm not sure if I read the code carefully enough, but that is how it looks.

**EDIT**

Code: Select all

if (newParent)
    nb->drop();

Should it be changed to this?

[CuteAlien]

1. You seem to be correct how it works.
2. Don't know. Would you prefer something else?

As for the additional check needed. Well, yes - it looks to me like that it should need your additional check unless I'm missing something. I will take a look again when I find time on the weekend, probably there are more clone() functions in the sources so this might have to be changed in other places as well.
Thanks for reporting.

[vitek]

This is a case where it would be useful to use the template method pattern.

Code: Select all

class ISceneNode : // ...
{
public:

  // not virtual, the behavior is the same for all derived classes
  // except for the part that actually creates the clone. this avoids
  // a lot of code duplication.
  ISceneNode* clone(ISceneNode* newParent, ISceneManager* newManager)
  {
    if (!newParent)
      newParent = Parent;
    if (!newManager)
      newManager = SceneManager;

    ISceneNode* newNode = _clone(newParent, newManager);

    if (!newParent)
      newNode->drop();

    return newNode;
  }

  // ...

private:

  // overridden by derived classes, behavior is implementation dependent
  virtual ISceneNode* _clone(IScneNode* newParent, ISceneManager* newManager)
  {
    ISceneNode* newNode = new ISceneNode(newParent, newManager);
    newNode->cloneMembers(this, newManager);

    // ...
    return newNode;
  }

  // ...
};

Unfortunately, it may take some tricks to do this without breaking existing code.

Travis

[Ulf]

2. Don't know. Would you prefer something else?

No. Actually I don't yet know what I'd prefer.
I'm trying to work out what makes sense.
Though I'm quite sure that it needs the parent check at least.

@vitek
Just a typo in your code.
It should be:

Code: Select all

if ( newParent )
      newNode->drop(); 

[CuteAlien]

OK, changed it in 1.7 branch for all the clone functions.

[Ulf]

@CuteAlien.
Maybe you want to move this to bugs section so people know what has happened.

I already had a kind of discussion about this in another post.