Sign in to follow this  

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

This topic is 830 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

	//Enemy projectile collide

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

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

			collideCounter2++;
		}
		collideCounter++;
	}

Seems to be the 2nd for loop giving me the subscript out of range. I have no idea where to start looking, it could be the amount of bullets that are being spawned because if I click my mouse fast, it will only spawn a few bullets instead of like 8-10 so maybe its conflicting with the collision but I'm not sure, only this part of the code is giving me the error.

Share this post


Link to post
Share on other sites
On your specific error, probably you are expecting to iterate over enemy.enemyArray for the inner loop rather than projArray.

If that was a typo in your post but different in your actual code, stop that in the debugger when the error occurs and look at the values of collideCounter and collideCounter2. You should find that one of them exceeds the length of the array they are being used on, either projArray or enemyArray.


As for the general condition, I sure hope you have a small number of items in those arrays. That kind of all-to-all comparison results in dangerously fast computational growth. Most systems build spatial trees and only compare the immediate neighbors.

Share this post


Link to post
Share on other sites

On your specific error, probably you are expecting to iterate over enemy.enemyArray for the inner loop rather than projArray.

If that was a typo in your post but different in your actual code, stop that in the debugger when the error occurs and look at the values of collideCounter and collideCounter2. You should find that one of them exceeds the length of the array they are being used on, either projArray or enemyArray.


As for the general condition, I sure hope you have a small number of items in those arrays. That kind of all-to-all comparison results in dangerously fast computational growth. Most systems build spatial trees and only compare the immediate neighbors.

wow thanks a lot! can't believe I was overlooking that this whole time

Share this post


Link to post
Share on other sites
Why are you using iterators in your for loops, in addition to counters? Just dereference the iterator...

Or better yet, if your compiler supports C++11, use the new range-for construct which eliminates these kinds of issues entirely smile.png

for (const auto& item : myArray)
{
  // do some calculation with item - which is basically "myArray[i]"
}

Share this post


Link to post
Share on other sites
I don't have experience with this:
 

for (const auto& item : myArray)
{
  // do some calculation with item - which is basically "myArray[i]"
}


But...

Why are you using iterators in your for loops, in addition to counters? Just dereference the iterator...


	//Enemy projectile collide
	
	for (iter2 = projArray.begin(); iter2 != projArray.end(); ++iter2){

		for (iter3 = enemy.enemyArray.begin(); iter3 != enemy.enemyArray.end(); ++iter3){
			if (*iter2.rect.getGlobalBounds().intersects(*iter3.enemySprite.getGlobalBounds()))
			{
				std::cout << "COLLIDE" << std::endl;
			}

		}
	}
Edited by MarkS

Share this post


Link to post
Share on other sites

I don't have experience with this:





for (const auto& item : myArray)
{
  // do some calculation with item - which is basically "myArray[i]"
}


This code basically is saying "for each item in myArray" - where "item" is a const ref of whatever the element type of myArray is.

It is the functional equivalent of:
const auto endIter = end(myArray);
for (auto iter = begin(myArray); iter != endIter; ++iter)
{
  const auto& item = *endIter;
  // do stuff here
}
"begin" and "end" are non-member functions that call "begin" and "end" as members on containers, but are non-members themselves so they can be specialized for compile-time arrays so you can use the ranged-for syntax on them:

int myArray[20];
// fill myArray
for (const auto& item : myArray)
{
  // do stuff with the items
}
This won't work, because the compiler has no way to get the size of the array:
int* myArray = new int[size];
// fill myArray
for (const auto& item : myArray) // compile error!
{
  // do stuff with the items
}
delete [] myArray;
Which is one reason it is typically preferred to use std::array and std::vector over raw C arrays.

Share this post


Link to post
Share on other sites

Why are you using iterators in your for loops, in addition to counters? Just dereference the iterator...

 

Or simpler still, iterators can be used like this....

	if(someIterator.hasNext()){
	     while(someIterator.hasNext()){
		  functionDoingSomeStuff( someIterator.next(), someIterator.previousIndex()  );// Parameters:get object referenced with its index
	    }
        }

Though in Java i expect the principle to be applicable in C++

Share this post


Link to post
Share on other sites

Though in Java i expect the principle to be applicable in C++

 

Nope. There does not exist anything like hasNext or conversion between iterators and indexes (previousIndex) in C++

Edited by Olof Hedman

Share this post


Link to post
Share on other sites


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

Edited by Doug Rogers

Share this post


Link to post
Share on other sites

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;
			}

		}
	}
Edited by MarkS

Share this post


Link to post
Share on other sites

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;
			}

		}
	}

Share this post


Link to post
Share on other sites
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++){

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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;
		}

	}

}

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

 

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.

Edited by Doug Rogers

Share this post


Link to post
Share on other sites

This topic is 830 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this