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

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 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 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 on other sites

We are sooo good at reading what we think rather than what it says

Edited by Alberth

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

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


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 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 on other sites

Thanks. Since I don't have experience with it, I couldn't use it as an example. It appears to be the better way though.

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

	//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 on other sites
Thanks! Still trying to get used to that syntax!

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 on other sites

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

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 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 on other sites

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

Edited by Gooey

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

Create an account

Register a new account

• Forum Statistics

• Total Topics
628751
• Total Posts
2984503

• 12
• 25
• 12
• 10
• 17