Handling game states for an online RPG game

Question

I'm writing an ORPG. The code below is client-side.

I have a main thread looping in Game::Run:

Game::~Game()
{
    PopAllStates();
}

void Game::Run()
{
    sf::Event Event;

    while(Window->isOpen())
    {
        if(LoadQueueMutex.try_lock())
        {
            while(!LoadQueue.empty())
            {
                LoadQueue.front().first->Load(LoadQueue.front().second);
                LoadQueue.pop();
            }
            LoadQueueMutex.unlock();
        }
        while(Window->pollEvent(Event))
        {
            StateStack.top()->HandleEvent(Event);
        }
        Window->clear();
        StateStack.top()->Draw();
        Window->display();
        StateStack.top()->Update();
    }
}

void Game::AddToLoadQueue(Loadable* pLoadable, WorldPacket Argv)
{
    boost::mutex::scoped_lock lock(LoadQueueMutex);
    LoadQueue.push(std::make_pair(pLoadable, Argv));
}

void Game::PushState(GameState* pState)
{
    StateStack.push(pState);
}

void Game::PopState()
{
    if(!StateStack.empty())
    {
        delete StateStack.top();
        StateStack.pop();
    }
}

void Game::PopAllStates()
{
    while(!StateStack.empty())
    {
        PopState();
    }
}

And another thread waiting for a packet:

void WorldSession::Start()
{
    Packet = WorldPacket((uint16)MSG_NULL);

    boost::asio::async_read(Socket,
        boost::asio::buffer(Packet.GetDataWithHeader(), WorldPacket::HEADER_SIZE),
        boost::bind(&WorldSession::HandleHeader, this, boost::asio::placeholders::error));
}

When the packet "arrives", this is usually done because OpenGL breaks if loaded from another thread:

void WorldSession::HandleSomeCoolOpcode()
{
    //...
    sGame->AddToLoadQueue(pSomethingCool, Packet);
}

When the main thread loads the Loadable*, it will usually mess with the state stack:

void World::Load(WorldPacket Argv)
{
    //...
    sGame->PopState();
    sGame->PushState(this);
}

Is there any way to improve this?

Answer 1

First and biggest point: I really dislike spreading mutex locking throughout the code if it can be avoided. The first (and biggest) thing I'd change would be to create a concurrent_stack that handles the locking and unlocking internally.

template <class T>
class concurrent_stack { 
    std::stack<T> data;
    Boost::mutex guard;
public:
    void push(T const &t) { 
        boost::mutex::scoped_lock lock(guard);
        data.push(t);
    }

    void pop() {
          boost::mutex::scoped_lock lock(guard);
          data.pop();
    }
    // and so on.
};

With this in place, the rest of the code can generally ignore threading issues for that stack, and just push and pop as needed. Obviously there are limits to that, but for the case at hand, it's probably adequate. The result is considerably safer code--it basically eliminates the possibility of forgetting to lock the mutex when you manipulate the stack.

I'd (sort of) disagree with @Jamal on his first point. While I agree with him that you should do something to distinguish type names from variable names, I don't think camelCase is the only (or necessarily best) convention to use. Personally, I prefer what's sometimes called "snake case", which uses underscores to separate words, so (for example) state_stack instead of StateStack. In any case, the basic point is pretty simple: you should use some convention to distinguish types from variables, but camelCase isn't the only convention available.

Answer 2

• Your variables and functions should either be in camelCase or another naming convention that differentiates them from user-defined types (such as Game here), which are capitalized.

• If a parameter is not supposed to be modified, and it's not a primitive type (such as an int), pass it by const-reference in order to avoid an unnecessary copy.

  In both AddToLoadQueue() and PushState(), you're passing a raw pointer to an object that will not be modified within the function. This is also discouraged as you should refrain from passing raw pointers to functions in C++, mostly due to issues such as dangling pointers. If you still need to pass around pointers, pass smart pointers instead (if you have C++11).

  Argv in AddToLoadQueue() should also be passed by const-reference, not by value.

• Prefer not to cast the C-way in C++:

  Packet = WorldPacket((uint16)MSG_NULL);
  

  Cast the C++ way, with static_cast<> in this case:

  Packet = WorldPacket(static_cast<uint16>(MSG_NULL));
  

  See this answer for more information on different types of casts.

• It's not exactly clear how StateStack is supposed to be used here. You use delete on its elements in PopState(), so it's assumed that push() uses new to add new elements, which then means it's a dynamic container. Either way, this implementation should be provided here.

  However, if this is entirely identical to a regular dynamic stack, then just use std::stack. It will also handle all the memory-management for you (no new and delete necessary).

  std::stack<GameState> stateStack;
  

  You'll also no longer need to use a separate top() and pop() when popping a state:

  stateStack.pop();
  

• If PopAllStates() is just done in the destructor, then you can just put that code there instead of defining a separate function.

  It also calls empty() twice (PopState() also calls empty()), which isn't necessary. If this is only needed in the destructor, put that code (with just one check) into it:

  Game::~Game()
  {
      while (!StateStack.empty())
      {
          delete StateStack.top();
          StateStack.pop();
      }
  }
  

  If you use std::stack, you'll just need a call to pop(), not top().