Dynamic Memory destruction in Objects

C6cb6c90c411f01492653729e7548a50
0
Flamesilver 101 Mar 16, 2011 at 15:56

Need some help on dynamic memory allocation (actually, de-allocation) as I’m re-navigating the learning waters for C++.

Say I have 2 classes -

class BulletCollection;
class Bullet;

BulletCollection is responsible for instantiating / destroying Bullet objects by allocating new Bullet instances and storing the pointer in its std::map<int, Bullet*> so it can be iterated through, etc. Looks something like:

Bullet b* = new Bullet( id, in_ttl );    // id is next available id, gotten elsewhere

blist[id] = b;      // blist is a std::map<int, Bullet*>, member of BulletCollection

Bullet is, well, a bullet. Has a TTL (time to live), which decreases when Bullet::update() is called. When TTL < 0, it will call BulletCollection::killBullet( id ); See, in my class Bullet I store a BulletCollection* BC that points to the BulletCollection it belongs to. So my Bullet::update() looks like this:

Bullet::update() {
TTL--;
  if (TTL < 0) {
    BC->killBullet ( this.id );
  }
}

My problem is my BulletCollection::killBullet() is giving me weird issues (crashes).

And in my BulletCollection::killBullet( in_id ) I have something like:

BulletCollection::killBullet( in_id ) {
  delete blist[in_id];      // frees the memory associated with the pointer
  blist[in_id].erase();     // erases the entry from my std::map blist
}

… as I just typed that, I realize a potential problem… should I be doing:

delete *blist[in_id]; instead of delete blist[in_id]; ???

My entire code works and keeps running if my Bullet TTL is infinitely large, so I know it’s a problem with entity destruction. Can someone please give me some insight?

9 Replies

Please log in or register to post a reply.

46407cc1bdfbd2db4f6e8876d74f990a
0
Kenneth_Gorking 101 Mar 16, 2011 at 16:09

I can’t see how ‘ blist[in_id].erase();’ could possibly compile. It should be ‘ blist.erase(in_id);’

340bf64ac6abda6e40f7e860279823cb
0
_oisyn 101 Mar 16, 2011 at 16:09

No. The operand to ‘delete’ must be a pointer to the object you are trying to destroy, so dereferencing it first makes no sense.

However, take a good look at what you’re doing. You first destroy the object, and after it’s destruction, you tell it to remove itself from the map. How can you tell an object something if it’s already destroyed?

So you have to reverse the order of things. Or you could simply use the erase() method on the map using the index of the bullet.

@Kenneth Gorking

I can’t see how ‘ blist[in_id].erase();’ could possibly compile. It should be ‘ blist.erase(in_id);’

Oh, I actually assumed it was blist[in_id]->erase().

A note to the topicstarter: copy/paste (the isolated parts of) your code as is, to avoid this kind of confusion.

C6cb6c90c411f01492653729e7548a50
0
Flamesilver 101 Mar 16, 2011 at 19:27

Thanks for all your input!

So… store the pointer in another pointer, erase it from the map, THEN delete the memory. Got it! I’ll try it when I get home.

Sorry about not copying the code as-is. I figured it was more theorycraft than anything. I’m actually at work where I have no access to my code. So thanks for bearing with me :)

C6cb6c90c411f01492653729e7548a50
0
Flamesilver 101 Mar 17, 2011 at 18:20

Tried copying to pointer first, still crashes with fatal error. I now have the code in front of me. Please take a look if you can.

void BulletCollection::update() {

    // ***************************** update() ******************************
    // update the collection and everything in it
    
    std::map < int, Bullet *>::iterator iter;     // temp iterator to go through the map blist

    for ( iter = blist.begin(); iter != blist.end(); iter++ ) {
        iter->second->update();
    }

}

void Bullet::update() {

    // ********************** Bullet Update() ******************************

    if ( ttl-- < 0 ) {
        BC->killBullet( id );        // tell the collection to delete, id is private member of Bullet
    }
}

void BulletCollection::killBullet( int bulletid ) {

    // ************** kill a bullet *************
    
    Bullet *btemp = blist[bulletid];                // temporary pointer to hold item to be erased
    blist.erase( bulletid );                             // remove it from the list
    delete btemp;                            // delete the memory

}

I believe something wonky is happening when I do blist.erase( bulletid )… When the crash happens, some file called xtree is opened up in my Visual Studio. Please help, I’m pulling hairs out over here.

BTW, even when I comment out // delete btemp; it still crashes, so it’s probably blist.erase

Thanks,

Flamesilver

340bf64ac6abda6e40f7e860279823cb
0
_oisyn 101 Mar 17, 2011 at 20:35

Ah, classic mistake :)
You’re iterating over the map, but while you’re iterating, you’re deleting items. The ‘iter’ variable in BulletCollection::update() is still referencing an element that is already removed from the map, and incrementing that invalid iterator and dereferencing it will result in undefined behavior.

To circumvent the problem, you could increment the iterator before you call Bullet::update(), so you know which iterator to use in the next iteration of the loop.

Be aware of the iterator invalidation rules of various containers. For a std::map it is defined that only iterators to deleted items become invalidated. But for a std::vector, however, any iterator that points to or past the deleted element is invalidated.

C6cb6c90c411f01492653729e7548a50
0
Flamesilver 101 Mar 18, 2011 at 10:59

Thanks for the help oisyn. The iterator range problem is fixed and I’m no longer crashing with Fatal Error. But now I’m getting a heap corruption error.

#include "vect2.h"
#include "GameForm.h"
#include "DarkGDK.h"
#include "FulcrumPhy.h"
#include <map>

#define BULLETSTARTNUM 100
#define BULLETENDNUM 1000

class Bullet {

    // **************************** Bullet Class *****************************

private:

    int id;
    int ownerid;            // id of creator
    int ttl;                // number of cycles of life remaining
    BulletCollection *BC;   // Collection so I know where to kill
        
public:

    Bullet( int in_id, int in_ownerid, BulletCollection *in_bc, int in_ttl = DEFAULTTTL );      // CTor
    
    ~Bullet();                                                          // DTor

    int getid() { return id; }      // accessor for id
    int getttl() { return ttl; }        // accessor for ttl
    int getownerid() { return ownerid; }        // accessor for ttl

    void update();

};


class BulletCollection {

    // ************************** Hack BulletCollection Class (Precursor to GameSpace) *****************************
    // *
    // *

private:

    TicketSystem *TS;                   // TicketSystem used to assigning object id's
    std::map< int, Bullet *> blist;       // bullet list - all currently live bullets

public:


    FulcrumPhy *FP;                     // Fulcrum Physics Pointer

    BulletCollection( FulcrumPhy *in_Physics );     // CTor - allocate TicketSystem TS
    ~BulletCollection();                            // DTor - free memory for TS and all

    void createBullet( int creatorid );     // make a bullet
    void killBullet( int bulletid );        // kill a bullet
    void update();                          // update the collection and everything in it

};


BulletCollection::BulletCollection( FulcrumPhy *in_Physics ) {
    
    // ********************** BulletCollection::CTor  ******************************
    // * allocate TicketSystem TS
    
    FP = in_Physics;
    TS = new TicketSystem(BULLETSTARTNUM, BULLETENDNUM);

}

BulletCollection::~BulletCollection() {                 

    // ********************** BulletCollection::DTor  ******************************
    // * // DTor - free memory for TS and all
    // *
    // *
    // delete the collection and everything in it
    
    std::map < int, Bullet *>::iterator iter;     // temp iterator to go through the map blist

    for ( iter = blist.begin(); iter != blist.end(); iter++ ) {
        killBullet ( iter->first );
    }

    delete TS;

}


void BulletCollection::createBullet( int creatorid ) {          

    // ********************** createBullet  ******************************
    // * 
    // *  make a Bullet and add it to blist
    // *  int creatorid    - the guy who made the bullet
    // *

    Bullet *b = new Bullet ( TS->getTicket(), creatorid, this );         // allocate a new Bullet object

    blist[b->getid()] = b;           // ** add b to the map of all bullets so it can get updated **

    // Create a vector that's pointing away from the ship
    vect2 vPadding;
    vPadding.setXYFromAM( dbObjectAngleZ( creatorid ), 20 );

    // ** Set the Bullet position to the player's, Move the bullet according to the player but faster by scale **
    FP->setPosition ( b->getid(),
        dbObjectPositionX( creatorid ) + vPadding.x,
        dbObjectPositionY( creatorid ) + vPadding.y,
        0
        );

    FP->setLinearVelocity ( b->getid(),
        vPadding.x * 5 + FP->getLinearVelocityX ( creatorid ),
        vPadding.y * 5 + FP->getLinearVelocityY ( creatorid ),
        0
        );

    /*FP->setLinearVelocity ( b->getid(),
        FP->getLinearVelocityX ( creatorid ) * 3,
        FP->getLinearVelocityY ( creatorid ) * 3,
        0
        );*/

}


void BulletCollection::killBullet( int bulletid ) {

    // ************** kill a bullet *************
    // remove it from the list
    Bullet *btemp = blist[bulletid];                // temporary pointer to hold item to be erased
    blist.erase( bulletid );
    delete btemp;
}

void BulletCollection::update() {

    // ***************************** update() ******************************
    // update the collection and everything in it
    
    std::map < int, Bullet *>::iterator iter = blist.begin();     // temp iterator to go through the map blist
    Bullet *tb;                                                     // temp bullet pointer

    while ( iter != blist.end() ) {
        tb = iter->second;
        iter++;                 // iterate BEFORE possibly calling killBullet() and invalidating the whole thing
        tb->update();            // now call update
    }
     
    /*for ( iter = blist.begin(); iter != blist.end(); iter++ ) {
        iter->second->update();
    }*/
}



Bullet::Bullet( int in_id, int in_ownerid, BulletCollection *in_bc, int in_ttl ) {
    
    id = in_id;
    ownerid = in_ownerid;
    BC = in_bc;
    ttl = in_ttl;

    dbMakeObjectSphere ( id, 3 );                           // create physical object
    BC->FP->makeSphere ( id, true );                  // add to physics program
    
}

Bullet::~Bullet () {

    // *********** DTOR ***********
    // * Release the object created

    BC->FP->releaseActor(id);     // release Actor (delete Object) from Physics through BulletCollection
    dbDelete(id);                   // delete the object from DGDK

}


void Bullet::update() {

    // ********************** Bullet Update() ******************************

    if ( ttl < 0 ) {
        BC->killBullet( id );        // tell the collection to delete, id is private member of Bullet
    }
    ttl--;
    
}

I did look up Heap Corruption, and now have the idea that I must be trying to free the same resource twice… but void BulletCollection::killBullet( int id ) is the only place I have delete!

Please advise.

Thanks,

Flamesilver

PS: Yes, I’m aware that \~BulletCollection() DTor still has that iterator problem. That’s an easy fix. But alas, it’s still crashing when a ttl runs to 0 on a bullet, I think.

340bf64ac6abda6e40f7e860279823cb
0
_oisyn 101 Mar 18, 2011 at 11:58

Heap corruption can be due to a number of issues

  • Deleting objects that weren’t allocated (globals, objects on stack, uninitialized pointers)
  • Deleting objects that are already deleted
  • Writing to any location you shouldn’t be writing to (already deleted objects, beyond buffer bounds, unitialized pointers)

.edit: are you sure GetTicket() never returns the same int twice?

C6cb6c90c411f01492653729e7548a50
0
Flamesilver 101 Mar 18, 2011 at 21:27

The only reason why I don’t think the TicketSystem class is at fault is because as soon as the first bullet dies (ttl < 0) I get the heap corruption error.

I’m really going to have to think long and hard and test a few things to see what’s really bugging the system…

C6cb6c90c411f01492653729e7548a50
0
Flamesilver 101 Mar 19, 2011 at 01:52

FOUND IT!

void Bullet::update() {

    // ********************** Bullet Update() ******************************

    ttl--;              // decrement BEFORE so we're not writing to already deleted memory

    if ( ttl <= 0 ) {
        BC->killBullet( id );        // tell the collection to delete, id is private member of Bullet
    }

}

Previously –ttl; was after the killbullet call, so it would try and write to a variable that was deleted.

Thanks for telling me about the rules of Heap. Devmaster.net is so awesome!

PS: Without you guys, I wouldn’t’ve gotten very far. So far I’ve got a 3D space game with overhead fixed camera, overhead chase, and 3rd person camera view, full physics implemented via fulcrum, and I can fire bullets that interact with other ships (and give knockback). Soon I’ll have a working game! Thanks guys!