Is this bad singleton design?

[KeyForge]

I'm writing a simple 3D Game engine with SFML and OpenGL.

I have managers like ResourceManager, StateManager, etc, and all of them are Singletons so I can access them between classes when I need to, like when I need to get a texture at ClassA and ClassB without making duplicates, or when I need to change States on a certain State after a GUI event, etc, etc... They also auto manage memory.

Would this be considered bad design or 'not thread safe'? If so, what is good design for doing what I want to do like what I listed above? The only times I think I'll need threading is for networking and loading resources.

[jsmith]

You don't get thread safety for free. What are you doing to ensure thread safety?

[KeyForge]

SFML Mutex locking was something I was thinking about, but I haven't implemented anything yet. The reason I ask though is because everywhere I've read people are saying Singletons are evil and not thread safe.

[Disch]

Singletons should only be used when it would logically make sense that there'd only be one of something.

For a resource manager, it makes sense because you wouldn't want to have the same resource loaded more than once regardless of what is going on elsewhere in the program.

For a state manager, not so much. It's conceivable to have multiple states going on simultaneously, in which case a Singleton would get in your way.

[KeyForge]

The way my state manager is setup is so one state runs and all the others are halted until they are called which therefor halts the previous state, so I think a Singleton is appropriate for it. Thanks Disch for the answer.

[Disch]

so I think a Singleton is appropriate for it

I don't agree. But maybe I'm misunderstanding what you mean by "state".

[KeyForge]

What I mean is that states are the states of the game such as if you're on the menu screen, in the game, options screen, etc. You should only run one of those at a time. The state manager basically allows you to change the current state and handles updating, getting events, and rendering the state.

[Disch]

Right... that's what I thought. I'd argue that's a poor use of a Singleton.

I would opt for something more like this:

class GameState
{
public:
  virtual ~GameState() { }
  virtual GameState* Run() = 0;

  static void Launch(GameState* state)
  {
    while(state)
    {
      GameState* nextstate = state->Run();
      delete state;
      state = nextstate;
    }
};

The idea is you derive different states from GameState, such as GameState_Intro, GameState_OpeningMenu, GameState_MainGame and the like, each with their own Run() function that controls the gameflow.

Flow between states would be handed off in one of two ways:

- return a new GameState from Run to "hand off" flow to the new state. (example: the intro screen would hand off to the opening menu)

- Start a new tier of states by calling GameState::Launch with a new GameState, with the idea that control will return to this state (like entering some kind of status screen or config menu from the main game, for example. You'd want to return to the main game when that menu exits)

Run would return 0 to exit the state and return flow to the previous state in the tier.