[SFML] [C++] vector subscript out of range

Started by
18 comments, last by Doug Rogers 8 years, 7 months ago



collideCounter = 0;

for (iter2 = projArray.begin(); iter2 != projArray.end(); iter2++) 
{
	collideCounter2 = 0;

	for (iter3 = projArray.begin(); iter3 != projArray.end(); iter3++) 
	{
		
		assert(collideCounter2 < enemy.enemyArray.size());

		if (projArray[collideCounter].rect.getGlobalBounds().intersects(enemy.enemyArray[collideCounter2].enemySprite.getGlobalBounds()))
		{
			std::cout << "COLLIDE" << std::endl;
		}

		collideCounter2++;
	}
	collideCounter++;
}

Try adding this assert against the size to see if it fires. collideCounter must be within bounds, but you should use the iterator to get projArray[collideCounter] and remove the collideCounter variable.

It is also recommended that you use ++iter, not iter++

Advertisement

collideCounter = 0;

for (iter2 = projArray.begin(); iter2 != projArray.end(); iter2++) 
{
	collideCounter2 = 0;

	for (iter3 = projArray.begin(); iter3 != projArray.end(); iter3++) 
	{
		
		assert(collideCounter2 < enemy.enemyArray.size());

		if (projArray[collideCounter].rect.getGlobalBounds().intersects(enemy.enemyArray[collideCounter2].enemySprite.getGlobalBounds()))
		{
			std::cout << "COLLIDE" << std::endl;
		}

		collideCounter2++;
	}
	collideCounter++;
}
Try adding this assert against the size to see if it fires. collideCounter must be within bounds, but you should use the iterator to get projArray[collideCounter] and remove the collideCounter variable.


How will the assert help? Look at the arrays. The first for loop is looping through the "projArray" vector. The second for loop is looping through... oops... the "projArray" vector again! He is then comparing "projArray" with "enemyArray", but using the length of "projArray" as a conditional for "enemyArray". Let's not forget that he is using iterators in the for loops without actually using them in his code. Look at my initial response. I eliminated the counters entirely by using the iterators for their intended purpose. The counters are redundant.

It is also recommended that you use ++iter, not iter++


You do have a point here, but even that is moot if he were to follow SmkViper's advice and use "for (const auto& item : myArray)". Using that, you get:

	//Enemy projectile collide
	
	for (const auto& iter2 : projArray){

		for (const auto& iter3 : enemy.enemyArray){
			if (*iter2.rect.getGlobalBounds().intersects(*iter3.enemySprite.getGlobalBounds()))
			{
				std::cout << "COLLIDE" << std::endl;
			}

		}
	}

You do have a point here, but even that is moot if he were to follow SmkViper's advice and use "for (const auto& item : myArray)". Using that, you get:




	//Enemy projectile collide
	
	for (const auto& iter2 : projArray){

		for (const auto& iter3 : enemy.enemyArray){
			if (*iter2.rect.getGlobalBounds().intersects(*iter3.enemySprite.getGlobalBounds()))
			{
				std::cout << "COLLIDE" << std::endl;
			}

		}
	}

It's actually this - the ranged-for gives you the items, not the iterators smile.png


	//Enemy projectile collide
	
	for (const auto& projectile : projArray){

		for (const auto& enemy : enemy.enemyArray){
			if (projectile.rect.getGlobalBounds().intersects(enemy.enemySprite.getGlobalBounds()))
			{
				std::cout << "COLLIDE" << std::endl;
			}

		}
	}
Thanks! Still trying to get used to that syntax!
How will the assert help? Look at the arrays. The first for loop is looping through the "projArray" vector. The second for loop is looping through... oops... the "projArray" vector again!

It would have fired and shown that collideCounter2 was out of bounds. I like SmkViper's suggestion, but a simple fix is to replace


for (iter3 = projArray.begin(); iter3 != projArray.end(); iter3++){

with


for (iter3 = enemy.enemyArray.begin(); iter3 != enemy.enemyArray.end(); iter3++){

But you failed to identify the obvious problems with his code. Not only that, but the issues had already been addressed and you contradicted those that had answered with the correct solution.

This is posted in "For Beginners." It can be safely assumed, just from that, that he is new to this. His poor use of iterators, coupled with him unnecessary use of counters, proved that. Reinforcing bad programming practices doesn't help him.

But you failed to identify the obvious problems with his code

Yes, I did miss that. I was suggesting a way to debug it, that is all.

you contradicted those that had answered with the correct solution.

I don't think I did. I just suggested a way to track it down.

Here is a way to keep the counters:




for (size_t collideCounter = 0; collideCounter < projArray.size(); collideCounter++)
{
	
	for (size_t collideCounter2 = 0; collideCounter2 < enemy.enemyArray.size(); collideCounter2++)
	{
		if (projArray[collideCounter].rect.getGlobalBounds().intersects(enemy.enemyArray[collideCounter2].enemySprite.getGlobalBounds()))
		{
			std::cout << "COLLIDE" << std::endl;
		}

	}

}

Here is a way to keep the counters:




for (size_t collideCounter = 0; collideCounter < projArray.size(); collideCounter++)
{
	
	for (size_t collideCounter2 = 0; collideCounter2 < enemy.enemyArray.size(); collideCounter2++)
	{
		if (projArray[collideCounter].rect.getGlobalBounds().intersects(enemy.enemyArray[collideCounter2].enemySprite.getGlobalBounds()))
		{
			std::cout << "COLLIDE" << std::endl;
		}

	}

}


Yes, and that is exactly the way I used to do it until I discovered what iterators are and how to use them. I do remember reading somewhere that using indices to access a vector was either bad form or dangerous, but its been so long I don't remember where I read that or exactly what it said. Regardless, he was attempting to use iterators and doing it incorrectly. I cannot think of a good reason not to use iterators. They can get tricky if you are iterating to erase an element, but there are ways around that. They do make for cleaner code.

Maybe it is just because iterators can be used on all the c++ containers?

Yes, and that is exactly the way I used to do it until I discovered what iterators are and how to use them. I do remember reading somewhere that using indices to access a vector was either bad form or dangerous, but its been so long I don't remember where I read that or exactly what it said. Regardless, he was attempting to use iterators and doing it incorrectly. I cannot think of a good reason not to use iterators. They can get tricky if you are iterating to erase an element, but there are ways around that. They do make for cleaner code.

Interesting. I have never heard that index access bad either bad for nor dangerous. I use both, depending on the structure. For flat lists, I use indicies like above. In debugging with indices, I get an bad index value (out of range usually). A bad operator usually does not give much info. Both should be about the same perf wise. I agree that using iterators usually makes it easier to switch container types. Occasionally, using iterators make it harder to switch, though.

I haven't made the leap to using auto yet. We didn't use them on my last job, but they look very useful and cleaner.

I learned something here.

Thanks.

This topic is closed to new replies.

Advertisement