Hello!

I'm currently trying to reduce the memory leak my program is causing and the process has me kind of dumbfounded.

I understand that everything you create using "new" you have to delete eventually so that there is no memory leak. The same goes for temporary pointers (right)?

Anyway I've located a part of my code that is leaking, to no surprise it's where i update one of the UI elements in my game. I'm going to simplify the code here because the question isn't about only that code but rather how i handle a situation like this (i have loads of them in my code at the moment).

So here goes:

void update(int timer)
{
//this is my update function which updates this certain element
//i have a reference to my loader class already loaded in, it's called imageLoader in this example. imageLoader stores all of the loaded
//images and can return a pointer to a certain image on function calls.

SDL_Surface* tempSurface = imageLoader.loadImg("example_image.png");
//tempSurface now stores a pointer to the image that is returned from loadImg();

SDL_BlirSurface(tempSurface, NULL, screen, NULL);
//now I've used the img and the local tempSurface is useless

//Now, if tempSurface only was a int* i could have just ignored it since it's just a pointer (right)?
//but as i understands it a SDL_Surface creates other pointers within itself that must be destroyed, and if i call
//SDL_FreeSurface(tempSurface); i will destroy the surface that it's pointing to, creating problems for other classes.

//memory is leaking here, and my guess is on the "tempSurface" which i am unable to destroy without killing it's reference!!!
}


How do i make sure i free all the memory possible without corrupting/deleting the reference?

I want to make sure that i have this same problem with TTF_Font, which is part of a "sub-library" to SDL.

I really appreciate any help i can get!

Hello BeerNutts!

My loader class is essentially a imageManager since it does basically everything you wrote down in that example of yours. It also doesn't load a new image each time, it keeps track of what has been loaded and returns a pointer if it exists.

Why i chose to allocate a new SLD_Surface in the example is because in a few of my line of code maybe 4 things were going to use the same image (within the same func) i found it easier to create a new variable instead of typing that lengthy "imageLoader.loadImg("example_image.png")" each time. Maybe that is the wrong way to go :/

Instead of doing either of the above maybe i should just do what you said, load all the images i need to display in that class globally within the class so that i never loads something new.

This rises up another problem thought, this is a quick example (i think i got this but i just want to be sure);

class.h

#include "loader.h"

class Example
{
public:
draw(SDL_Surface* screen);

private:
SDL_Surface* imageToUseInClass;
Loader imageLoader;
}


class.cpp

void Example::draw(SDL_Surface* screen)
{
//for a shorter example i update the surface in the draw func, will have a special function for this in the real program
//if i want to change a surface that is global within the class this would be a way of doing it, right?
//I first try to load the new image and then if it succeeds i free the memory from the last one and eventually change the image while i
//free the temporary loaded img. This should free all the memory that is needed and wont result in a memory leak.

SDL_Surface* newimage = imageLoader.load("image.png");
if(newImage != NULL)
{
SDL_FreeSurface(imageToUseInClass);
imageToUseInClass = newImage;
SDL_FreeSurface(newImage);
}
else
{
SDL_FreeSurface(newImage);
}


SDL_BlitSurface(imageToUseInClass, NULL, screen, NULL);
}


Firstly, ImageLoader should be a singular entity, there should not be multiple imageLoaders roaming around. The ImageLoader (what i call ImageManager) needs to be created once, and then given to other classes. Or, you could not even go the class route, and just use a normal function LoadImage(), which checks a statically held list of images; otherwise, you're going to have multiple images being loaded when you should only have one. This is actually what I do in my games; I have a Utilities file that is just functions for different things, and not wrapped in a class.

I dont agree here. I have different loaders for different stuff. Like a loader for the UI and another one to load player related stuff, i never load an image twice thought.

At first i explored a few ways of doing this and the static solution was definitely on my mind for a long time but i ended up doing this instead after taking some advice from people.

Second, No, you should never be freeing a surface outside of your imageLoader class. That's going to cause problems. The imageLoader class should be the only one ever doing loads and free or surfaces. If you know an image won't be used anymore in your program, you can call imageLoader->Deallocate(imageName), and have it free it and remove it from it's list of open images.

I see the problem it can cause, but i am truly out of clues why my program would be leaking memory if it isn't that i didn't free the surfaces.

Third, your example class is wrong:

if(newImage != NULL)
{
SDL_FreeSurface(imageToUseInClass);
imageToUseInClass = newImage;
SDL_FreeSurface(newImage);
}
else
{
SDL_FreeSurface(newImage);
}

You cannot assign the pointer to a surface, and then Free the surface; the pointer imageToUseInClass now points to memory which ash been freed, and any access to it will be invalid.

Also, If newImage is NULL, you are trying to Free the NULL surface, which is bad.

That is true... Then this goes back to my original problem, how do i free the surface newImage without screwing imageToUseInClass up? Because it's situations like this that is leaking memory (i'm guessing, couldn't be anything else).

Hello
I think freeing surfaces should only be done when you are absolutely sure it will never be used again. Are you really going to use this example_image.png once? If you are sure, then the correct way to do it is to use the complement of imageLoader.loadImg

imageLoader.unloadImg("example_image.png");

If you don't have that function for your loader class, then you need to write one, because maker and unmaker functions should always come in pairs. It should undo whatever imageLoader.loadImg did to change your loader object, and deallocates whatever was allocated by imageLoader.loadImg.

Examples of such pairs: new and delete, new[] and delete[], malloc and free, IMG_Load and SDL_FreeSurface.

What you should never do is mixing these maker and unmaker functions. That means something created by new should not be destroyed by free, it has to be done by delete. A surface created IMG_Load should not be destroyed by free either, it should be done by SDL_FreeSurface. Likewise, something gotten from imageLoader.loadImg should not be destroyed by SDL_FreeSurface, it should only be done by imageLoader.unloadImg.


SDL_Surface* tempSurface = imageLoader.loadImg("example_image.png");
//tempSurface now stores a pointer to the image that is returned from loadImg();
.......
//Now, if tempSurface only was a int* i could have just ignored it since it's just a pointer (right)?


That's not quite right. Both SDL_Surface* and int* are pointers. That means tempSurface is as much a pointer as any int*

How do i make sure i free all the memory possible without corrupting/deleting the reference?

There aren't any references in your code, only pointers D:

But to answer your question, one way to prevent memory leaks is to unload all the surfaces you've loaded when the program ends.

If it is truly the function you showed is causing a leak (BTW, how did you find out it's causing a memory leak?), then you have not written your imageloader properly; it MUST be loading the same image over and over, because this is all you're doing:

SDL_Surface* tempSurface = imageLoader.loadImg("example_image.png");

SDL_BlirSurface(tempSurface, NULL, screen, NULL);


I know it's a memory leak because i am tracking the resources it uses with windows resource monitor. All i see is a steady rise of KB used until it has eaten up most of the memory and crashes.

I agree with you that the code you wrote shouldn't cause any memory loss, but it still does (or i am just missing something mayor). Take a look at this code, this is causing me some memory leak.

enemy.h

class Enemy
{
public:

draw(); //draws everything including hpNumber
hpInfo(); //updates the graphics for the HP to be displayed

private:

SDL_Surface hpNumber;
SDL_Rect hpNumberPos;

}


a function in enemy.cpp

void Enemy::hpInfo()
{
//I load the font and color that I've saved in the respective loaders. These are references to loaders i pass around to any class that need
//to use them
TTF_Font* sFont8B = const_cast<TTF_Font*>(m_fontLoader.loadFont("sfont8b"));
const SDL_Color textWhite = m_colorLoader.loadColor("white");

//create a string from a int, just takes an int and return a normal string
string hpS = mEntity.gEntity.intToString(m_currentHp, false);

//i update the Surface with the new text to be shown, i have to change the string to a Char* to use the funcion
hpNumber = TTF_RenderText_Solid(sFont8B, mEntity.gEntity.stringToChar(hpS,"",""), textWhite);

//calculate how wide and high the text is
TTF_SizeText(sFont8B, mEntity.gEntity.stringToChar(hpS,"",""), &w, &h);

//assing the new position based on the With and Height of the text written.
SDL_Rect temp5 = {200,m_nameBox.x+m_nameBox.h+10,w,h};
m_hpBox = temp5;

}


now... if i free the surface before i assign a new surface to it the memory loss is lower. I know that this time the image isn't loaded from a imageManager but still worth noting.

I then assume that the decrease in memory stacking up (while doing nothing in the game) is a sign that the surface is no longer causing any memory loss. The memory still leaking must then be caused by something else, and my only guess is the Font.

In case you want to see my loadFont and loadImg functions:


const SDL_Surface* Loader::loadImg(const string name)
{
//check if the file exists, if it does return it
map<string, SDL_Surface*>::const_iterator it = m_loadedImages.find(name);
if(it != m_loadedImages.end())
{
return it->second;
}
SDL_Surface* surface = NULL;
//else load the file
string loadPath = m_prePath + name;
surface = lEntity.loadImage(loadPath.c_str(), true);
if(surface == NULL)
{
//load a default texture if that fails?
}
//put the loaded img in the map
m_loadedImages.insert(pair<string, SDL_Surface*>(name, surface));

return surface;
}


const TTF_Font* Loader::loadFont(const string name)
{
//check if the file exists, if it does return it
map<string, TTF_Font*>::const_iterator it = m_loadedFonts.find(name);
if(it != m_loadedFonts.end())
{
return it->second;
}

//if not, return NULL
return NULL;
}


This is my first time ever working with memory management since this is also my first "real" project ever. So this is totally new territory for me and i am completely stuck since i cant seem to figure out what is wrong.

There aren't any references in your code, only pointers D:

Yeah, my mind were some place else.

I think freeing surfaces should only be done when you are absolutely sure it will never be used again. Are you really going to use this example_image.png once? If you are sure, then the correct way to do it is to use the complement of imageLoader.loadImg

This is true and i plan on adding this function. But i am planning on writing a system which takes care of what is in use and what's not so i am waiting with this. There really aren't many images now to load either so the unload functions isn't needed at the moment.

Thanks for your reply!