9 replies to this topic

[Flamesilver]

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?

[Kenneth Gorking]

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

[.oisyn]

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 said:

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.

[Flamesilver]

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 :)

[Flamesilver]

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

[.oisyn]

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.

[Flamesilver]

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.

[.oisyn]

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?

[Flamesilver]

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...

[Flamesilver]

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!