Hmm. A few strange ideas here.
First off, you're about to make a vector of strings and a separate vector of Textures, so that you can find a Texture by looking for a string in the vector and then going to the corresponding index of the Texture vector. This shows a profound ignorance of the other standard library containers. What you really want is an associative container, i.e. one where you can use the string *as* an index. That is, std::map.
Next, let's fix the Texture representation itself a little, shall we?
- We don't need load and free functions. That's what constructors and destructors are *for*.
- We don't need a 'loaded' boolean (I refuse to "pronounce" your Hungarian notation). We already have a pointer; we can let it be null if there is no surface. (BTW, thanks for the tip that SDL_FreeSurface guards against NULLs. I never do trust these C-style API writers to do things like that. :P)
- Don't use (void) for function prototypes (unless you must interoperate with C code which expects it). Use ().
- Const-overloading for accessors would probably be a good idea here.
- FFS, use initializer lists.
- std::string comes from its own header, TYVM. :) Also, prefer to pass strings by const reference.
- What's with all the explicit this->'s?
- The whole thing is simple enough that we don't really need a .cpp file (yet):
#ifndef JAKELIB_TEXTURE_H#define JAKELIB_TEXTURE_H#include "SDL.h"#include <string>// Everything has been reformatted in the ways I usually do it.namespace jakelib {class Texture { // temporarily disabled Texture(const Texture&); Texture& operator=(const Texture&); SDL_Surface* surface; // surface for the image (NULL if not loaded) // (There are arguments to be made for throwing an exception instead if // the surface could not be loaded.) public: Texture() : surface(0) {} // Load from the specified file, with no transparency. Texture(const std::string& path) : surface(SDL_LoadBMP(path.c_str()) {} // Load from the specified file, using the specified colour as a transparent // colour key. Texture(const std::string& path, int r, int g, int b) : surface(SDL_LoadBMP(path.c_str())) { if (surface) { SDL_SetColorKey(surface, SDL_SRCCOLORKEY | SDL_RLEACCEL, SDL_MapRGB(surface->format, r, g, b)); } } ~Texture() { SDL_FreeSurface(surface); } bool isLoaded() const { return surface; } SDL_Surface* getSurface() { return surface; } const SDL_Surface* getSurface() const { return surface; }};} //namespace jakelib#endif // JAKELIB_TEXTURE_H
Now, on to the real stuff. :)
You are correct that STL containers will copy, assign and destroy old versions of your objects all over the place, which makes the naive approach to RAII problematic. I don't think a boost::shared_ptr would really be overcomplicating things; they're there to make your life easier, after all. But since you only want one "real" copy of each SDL_Surface, there's a simpler way. That is to let the 'TextureManager' manage all of the Surfaces, by storing the pointers in its container and freeing *all* of them in *its* destructor. That takes advantage of the fact that copying the *pointers* around is no problem. :)
Then, we can make a map from texture names to Surface pointers. There's no real need for the Texture object any more; it wasn't really doing anything for us anyway (there's no encapsulation of the Surface there, as it stands, and no really meaningful binding together of data.)
That could look like this:
class TextureManager { typedef std::map<std::string, SDL_Surface*> storage_t; storage_t storage; public: void load(const std::string& texname, const std::string& filename) { // Deallocate any texture previously associated with this name SDL_FreeSurface(storage[texname]); // If it wasn't already referred to, that will basically insert a mapping // to a NULL pointer into the map implicitly, then call SDL_FreeSurface on // the NULL pointer. Then we overwrite it in the load step: storage[texname] = SDL_LoadBMP(filename.c_str()); } // Similarly, the colour-keyed version. void load(const std::string& texname, const std::string& filename, int r, int g, int b) { SDL_FreeSurface(storage[texname]); SDL_Surface* tmp = SDL_LoadBMP(filename.c_str()); if (tmp) { SDL_SetColorKey(tmp, SDL_SRCCOLORKEY | SDL_RLEACCEL, SDL_MapRGB(tmp->format, r, g, b)); } storage[texname] = tmp; } // Get the associated SDL_Surface*; NULL if never successfully loaded. SDL_Surface* get(const std::string& texname) { return storage[texname]; } ~TextureManager() { // If you have the SGI extension 'select2nd', you could do: // std::for_each(storage.begin(), storage.end(), compose1(SDL_FreeSurface, select2nd<SDL_Surface*>)); for (storage_t::iterator it = storage.begin(); it != storage.end(); ++it) { SDL_FreeSurface(it->second); } }}
Note that this does NOT guard against multiple texture names being used to open the same file name. Although that would only waste memory, instead of crashing or something. If you need to allow for that, you could add indirection to the mapping: have a map<string, string> from texture name to file name, and a map <string, SDL_Surface*> from file name to surface. The necessary modifications are left as an exercise. :)
This is also a lazy way of using the map; it will gradually fill it with useless NULL-surface nodes when failed accesses occur. That memory doesn't leak (the map destructor takes care of it), but it can degrade performance (not that it's likely to matter compared to the size of the SDL_Surface objects themselves, or compared to the time of the operations you perform upon them). To "do it right", use .find() and .insert() appropriately instead of the operator[] accesses.
Also, I deliberately didn't make this a singleton because you might want to use different managers to "namespace" your textures. If you specifically want to prevent that from happening (as part of guarding against redundant loads, for example), I'll let you do the dirty work. ;)
