19 replies to this topic

[Moehammered]

Now today whilst riding the train I started reading up on singletons (I had been taught about it from my college teacher a year ago) and started digging into the theory behind the design pattern.
Apparently singleton is a bad approach, and a lot of the discussions I've read had been saying stuff like 'it's bad for unit testing' or stuff like (BTW what exactly is meant by unit testing in relation to Game Development?).

Now the thing is I have a situation where I can't find any other pattern or implementation method where singleton wouldn't be the best approach. Currently I'm making a state based game engine to help with transitioning between menu and gameplay and anything else which can be considered it's own 'game state' as to only have ONE instance of a gamestate running at one time. So e.g. hit play, go to gameplay, delete the previous state from memory(to keep it from leaking or anything tragic otherwise) and than continue gameplay, hit exit in game, return to menu, delete game play from memory. So rinse repeat this. The way I do it is I have a 'state' which any class inherits to maintain base functionality. Than if I want to switch, all I do in the game is call StateSystem::switchStates(new Menu()); and it will go to a menu. Is this bad and if so how have others managed to do the same? From switching to game to menu and not having to change things inside the main while loop?
So enough of my rant here's the code

main.c

Display *display= NULL;
GL_Controller *myGLController = NULL;
StateSystem *stateSystem = NULL;
State *tempState = NULL;

void setupDisplay()
{
	display = Display::createInstance();
	display->initialise(640, 480, 24);
	display->createPerspectiveProjection(60, 1, 5.0f, 500);
}

void setupGLController()
{
	myGLController = GL_Controller::createInstance();
	myGLController->initialiseGLController();

	printf("OpenGL Version: %f\n", myGLController->returnGLVersion());

	if(myGLController->checkVBOOn())
    	printf("Vertex Buffer Objects Available: True\n");
	else
    	printf("Vertex Buffer Objects Available: False\n");

	if(myGLController->returnGLVersion() < 1.5f)
    	glClearColor(0, 0, 1, 0);
}

int main(int argc, char* args[])
{
	float rot = 0;
	setupDisplay();

	setupGLController();

	SDL_Event event;

	Timer *timer = new Timer();
	stateSystem = StateSystem::create();

	bool switched = false;
	stateSystem->switchStates(new FileReadTest());

	glEnable(GL_CULL_FACE);
	glEnableClientState(GL_VERTEX_ARRAY);
	glEnableClientState(GL_COLOR_ARRAY);

	while(1)
	{
    	timer->processTime();
    	timer->processFrameRate();

      	if((SDL_PollEvent(&event) && event.key.keysym.sym == SDLK_ESCAPE))
        	break;

    	rot += 0.1f;

    	glTranslatef(-25,0,-80);

    	stateSystem->updateState();
    	display->updateDisplay();
	}

	glDisableClientState(GL_COLOR_ARRAY);
	glDisableClientState(GL_VERTEX_ARRAY);
	timer->saveHighestFrameRate();

	return 1;
}

StateSystem .h

#include "State.h"

class StateSystem
{
	public:
    	StateSystem();
    	virtual ~StateSystem();

    	void initialise();
    	void updateState();
    	void shutdownState();

    	void switchStates(State*);

    	static StateSystem* create();

	protected:
	private:
    	static StateSystem* instance;
    	State *current;
    	bool updatePrint;
};

StateSystem.cpp

#include "StateSystem.h"
#include "stdio.h"

StateSystem* StateSystem::instance = 0;

StateSystem::StateSystem()
{
	current = 0;
	updatePrint = false;
}

void StateSystem::initialise()
{

}

void StateSystem::switchStates(State* newState)
{
	if(current != 0)
	{
    	delete current;
    	current = 0;
	}

	current = newState;
	current->initialise();
}

void StateSystem::updateState()
{
	current->update();
}

StateSystem* StateSystem::create()
{
	if(instance==0)
	{
    	instance = new StateSystem();
	}

	return instance;
}

StateSystem::~StateSystem()
{
	//dtor
}

State.h

class State
{
	public:
    	State();
    	virtual ~State();

    	virtual void initialise()=0;

    	virtual void update()=0;

	protected:
	SDL_Event event;
	private:
};

[Cornstalks]

Now the thing is I have a situation where I can't find any other pattern or implementation method where singleton wouldn't be the best approach.

Just using an actual global variable? 'Cause that's what a Singleton is, just wrapped up in a fancy package. You might as well just make a global variable for the game state, and when the state changes just reassign it. You can't have two instantiations* of the global variable, so it's just as "single" as the singleton, plus it's less ambiguous in my eyes, as it doesn't have "pretty" cruft mixed with it.

Or you could use a stack of states, and the one on top is the current state, and then just pop from the top of the stack to go back, or push onto the stack to go forward.

Or you could have a switch statement that reads a single enum for the game state (i.e. E_MAIN_MENU, E_IN_GAME, E_PAUSED) and just switches the logic based on the game state.

There's lots of options. I have yet to see an instance where a singleton is the best (I don't want to say that it's always the wrong choice, as such blanket statements are dangerous, however, I really haven't ever seen an instance where a singleton is the best solution).


*How the heck is that word spelled? It's a word, right?

[Moehammered]

Instantiation is a word and I think you did spell it right

As for what you've said I actually hadn't thought of using the enum approach, (which I have actually done before >.<)

but than the enum state would have to have a global variable and inside the switch all states would have to be pre-defined, would they not?

the global variable for 'state' is actually a good idea now that I think about it, but if for example I went and had 'Menu' and it's pointer is the same as the global variables pointer 'currentState' and I call delete currentState, wouldn't I then be unable to continue past that line to re-instantiate the currentState into say 'Game' ?

void Menu::switchState(State*)
{
 	delete currentState;
 	//This line won't fire off due to Menu's pointer referencing a NULL address?
}

[ApochPiQ]

the global variable for 'state' is actually a good idea now that I think about it, but if for example I went and had 'Menu' and it's pointer is the same as the global variables pointer 'currentState' and I call delete currentState, wouldn't I then be unable to continue past that line to re-instantiate the currentState into say 'Game' ?

I'm not sure what you're asking here, so I'll hit both possibilities :-)

• Your Menu object's pointer will never be the same as the pointer to some totally different object in your program. We'd all be in for a disaster if that were possible!
• If your currentState object is pointing to a Menu object and you delete it while still using that Menu object, then yes, you have a bug. This is known as a dangling reference.

Of course, in the latter case, the question arises: why is your menu of all things deleting game states?

[Moehammered]

the global variable for 'state' is actually a good idea now that I think about it, but if for example I went and had 'Menu' and it's pointer is the same as the global variables pointer 'currentState' and I call delete currentState, wouldn't I then be unable to continue past that line to re-instantiate the currentState into say 'Game' ?

I'm not sure what you're asking here, so I'll hit both possibilities :-)

• Your Menu object's pointer will never be the same as the pointer to some totally different object in your program. We'd all be in for a disaster if that were possible!
• If your currentState object is pointing to a Menu object and you delete it while still using that Menu object, then yes, you have a bug. This is known as a dangling reference.

Of course, in the latter case, the question arises: why is your menu of all things deleting game states?

State *currentState = new Menu();

void Menu::update()
{
	if(Play button is pressed)
	{
    	currentState = new Game(); // This would leave garbage behind as the pointer for currentState has changed has it not?
    	/*OR*/
    	delete currentState;
    	currentState = new Game(); 
//But now since this is a function call inside the Menu class, and the currentState has been deleted, 
//than the menu would not be able to run this piece of code because update is a function inside the menu class.
	}
}

State *currentState

So than let me clarify, currentState would be a global variable which would constantly be used to update whatever is actually instantiated be it a menu, game, cinematic, etc etc.
Menu contains the code to switch the currentState's pointer reference to a new object. The singleton I have does this but acts as a mediator interface to make sure that the instance variable won't become null
and that no matter which type of state is calling it, it will initialise correctly. I do apologise if I'm unclear but I really want to see if there is another convinient and non-bad-habit developing(for lack of a word) way of doing this.

[ApochPiQ]

Make currentState a smart pointer instead of a raw pointer. Then it'll take care of the replacement semantics for you.

[jameszhao00]

While the original intention of a "singleton" is to guarantee 'only 1 instance', singletons are most often used like global variables (bad).
Best to avoid them

Looking at your code, it seems like you're making it too complex for your use case. It's perfectly fine to not use classes.

bool menu_loop(Renderer a, K b, L c, ...) { }
bool game_loop() { }

Renderer renderer;
while(!quit)
{
 	while(menu_loop(renderer, a, b, ...)) { }
 	while(game_loop()) { }
}

For any complex state stuff, check out coroutines (Lua has a good implementation). Coroutines minimize boilerplate execute(), entry/exit functions, having to keep track of stuff across execute() calls.

[Moehammered]

Smart pointer seems like the thing I should do than.

But just for discussion sake, I seem to find singleton convenient, is there anything wrong with this? What are the pitfalls I can expect and if so would they be very bad to the point they would cause negligence in my code? Of course I don't like to overuse them because well... if anyone's used Ogre3D IT IS HELL TO USE THEIR SINGLETON DESIGN!! :/

I do however like it because I can create something I need everywhere with functionality attached to it. I'm a bit put down cause I feel I'm somewhat making myself a bad programmer by using this pattern now...

[Moehammered]

While the original intention of a "singleton" is to guarantee 'only 1 instance', singletons are most often used like global variables (bad).
Best to avoid them

Looking at your code, it seems like you're making it too complex for your use case. It's perfectly fine to not use classes.

For any complex state stuff, check out coroutines (Lua has a good implementation). Coroutines minimize boilerplate execute(), entry/exit functions, having to keep track of stuff across execute() calls.

Hmm never heard of coroutines before, sounds very interesting

But my use case is I'd like only one game element ever in memory and running. Doesn't a singleton fulfill this?

[Hodgman]

But just for discussion sake, I seem to find singleton convenient, is there anything wrong with this?

The first 4 lines of code that you posted create 4 global variables:

//main.c
Display *display= NULL;
GL_Controller *myGLController = NULL;
StateSystem *stateSystem = NULL;
State *tempState = NULL;

The problem with singletons is that they are global variables and globals are evil.
...but seeing as you're very comfortable writing code that's full of globals, then you've got no need to worry about using singletons as well!

Globals/singletons are bearable on small projects. On large projects or reusable software, they make it impossible to reason about the code-base. At work (as an engine programmer), I would be dragged into an office and be given a stern talking to if I used these kinds of patterns, because of all the pain and suffering they cause.

[ApochPiQ]

I do however like it because I can create something I need everywhere with functionality attached to it.

This is precisely why singletons are bad.

If you get in the habit of "oh, hey, I'll just snag a little of this and throw it over there," you're screwing yourself in the long term. Your code will, as it increases in size, become difficult to reason about. It will be hard to maintain. And you will get weird bugs that are extremely frustrating, because there will be a tangled mess of intertwined responsibilities spread across half your code base.

There are fabulously few things you "need everywhere" in any program. Most of them do not deserve to be objects, let alone singletons; and the few that do need to be objects rarely need to be singletons anyways because engineering your code to assume there is only "one of these" makes it needlessly brittle. Someday you'll find a reason to need two, and then you're screwed.

Don't think of this so much as "doing X will lead to badness" but more of "not doing X will enable more goodness." Not using singletons will allow you to design code that is loosely coupled - things that shouldn't be related won't be. This is great because it means localization of concerns. You no longer have to look at your physics code to figure out why the high scores are showing up wrong. You no longer have to worry about things like your menus accidentally deleting themselves.


The key to good software design is minimalism. One class should have one and only one responsibility, for instance. One concept should be mapped to as few implementation details as possible. One implementation detail should affect as few high-level concepts as possible. The more simplistic and contained your code is, the easier it is to think about and reuse.


Consider two alternatives.

In the first world, you manage game states using a singleton. Every time you want to update the current game state, you ask the singleton to do it for you. Game state management is spread across most of your code.

In the second world, you manage game states in a centralized location. No implementation of any one game state says where to go next, or how to do it; instead, you contain the entire state machine to a single function in a single file.


Maybe you find the first method simpler now, but that's just because you've dug yourself a rut and need to pop out of it. The second method is probably actually easier to write.

But now you have to add a new game state, or maybe make the high score screen pop up after you die but before you go back to the main menu.

Which code is going to be easier to work on?

[Moehammered]

But just for discussion sake, I seem to find singleton convenient, is there anything wrong with this?

The first 4 lines of code that you posted create 4 global variables:[code=auto:0]
Globals/singletons are ok on small projects. On large projects or reusable software, they make it impossible to reason about the code-base. At work (as an engine programmer), I would be dragged into an office and be given a stern talking to if I used these kinds of patterns, because of all the pain and suffering they cause.

Thank you for that, made me feel less awful

But than 'from your experience' what would you do? Psuedo code is fine I can understand it well or even logic will give me perfect insight

[Moehammered]

ApochPiQ thank you for generalizing it for me. I see how you've explained it and I myself understand singleton is the exact opposite of OOP so I see now that I have been very 'lazy' to look for a more effective solution. And now that I think about it I can see a use case where my code would fail. If I were to have 'split screen' gameplay.... this would fail. So yes I want to re-design it tonight. Thanks for the help, any implementation suggestions?

(Side Note: Anyone know where I can go to get info on how to integrate scripting into an engine? like say write a C++ game engine, and have it accept Ruby script for example)

[L. Spiro]

I see no reason to use globals at all, and in fact a lot of the system wouldn’t even work as globals.

Let’s say you have a CStateBase class. I guess this is your CState class.
The CStateManager maintains the current game state, which must be derived from CStateBase.

The engine has no idea what classes you are going to create over CStateBase when it is time to actually make a game on top of your engine.
And of course it is not acceptable to modify the engine for each new game, adding the new states needed for that game etc.

So you need a CStateFactory.
The engine can provide a small set of default states (mine has 2: One prints a message saying the engine is running and the other causes the engine to shut down).

The class has only one function: GetState(). The function accepts an integer that identifies the state you want to have created and returned.
You can just make an enumeration for all of the states in your game and inside CMyGameStateFactory::GetState() you just use a switch case to create the proper state class given the enumerated value.

Thus if your state factory is global you can’t override it and you can’t make custom states.


And what brings these functions together?
The CGame class is intended to bring together the major sub-systems. It holds a pointer to a CStateMachine and a CStateFactory, and a pointer to the CGame class is passed to the Initialize(), Tick(), Draw(), and Destroy() functions in CStateBase, so states have access to any of the major systems they need as well as the game’s data.

When you create a game you make your own game class (CMyGame : public CGame) and set its state factory.

mgMyGame.SetStateFactory( new CMyStateFactory() );  // It doesn't leak.

With this system in place, you can go from any state to any other just by calling CGame::SetNextState(). CGame will go to that state on the next cycle (this is important, because the only thing that can change to a new state is the current state (it is the only thing that can do anything in your game) and if you delete it, control will return to its Tick() function but its this pointer will be invalid), asking for the new state pointer from your custom state factory and handing it off to the state machine


This describes a simple way to organize your game into modular states and has no logical use of globals of any kind.
Even the CMyGame class would just be created on the stack.


L. Spiro

[Tom KQT]

But just for discussion sake, I seem to find singleton convenient, is there anything wrong with this? What are the pitfalls I can expect and if so would they be very bad to the point they would cause negligence in my code? Of course I don't like to overuse them because well... if anyone's used Ogre3D IT IS HELL TO USE THEIR SINGLETON DESIGN!! :/

I do however like it because I can create something I need everywhere with functionality attached to it. I'm a bit put down cause I feel I'm somewhat making myself a bad programmer by using this pattern now...

I think the biggest problem with singletons is when people use them too much, with no real reason, without really thinking about it, and even when they are just starting with OOP. Some people simply think using singletons is cool.
You are neither of this, because otherwise you wouldn't have created this thread.

My opinion - if it's your own project, if you are not in a team with other programmers who don't like singletons, you can use them if you want. IMHO, if you know very well what you are doing, you can do it. And knowing what you are doing includes also knowing other ways how to do it (without singletons) and being able to implement them.
If you prefer a global variable for something what you need access to in a lot of modules, then why not? And if you like a singleton more than a "simple" global variable, then why not?

[Zahlman]

Of course, in the latter case, the question arises: why is your menu of all things deleting game states?

State *currentState = new Menu();

void Menu::update()
{
	if(Play button is pressed)
	{
    	currentState = new Game(); // This would leave garbage behind as the pointer for currentState has changed has it not?
    	/*OR*/
    	delete currentState;
    	currentState = new Game(); 
//But now since this is a function call inside the Menu class, and the currentState has been deleted, 
//than the menu would not be able to run this piece of code because update is a function inside the menu class.
	}
}

Ugh. Please, no. That confuses (fails to properly segregate) responsibility.

See how it looks when you separate concerns - the state determines what comes next, and the code that logically contains the "current state" value actually manages that value:

State* Menu::update() {
	if (Play button is pressed) {
		return new Game();
	}
	return this;
}

// And then for example you could do something like:
int main() {
	State* current = new Menu();
	while (current) {
		State* next = current->update();
		if (next != current) { delete current; }
		current = next;
	}
}

Notice how you no longer need a global variable at all. Also you get a way to signal program exit for free: just return null.

You could also instead use some other policy for managing the states: it might make sense for your application to cache game states, or re-use them, or store them in a stack, or whatever. The important thing is to make sure each instance is deleted exactly once. The easiest way to handle this is to not try to make the states themselves responsible for it.

But please don't use raw pointers for this; use a smart pointer (e.g. boost::shared_ptr).

[yckx]

Also, what happens if you want a Paused state? You certainly don't want to delete the old state, else you would have no game to resume. This is one reason I like a stack to manage my game state.

[ApochPiQ]

Also, what happens if you want a Paused state? You certainly don't want to delete the old state, else you would have no game to resume. This is one reason I like a stack to manage my game state.

A stack is certainly one way of doing it; I personally lean towards confining all state transition logic to a single function or object someplace, as I outlined above, but that's just me ;-)

[Zahlman]

IMX a stack is overkill. A Paused state can just hold a pointer to the state that's been paused (and if you're using reference-counted smart pointers, or a GC language, then that keeps the instance around).

[L. Spiro]

I agree that a stack is overkill when all you want to do is pause the game. I do allow a stack of states, but it is for system states such as when a Nintendo Wii pops up some kind of warning message that interrupts your game, such as having the disk taken out.
To me this is a more sensible use of a stack of game states.

Pausing is done by simply not advancing the virtual time accumulators. Any object that uses the virtual time instead of the actual time will simply stop moving. That simple.
This way menus and graphical effects etc., can continue bouncing and flashing while the game is paused while the 3D game scene has stopped. If you want to show a message or darken the screen while paused, simply check the CGame::Paused() function and do so.

When a state is pushed over the current state via a notification from the console, all bottom states are inherently paused (a virtual function on the CStateBase class can be overloaded to prevent a state from being paused, which may be necessary for online games).


L. Spiro