Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm trying to make a sprite cache for SFML sprites. The basic idea I got was to use a map of an enum and a sprite pointer. Then when I would ask for a certain sprite I'd have a manager that would check if the pointer for that sprite is null. If it is, then it would create a new sprite object and clip it to fit what it's supposed to be. If it isn't null, then it means that the sprite object for this particular enum already exists, and I get that.

So far I've made this and it works, but I'm not sure I did it quite right.

This is my example code. It contains a small class called SpriteManager which holds the sprite map. It does the before mentioned pointer checking and creating, as well as assigning a texture to, and clipping the sprite.

Here's the example code:

#include <SFML/Graphics.hpp>
#include <algorithm>
#include <map>

enum Sprites
{
    ITEM1,
    ITEM2,
    ITEM3,
    ITEM4,
    ITEM5
};

const int WIDTH = 100;
const int HEIGHT = 100;

typedef std::map< Sprites, sf::Sprite* > SpriteMap;
typedef std::map< Sprites, sf::Sprite* >::iterator iter;

class SpriteManager
{
private:
    SpriteMap spriteMap;
    sf::Texture& m_texture;
    void clipSprite(Sprites name)
    {
        spriteMap.at(name)->setTexture(m_texture);
        switch(name)
        {
        case ITEM1: spriteMap.at(name)->setTextureRect(sf::IntRect(0,0,WIDTH,HEIGHT));break;
        case ITEM2: spriteMap.at(name)->setTextureRect(sf::IntRect((1*WIDTH),0,WIDTH,HEIGHT));break;
        case ITEM3: spriteMap.at(name)->setTextureRect(sf::IntRect((2*WIDTH),0,WIDTH,HEIGHT));break;
        case ITEM4: spriteMap.at(name)->setTextureRect(sf::IntRect((3*WIDTH),0,WIDTH,HEIGHT));break;
        case ITEM5: spriteMap.at(name)->setTextureRect(sf::IntRect((4*WIDTH),0,WIDTH,HEIGHT));break;
        }
    }
public:
    SpriteManager(sf::Texture& texture) : m_texture(texture)
    {}
    sf::Sprite* getSprite(Sprites name)
    {
        if(!spriteMap[name])
        {
            spriteMap[name] = new sf::Sprite();
            clipSprite(name);
            return spriteMap[name];
        }
        else
            return spriteMap[name];

    }
    ~SpriteManager()
    {
        for(iter i = spriteMap.begin(); i != spriteMap.end(); ++i )
        {
            delete i->second;
        }
    }
};

int main()
{
    sf::RenderWindow window(sf::VideoMode(800,600), "Test", sf::Style::Titlebar | sf::Style::Close);

    sf::RectangleShape background(sf::Vector2f(800.0f,600.0f));

    window.setFramerateLimit(30);

    sf::Texture spriteSheet;

    if(!spriteSheet.loadFromFile("TestSprites.png"))
    {
        return 1;
    }

    SpriteManager sprites(spriteSheet);

    sf::Sprite *sprite = sprites.getSprite(ITEM1);
    sf::Sprite *sprite2 = sprites.getSprite(ITEM3);

    sprite->setPosition(100,100);
    sprite2->setPosition(200,100);

    while(window.isOpen())
    {
        sf::Event event;
        while( window.pollEvent(event))
        {
            if(event.type == sf::Event::Closed)
            {
                window.close();
            }
        }

        window.clear();
        window.draw(background);
        window.draw(*sprite);
        window.draw(*sprite2);
        window.display();
    }

    return 0;
}

So far my biggest concerns are:

  1. Is the destructor ok? I figured that since I'm allocating sprites that I should delete them all upon the manager going out of scope, though the more I think about it the more uneasy I feel.

  2. What would I do if I have to have two of the same sprites on the screen at the same time? Basically what happens now is, if I have two calls for a sprite with the same name:

    sf::Sprite *sprite1 = sprites.getSprite(ITEM1);
    sprite1->setPosition(100,100);
    sf::Sprite *sprite2 = sprites.getSprite(ITEM1);
    sprite2->setPosition(100,200);
    

I won't actually have two sprites on the screen but only one at the position 100x, 200y, since when I called the first getSprite I created a sprite under ITEM1 key and then when I call the getSprite method the second time I get that first sprite again. Now this is great for memory conservation, however a lot of the times I'll need to have multiple instances of the same sprite on screen. For that I have an idea to add some sort of a counter or something, but nothing concrete yet. Any suggestions?

EDIT:

Here's the improved version:

#include <SFML/Graphics.hpp>
#include <memory>
#include <map>


enum SpriteId
{
    ITEM1,
    ITEM2,
    ITEM3,
    ITEM4,
    ITEM5
};

const int WIDTH = 100;
const int HEIGHT = 100;

typedef std::tr1::shared_ptr< sf::Sprite > SpritePtr;
typedef std::map< SpriteId, SpritePtr > SpriteMap;
typedef std::map< SpriteId, SpritePtr >::iterator iter;

class SpriteManager
{
private:
    SpriteMap spriteMap;
    sf::Texture& m_texture;
    void clipSprite(SpriteId name)
    {
        spriteMap.at(name)->setTexture(m_texture);
        switch(name)
        {
        case ITEM1: spriteMap.at(name)->setTextureRect(sf::IntRect(0,0,WIDTH,HEIGHT));break;
        case ITEM2: spriteMap.at(name)->setTextureRect(sf::IntRect((1*WIDTH),0,WIDTH,HEIGHT));break;
        case ITEM3: spriteMap.at(name)->setTextureRect(sf::IntRect((2*WIDTH),0,WIDTH,HEIGHT));break;
        case ITEM4: spriteMap.at(name)->setTextureRect(sf::IntRect((3*WIDTH),0,WIDTH,HEIGHT));break;
        case ITEM5: spriteMap.at(name)->setTextureRect(sf::IntRect((4*WIDTH),0,WIDTH,HEIGHT));break;
        //default: should i create a special "Error" sprite and load it, akin to Valve's ERROR models?
        //         or maybe create and throw an exception.
        }
    }
public:
    SpriteManager(sf::Texture& texture) : m_texture(texture)
    {}
    SpritePtr getSprite(SpriteId name)
    {
        if(!spriteMap[name])
        {
            spriteMap[name] = std::tr1::make_shared< sf::Sprite >();
            clipSprite(name);
            return spriteMap[name];
        }
        else
            return spriteMap[name];

    }
    // I can call reset for each pointer to force them to deallocate, 
    // but that would just be replicating the initial raw pointer problem
    // I don't really need a destructor here, right?
    //~SpriteManager()
    //{
    //  for(iter i = spriteMap.begin(); i != spriteMap.end(); ++i )
    //  {
    //      i->second.reset();
    //  }
    //}
};

int main()
{
    sf::RenderWindow window(sf::VideoMode(800,600), "Test", sf::Style::Titlebar | sf::Style::Close);

    sf::RectangleShape background(sf::Vector2f(800.0f,600.0f));

    window.setFramerateLimit(30);

    sf::Texture spriteSheet;

    if(!spriteSheet.loadFromFile("SpriteSheet.png"))
    {
        return 1;
    }

    SpriteManager sprites(spriteSheet);

    SpritePtr sprite = sprites.getSprite(ITEM1);
    SpritePtr sprite2 = sprites.getSprite(ITEM2);

    sprite->setPosition(100,100);
    sprite2->setPosition(200,100);

    while(window.isOpen())
    {
        sf::Event event;
        while( window.pollEvent(event))
        {

            if(event.type == sf::Event::Closed)
            {
                window.close();
            }
        }

        window.clear();
        window.draw(background);
        window.draw(*sprite);
        window.draw(*sprite2);
        window.display();
    }

    return 0;
}

A thing to note here that this is just an example; it's a test model. That is why everything is in a single .cpp file and the item names are not descriptive.

share|improve this question
add comment

1 Answer

The code looks good, and your question is well written: thanks.

  1. I would call the Sprites enum SpriteId since at any given point it only store one sprite id.

  2. Returning a pointer could be quite problematic here: as you say, if SpriteManager goes out of scope then any reference to those sprites will cause issues. The simplest thing to do is to use a shared_ptr: it won't be deleted until no one references it.

  3. There is no default case in your switch and your names are not descriptive.

  4. The number and position of sprites is hardcoded and isn't easy to change. Did you consider using a string as a key, and storing the sprite coordinates in a file elsewhere?

EDIT answer:

  1. When the enum is not found, you certainly don't want to fail silently, and the way you fail depends on your objectives. You can have a production mode where you don't fail, but otherwise you want this error to be as visible as possible: throwing an exception/ending the program is a good idea.
  2. You no longer need a destructor indeed, but you still need to think about potential child classes: if you want to allow inheritance, then add an empty virtual constructor. Another thing to consider is that you'll need this code when you'll be hunting for memory leaks: a tool like valgrind will tell you that there are still references to those pointers only if they get reset in SpriteManager.
  3. I think you need to use typedef typename instead of just typedef.
share|improve this answer
    
Thanks for the answer. –  MrPlow Feb 6 '13 at 13:25
    
Whoops, pressed enter by accident. This is what i meant to write: Thanks for the answer. 1. True, I'll change it. 2. Thanks, I'm not that experience when it comes to smart pointers so i wasn't sure if I could and should use one, and which one. 3. As far as the names go, these are just names for a test case, if I'd use this code in an actual application I'd use more descriptive enums. I'm not quite sure as to what should i make my default do. 4. True, i probably should use a string as a key. Storing the sprite coordinates in a file? Similar to Properties in Java? –  MrPlow Feb 6 '13 at 13:30
    
I don't know about Properties. That's up to you: if you think it's fine in a C++ file, then it can work too. It mostly depends on the size and complexity of the game. –  Quentin Pradet Feb 6 '13 at 14:28
    
I updated the OP with an improved version and I'd really appreciate any comments on it. Also I thought about using std::strings as keys but wouldn't that allow "duplicates"(spelling/uppercase/lowercase mistakes), and make clipping the sprites quite a bit harder? Unless I first set them to all uppercase or all lowercase. Of course i could use files to load sprite info like you said. –  MrPlow Feb 7 '13 at 23:01
    
You're right, strings aren't such a good idea because the typos won't be caught at compile time. I edited my answer for the other issues. (if you liked it, feel free to upvote it :) –  Quentin Pradet Feb 8 '13 at 8:24
show 3 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.