Handling game states for an online RPG game

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?

-
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(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 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().

StackExchange Code Review Q#17588, answer score: 11