fully deleting objects

[Rick]

I'm looking at some old tower defense code I had and ran into a problem and was hoping someone could maybe help me out. I have an Actor class which has a model entity in it. I also have a Projectile class that stores a pointer to an Actor as it's target. The Update() method of the Projectile class gets the actors position via it's model and moves towards it. What can happen though is that the actor object can be killed while the projectile is moving towards it. I only delete actors in 1 spot in where I use:

 

delete actor;

actor = NULL;

 

In my projectile Update() method I do NULL checks against the target actor but it seems to pass these checks and go into an LE function where the model is a <bad ptr> and bomb out. I'm deleting these actors and setting them to NULL (and in the destructor I free the model and set it to NULL) so not sure if I'm missing something.

 

When the error happens and it breaks in the code I notice the target pointer address is still 0x0c917030. All the variables in the class are filled with garbage (not the actual values they were filled with) so it's like it's sort of deleting the class but doesn't fully set the pointer of the class to NULL and so it passes the if(_target) check. I can tell all of this when I mouse over it in VS. If it's truly NULL I would think it would just say NULL and not give all the details about the variables in the class so it leads me to believe it's not truly getting deleted, but besides calling the delete statement on it and setting to NULL I'm not sure what else to do.

 

The code is long and complex, but any general tips around deleting objects and why they might not get "fully" deleted?

[Rick]

Maybe I need to look into shared_ptr's or a pointer to a pointer when passing these around.

[Mumbles]

Sounds like a stray pointer issue to me... Are you using any pointers to your Actor class?

 

Actor * Blah = new Actor(BlahBlah);
Actor * ActorPtr = &Blah; //ActorPtr might be a void pointer in another class (like "UserData") but you get the idea
...
delete Blah;
Blah = 0;

//ActorPtr still thinks the actor is valid - attempting to use it will crash

[Rick]

Nice pic mumbles! Yeah what you have there is exactly what's happening. I'm going to use a pointer to a pointer and check against NULL with that. Tested it out in a smaller app and that seems to work.

 

I don't want a smart pointer because sounds like as long as there are references to it it'll stay alive, but I need my actor dead the moment I call delete on it even if other classes have it. With pointer to pointers I can check against that original pointer memory itself for NULL instead of what it was pointing to.

 

class Actor
{
public:
Actor() { i = 50; }
int i;
};
class Projectile
{
public:
Actor** a;
};

int _tmain(int argc, _TCHAR* argv[])
{
Actor* a = new Actor();
Projectile p;
p.a = &a;
delete a;
a = NULL;

// it won't get inside this check because it's NULL which is what I'm looking for
if((*p.a))
{
int test;
test = (*p.a)->i;
}

return 0;
}

[Mumbles]

Would you not write p.a = a? since Projectile::a is a pointer to a pointer, and _tmain::a is already a pointer... That would allow a "straight passthrough"

 

I could be wrong, but it looks like p.a isn't actually pointing to _tmain::a's object instance - it's pointing to where _tmain::a's pointer is located...

 

 

p.a = &a would be correct if your actor object instance wasn't created by pointer... as in:

 

Actor a();

 

rather than

 

Actor* a = new Actor();

[Rick]

I could be wrong, but it looks like p.a isn't actually pointing to a - it's pointing to where _tmain::a's pointer is located...

 

But that's what I want right. I want the pointers memory location (hence pointer to a pointer) so that when that gets set to NULL after I deleted what it was pointing to, I can then check the pointers memory location against NULL to tell me what it's pointing to has been deleted.

 

This example doesn't show 100% why I want that because it's checking the NULL in the main function and I could just check 'a' for null there, BUT let's say I was checking for NULL inside the projectile class. The NULL check would always pass even if I deleted a in the main function (if I didn't use a pointer to a pointer). So from other classes the fact that the pointer to the pointer is NULL is telling me that what it was pointing to was deleted and I should not call anything with it.

[Mumbles]

But you can't delete where _tmain::a's pointer is located in memory because you didn't allocate it. That was allocated implicitly when it was created. You can delete the object it points to but not the pointer itself. That will be lost when it falls out of scope same as any other variable, although what it points to stays valid until explicitly deleted - hence what memory leaks are.

[Rick]

This example is to confusing because of variables used (2 a's) and all done in main. Writing another one up that will be easier to talk about

[Rick]

This probably better shows what I'm wanting. Actor is created in main and passed to Projectile. It's then destroyed in main, but we need a way to tell projectile that it's destroyed and to not use it. By using a pointer to a pointer and checking what it's pointing to is NULL (because I set 'a' to NULL after deleting what it was pointing to) allows me to do this. If I just passed the original pointer I would have no way of knowing the actor was deleted and so I shouldn't call any members of it.

 

The reason I need to do this is because I can have 5 projectiles all moving towards this actor. When one of them hit it, it'll kill the actor which will then be deleted invalidating all the other targets for the remaining 4 projectiles. I want the actor to die and be deleted instantly, so this raised a problem because the other 4 projectiles no don't have a valid target. I get around this by setting a pivot to the target's location AS LONG AS the target is valid. However just checking a regular pointer wasn't working because the pointer in projectile to the actor wasn't NULL, it was still pointing to the same memory location which now has garbage in it.

 

class Actor
{
private:
  int i;
public:
  Actor() { i = 50; }
  void Increase() { i++; }
};


class Projectile
{
private:
  Actor** _actor;
public:
  Projectile(Actor** actor) { _actor = actor; }
  void Update()
  {
  if((*_actor))
     (*_actor)->Increase();
  }
};

int _tmain(int argc, _TCHAR* argv[])
{
  Actor* a = new Actor();
  Projectile p(&a);;
  delete a;
  a = NULL;

  p.Update();
  return 0;
}

 

In this example it gives me what I want. Trying to translate it to my game doesn't seem to give me what I want yet. hmmm

[Mumbles]

OK, let's start at _tmain...

 

 

I'm going to show where I've allocated memory but with low numbers just for example. Remember, we can't normally choose where our variables are allocated. Addresses in red, their values in blue.

 

 

 

New variable a located at memory position 100 (for example). It is a pointer, so we create an Actor object at 150 for example. value of _tmain::a is therefore 150

 

 

 

New variable p located at memory position 200. It is not a pointer or a primitive, so it has no value of it's own.

p has a pointer member _actor, it is stored at 210, it has no value at this time

 

 

_tmain:._actor is located at 210, the constructor for _tmain: has assigned it a value of 100, this would probably be better as 150, by leaving off the & sign

 

a has been deleted. Value at 100 is now 0

 

p.update has been called.

 

Value of p::_actor checked. It is 100, so it is valid. We attempt to call a function - but this object instance is not there. Runtime error

 

 

Now, if Projectile::_actor was assigned 150 by the constructor rather than 100 by simply passed a, rather than address of a. Then when a is deleted in _tmain, Update's if check would return a value of 0, not 100 and thus refuse to run the block.

 

edit: emoticons disabled

[Mumbles]

Basically, every red should be paired off with a blue, but your Projectile::_actor has two reds instead

[Rick]

Now, if Projectile::_actor was assigned 150 by the constructor rather than 100 by simply passed a, rather than address of a. Then when a is deleted in _tmain, Update's if check would return a value of 0, not 100 and thus refuse to run the block.

 

Below is a modified version of what I posted and seems to be what you are suggesting above. I pass 'a' instead of the address of 'a'. However this is not correct. If you run it and step through it you'll see that the if check passes, and it calls the Increase() method and the _actor is pointing to garbage.

 

class Actor
{
private:
  int i;
public:
  Actor() { i = 50; }
  void Increase() { i++; }
};

class Projectile
{
private:
  Actor* _actor;
public:
  Projectile(Actor* actor) { _actor = actor; }
  void Update()
  {
     if(_actor)
        _actor->Increase();
  }
};

int _tmain(int argc, _TCHAR* argv[])
{
  Actor* a = new Actor();
  Projectile p(a);
 delete a;
  a = NULL;
  p.Update();
  return 0;
}

[Mumbles]

You have changed _actor's definition from Actor** to Actor *

 

Put it back to Actor** but leave Projectile p(a); as it is.

[Rick]

Can't get that to work because then Projectile ctor has to be Actor** and if you do that Projectile p(a); fails because it's expecting a pointer to a pointer not just a pointer. If I don't change Projectile ctor then the assignment in the ctor fails because of the same reason: Projectile(Actor* actor) { _actor = actor; }

 

If anything is defined as a pointer to a pointer then it needs to point to the memory location of a pointer right. It can't just point to a pointer.

 

If you're able to get my example to run in how you are saying please post it because it doesn't seem to work the way you are saying it does.

[Rick]

Interestingly enough the other element I have in my game is that these actors are stored in a list, and removed from the list when they are deleted via remove_if(). This seems to give different behavior as well. Now it passes the if check with the following below. Guessing the pointer has gone out of scope once it was removed from the list so now the pointer to the pointer is pointing at garbage.

 

class Actor
{
private:
   int i;
public:
   Actor() { i = 50; }
   void Increase() { i++; }
};
class Projectile
{
private:
   Actor** _actor;
public:
   Projectile(Actor** actor) { _actor = actor; }
   void Update()
   {
   if((*_actor))
       (*_actor)->Increase();
   }
};
bool DeleteActor(Actor* a)
{
   delete a;
   a = NULL;
   return true;
}
int _tmain(int argc, _TCHAR* argv[])
{
   list<Actor*> lst;
   Actor* a = new Actor();

   lst.push_back(a);
   Projectile p(&a);
   lst.remove_if(DeleteActor);
   //delete a;
   //a = NULL;
   p.Update();
   return 0;
}

[Mumbles]

OK yep, not quite what I was thinking, but does this work?

 

 

class Actor
{
private:
   int i;
public:
   Actor() { i = 50; }
   void Increase() { i++; }
};

class Projectile
{
private:
   Actor** _actor;
public:
   Projectile(Actor** actor) { _actor = actor; }
   void Update()
   {
   if(_actor[0])
       _actor[0]->Increase();
   }

};

int main(int argc, char * argv[])
{
   Actor * a = new Actor();
   Projectile p((Actor**)a);
   delete a;
   a = 0;
   p.Update();
   return 0;
}

[Rick]

Aha, the list needs to store a pointer to a pointer as well for this to work!

 

 

That may work but you're casting instead of passing the address of the pointer. Not sure what the difference would be between that and doing Projectile p (&a);

[Mumbles]

Yes I must have been sniffing permanent markers earlier because I was sure that p(a) was valid, but sure enough the compiler didn't like it.

 

There may not be a difference but the way I look it, the cast means I'm still pointing to a pointer, rather than pointing to a pointer's address, which perhaps it's just me, but that sounds wrong.

[Rick]

Dangit, now I have another issue with this It's a rabbit hole I tell you.

 

Now in my list I store a pointer to a pointer. If I have a function that creates the Actor*, then I add the memory of that point to the list, as soon as the function is done (scope) that Actor* isn't valid anymore. That seems odd to me because without any delete I would think Actor* would still be valid, but I guess that variable is out of scope once the function leaves (but the object is still created and in scope).

 

list<Actor**> lst;
void CreateEnemy()
{
Actor* a = new Actor();

lst.push_back(&a);
}
int _tmain(int argc, _TCHAR* argv[])
{
CreateEnemy();
// doesn't work because the pointer to a pointer now points to nothing
Projectile p((lst.front()));
}

 

 

This makes sense since 'a' itself (not it's object) is local to the function. However I need a way to get around this.

[Mumbles]

Note my proposed change to your Actor Projectile (whoops) class, and where it is deleted in main...

 

Yes, we deleted a in _tmain before. but as far as p was concerned, it was still there...

 

 

class Actor
{
private:
   int i;
public:
   Actor() { i = 50; }
   void Increase() { i++; }
};

class Projectile
{
private:
Actor* _actor;
public:
   Projectile(Actor* actor) { _actor = actor; }
   void Update()
   {
       if(_actor)
           _actor->Increase();
   }

   void DeleteActor(void)
   {
   delete _actor;
   _actor = 0;//This is what we weren't doing before, hence why the if would still run
   }
};

int main(int argc, char * argv[])
{
   Actor* a = new Actor();
   Projectile p(a);
   //delete a;
   p.DeleteActor(); //Presumably, your projectile class could be in charge of allocating its own Actors in the same way, but would obviously need a rewritten constructor
   a = 0; //Ideally, your projectile class does do it's own allocation and destruction. If it "new"s its Actor, then this isn't necessary
   p.Update(); 

   return 0;
}

[Rick]

The thing is I have a list of these actors in my Gameplay class (where they get created from originally) and in there it's looping through them each frame calling Update() and Draw() functions on them. If I delete the Projectiles pointer of the actor it'll work for the Projectile class but the Gameplay classes list of these will then have the same issue. Gameplay won't know that projectile delete it. So same issue just on the other side.

[Mumbles]

Maybe it's just my way of thinking. But I know in my project I have a base "GameEntity" class (which I think translates to your Actor class), that has a protected constructor. The way I'm set up, there is no reason for me to be creating these entities. A light, a door, a character, a weapon etc - they're all types of entities, and they inherit from it, so they can call the constructor themselves when they need it.

 

 

If it were my project, it's likely that the Actor would not have a public constructor, and any type of Actor would either be a derived class, or a friend class. That doesn't mean there can't be a list of Actors available to read in the main gameplay class (or namespace as I have it), it's just that if I want to add an object to it, I have to create one of these special types instead.

 

 

 

If I delete the Projectiles pointer of the actor it'll work for the Projectile class but the Gameplay classes list of these will then have the same issue.

 

I can't see why you would want to delete a projectile's associated actor but not delete the rest of the projectile. Now obviously I'm sure you don't want to reveal too much about your game, because surprises are nice - but personally, I would just delete the entire projectile, which would delete its actor as part of its clean up.

 

 

Now please don't take this as a lecture, because I'm sure of the two of us, you'd normally be the one teaching me...

[Rick]

I would just delete the entire projectile, which would delete its actor as part of its clean up.

 

Because to me the projectile doesn't own the actor, it's just using it for information (it's location so it can move towards it). When it reaches the actor it calls a TakeDamage() but that doesn't always kill the Actor it just gives it dmg. Inside TakeDamage() the actor determines if it's dead or not and sets an alive flag. Then outside in the gameplay class where the actor was originally created is where it checks the alive flag of these actors from a list of actors.

[Mumbles]

Well, since that's not the way I handle it, I don't know what the second problem would be caused by, but at least the first one was found.

 

Delete an actor and null it in the main code by all means (whether it'd good practice or not is purely personal opinion), but make sure any other class using that specific actor has it's pointer nulled too. Since for Projectile, the Actor pointer is private access, it just needs a member function to null it (which for what its worth, is what I'd call a stray pointer. Not exactly hidden, but one that should have been updated but wasn't)

 

Beyond that, I've no idea if I've been helpful earlier, but I don't think I'm particularly helpful now at this stage, sorry.

[Rick]

You have. You helped me walk through the problem. You need to give yourself more credit Mubmles! About my other problem I think I need another list that has the scope of my Gameplay class and create the pointers to those objects in there so they stay alive.