Jump to content

  • Log In with Google      Sign In   
  • Create Account

C++ SDL - Deleting dynamically allocated objects


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
16 replies to this topic

#1 aliasc   Members   -  Reputation: 180

Like
0Likes
Like

Posted 05 February 2014 - 09:33 PM

Hello GDNet. I am studying the SDL library and while doing so i find myself in troubles back in the C++ basics. 
The question is : If you delete an object that is dynamically allocated, and then try it again does it cause problems or leaks ?

 

The issue is as follows:

I have a Sprite class which basically deals with loading sprites and drawing them onto the screen.

I have Game class with functions like Init() Draw() Update() Quit().

I dynamically create new sprites in the Game class for each image i want to draw, and then i delete objects in the Quit() function.

 

In the main game loop i experimented with some things. I attached a key event to delete specific sprite object which actually removes the

sprite from the screen and clears the allocated memory. Note that i did that in the game loop. And in the Quit function i also delete

that object. Is this ok ? It does not seem to crash the program or anything but what happens behind the scenes ?

This can be an issue when the Player kills an Enemy and i free that specific object, and then back in the Quit function i try to 
free the garbage again.

Here is some code for reference

game.cpp

 Sprite Background = new Sprite();

Game::Initialize()
{
    Background->LoadSprite("bg.bmp");
    /........../
}

Game::Loop()
{
    while(Running){
      while(PollEvent){
         if(KEY == "x"){
            delete Background;
           Background = NULL;
        }
     }
   }
}

Game::Quit()
{
    //Background once deleted and now again ?
    delete Background;
}


Sponsor:

#2 dejaime   Crossbones+   -  Reputation: 4027

Like
2Likes
Like

Posted 05 February 2014 - 09:49 PM

If you try to call delete on a NULL pointer, it will be ignored.

There's no necessity of a null check like:

if (pointer)
    delete pointer;

Even less:

delete pointer; //Free allocated memory.
pointer = NULL;
delete pointer; //Just to make sure.

That second delete in your code is doing nothing, but it detracts from your code's readability, potentially making it confuse to anyone reading it (even you).

Even if it does no harm directly, it is doing no good either, and I would advise against this.

Always prefer to make clear code and it will make your life way easier when a nasty bug appear!


Edited by dejaime, 05 February 2014 - 11:21 PM.


#3 aliasc   Members   -  Reputation: 180

Like
0Likes
Like

Posted 05 February 2014 - 10:02 PM

I am confused here. Basically i delete all objects in the Quit function. But if an enemy object gets killed in the main loop where game logic happens i free the texture and delete the object. Also you never know if the enemy will be killed in the main loop so you need to take care of it in the end function also.

 

So the best practice is to assign NULL to pointers that don't get to play anymore ( if they ) and delete them in the end just to make sure or if they survived game logic ?


Edited by aliasc, 05 February 2014 - 10:04 PM.


#4 frob   Moderators   -  Reputation: 21322

Like
5Likes
Like

Posted 05 February 2014 - 10:45 PM

delete pointer; //Free allocated memory.
delete pointer; //Just to make sure.
That second delete in your code is doing nothing, but it detracts from your code's readability, potentially making it confuse to anyone reading it (even you).
Even if it does no harm directly, it is doing no good either, and I would advise against this.

 

Don't do that.

 

Double-delete is a very serious bug.

 

The general idiom is that if you are doing your own memory management with new/delete (which is not recommended) to always follow a delete with nullification:

 

delete foo;

foo = NULL;

 

That doesn't completely address it, since there may be other pointers that all become invalid after the object is deleted, but it does help with the most common case.  Attempting to delete a pointer to NULL does nothing untoward, since the delete operator checks for a null pointer as part of the process.

 

So the best practice is to assign NULL to pointers that don't get to play anymore ( if they ) and delete them in the end just to make sure or if they survived game logic ?

 

Nope, C++ doesn't have an automatic garbage collector that will handle that case cleanly.

 

 If you allocate it you must release it. Blindly assigning NULL to pointers means the resources will be leaked unless you clean them up elsewhere. As your program leaks memory and other resources performance will degrade and eventually cause the program to crash or otherwise seriously misbehave.


Check out my personal indie blog at bryanwagstaff.com.

#5 fastcall22   Crossbones+   -  Reputation: 4333

Like
5Likes
Like

Posted 05 February 2014 - 10:57 PM

To clarify dejaime's post, you cannot double delete an object:
int* a = new int;
delete a;
delete a; // <-- NO
... but you can delete a null pointer:
int* a = new int;
delete a;
a = nullptr;
delete a; // <-- OK

So the best practice is to assign NULL to pointers that don't get to play anymore ( if they ) and delete them in the end just to make sure or if they survived game logic ?


Well, no. You cannot assign a pointer to NULL and later expect to delete the object it once referenced. But it is safe to assign a pointer to NULL after deleting it, if you expect to delete it later.

There are some other things you should also consider: Texture resources are usually shared between sprite objects and are maintained outside the sprite's control. This way, you can load from disk once, and reference them freely until the game ends.
Second: You usually use an dynamic array (std::vector) to keep track of game objects. The logic updating those objects typically receives some sort of signal from the object to delete it. From there the logic removes that object from the array, removes the object from the array, and finally deletes the object.

You might also want to consider reading up on object lifetimes and smart pointers.
c3RhdGljIGNoYXIgeW91cl9tb21bMVVMTCA8PCA2NF07CnNwcmludGYoeW91cl9tb20sICJpcyBmYXQiKTs=

#6 dejaime   Crossbones+   -  Reputation: 4027

Like
1Likes
Like

Posted 05 February 2014 - 11:20 PM

Don't do that.
 
Double-delete is a very serious bug.

Sorry about that, fixed the code with what I originally meant to write.

I'm just too used to my overloaded delete that nullifies it for me; my bad.

 

 

@ Back on topic,

Under normal circumstances, you delete any objects when you know for sure they won't be reused.
Sometimes that happens during the gameplay, and sometimes at the end.
 
Still, you can avoid going around calling delete, by, for example, calling a function to make unused objects delete themselves.
 

#include <algorithms>
#include <vector>

bool exit;

class AwesomeGameObj {
    private:
        static std::vector<AwesomeGameObj*> MasterVector;
        bool isUseless;
    public:
        AwesomeGameObj () {
            /*Initialize everything*/
            isUseless = false;
            MasterVector.pushback(this);
        }
        
        ~AwesomeGameObj () {
            /*Do the cleanup*/
            MasterVector.erase(/*this object*/);
        }
        
        void de_init () {
            while ( !MasterVector.empty() ) {
                delete MasterVector.back();
                MasterVector.pop_back();
            }
        }
        
        bool useless () const {return isUseless;}
        
        static void clean_up () {
            for (/*every object in MasterVector*/)
                if (object->useless()) {
                    vec.erase(/*object*/);
                    delete /*object*/;
                }
        }
        
        static void clean_up (std::vector<AwesomeGameObj*> vec*) {
            for (/*every object in vec*/)
                if ( object->useless() ) {
                    vec->erase(/*object*/);
                    delete /*object*/;
                }
        }
        
        void update () {
            /*do anything it needs to do*/
            if (/*something very very sad happened*/)
                isUseless = true;
        }

};

void quit () {
    AwesomeGameObj::de_init();
    /*do it for any other object types that needs this clean up here*/
}

void init_all () {
    /*init anything*/
}

int main()
{
    //Game Objects in general
    std::vector<AwesomeGameObj*> gameObjects; 
    
    //Fix don't-delete-while-running game objects.
    std::vector<AwesomeGameObj*> miscUseObjects;
    
    init_all();
    
    //Initialize some special objects
    while (/*needs misc objects*/)
        miscUseObjects.push_back (new AwesomeGameObj);
    
    //Main loop
    do {
        while (/*needs new objects*/) {
            gameObjects.push_back (new AwesomeGameObj);
        }
        
        for (/*all gameObjects and miscUseObjects*/) {
            object.update();
        }
        
        if (/*you want to free memory*/)
            AwesomeGameObj::clean_up(gameObjects);
        
    } while (!exit);
    
    //We'll most probably have objects remaining;
    quit();
    
    return 0;
}

As you can see in this example pseudo code, I avoid to call delete on the main and quit functions. I do this by passing on the responsibility of doing so to the object's own class, and all I have to do is tell it to clean up its memory, despite how many objects I need to free and where they are, where they were allocated or anything. This way my main and end functions doesn't have to worry about remaining objects nor freed objects.


Edited by dejaime, 05 February 2014 - 11:31 PM.


#7 aliasc   Members   -  Reputation: 180

Like
0Likes
Like

Posted 05 February 2014 - 11:24 PM


Second: You usually use an dynamic array (std::vector) to keep track of game objects.


Do you mean for specific types of objects. How would you store all of game objects in a dynamic array when they all are of different types.
You can of course std::vector<Sprite> spriteList. Also can you check if specific object exists in the list before deletion ?

Anyway this : 
int* a = new int;
delete a;
a = nullptr;
delete a; // <-- OK

means after delete pointer assign nullptr and then delete the null pointer which is safe and so it solves my problem! Thanks !



#8 ProtectedMode   Members   -  Reputation: 1214

Like
6Likes
Like

Posted 06 February 2014 - 12:18 AM

Why don't you use std::unique_ptr's anyway?



#9 SeanMiddleditch   Members   -  Reputation: 5859

Like
5Likes
Like

Posted 06 February 2014 - 01:08 AM

Assuming a recent compiler, follow ProtectedMode's advice:
 
std::unique_ptr<Sprite> Background;

Game::Initialize()
{
    // may require a _very_ recent compiler, as make_unique is technically part of the
    // yet-to-be-released C++14, but Visual Studio 2013 has it
    Background = std::make_unique<Sprite>();
    // alternative if you don't have make_unique:
    // Background.reset(new Sprite());
    Background->LoadSprite("bg.bmp");
    /........../
}

Game::Loop()
{
    while(Running){
      while(PollEvent){
         if(KEY == "x"){
           // automatically deletes Background
           Background = nullptr;
        }
     }
   }
}

Game::Quit()
{
    // automatically deletes Background
    // totally safe to call again and again
    Background = nullptr;
}
std::unique_ptr will ensure that the object it owns is deleted if you assign something else to it (including nullptr/NULL). Note that you can only have a single std::unique_ptr to a particular object at a time. Use plain pointers or references (Sprite&) if you need to borrow the object in a function signature or the like. I would avoid std::shared_ptr; it's an enticing trap for the unwary.

more on unique_ptr here

#10 Strewya   Members   -  Reputation: 1435

Like
4Likes
Like

Posted 06 February 2014 - 01:51 AM

I may have missed something, but is there a reason to keep the Background object as a pointer? Would it be better to keep it on the stack and rely on it being automatically cleaned up when the Game object dies?

You obviously use a load function to get the texture into memory, you could also use an unload function to remove it from memory if the need exists. And deciding if it should be drawn or not is also something that should exist if you have such requirements, it's easier to give it a bool isDrawn, and instead of deleting the object, simply set it as isDrawn = false. Seems like a nicer choice to me.


devstropo.blogspot.com - Random stuff about my gamedev hobby


#11 aliasc   Members   -  Reputation: 180

Like
0Likes
Like

Posted 06 February 2014 - 06:21 AM


I may have missed something, but is there a reason to keep the Background object as a pointer?


Then you don't have much control over memory. I could call the Unload function to drop a texture but the object and its data will still be there till 
out of scope or game end, which is pretty much a waste. Why remain the object till game loop ends when it is no useless anymore (sprite without texture in this case) and you could drop it from memory.

std:make_unique is awesome, although i use MinGW not Visual Studio's compiler.
I guess i will stick with 

int* a = new int;
delete a;
a = nullptr;
delete a; // <-- OK

if its totally safe.

 

 

 


bool useless () const {return isUseless;}

static void clean_up () {
for (/*every object in MasterVector*/)
if (object->useless()) {
vec.erase(/*object*/);
delete /*object*/;
}
}

static void clean_up (std::vector vec*) {
for (/*every object in vec*/)
if ( object->useless() ) {
vec->erase(/*object*/);
delete /*object*/;
}
}

void update () {
/*do anything it needs to do*/
if (/*something very very sad happened*/)
isUseless = true;
}


dejaime im i missing a point or you i dont know. I dont understand your solution pretty much.
You keep objects in dynamic array and check for their state if they are not useless then you delete them.
This isUseless is only checked in clean_up() function
As far as i understood you remove the object from the array when it is useless but you are not deleting it till cleanup, which pretty much can be called within game loop or end game. And again if you call clean_up() in game loop you need to make sure to call it in the end game if object is not useless, which again leads back to my problem.
But if you only assign isUseless in the game logic and do clean_up() in the end it basically means that the object with its data except the texture will still remain in memory till endgame. I want to free things from memory since they are not usable anymore. Probably i miss something i don't know and your solution may be efficient enough, but it does not seem to be structured well as you know that in game you don't deal with specific objects only. So keeping structured attaching same logic for everything helps readability and not getting frustrated when things go complex.


Edited by aliasc, 06 February 2014 - 06:32 AM.


#12 Strewya   Members   -  Reputation: 1435

Like
1Likes
Like

Posted 06 February 2014 - 07:26 AM

MinGW 4.7 onward (if i remember the version correctly) supports most of C++11, if not all of it, and unique_ptr is part of that.

 

His solution avoids deleting stuff in the middle of the game loop. If you do a lot of deleting and allocating within the main loop, you're going to run into hiccups in your framerate when you kill a lot of stuff at the same time. Also, iterating over a container of objects and deleting them at the same time is a bad idea and just invites bugs and/or crashes.


devstropo.blogspot.com - Random stuff about my gamedev hobby


#13 BitMaster   Crossbones+   -  Reputation: 4088

Like
1Likes
Like

Posted 06 February 2014 - 07:35 AM

MinGW 4.7 onward (if i remember the version correctly) supports most of C++11, if not all of it, and unique_ptr is part of that.


Yes, however std::make_unique requires C++14. If and how much C++14 is supported will depend on what version of MinGW is used. Of course it can't hurt to specify -std=c++14 instead of -std=c++11 and just try it out.

#14 SeanMiddleditch   Members   -  Reputation: 5859

Like
5Likes
Like

Posted 06 February 2014 - 11:02 AM

Then you don't have much control over memory. I could call the Unload function to drop a texture but the object and its data will still be there till
out of scope or game end, which is pretty much a waste.


That's a terrible reason. We're talking about some bytes on the stack. Your software _design_ should trump micro-optimization of a few bytes. Yes, that applies to games, even really big and complex multi-million-dollar-budget ones.
 

MinGW 4.7 onward (if i remember the version correctly) supports most of C++11, if not all of it, and unique_ptr is part of that.


Yes, however std::make_unique requires C++14. If and how much C++14 is supported will depend on what version of MinGW is used. Of course it can't hurt to specify -std=c++14 instead of -std=c++11 and just try it out.


It's in libstdc++ 4.8 at least, which I believe is in the latest MingW. You don't _need_ make_unique if you aren't using exceptions (the purpose of make_unique is to add sequencing), and if you do decide you need it but currently don't have it, it is trivial to write your own copy:
 
template <typename T, typename ...P>
auto make_unique(P&&... argv) -> std::unique_ptr<T>
{
   return std::unique_ptr(new T(std::forward<P>(argv)...));
}
I personally use an alias 'my' for unique_ptr (actually a custom reimplementation of unique_ptr for reasons irrelevant to this discussion) and a custom 'box' version of make_unique since I use them so often (far more often than raw pointers or new/delete operator calls, respectively) and firmly believe they should have short and easy-to-type names that encourage you to use them more even when you're in a crunch for deadline (when most devs throw out many notions of good practice to get things done in time, in my experience). I'm not 100% certain that using these alternate non-standard names is the best idea long-term as I've had relatively few other developers give me any real-world feedback on this particular issue (but what I've gotten has been positive, at least), but it's something you might consider.

#15 Bregma   Crossbones+   -  Reputation: 5133

Like
5Likes
Like

Posted 06 February 2014 - 12:16 PM

I guess i will stick with

int* a = new int;
delete a;
a = nullptr;
delete a; // <-- OK
if its totally safe.

 

int* a = new int;
int* b = a;
delete a;
a = nullptr;
delete b; // <-- ooh, ouch, this is a more likely scenario

Stephen M. Webb
Professional Free Software Developer

#16 Sunsharior   Members   -  Reputation: 442

Like
0Likes
Like

Posted 07 February 2014 - 03:11 PM

What i like to do is have a class be the manager of actors (an entity in the game).

This class is in charge of everything related to actors like maintaining a list of them, checking if they are alive, updating them and even drawing them (well, the calss check if it need to be drawn, then call another class to draw). I keep a std::vector of all actors.

 

When an actor is killed, i do not delete it yet, i only set the flag for it to be deleted on the next frame.

 

In my game loop is a function i call "MaintainActiveList". What this do is loop throught all actors and check if they are alive and if they are on screen.

 

If the dead flag is set, i delete them and remove them from the array.

 

When the program quit (or change map etx...) i loop through all remaining actors and delete them.

 

The advantage this did to my game is actors are only updated and drawn if they are active and alive.

 

void CActorManager::MaintainActiveList()
{
	mActiveActors.clear();
	for (std::vector<IActor*>::iterator i = mActors.begin(); i != mActors.end();)
	{
		if((*i)->IsState(STATE_DEAD))
                {    
                    IActor * actor = (*i);
                    mActors.erase(i);

                    delete actor;
                    actor = NULL;
                }
		else if ((*i)->IsActive() )
		{
			mActiveActors.push_back((*i));
			 ++i;
		}
		else
			 ++i;
	}
}

Also, for the texture: The same thing for the actor Manager can be done for texture. If you use the same texture for different actor (Let's say you have a lot of clone) there is no need to have the same texture allocated in memory more than once. A texture manager can solve this problem. The actors can then store a pointer to the texture. Be warned that you then can only delete the texture if all actor using it are gone.



#17 Servant of the Lord   Crossbones+   -  Reputation: 19556

Like
2Likes
Like

Posted 07 February 2014 - 04:41 PM

 Sprite Background = new Sprite();

'Background' here isn't a pointer (unless Sprite is typedef'd as a pointer), so calling "new" here shouldn't compile.
But 'Background' shouldn't be global anyway, it should probably be a member variable of 'Game'.
And unless required by your API, 'Background' shouldn't be a pointer at all. Just a regular variable (Sprite should handle it's own dynamic memory internally).

In my opinion a programmer should, by default, use memory whose lifetime is automatically managed:
int x = 0;
Sprite sprite;
 
int *ptrX = &x;
Sprite &spriteRef = sprite;
 
struct MyStruct
{
     Sprite background;
     std::vector<Sprite> arrayOfSprites;
};
If I must use manually-managed variables, then I prefer smart pointers.
C++11's smart pointers include: std::shared_ptr, std::weak_ptr, and std::unique_ptr. Each have different uses for different situations. Your compiler will need C++11 enabled to use.
(Don't use std::auto_ptr which is deprecated and no longer recommended for use)
std::shared_ptr<Sprite> sprite = std::make_shared<Sprite>("path/to/image.png");

In very rare occasions, I need to actually manually micro-manage your memory. In those situations, then I use new and delete. In normal hobbyist code, this shouldn't occur too often. It occurs more often when certain APIs enforce the use of new and delete, or when maintaining older code bases or using outdated compilers.

Edited by Servant of the Lord, 07 February 2014 - 04:46 PM.

It's perfectly fine to abbreviate my username to 'Servant' rather than copy+pasting it all the time.
All glory be to the Man at the right hand... On David's throne the King will reign, and the Government will rest upon His shoulders. All the earth will see the salvation of God.
Of Stranger Flames - [indie turn-based rpg set in a para-historical French colony] | Indie RPG development journal

[Fly with me on Twitter] [Google+] [My broken website]

[Need web hosting? I personally like A Small Orange]





Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS