std::vector wierdness (UPDATE: now about memory not freeing)

Started by
37 comments, last by d h k 15 years, 2 months ago
Okay, I have a simple set-up for a game: there's a player who has a weapon which in turn has a vector of projectiles. It's really pretty basic, take a look at the source (I commented it all very extensively so it should really be self-explanatory):

void CWeapon::Draw ( )
// draws a weapon
{
	// draw weapon itself

	// draw all its projectiles
	std::vector<CProjectile*>::iterator i;
	for ( i = Projectile.begin ( ); i < Projectile.end ( ); i++ )
	// loop through all projectiles currently in the vector
	{
		// draw this projectile
		(*i)->Draw ( );

		if ( (*i)->Distance > 200.0f )
		// if this projectile has gone too far
			// erase it from vector
			Projectile.erase ( i );
	}
}

void CWeapon::Shoot ( void )
// shoots a weapon
{
	// do a bunch of unrelated stuff (play a sound etc.)

	// add a new projectile to this weapon's vector
	Projectile.push_back ( new CProjectile ( ) );
}





Weapon.Projectile is the std::vector<CProjectile*>, Weapon.Draw ( ) is called every frame and Weapon.Shoot ( ) is called repeatedly when the left mouse button is held down. Now, all this works only for every second or third shot. It's really unpredictable. Sometimes three shots in a row work, sometimes three won't work, or two etc. It seems completely random. Shots that work are drawn, move away from the player and get erased once they've gone to far, shots that don't "work" are not drawn although they DO get added to the vector (I check std::vector.size ( ) via debug output) but then do not move and thus never get deleted either. I have no idea what's going wrong here. I cross-checked this code with the examples over at CodeGuru.com and it seems to be the correct way. On the other hand, I'm still somewhat new to std::vectors. Any ideas? Help/feedback is greatly appreciated. Thanks. [Edited by - d h k on January 31, 2009 9:09:54 AM]
Advertisement
Erasing elements while you're iterating through a vector has to be done very carefully. Basically, when you erase(i), the i iterator is no longer valid, so bad things happen when the loop iterates it. You can use the return value of erase() to get the iterator to the element after the one you just erased. So your modified loop would look like:
	for ( i = Projectile.begin ( ); i < Projectile.end ( );)	{		// draw this projectile		(*i)->Draw ( );		if ( (*i)->Distance > 200.0f ) {			i = Projectile.erase ( i );                } else {                        ++i;                }	}

edit: also, your loop leaks memory. Consider adding a delete call, or use a smart pointer to handle deletion for you.
The problem is that you are erasing elements from the vector *while* looping through it. Erasing an element from the vector invalidates the iterator you are using, check the documentation of the STL. What you might consider is to have each projectile have an is_valid flag, or something of the sort. Rather than erase it in the loop, you just mark it as invalid when the distance is too great (it no longer gets drawn or updated). Then, after you've iterated over all elements, you erase those (maybe using remove_if) which are no longer valid.

Cheers,
Rob
You can use std::for_each to apply the drawing. Then use std::remove_if to swap all dead projectiles to the back of the vector where they can be efficiently erased in bulk.

Consider storing smart-pointers (boost::shared_ptr for example) rather than raw pointers to your projectiles; this is even more relevant since you've made the mistake of forgetting to delete the memory that you new.
Thanks for your quick answers!

It looks as if SiCrane's way is the quickest to implement, so I decided to go with that for the moment (the other solutions seem more efficient/elegant, though, so I will replace the implementation later on, once I get it to work in the really simple way). My loop now looks like this:

std::vector<CProjectile*>::iterator i;for ( i = Projectile.begin ( ); i < Projectile.end ( ); ){	(*i)->Draw ( );	if ( (*i)->Distance > 200.0f )	{		delete *i;		i = Projectile.erase ( i );	}	else		++i;}


Now, only the first shot works and the rest are never added to the vector..?

Thanks again.
Further investigation has shown that the problem with shots appearing sometimes and sometimes not has nothing to do with the erasing stuff. If I just let projectiles fly forever it still doesn't always create a new one when I shoot. Again, they ARE added to the vector (::size ( ) says so) but they are not drawn.

Any more ideas? I'm really not sure what's going on.

[Edited by - d h k on January 24, 2009 4:25:37 PM]
Relevant code would include the initialization (are the bullets being generated where they should be? If not, they could be off the screen), and the rendering code.
I'll try my best to present all relevant code.

// projectile definition, each weapon has a std::vector<CProjectile*>class	CProjectile{	public:		float		X, Y, Z;		float		Yaw, Pitch, Roll;		float		Distance;		CVector3	ForwardAxis;		CProjectile ( CVector3 forward_axis, float yaw, float pitch, float roll )		// constructor		{			ForwardAxis = forward_axis;			Yaw = yaw;			Pitch = pitch;			Roll = roll;		}		void		Draw ( CModel *shot_model );};// called every frame by a weapon in order to draw the vectorstd::vector<CProjectile*>::iterator i;for ( i = Projectile.begin ( ); i < Projectile.end ( ); i++ )	(*i)->Draw ( &ProjectileModel );// in the line above, ProjectileModel is a pointer stored as a member in CWeapon which is// loaded on init and represents the projectile// when player shootsProjectile.push_back ( new CProjectile ( player.ForwardAxis, player.Ship.Model.Yaw, player.Ship.Model.Pitch, player.Ship.Model.Roll ) );


And this is what CProjectile::Draw ( ) does...

void CProjectile::Draw ( CModel *shot_model ){	shot_model->Position.X = ForwardAxis.X * Distance;	shot_model->Position.Y = ForwardAxis.Y * Distance;	shot_model->Position.Z = ForwardAxis.Z * Distance;	shot_model->Yaw = Yaw;	shot_model->Roll = Roll;	shot_model->Pitch = Pitch;	shot_model->Draw ( );	Distance += 7.0f;}


The draw function gets passed a pointer to a model, positions the model accordingly, draws it and then increases the distance. I know the model stuff is working fine because it works for players/enemies/objects etc. I personally don't see any reason why it would NOT draw/handle every 3rd or sometimes 4th shot...

I'm really clueless and extremely thankful for any food for thought etc.
You never seem to initialize Distance. Try throwing Distance = 0.0f; into CProjectile's constructor. You'd be able to confirm this by using a debugger to put a breakpoint on the call to shot_model->Draw();, and taking a look at the variable values.

You also seem to never use X/Y/Z of CProjectile -- I'd remove them if this is the case. Otherwise, I'd initialize them as well -- uninitialized variables tend to lead to random-ish behavior like you're experiencing now.



What's probably happening, for the bullets to show up at all in the first place, is that std::vector's allocation buffer is initialized to zero at first. (You should not rely on this behavior as it is not required by the standard!) However, when you erase items, then add new ones, the memory isn't rezeroed -- so any uninitialized variables (e.g. Distance) actually get given the value of an old bullet (e.g. one with Distance > 200.0f).
Wow, you were absolutely right! What a stupid mistake, too... :)

X, Y, Z were a relict from an older version, I removed them. Putting a "Distance = 0.0f;" into the Projectile constructor did the trick!

Thanks a ton!

This topic is closed to new replies.

Advertisement