My Graphics Manager

[Roose Bolton of the Dreadfort]

Hey Guys!

Hope you are all well.

I have just started a new project for a small 2D game which I am hoping to finish this month.

This is pretty much my first SFML project and have written a Graphics Component.

I was wondering if you guys can have a look and comment?

I know std::map is better then calling explicit indexes, but I prefer index way as I can edit the texture directly also if I want.

I also am using the GraphicsManager to be a black-boxed component for all Graphics related stuff.

Graphics Manager.h

#include <SFML/Graphics.hpp>

class GraphicsManager
{
public:
        ~GraphicsManager(void);
        static GraphicsManager* Instance();
        void GraphicsManager::Release();
        void GraphicsManager::InitWindow(std::string iTitleName, int iWidth, int iHeight, bool iFullscreen);
        void GraphicsManager::LoadTexture(std::string iFileName);
        sf::RenderWindow& GetWindow();
        void GraphicsManager::PrintLoadedTextures();

        std::vector<sf::Texture*>& GetTextureList();
       
private:
        static GraphicsManager* mGraphicsManagerInstance;
        std::vector <sf::Texture*> mTextureList;
        sf::RenderWindow mWindow;

protected:
        GraphicsManager(void);
};
 

GraphicsManager.cpp

#include "GraphicsManager.h"
#include <iostream>

GraphicsManager* GraphicsManager::mGraphicsManagerInstance = NULL;

GraphicsManager* GraphicsManager::Instance()
{
        // Return single instance
        if(mGraphicsManagerInstance)
        {
                return mGraphicsManagerInstance;
        }

        mGraphicsManagerInstance = new GraphicsManager;

        return mGraphicsManagerInstance;
}

std::vector<sf::Texture*>& GraphicsManager::GetTextureList()
{
        return mTextureList;
}

void GraphicsManager::PrintLoadedTextures()
{
        int loadedtexture = 0;

        for(unsigned int i = 0; i < mTextureList.size(); i++)
                loadedtexture++;       
       
        std::cout << "" << std::endl;
        std::cout << "There are currently: " << loadedtexture << " Textures loaded" << std::endl;
}

void GraphicsManager::Release()
{
        // Release single instance
        if(mGraphicsManagerInstance)
        {
                delete mGraphicsManagerInstance;
                mGraphicsManagerInstance = NULL;
        }
}



GraphicsManager::GraphicsManager(void)
{
        //ctor
}

GraphicsManager::~GraphicsManager(void)
{
        for (std::vector<sf::Texture*>::iterator deleter = mTextureList.begin(); deleter < mTextureList.end(); deleter++)
        {
                delete *deleter;
        }
}

void GraphicsManager::InitWindow(std::string iTitleName, int iWidth, int iHeight, bool iFullscreen)
{
        if ( iFullscreen )
                mWindow.create(sf::VideoMode(iWidth, iHeight), iTitleName, sf::Style::Fullscreen);
        else
                mWindow.create(sf::VideoMode(iWidth, iHeight), iTitleName);
}

sf::RenderWindow& GraphicsManager::GetWindow()
{
        return mWindow;
}

void GraphicsManager::LoadTexture(std::string iFileName)
{
        sf::Texture* mTexture = new sf::Texture();

        if(!mTexture->loadFromFile(iFileName))
                std::cout << "Error loading texture: " + iFileName << std::endl;
       
        this->GetTextureList().push_back(mTexture);
}
 

Main.cpp

#include <SFML/Graphics.hpp>
#include "GraphicsManager.h"

int main()
{
        #ifdef _DEBUG
        _CrtSetDbgFlag( _CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);
        _CrtSetReportMode( _CRT_ERROR, _CRTDBG_MODE_DEBUG );
        #endif
       
        //Grab Instance.
        GraphicsManager *Graphics = GraphicsManager::Instance();

        Graphics->InitWindow("Texture Manager", 512, 512, false);
        Graphics->LoadTexture("background_test.png");

        sf::Sprite mSprite;
        mSprite.setTexture(*Graphics->GetTextureList().at(0));

        Graphics->PrintLoadedTextures();
       
        while(Graphics->GetWindow().isOpen())
    {
        sf::Event event;
                while (Graphics->GetWindow().pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
                Graphics->GetWindow().close();
        }

                mSprite.move(1,0);

        Graphics->GetWindow().clear();
        Graphics->GetWindow().draw(mSprite);
        Graphics->GetWindow().display();
    }

        //Clean up singletons.
        Graphics->Release();

    return 0;
}
 

[eXpl0it3r]

Here a few pointers:

• The question you have to ask yourself is: Do I really need such a manager? For simple games it doesn't really make sense to have such a class and things will be way easier by having the resources declared as a class member of the current 'state' or similar...
• Try to avoid global variables which also singletons. Some troubles you can run into can be read here.
• You're aware that all your allocated resource will be leaking, since you never delete them?
• Don't use RAW pointers & manual memory management when you got great things like references, smart pointers or RAII. For more information about smart pointers and RAII take a look at this thread.
• You're mixing naming conventions (release(), InitWindow), make sure you're using the same convention for the whole class.
• Why do you bother with const char* when you got awesome things like std::string? Use STL functionalities over C-like code.
• Do not use #define to 'override' some names, that's a very very bad coding style and can cause you a lot of trouble and even more for any person that would have to work with your code.
• I personally prefer to use the .hpp file extension over the .h one, mostly because it makes it clearer what C++ code is and what C code is and it also matches nicely the source file extensions (.c vs .cpp)
• Don't use C-casts (e.g. (char)filename) but use C++ casts like static_cast<char>(filename) but overall try to avoid casts.
• Keep in mind that #pragma once isn't from the standard and might cause problems for some compilers.

I hope, I could show you some flaws in your code and even more in your code design.

[Roose Bolton of the Dreadfort]

I have taken your comments on-board exploiter and have updated the code Love reading your replies.

I want to make a manager like this because I like the tidiness and I have been taught to black-box stuff.

I haven't looked at smart pointers or RAII before, I will take a look!

everything else you said I have changed