Jump to content
  • Advertisement
Sign in to follow this  
TristanCrockett

How does my resource manager suck?

This topic is 2716 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I started making a sprite-based game using SDL, and one of the first things I needed to do is manage these sprites. So I coded one up, and it seems to work good enough for now. I'm wondering if people with more experience have any comments? I figured I would show people the code right now, when it's functional enough to have problems but small enough so that it's not a pain to look through. In particular I'd like to know if there's anything horribly inefficient about the logic, or that set me up for memory leaks. I understand this is only one class but right now it looks like it's the best candidate for inefficiencies and I don't think anybody wants to look through an entire project (I could post the rest if for some odd reason anyone wanted to see it). The rendering of the game objects should be implicit from my apply_surface method. The plan is for pretty much all sprite rendering to be handled through this object, and that is what I am doing now. Game objects have draw() methods, but they simply call apply_surface here with their resource id and it does the rendering.

I do get a Valgrind error when running it.
At this statement: " imageSurfaces[newIndex] = *optimizedImage;"
It says that "address 0x0 is not stack'd malloc'd or recently free'd".
This should mean that I'm trying to dereference a null pointer, right? It looks like it could theoretically happen but I've ran the program, printing out the value of optimizedImage and imageSurfaces (the address, not the value) the line before the incriminating call, and at no point did they display 0x0.

ResourceManager.h

class ResourceManager
{
private:
SDL_Surface * imageSurfaces;
int numImageSurfaces;
int sizeOfImageSurfaceArray;
public:
void apply_surface( int x, int y, int resourceId, SDL_Surface* destination );
int load_image( std::string filename );
void displayList();
ResourceManager() { imageSurfaces = new SDL_Surface[1]; sizeOfImageSurfaceArray = 1; }
~ResourceManager() {
for ( int i=0; i< numImageSurfaces; i++ ) { SDL_FreeSurface( &(imageSurfaces) ); } delete imageSurfaces;
}
};


ResourceManager.cpp

#include "resourcemanager.h"

int ResourceManager::load_image( std::string filename ) {
SDL_Surface* loadedImage = NULL; //Temporary storage for the image that's loaded
SDL_Surface* optimizedImage = NULL; //The optimized image that will be used
loadedImage = IMG_Load( filename.c_str() );

if( loadedImage != NULL ) {
//Create an optimized image
optimizedImage = SDL_DisplayFormatAlpha( loadedImage ); //Free the old image
SDL_FreeSurface( loadedImage );
}
int newIndex = numImageSurfaces;


// manually manage the resource array, because references to vector contents apparently do not persist through expansions
if( numImageSurfaces >= sizeOfImageSurfaceArray)
{
sizeOfImageSurfaceArray *= 2;
SDL_Surface * newArray = new SDL_Surface[sizeOfImageSurfaceArray];
for( int i = 0; i<newIndex; i++)
{
newArray = imageSurfaces;
}

delete imageSurfaces;
imageSurfaces = newArray;
}

numImageSurfaces++;
imageSurfaces[newIndex] = *optimizedImage;

return newIndex;
}

// Applies the surface referenced by the resourceId to the destination surface
void ResourceManager::apply_surface( int x, int y, int resourceId, SDL_Surface* destination ) {
SDL_Rect offset;
offset.x = x;
offset.y = y;
SDL_BlitSurface( &(imageSurfaces[resourceId]), NULL, destination, &offset );
}

Share this post


Link to post
Share on other sites
Advertisement
Your first mistake is trying to manage memory manually, and arent obeying all the rules. As a result, horrible things are being done to your stuff.
Rely on RAII to manage your SDL_Surface's.

First off, store your SDL structs in a wrapper struct like this:


struct SurfaceHolder
{

SurfaceHolder() : surface(null), valid(false) {}

void load(const std::string& filename)
{
// your current code to load your texture
}

virtual ~SurfaceHolder()
{
unload();
}

void unload()
{
if (valid)
{
SDL_FreeSurface(this->surface);
valid = false;
}
}

SDL_Surface * surface;
bool valid;
};

This will guarantee you creation and timely deletion of your SDL surface, providing you don't do anything stupid with your SDL surfaces.

Secondly, I note that you load_surface function returns an ID for the surface it represents. Thats perfectly sensible. However, storing things as old fashioned arrays means you don't have all the semantics you need; you need associativity (an array is only associative because your keys are integers) but you also need sparseness - right now you can't unload a single surface and have things remain valid. Your issue of references being invalidated is down to the fact that vectors move their memory around when they resize.


Try a map instead; you will be able to use a key type other than an integer to load your resources.

std::map<int, SurfaceHolder> surfaces;

You can then use map's functions such as find, remove, etc. to load, unload, and store your resources. For more powerful control over the system, consider a smart pointer such as boost.auto_ptr which will manage deletion for you but still give you pointer semantics:

std::map<int, boost::auto_ptr<SurfaceHolder> > surfaces;


There is also shared_ptr (which might be a better option as you can then pass around a shared_ptr<SurfaceHolder> ) and intrusive_ptr(which does a similar job and is worth looking into)

You might want to keep something similar to your current behaviour and use a deque, however; deque does not move its memory around (reallocate) when it is resized so you will be totally safe, but it does exhibit most of the behaviour you have now. Either way, because you are storing your SurfaceHolder by value rather than by pointer, there is no manual operation required to free up memory when it is no longer needed. Your problem of freeing a pointer will disappear.

As an aside, a couple of recommendations;
1) Avoid manual memory operations at all costs unless you are really trying to squeeze performance out of something and know what you are doing; use existing stuff, slow or otherwise, until you reach that point. 90% of your code is only executed 10% of the time.
2) Check out this popular link when you next come to choose a storage medium: Container Choice Flowchart
3) Before you say "but I need the best performance" I would advise you do do some profiling first and then optimise this some more later. To start out you need safe and reliable resource management, because it is the backbone of any game.

Share this post


Link to post
Share on other sites
Thanks! I have a few questions.


Your first mistake is trying to manage memory manually, and arent obeying all the rules. As a result, horrible things are being done to your stuff.



I will take your advice and use a map or some other structure that allows me to avoid manually managing the array.
For the sake of argument, what rules am I breaking? I'm doing this game as much to educate myself as anything else. I'd like to understand it better and be able to prevent code this small and simple from leaking memory, even if going forward I'm not going to rely on manual management.




virtual ~SurfaceHolder()
{
unload();
}

void unload()
{
if (valid)
{
SDL_FreeSurface(this->surface);
valid = false;
}
}

SDL_Surface * surface;
bool valid;
};

This will guarantee you creation and timely deletion of your SDL surface, providing you don't do anything stupid with your SDL surfaces.


1. Why is the destructor virtual? This doesn't look like something I would derive from, and I'm not sure in what other scenarios you would use virtual.

Thanks again, I will take your advice. A map is the most likely route for me. I will look into smart pointers, though I'm not sure I understand the benefits of pointer semantics (I mostly just use them out of perceived necessity).

Share this post


Link to post
Share on other sites

1. Why is the destructor virtual? This doesn't look like something I would derive from, and I'm not sure in what other scenarios you would use virtual.
Why not? Doesn't cost you anything and if you ever do derive it you'll want to have it. I'm guessing speciesUnknown creates all classes like that out of habit. There are worse habits to adopt in C++!

Share this post


Link to post
Share on other sites
SpeciesUnkown's code does not handle the rule of three, so I would be suspicious of using it with a Standard C++ Container. In the long run, it will probably break unless you explicitly make the SurfaceHolder type noncopyable (e.g. boost::noncopyable, or make the copy constructor and assignment operator private). Alternatively, implement these operations correctly. One potential solution can be achieved by holding the SDL_Surface using a reference counted smart pointer.

My criticism of your resource manager is that it does too much - you need to look at the Single Responsibility Principle. Your class contains the logic for three classes, a Loader object, a Renderer object and finally it contains container logic that could be replaced using std::vector<>.

I would apply this to the SurfaceHolder class too, which handles both loading and being a surface. These responsibilities should be separated. Already you can see how it complicates the class implementation, the "invalid" state of an unloaded Surface must be accounted for - what happens if we try to load a surface into an already loaded one? Instead, have a separate class or function that loads the image, and then return a correctly constructed SurfaceHolder from this. The SurfaceHolder can assert() that it has a valid surface in its constructor, or it could throw an exception in the case it is passed a NULL surface.

Consider the following, the code interacting with this class obtains an ID. This ID is then sent back to the class to manipulate the object. If you return a more direct "Handle" to the object, for example a shared_ptr<>, then you can get nicer client experience from your code (type safety). Using opaque IDs isn't necessarily a bad idea, in fact if you were manipulating the surfaces from an external environment like a scripting language it can be great! But inside your own application you should try use the type system to get the compiler to eliminate bugs. For instance, at the moment a client could do something stupid like decrement or increment the ID value it obtains, and start messing with the wrong image or potentially step out of the bounds of your array.

Finally, consider what you haven't achieved. Most "resource managers" include a caching mechanism, which helps avoid loading the same resources repeatedly. You can save memory and processing time by doing so. A classic implementation is a std::map<std::string, shared_ptr<Surface>>, which you can use to quickly check if a particular image has been loaded already. You have to be careful with such an approach though, for example the "SurfaceHolder" class should be immutable for this to work. This is yet another reason for the "load" function to go.

Share this post


Link to post
Share on other sites
I made the dtor virtual because it costs nothing to type one word and, if there is even the remotest possibility that you extend from it later, you dont have to go back and remember to turn it virtual. Yes, I know this is a performance issue but again, you need to look at robustness first, then profile it, and then apply targeted optimisations. (such as removing virtual qualifiers to avoid a double indirection)



SpeciesUnkown's code does not handle the rule of three, so I would be suspicious of using it with a Standard C++ Container. In the long run, it will probably break unless you explicitly make the SurfaceHolder type noncopyable (e.g. boost::noncopyable, or make the copy constructor and assignment operator private). Alternatively, implement these operations correctly. One potential solution can be achieved by holding the SDL_Surface using a reference counted smart pointer.



A smart pointer isn't going to help with SDL_Surface because you dont just delete it; you have to call SDL_FreeSurface on it. This is why I made a holder. Putting it in a shared_ptr without a wrapper is going to result in incorrect behaviour and crashes.

Truth be told, c++ is such a complicated language and I didn't have long to make my post; I couldn't explain all the problems with the OP's original design in one go. He now has to learn about STL containers, smart pointers, the rule of 3, RAII, copy constructors, assignment operators, virtual destructors, boost and how to build it, and SRP. I addressed the major issue: manual memory management, and imparted a few points of wisdom.

This may be a good time to choose a language which is more beginner friendly, because it doesn't end here. You'll spend the next year learning how to manually do things you can take for granted in pretty much any other language.

Share this post


Link to post
Share on other sites

A smart pointer isn't going to help with SDL_Surface because you dont just delete it; you have to call SDL_FreeSurface on it. This is why I made a holder. Putting it in a shared_ptr without a wrapper is going to result in incorrect behaviour and crashes.

No, a std::shared_ptr or a boost::shared_ptr both can accept a deleter argument to specify that SDL_FreeSurface() be called instead of delete. Ex:

boost::shared_ptr<SDL_Surface> foo(SDL_LoadBMP("abc.bmp"), &SDL_FreeSurface);

Share this post


Link to post
Share on other sites
Are you sure that you actually need to be able to unload resources? My game has grown fairly large, but I've never even come close to running out of memory.

Designing it this way has a number of advantages. Not only do you get rid of all the code concerned with deciding if, what, and when to unload, but you also no longer need to reference count your handles, and you can safely use a plain vector to associate handles and surface pointers.


Also, don't be afraid to use a simple vector with linear probing to map image filenames to handles. That's what I do, and it's plenty fast for me. Theoretically, a hashtable or map would be more efficient, but it's never shown up on any profiling I've done, so I say the simplest solution wins.



By the way, here's my code. It seems to work pretty well, although there may be issues I don't know about.


#pragma once

#include "SDL/SDL.h"
#include <vector>
#include "imagedatastruct.h"
#include "utilityheaders/rectfuncts.h"

class imageManager
{
protected:
imageManager(); //Apparently, it's a good idea to make a Singleton constructor protected

public:
static imageManager& Instance();
typedef Uint32 ImageID;

static ImageID NullID() {return 0xFFFFFFFF;}
static bool IsValid(ImageID h);

ImageID GetImageID(const ImgDataStruct& data);
void draw_perSurfaceAlpha(ImageID id, SDL_Surface *screen, int x, int y, SDL_Rect* clip = NULL, int alpha = 255);

private:
std::vector<SDL_Surface*> loadedimages;
std::vector<ImgDataStruct> loadedimgdata;

SDL_Surface* LoadImage(const ImgDataStruct& data);
ImageID AddImage(const ImgDataStruct& data);
SDL_Surface* DereferenceID(ImageID h);
};



#pragma once

#include "imagedatastruct.h"
#include "imagemanager.h"
#include "optionalrect.h"

class ImgHandle
{
imageManager::ImageID myid;

public:
ImgHandle(): myid(imageManager::NullID()) {}
ImgHandle(const ImgDataStruct& imgdata): myid(imageManager::Instance().GetImageID(imgdata)) {}

void draw(SDL_Surface *screen, Sint16 screenx, Sint16 screeny, OptSdlRect clip = OptSdlRect(), Uint8 alpha = 255) const
{
assert(myid != imageManager::NullID());
imageManager::Instance().draw_perSurfaceAlpha(myid, screen, screenx, screeny, clip.get_ptr(), alpha);
}

operator bool() const {return myid != imageManager::NullID();}
};




#pragma once

#include "SDL/SDL.h"
#include <string>
#include "utilityheaders/rectfuncts.h"

//This class holds the data required to construct an SDL Surface. It can be an image, with colorkey or alpha, or text, possibly outlined
class ImgDataStruct
{
enum DrawableDataType{ IMAGE, COLORKEYIMAGE, RGBAIMAGE, SIMPLETEXT, OUTLINEDTEXT};

static SDL_Color NOCOLOR() {return MakeColor(0,0,0);}

DrawableDataType type;
std::string string1;
std::string string2;
SDL_Color color1;
int int1;
SDL_Color color2;
int int2;

////////////////////////////////////////////////////////////////////////////////////////

void ConvertToDisplayFormat( SDL_Surface*& surface, bool AddAlphaChannel) const;
////////////////////////////////////////////////////////////////////////////////////////
public:
ImgDataStruct(const std::string& filename, bool alphachannel=false);
ImgDataStruct(const std::string& filename, Uint8 red, Uint8 green, Uint8 blue);
ImgDataStruct(const std::string& buttontext, const std::string& fontname, int ptsize, SDL_Color forecolor);
ImgDataStruct(const std::string& buttontext, const std::string& fontname, int ptsize, int outlinewidth,
SDL_Color forecolor, SDL_Color outlinecolor);

bool Equals(const ImgDataStruct& other) const;
SDL_Surface* Load() const;
std::string DebugInfo() const;
};

Share this post


Link to post
Share on other sites
Thanks for all of the replies! So far what I've done is:

1. Created a SurfaceHolder struct similar to speciesUnknown's suggestion.
2. Created a separate ResourceLoader class which actually loads the resource and returns it to the SurfaceHolder.
3. Stored the resources using a map<string, SurfaceHolder>, which now refers to the local path. The actual loader uses the complete loading path ("../data/images/image.png"), but the key is stored starting from the data root ("images/image.png").
4. Implemented caching so if you load multiple objects with the same sprite filename, the resource will only be stored once.

There are a couple of things that I haven't implemented yet that I plan on doing, but won't yet.
1. Support for sprite sheets. Right now, file = sprite. This will change in the future.
2. There's currently no way to delete resources from the manager. This probably won't be a problem until there's multiple levels and I actually have a reason to delete resources.

I'll look into some of the other things mentioned (like smart pointers and raii) in the future, but for now this is fine.

It finally exits normally, instead of 'crashing with return code 0'.

edit: Storyyeller, I saw your last edit after I posted. I'll look at your code when I revisit this, particularly when I add in other types of resources like fonts

Share this post


Link to post
Share on other sites
I only posted the headers. I can post the cpps too if you want to look at those.

Anyway, the system I posted only handles images. I have a separate, much simpler system for sounds. It's simpler because sounds are only identified by filename and there is no need to keep handles to reference them. So the sound system is just a map of smart pointers to a holder class, indexed by filename.

I don't even bother storing fonts at all. I just load and immediately unload them inside the text rendering function.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

Participate in the game development conversation and more when you create an account on GameDev.net!

Sign me up!