• Announcements

    • khawk

      Download the Game Design and Indie Game Marketing Freebook   07/19/17

      GameDev.net and CRC Press have teamed up to bring a free ebook of content curated from top titles published by CRC Press. The freebook, Practices of Game Design & Indie Game Marketing, includes chapters from The Art of Game Design: A Book of Lenses, A Practical Guide to Indie Game Marketing, and An Architectural Approach to Level Design. The GameDev.net FreeBook is relevant to game designers, developers, and those interested in learning more about the challenges in game development. We know game development can be a tough discipline and business, so we picked several chapters from CRC Press titles that we thought would be of interest to you, the GameDev.net audience, in your journey to design, develop, and market your next game. The free ebook is available through CRC Press by clicking here. The Curated Books The Art of Game Design: A Book of Lenses, Second Edition, by Jesse Schell Presents 100+ sets of questions, or different lenses, for viewing a game’s design, encompassing diverse fields such as psychology, architecture, music, film, software engineering, theme park design, mathematics, anthropology, and more. Written by one of the world's top game designers, this book describes the deepest and most fundamental principles of game design, demonstrating how tactics used in board, card, and athletic games also work in video games. It provides practical instruction on creating world-class games that will be played again and again. View it here. A Practical Guide to Indie Game Marketing, by Joel Dreskin Marketing is an essential but too frequently overlooked or minimized component of the release plan for indie games. A Practical Guide to Indie Game Marketing provides you with the tools needed to build visibility and sell your indie games. With special focus on those developers with small budgets and limited staff and resources, this book is packed with tangible recommendations and techniques that you can put to use immediately. As a seasoned professional of the indie game arena, author Joel Dreskin gives you insight into practical, real-world experiences of marketing numerous successful games and also provides stories of the failures. View it here. An Architectural Approach to Level Design This is one of the first books to integrate architectural and spatial design theory with the field of level design. The book presents architectural techniques and theories for level designers to use in their own work. It connects architecture and level design in different ways that address the practical elements of how designers construct space and the experiential elements of how and why humans interact with this space. Throughout the text, readers learn skills for spatial layout, evoking emotion through gamespaces, and creating better levels through architectural theory. View it here. Learn more and download the ebook by clicking here. Did you know? GameDev.net and CRC Press also recently teamed up to bring GDNet+ Members up to a 20% discount on all CRC Press books. Learn more about this and other benefits here.
Sign in to follow this  
Followers 0
Tallkotten

Memory management, mainly using SDL

17 posts in this topic

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:
[CODE]
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!!!
}
[/CODE]

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! :) Edited by Tallkotten
0

Share this post


Link to post
Share on other sites
Hi Tallkotten,

Is imageLoader loading/allocating the image every time (ie calling SDL_LoadImage or whatever)? That would be the only way you would be leaking. However, I would assume imageLoader keeps track of which images have been loaded and it just returns a pointer to that surface. In that case, you should NOT be calling SDL_FreeImage. Only the imageLoader should ever call SDL_FreeImage, and only then when the user explicitly tells it to.

You are also incorrect about the int*. An int* is a pointer to an int, but if it was created with a new or malloc, then it must be delete'd or free'd, otherwise it is still taking up room on the heap.

You should not be generating new SDL_Surface's for each update. A good solution is to create an ImageManager, which handles all the images you need. Something like this:

[code]
// in TImageManager.h
class TImageManager {
private:
// this maps the filename to a pre-defined SDL_Surface
std::map<std::string, SDL_Surface*> ImageMap;

public:
SDL_Surface* GetImage(std::string imageFileName);
};

// in TImageManager.cpp
TImageManager::GetImage(std::string imageFileName)
{
// check if this image has already been loaded
if (ImageMap.find(imageFilename) == ImageMap.end()) {
// It hasn't been loaded, load it here and store in map
ImageMap[imageFileName] = SDL_LoadImage(imageFileName.c_str()); // or whatever the SDL function name is
}

return ImageMap[imageFileName];
}
[/code]
1

Share this post


Link to post
Share on other sites
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
[CODE]
#include "loader.h"

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

private:
SDL_Surface* imageToUseInClass;
Loader imageLoader;
}
[/CODE]

class.cpp
[CODE]
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);
}
[/CODE]
0

Share this post


Link to post
Share on other sites
[quote]
//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.
[/quote]

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.

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.

Third, your example class is wrong:
[code]
if(newImage != NULL)
{
SDL_FreeSurface(imageToUseInClass);
imageToUseInClass = newImage;
SDL_FreeSurface(newImage);
}
else
{
SDL_FreeSurface(newImage);
}
[/code]
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.
0

Share this post


Link to post
Share on other sites
[quote name='BeerNutts' timestamp='1352232496' post='4998178']
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.
[/quote]

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.

[quote name='BeerNutts' timestamp='1352232496' post='4998178']
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.

[/quote]

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.

[quote name='BeerNutts' timestamp='1352232496' post='4998178']
Third, your example class is wrong:
[code]
if(newImage != NULL)
{
SDL_FreeSurface(imageToUseInClass);
imageToUseInClass = newImage;
SDL_FreeSurface(newImage);
}
else
{
SDL_FreeSurface(newImage);
}
[/code]
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.
[/quote]

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

Share this post


Link to post
Share on other sites
It's obvious you don't understand the point of an image manager if you are trying to free an image outside of it, and yet you want to disagree with me on having multiple instances of it (which defeats the purpose of having a resource manager in the first place!); I guess you have to make your own mistakes to learn them.

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:
[code]
SDL_Surface* tempSurface = imageLoader.loadImg("example_image.png");

SDL_BlirSurface(tempSurface, NULL, screen, NULL);
[/code]

Which will not leak any memory, again, assuming imageLoader.loadImg() is written correctly.
0

Share this post


Link to post
Share on other sites
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

[CODE]imageLoader.unloadImg("example_image.png");[/CODE]

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.

[quote name='Tallkotten' timestamp='1352201082' post='4997979']
[CODE]
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)?
[/CODE]
[/quote]

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

[quote name='Tallkotten' timestamp='1352201082' post='4997979']
How do i make sure i free all the memory possible without corrupting/deleting the reference?
[/quote]

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

Share this post


Link to post
Share on other sites
[quote name='BeerNutts' timestamp='1352239582' post='4998219']
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:
[code]
SDL_Surface* tempSurface = imageLoader.loadImg("example_image.png");

SDL_BlirSurface(tempSurface, NULL, screen, NULL);
[/code]
[/quote]

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
[CODE]
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;

}
[/CODE]

a function in enemy.cpp
[CODE]
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;

}
[/CODE]

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:

[CODE]
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;
}
[/CODE]

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

Share this post


Link to post
Share on other sites
[quote name='ultramailman' timestamp='1352247935' post='4998277']
There aren't any references in your code, only pointers D:
[/quote]

Yeah, my mind were some place else.

[quote name='ultramailman' timestamp='1352247935' post='4998277']
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
[/quote]

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

Share this post


Link to post
Share on other sites
Hi, if you are using C++, perhaps you can use a smart pointer for this (These two lines looks ugly as sin, but provides memory safety I have yet to see with any other language / platform)...

[code]
std::tr1::shared_ptr<SDL_Surface> surface;
surface.reset(SDL_LoadImage(imageFileName.c_str()), std::tr1::bind(SDL_FreeSurface, std::tr1::placeholders::_1));
// Now you can do whatever and RAII will take care of the memory management.
[/code]

Remember: You must set the deleter to be SDL_FreeSurface, because otherwise (and in std::auto_ptr) the c++ [b]delete[/b] will just be called, which will cause undefined behaviour with things not created via [b]new[/b].

I generally do this with OpenGL textures but there is no reason this wont work for SDL textures.

If you are not using the very latest compiler you will need to include the technical report 1 versions of memory and functional.
#include <tr1/functional>
#include <tr1/memory> Edited by Karsten_
0

Share this post


Link to post
Share on other sites
Hello again
You are sure this snippet is leaking.
[CODE]
SDL_Surface* tempSurface = imageLoader.loadImg("example_image.png");
SDL_BlirSurface(tempSurface, NULL, screen, NULL);
[/CODE]

Does that mean loadImg creates a new surface pointer everytime you call [CODE]imageLoader.loadImg("example_image.png");[/CODE] ? If so, then you really have to free that surface via imageLoader.unload as soon as you don't need it (i.e. in the update(int) function), because as soon as you exit that update function, you lose access to that surface. If however I guessed wrong, and your loader does cache the surface pointers, then it's all cool. However you did observe steadly growing memory usage, so I'm pretty sure you are creating a new copy with every call to the loader.
0

Share this post


Link to post
Share on other sites
[quote name='Karsten_' timestamp='1352315450' post='4998532']
Hi, if you are using C++, perhaps you can use a smart pointer for this (These two lines looks ugly as sin, but provides memory safety I have yet to see with any other language / platform)...

[code]
std::tr1::shared_ptr<SDL_Surface> surface;
surface.reset(SDL_LoadImage(imageFileName.c_str()), std::tr1::bind(SDL_FreeSurface, std::tr1::placeholders::_1));
// Now you can do whatever and RAII will take care of the memory management.
[/code]

Remember: You must set the deleter to be SDL_FreeSurface, because otherwise (and in std::auto_ptr) the c++ [b]delete[/b] will just be called, which will cause undefined behaviour with things not created via [b]new[/b].

I generally do this with OpenGL textures but there is no reason this wont work for SDL textures.
[/quote]

Yes that is one option i might go with later on when i add the manager system. Like i described earlier i dont really have to unload images at the moment since i have so few loaded.

But i'll definitely consider it!

[quote name='Karsten_' timestamp='1352315450' post='4998532']
If you are not using the very latest compiler you will need to include the technical report 1 versions of memory and functional.
#include <tr1/functional>
#include <tr1/memory>
[/quote]

How do i know if i have to do that or not? And what difference does it make?

I am running codeblocks
0

Share this post


Link to post
Share on other sites
I just found out where one of the leaks were located... In one of my functions where i turn a string to a char* array i never used delete after new...

Thanks everyone! Hopefully i wont get stuck on the rest of the code (that i have yet to check for leaks :)).
0

Share this post


Link to post
Share on other sites
[quote name='Tallkotten' timestamp='1352366168' post='4998799']
How do i know if i have to do that or not? And what difference does it make?
[/quote]

If you get a compiler error stating that std::tr1::shared_ptr does not exist, then you will need to include the tr1 versions of the header.

If you are using the CodeBlocks IDE, that probably means you are using GCC/G++ as your compiler, so it is quite likely you will need to include them.
0

Share this post


Link to post
Share on other sites
So why are you using `new' and `delete' for things like a "char * array"? You probably should be using std::vector or std::string instead of using new to allocate arrays. It's much harder to have a memory leak if you don't use `new'.
1

Share this post


Link to post
Share on other sites
[quote name='Karsten_' timestamp='1352477385' post='4999333']
[quote name='Tallkotten' timestamp='1352366168' post='4998799']
How do i know if i have to do that or not? And what difference does it make?
[/quote]

If you get a compiler error stating that std::tr1::shared_ptr does not exist, then you will need to include the tr1 versions of the header.

If you are using the CodeBlocks IDE, that probably means you are using GCC/G++ as your compiler, so it is quite likely you will need to include them.
[/quote]

Ok thanks! Yes i am using GCC/G++ :)

[quote name='Álvaro' timestamp='1352486514' post='4999377']
So why are you using `new' and `delete' for things like a "char * array"? You probably should be using std::vector or std::string instead of using new to allocate arrays. It's much harder to have a memory leak if you don't use `new'.
[/quote]

Well that is old code and when i looked up how to do it that was one of the only ways. Also TTF_RenderText_Solid requires a char*. http://www.libsdl.org/projects/SDL_ttf/docs/SDL_ttf_43.html
0

Share this post


Link to post
Share on other sites
No, it requires a [i]const[/i] char *, which you can from a std::string with the .c_str() member function.
1

Share this post


Link to post
Share on other sites
[quote name='SiCrane' timestamp='1352487659' post='4999386']
No, it requires a [i]const[/i] char *, which you can from a std::string with the .c_str() member function.
[/quote]

Well... f*ck me...

I've used that to some extent but now i just feel silly... Is it even worth going back and using that instead of my newly implemented system?
0

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!


Register a new account

Sign in

Already have an account? Sign in here.


Sign In Now
Sign in to follow this  
Followers 0