Sign in to follow this  

Texture Manager design ( memory and copy-constructor )

This topic is 4070 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'm not sure what route to take to with my basic TextureManager. The part I'm not sure about is how to handle the memory. I first created a Texture() class that handles the memory for a single texture. ( It makes sure memory has been freed in its destructor. ) Now that I'm trying to use it in a std::vector , I'm getting problems. The TextureManager() will use a std::vector of TextureType's, which are just a std::string, and an instance of the Texture() object. The string is so I can reference the texture by a name. My Texture class works like this:
	Texture foo;
	foo.load("myImage.bmp");
	// ... do stuff
	// when the Texture() object goes out of scope, it gets cleaned up.
I thought I could use a list of them like this:
	std::vector<Texture> list;
	list.push_back( Texture("data/image1.bmp") );

Now that requires me to write a copy-constructor, and a assignment operator? 1) That's what I'm stuck on. Do I write a copy-constructor so it creates a new Texture() object, calls load(), and returns the class instance? But then doesn't the instance go out of scope when the function returns, causing a problem? 2) Should I use a boost::shared_ptr to create the object, or returning a shared_ptr to the object keep the memory allocated while the list is using it? Or is that over-complicating things? Here's my Texture() class: Texture.h
#ifndef JAKELIB_TEXTURE_H
#define JAKELIB_TEXTURE_H

#include "SDL.h"    // for SDL_Surface
#include <iostream> // std::string

namespace jakelib {

/*!
    \brief Stores a SDL_Surface, manages memory by RAII.
    \author Jake Bolton
    \date 2006/10/20
    \version v0.4

    This class is not meant to be used by itself, ( but can be ). But to
    be used with a texture manager, using a list of Texture() objects
*/
class Texture
{

    public:
        //! constructor
        /*! default constructor. longer desc. */
        Texture();

        //! destructor
        /*!
        currently just calls free() */
        ~Texture();

        //! frees memory.
        /*!
        frees the memory allocated/used by the texture. Probably only ever
        called internally. */
        void free();

        //! check if the surface has loaded.
        /*!
        check if the surface has been loaded or not */ 
        bool isLoaded(void) const { return m_bLoaded; }

        //! load a texture
        /*! load a bitmap image, using no transparency. If a texture is already
		loaded, it is free'd, and then it loads the new texture.
        \param path the path to the bitmap
        \sa load(std::string,int,int,int); */
        void load( std::string path );

        //! load a texture with transparency
        /*! load a bitmap image, using transparency
        \param path the path to the bitmap
        \param r the red value of the transparent color
        \param g the green value of the transparent color
        \param b the blue value of the transparent color
        \sa load(std::string)
        */
        void load( std::string path, int r, int g, int b );

        //! get a pointer to the SDL_Surface
        /*! Get a pointer to the SDL_Surface, for blit'ing.
        \return a SDL_Surface pointer to the surface
        */
        SDL_Surface *getSurface();

    private:
        /*! copy constructor : prevent copy-ing.
        At least for now, I don't want to allow copying. */
        Texture(const Texture& );
        /*! assignment operator : prevent copy-ing.
        At least for now, I don't want to allow copying.*/
        Texture& operator=( const Texture& );

        // == members ==
        SDL_Surface *m_surface; //!< surface containing the image
        bool m_bLoaded; //!< has the surface been loaded successfully?

};

} //namespace jakelib


#endif  // JAKELIB_TEXTURE_H

Texture.cpp
#include "Texture.h"
using namespace std;

namespace jakelib {

/* default constructor */
Texture::Texture()
{
    // set surface to NULL. This is required for the other functions to work.
    m_surface = NULL;
    m_bLoaded = false;
}

/* destructor */
Texture::~Texture()
{
    //free SDL_Surface memory
    this->free();
}

/* frees the memory allocated/used by the texture. */
void Texture::free()
{
    // free the surface. (it's okay to call the function with a NULL surface)
    SDL_FreeSurface(m_surface);
    m_bLoaded = false;
}

/* load a bitmap image, using no transparency */
void Texture::load( std::string path )
{
    // make sure surface is currently null. If not, free it, then load new surface
    if( m_surface != NULL )
    {
        this->free();
    }

    // load the surface
    m_surface = SDL_LoadBMP( path.c_str() );
	
	// check that it worked
    if( m_surface == NULL )
    {
        // it didn't work!
    }
    else
    {
        //success, log the load
        m_bLoaded = true;
    }
}

/* load a bitmap image, using transparency */
void Texture::load( std::string path, int r, int g, int b )
{
    //first load like normal
    this->load( path );

    //then set the color key
    SDL_SetColorKey( this->m_surface, SDL_SRCCOLORKEY|SDL_RLEACCEL,
        SDL_MapRGB( this->m_surface->format, r, g, b) );
    cout << "Texture::load(r,g,b) key set" << endl;
}

SDL_Surface* Texture::getSurface()
{
    return this->m_surface;
}

} // namespace jakelib


Share this post


Link to post
Share on other sites
Why don't you put the name of the texture into the texture class itself, rather than having an additional TextureType struct?

As to your actual problem - allocate your Texture instances on the heap and keep a vector of pointers to them instead.

cheers
sam

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Just don't forget you have to manually deallocate those pointers before you destroy the vector.

Share this post


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

Share this post


Link to post
Share on other sites
Thanks for all the answers.

I started writing a new version using .find(), and .insert() instead of the [] operator. I'm having a problem using .find() and calling SDL_FreeSurface(). It looks like the surface isn't being free'd.

Here's what I'm trying:
storage_map::iterator search = storage.find("image");
if( search != storage.end() )
SDL_FreeSurface( search->second );



This is a snippet from a test program that I wrote. It loads a surface, tests that it exists, and tries to free it. ( Apparently failing )

Here's the output:
Check if it exists:
image is not NULL
free memory:
attempting: SDL_FreeSurface(search->second);
check that it's free
image is not NULL
Done



Here's the test code:
void test()
{
typedef std::map<std::string, SDL_Surface*> storage_map;
typedef std::pair<std::string, SDL_Surface*> storage_pair;

storage_map storage; //!< map to hold textures
storage_map::iterator search; //!< to search map

//load a texture
SDL_Surface *tmp = SDL_LoadBMP("data/image1.bmp");

//insert a pair into the map:
storage.insert( storage_pair("image", tmp ));

//check if it exists
cout << "Check if it exists:" << endl;
search = storage.find("image");
if( search != storage.end() )
{
// check key
if( search->second == NULL )
cout << "\timage is equal to NULL" << endl;
else
cout << "\timage is not NULL" << endl;
}

// free it
cout << "free memory:" << endl;
search = storage.find("image");
if( search != storage.end() )
{
cout << "\attempting: SDL_FreeSurface(search->second);" << endl;
SDL_FreeSurface( search->second );
}

//this will cause a segfault, I'm not sure exactly why?
//SDL_FreeSurface( tmp );

//verify
// check key
cout << "check that it's free" << endl;
if( search->second == NULL )
cout << "\timage is equal to NULL" << endl;
else
cout << "\timage is not NULL" << endl;
}

Share this post


Link to post
Share on other sites
Quote:
Original post by ninmonkeys
//this will cause a segfault, I'm not sure exactly why?
//SDL_FreeSurface( tmp );

Because you already called SDL_FreeSurface on that pointer.

Quote:
Original post by ninmonkeys
if( search->second == NULL )
cout << "\timage is equal to NULL" << endl;
else
cout << "\timage is not NULL" << endl;

Why do you expect search->second to be NULL here? You didn't set it to NULL, you only passed it to SDL_FreeSurface.

Share this post


Link to post
Share on other sites
I'll just expand on that a bit if I may:


void FreeObject(Object *O)
{
O->Free();
O=NULL; // this only affects the local copy of O
}

void f()
{
Object *O=CreateObject();

// ... //

FreeObject(O);

// at this point, the local variable O has been unaffected by
// calling FreeObject so still points to the memory address
// that the Object used to reside at before it was freed

if(O==NULL) ThisWillNotExecute();
}







This is why you frequently see stuff like the below in the DirectX interfaces:


void FreeObject(Object **O)
{
(*O)->Free;
*O=NULL;
}

void f()
{
Object *O=CreateObject();

// ... //

FreeObject(&O);

if(O==NULL) ThisWillExecute();
}







Clearly SDL is following the first model, so after you call SDL_FreeSurface (or whatever its called), you also need to explicitly set the pointer to NULL yourself.

DirectX has a macro called SAFE_DELETE that sort of looks like:

#define SAFE_DELETE(x) x->Release(); x=NULL

that you could approximate, but you'd be better to write a function that took a reference to a pointer instead:


void MySDLFree(SDLSurface* &T)
{
SDL_FreeSurface(T); T=NULL;
}


and call that instead of SDL_FreeSurface directly.

Even better, as often with these C style interfaces, wrap the SDLSurface* in a wrapper class that provides member functions for acquisition, releasing and other accessing that you need. You can then also centralise checking for the NULL pointer inside the class rather than spread it across your whole program.

Share this post


Link to post
Share on other sites

This topic is 4070 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.

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