23 hours ago, MarcusAseth said:

if( missileVec[i]->destroyNow == true )
{
    missileVec.pop_back();
}

This seems fishy to me... if your first missile is set to be destroyed you are doing pop_back() effectively destroying the last missile which has nothing to do with the first

Also if I am not mistaken, by doing pop_back() on a vector of raw pointers you now have leaked a missile because delete is never called on that missile thus its destructor is not fired and you thrown away the only pointer to it

I would try using a different data structure to store the Missiles, one that allows you to remove from the middle of it as well like a DoubyLinkedList (called list in C++ I think) or maybe a map where the key is the missile ID which you assign to every missile at spawn and have a static variable distributing IDs, or maybe I would try to keep it a vector but when you find a missile that need to be destroyed, you swap it with the last, carefully destroy the last element and decrese your counter loop by one so you process again the current ID which is the previous(before the swap) last element.

Hey Marcus,

Oh right, pop_back removes the last element, which is not the first Missile if I click the left button more than once, hmmm.

I actually tried to place a delete statement in my code after destroyNow is set to true, but then I figured out that newMissile only exists within the scope of the if( isFiring == true ) block and falls out of scope when the If block is done, so I pushed it into the Vector in the code above because I didn't want the missile to be lost (that was my thinking at the time).  

And my program freezes if I try to call delete missileVec within the loop so yes, I seriously have to reconsider my Missile container.

20 hours ago, Satharis said:

Missile* newMissile = new Missile(renderer, cannon->x, cannon->y, clickedX, clickedY);
missileVec.push_back(newMissile);

If it isn't clear what you are doing here, you are essentially creating a pointer named newMissile, which then has its value set to point to the memory that is allocated by using new, so now only newMissile refers to that memory.

Then you use push_back on it, which copies the value of the pointer into the vector, so now we have two things pointing at that memory.

for( int i = 0; i < missileVec.size(); i++ )
{
  float targetX = missileVec[i]->endX - missileVec[i]->x;
  float targetY = missileVec[i]->endY - missileVec[i]->y;

  missileVec[i]->angle = SetMissileAngle(targetX,targetY);

  missileVec[i]->Update(delta);

  if( missileVec[i]->destroyNow == true )
  {
    missileVec.pop_back();
  }
}

Now this part is an issue, basically you're iterating over each pointer to a missile. The problem is that presumably the first missile in the container will be the earliest fired one, and likely to explode the first. What happens when it explodes? it sets destroynow to true, and then the next time the loop runs the first missile it sees is one that is destroyed.

Problem is push_back and pop_back do exactly what they say, they push or pop the element on the back. So now when your first missile explodes, whether you have 1 missile or 10, it removes the last one. Then it goes to the next ones, they may not be destroyed yet, so it exits the loop. But what happens the next update loop? It checks the first missile, sees it is destroyed, and pops the back again. Basically you're going to infinitely pop off all of your missiles.

In addition, you are popping the missile pointers off without ever calling delete on them to free the memory they point to.

There's a few methods you could use to remove elements in a vector like this. If you use an iterator(missileVec::iterator) then you can use erase when you get to a missile you want gone. Erase will remove that element and shift the other elements back to fill in the space. Another method is swap and pop, essentially if there are more than two elements in the vector you take the last element and swap it with the one you want to erase, then pop it off the back.

Just make sure to delete these missiles, or use a smart pointer like unique_ptr, which is vector safe.

EDIT: Managing memory and ownership like this is a pretty big part of any program, especially stuff like this for games. It might be worthwhile for you to research your options a bit and understand some of the various ways you can store things and pass ownership or references around.

 

Hi Satharis, I wasn't kidding when I said I was an absolute beginner, and I would have never realized what I was doing without someone pointing it out for me.

As I mentioned to Marcus, my worry with newMissile was that it would be out of scope as soon as the isFiring IF block was completed, which is why I thought that adding it to the vector was the only way to save the missile data so I can update it in a For loop further down.  I didn't even consider that I had two pointers instead of just one, so thanks for letting me know.

That also lended itself to my delete issue with a missile that was to be destroyed, I tried earlier to call destroy missileVec but it just ends up crashing the program.  So I need to figure out how to delete the memory after popping off the pointers.

The "swap and pop" method for deleting objects from a vector sounds like a good idea and I would get some good practice in to implement it in my game.  I'll give it a shot.

But first things first, I'll have to look at how to free the memory for destroyed missiles.

Yes, I definitely need to research memory management.  Can you clarify what you mean by ownership in this context?  Thank you.

12 hours ago, the incredible smoker said:

I looked at your code real fast :

Why does the missile need to delete a texture ?

You should have only 1 texture loaded for all missiles.

Hello Smoker,

I'm not 100% sure what you mean.  The missile destructor frees the memory that was used by its own missile texture upon deletion.

Don't objects have to be responsible for deleting their own resources?  Please clarify.

Yes, I do load the same texture every time a new missile is created.  Hmmm, how would I go about loading the texture only one time if a missile is created when a Left mouse click occurs?  I can't seem to visualize how to load the missile texture only once if I'm calling the constructor with each new missile.  Thanks.

Quote

But first things first, I'll have to look at how to free the memory for destroyed missiles.

Just let a smart pointer handle it for you

#include <memory>
std::vector<std::shared_ptr<Missile>> Missiles;
std::shared_ptr<Missile> NewMissile = std::make_shared<Missile>(/*Missile Constructor arguments here */);
Missiles.push_back(NewMissile);

I am not too familiar with shared_ptr yet, but if I got it right it keeps an internal count of "how many co-owners to the object he is managing there are" (object he is managing is your Missile), and when all this co-owners (meaning multiple shared_ptr) are destroyed, only then your Missile destructor is called, so basically when you call pop_back() the missile destructor is fired immediately since you have a single shared_ptr managing your Missile, and even with many of this pointing to the same object you can't delete it twice.

As a rule of thumb, if you're a begginner like me in 2017 and you're writing new and delete, then something is wrong (I often do the same though, this old books fault  )

1 hour ago, Glydion said:

As I mentioned to Marcus, my worry with newMissile was that it would be out of scope as soon as the isFiring IF block was completed, which is why I thought that adding it to the vector was the only way to save the missile data so I can update it in a For loop further down.  I didn't even consider that I had two pointers instead of just one, so thanks for letting me know.

That also lended itself to my delete issue with a missile that was to be destroyed, I tried earlier to call destroy missileVec but it just ends up crashing the program.  So I need to figure out how to delete the memory after popping off the pointers.

The "swap and pop" method for deleting objects from a vector sounds like a good idea and I would get some good practice in to implement it in my game.  I'll give it a shot.

But first things first, I'll have to look at how to free the memory for destroyed missiles.

Well no, you are right, it does go out of scope, then you only have the vector pointer referring to the missile object. I only really pointed that out to make it clear that they were pointers and where they were going.

Thing to remember about pointers is that they are just variables themselves so you can delete the memory they point to even while they're still in the vector, they just will be pointing to unallocated memory if you try to use them after that.

if( missileVec[i]->destroyNow)
{
   auto size = missileVec.size();
   auto lastIndex = size - 1;
   
   if(size > 1 && i != lastIndex)
   {
      Missile* swap = missileVec[lastIndex];
      missileVec[lastIndex] = missileVec[i];
      missileVec[i] = swap;
      --i;
   }
  
   delete missileVec[lastIndex];
   missileVec.pop_back();
}

Something like this would work for deleting.. though I probably did something stupid in it, but you get the idea.

Alternative is to use a smart pointer in the container, then it just deletes it when you pop it off.

Hi, normally i have a class for the list, wich holds the texture and first missile.

You pass the list to the missile constructor so the missile can have a pointer to the texture from the list.

You only load the texture once, at startup of your level, and delete it when you close your level.

12 hours ago, Satharis said:

Well no, you are right, it does go out of scope, then you only have the vector pointer referring to the missile object. I only really pointed that out to make it clear that they were pointers and where they were going.

Thing to remember about pointers is that they are just variables themselves so you can delete the memory they point to even while they're still in the vector, they just will be pointing to unallocated memory if you try to use them after that.

if( missileVec[i]->destroyNow)
{
   auto size = missileVec.size();
   auto lastIndex = size - 1;
   
   if(size > 1 && i != lastIndex)
   {
      Missile* swap = missileVec[lastIndex];
      missileVec[lastIndex] = missileVec[i];
      missileVec[i] = swap;
      --i;
   }
  
   delete missileVec[lastIndex];
   missileVec.pop_back();
}

Something like this would work for deleting.. though I probably did something stupid in it, but you get the idea.

Alternative is to use a smart pointer in the container, then it just deletes it when you pop it off.

That isn't the best way to delete an item in a vector at all IMO.  You can do something much simplier:

for (auto it = missileVec.begin(); it != missileVec.end();) {
  if (it->deleteNow) {
    delete *it;
    it = missileVec.erase(it);
  }
  else {
    ++it;
  }
}	

6 hours ago, BeerNutts said:

Basically using iterators to loop through the missile vectors and if one needs to be deleted, you can delete the pointer, then remove the pointer form the vector, else move to the next one.

Yes, and I specified that you could use erase, or something like swap and pop if he didn't want to use an iterator. Which is why I rewrote it using the same method he was using.

The iterator method is also a waste of time if you already have the index of what you want to get rid of, in this case it is fine since he is examining the entire container, but yeah.

EDIT: More specifically in situations where performance is a concern erase is actually slower than swap and pop anyway since it shifts the elements back and ends up being O(n).

On 9/12/2017 at 11:24 PM, Satharis said:

Well no, you are right, it does go out of scope, then you only have the vector pointer referring to the missile object. I only really pointed that out to make it clear that they were pointers and where they were going.

Thing to remember about pointers is that they are just variables themselves so you can delete the memory they point to even while they're still in the vector, they just will be pointing to unallocated memory if you try to use them after that.

if( missileVec[i]->destroyNow)
{
   auto size = missileVec.size();
   auto lastIndex = size - 1;
   
   if(size > 1 && i != lastIndex)
   {
      Missile* swap = missileVec[lastIndex];
      missileVec[lastIndex] = missileVec[i];
      missileVec[i] = swap;
      --i;
   }
  
   delete missileVec[lastIndex];
   missileVec.pop_back();
}

Something like this would work for deleting.. though I probably did something stupid in it, but you get the idea.

Alternative is to use a smart pointer in the container, then it just deletes it when you pop it off.

Hey Satharis, 

Thanks for providing a prototype of how to implement the "swap and pop" method with respect to the code I provided.  It really helped me understand how to delete the desired missile, and now the missiles are popped out properly and they are now being deleted to free the memory, hopefully preventing any memory leaks.

Since this is just a "version 1" of this Missile Command game, I'm okay with swap and pop to remove missiles at this point as I will be returning to this game a tiny bit later and re-factoring it (as AlphaProgDes recommends in his article).

I'll practice "iterating" through the vector the second time around, as per Beer Nut's suggestion.

Yeah, so the issue was indeed the fact that "pop_back" on its own just removes the last missile that was added at the end, without realizing that the missile I wanted deleted was the "older" missile that was already in the vector container.  

I'll be moving on to creating and managing falling asteroids next as part of the Missile Command game, and I'll post an update later on once I've made progress.

Thanks to everyone who posted a response to help me out, I'm going to read up on memory management until I understand it.  Have a good night.