3

[EDIT 2] It's probably because the surface is being freed while it's still in use by the other objects...So I want to write it so it's only freed if no more of the object is on the screen.

[EDIT][UPDATE] I fixed the code to use the iterator, and I was still having the same issue...I just confirmed this to be an issue with the destructor for the object. In the class the destructor calls SDL_FreeSurface(image); to free the object's image from memory...But objects created with the copy constructor apparently aren't okay with that. What do I need to do to make it work correctly with objects created with the copy constructor? I can't seem to find anything on Google pertaining to this.

Number refers to the number of bullets on the screen at a given time...I have a vector of the object "bullet" (vector bullet;)...The code checks if the trigger button (z) is down, and adds a new bullet to the vector if it is. Then it updates (moves, blits), then checks if the bullet is at the end of the screen...Everything works properly until a bullet reaches the end of the screen where it's supposed to be destroyed.

if (trigger)
{
  bullet.push_back(Bullet(position.x, position.y));
  ++number;
}

for(int i = 0; i < bullet.size(); i++)
{
  bullet[i].update();
}

Below is where the issue is...When a bullet object is erased, the program exits with a segmentation fault...Now, I understand why this is happening since the vector size is changed while it's in the loop, but I can't figure out how to fix it...I added the break thinking that it would solve the problem since only one bullet can be <= 0 at a given time anyway, but it doesn't...I initially had the if statement in the last for loop after the update function, but I put it in its own for loop so I could use the break.

for(int i = 0; i < bullet.size(); i++)  
{
  if (bullet[i].position.y <= 0)
  {     
    bullet.erase(bullet.begin() + i);
    --number;
    break;
  }
}

I'm new to vectors, so please bear with me here if I don't understand something or if I'm making a n00b mistake.

user1122136
  • 98
  • 2
  • 8
  • The problem in your code is that the meaning of `i` changes after the `erase`. You have a function that assumes the vector's object positions are constant for the life of the function that calls a function that changes those positions. – David Schwartz Jan 07 '12 at 01:57
  • Concerning your edit and copy constructors, read [C++ FAQ: What is the Rule of Three?](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) – Ben Voigt Jan 07 '12 at 04:42

1 Answers1

11

The standard erase loop for sequence containers goes like this:

for (std::vector<Bullet>::iterator it = v.begin(); it != v.end() /* not hoisted */; /* no increment */)
{
    if (delete_condition)
    {
        it = v.erase(it);
    }
    else
    {
        ++it;
    }
}

The use of iterators is preferable over the use of a counting index since it frees your code from counter arithmetic which adds nothing to the clarity of the code and is rather much of a nuisance, while the iterator version here presented is fairly self-explanatory.

The key is not to increment the loop variable in the event of an erase.

You may also like to look into the remove/erase idiom:

v.erase(std::remove_if(v.begin(), v.end(),
                       [](Bullet & b) -> bool { return delete_condition }),
        v.end());
Kerrek SB
  • 464,522
  • 92
  • 875
  • 1,084
  • still a little early to be providing a C++11 only answer IMHO - – Adrian Cornish Jan 07 '12 at 01:58
  • @AdrianCornish: The lambda is trivially converted into a spelt-out function/functor, as is `auto` replaced by the spelt-out type name. Those are really just typographic shorthands. You can be grateful I didn't say `auto it(std::begin(v))` ;-) – Kerrek SB Jan 07 '12 at 01:59
  • Trivial when you have the standard and have been programming for years :-) like I said IMHO – Adrian Cornish Jan 07 '12 at 02:00
  • What can I use in place of auto? I'm getting compiling issues that seem to be based on that...I get "error: cannot convert ‘__gnu_cxx::__normal_iterator > >’ to ‘int’ in initialization". – user1122136 Jan 07 '12 at 02:10
  • Both GCC and MSVC support lambdas and auto already. Only clang still doesn't support lambdas :( – R. Martinho Fernandes Jan 07 '12 at 02:18
  • player.cpp:52: error: ISO C++ forbids declaration of ‘it’ with no type player.cpp:52: error: cannot convert ‘__gnu_cxx::__normal_iterator > >’ to ‘int’ in initialization – user1122136 Jan 07 '12 at 02:23
  • Okay...Now the compiler is complaining about this line... if (bullet[it].position.y <= 0) error: no match for ‘operator[]’ in ‘((Player*)this)->Player::bullet[it]’ /usr/include/c++/4.4/bits/stl_vector.h – user1122136 Jan 07 '12 at 02:30
  • @user1122136: No no... the iterator dereferences to the container element. So instead of `bullet[i]` you now write `*it`; or `it->position.y` in your case. Take an hour off and read up about iterators; they are a core concept of C++. – Kerrek SB Jan 07 '12 at 02:35
  • It runs now...But, unfortunately, I'm still getting a segmentation fault when an object is erased...It doesn't happen if there's only one on the screen, though. I've started reading up on iterators. – user1122136 Jan 07 '12 at 02:46
  • Okay, I'm noticing something peculiar...If I change the condition from "if (it->position.y <= 0)" to either "if (it->position.y != 0)" or "if (it->position.y >= 0)", it doesn't result in a segmentation fault, although it obviously doesn't work since the bullet is destroyed immediately...Changing it to "if (it->position.y == 0)" does result in a segmentation fault, though. Any ideas as to why? – user1122136 Jan 07 '12 at 04:06
  • Scratch that, actually...It's obviously for the same reason that having one bullet erased on screen doesn't result in a segmentation fault...Which doesn't explain why it's okay with one bullet, but not multiple bullets. Could this be an issue with the copy constructor/destructor? – user1122136 Jan 07 '12 at 04:21
  • Okay...I have everything figured out now. Thank you for your help and patience. – user1122136 Jan 07 '12 at 04:56