Double loop for input handling

Question

I'm writing a simple game with multiplayer mode in C++, using the SFML framework. Since I don't want to make it more complicated than necessary, I handle everything on a single thread, so that when the client is waiting for the remote player's turn, I check if there is an incoming message about the remote player's move. I do this in the following way, inside OnlineGame class.

void OnlineGame::PlayGame(int& scoreOfPlayer1, int& scoreOfPlayer2) {
    socket.setBlocking(false);

    WaitForStartSignal();

    map.Show(gameWindow);
    currentPlayer->YourTurn();

    while(!gameEnd) {
        RunOneGameCycle();
    }

    SetPlayersScores(scoreOfPlayer1,scoreOfPlayer2);
}


void OnlineGame::RunOneGameCycle(){
    gameWindow.GetInput();
    GetRemoteMove();
    if(turnEnd){
        currentPlayer->YourTurn(); //the notification can only take place after the previous move was entirely completed,
        //so that the current move will only be handled by one player, who is finishing their turn
        turnEnd = false;
    }
}

I wanted to refactor the GetInput method of GameWindow class, this method is the one responsible for getting input until some specific event happens, at which point the method should return.

void GameWindow::GetInput() {
    Event event{};

    //clearing event queue
    while (pollEvent(event));

    while (true) {
        while (pollEvent(event)) {
            if (active && event.type == Event::KeyPressed){
                switch (event.key.code) {
                    case Keyboard::Up:
                        NotifyOnDirectionSelected(up);
                        return;
                    case Keyboard::Down:
                        NotifyOnDirectionSelected(down);
                        return;
                    case Keyboard::Right:
                        NotifyOnDirectionSelected(right);
                        return;
                    case Keyboard::Left:
                        NotifyOnDirectionSelected(left);
                        return;
                    case Keyboard::Escape:
                        NotifyOnExit();
                        return;
                    case Keyboard::Return:
                        NotifyOnConfirmation(true);
                        return;
                    default:
                        NotifyOnConfirmation(false);
                        return;
                }
            }
            else if (event.type == Event::Closed) {
                NotifyOnExit();
                close();
                return;
            } else if (!active)
                return;
        }
    }
}

It was obviously too long so I wanted to separate everything inside of the second while loop in a different method. Thus I refactored it in the following way:

void GameWindow::GetInput() {
    ClearEventQueue();

    Event event{};
    inputEnd = false;
    while (!inputEnd) {
        while (!inputEnd && pollEvent(event)) {
           HandleEvent(event);
        }
    }
}

void GameWindow::ClearEventQueue() {
    Event event{};
    while (pollEvent(event));
}

void GameWindow::HandleEvent(const Event& event) {
    if(active && event.type == Event::KeyPressed){
        HandleKeyPress(event.key.code);
        inputEnd = true;
    } else if(event.type == Event::Closed){
        NotifyOnExit();
        close();
        inputEnd = true;
    } else if (!active){
        inputEnd = true;
    }
}

void GameWindow::HandleKeyPress(const Keyboard::Key& key) {
    switch (key) {
        case Keyboard::Up:
            NotifyOnDirectionSelected(up);
            break;
        case Keyboard::Down:
            NotifyOnDirectionSelected(down);
            break;
        case Keyboard::Right:
            NotifyOnDirectionSelected(right);
            break;
        case Keyboard::Left:
            NotifyOnDirectionSelected(left);
            break;
        case Keyboard::Escape:
            NotifyOnExit();
            break;
        case Keyboard::Return:
            NotifyOnConfirmation(true);
            break;
        default:
            NotifyOnConfirmation(false);
            break;
    }
}

inputEnd is a member variable, and it is used to substitute the return statements in the previous solution. active denotes whether the window should wait for the local player's interaction (it is true when it's the local player's turn). I think that this second version looks much more cleaner and is much more readable than the first one, but I still find the double while loop with a partly same (similar looking) condition somewhat strange.

The declarations of these classes (with all the relevant methods):

GameWindow.h

using sf::RenderWindow;
using sf::Keyboard;
using sf::Event;

class GameWindow : public RenderWindow {
public:
    void GetInput();
    /**
     * @brief Notifies the subscribed observers after the window containing the map has been closed.
     */
    void NotifyOnExit() const;
    /**
     * @brief Notifies the subscribed observers after the user has selected a direction.
     */
    void NotifyOnDirectionSelected(Direction) const;
    /**
     * @brief Notifies the subscribed observers whether the user has confirmed the action at hand.
     */
    void NotifyOnConfirmation(bool) const;

private:
    bool active; ///< Should the window create events?
    bool inputEnd;

    void ClearEventQueue();
    void HandleKeyPress(const Keyboard::Key&);
    void HandleEvent(const Event&);
};

OnlineGame.h

class OnlineGame : public Game {
public:
    void PlayGame(int& scoreOfPlayer1, int& scoreOfPlayer2) override;

private:

    /**
     * @brief Waits for the remote player's move and notifies the Player instance that
     * represents the remote player.
     */
    void GetRemoteMove();
    void WaitForStartSignal();
    void RunOneGameCycle();
    void SetPlayersScores(int& scoreOfPlayer1, int& scoreOfPlayer2);
};

Is there a way to make it even cleaner, and readable while retaining the same functionality? (it's important to note that the second while should stop right after some specific events happen, that's why I had to check the inputEnd there as well) Also, did I otherwise do a good job of refactoring the function, is there any modification I should make here?

Answer 1

Logic separation:

At the moment, the GameWindow class appears to be responsible for event-handling, but also has parts of the game logic inside it, specifically:

• The active variable.
• The inputEnd variable.
• Clearing the event queue.
• Closing the window.

Removing all these makes the code much cleaner, and keeps the game logic together. Clearing the event queue can be done by the calling code, as can the outer while loop. The things subscribed to the Notify*() functions can simply ignore the input if it's inappropriate (set a boolean flag when the first input is received, or unsubscribe).

void OnlineGame::RunOneGameCycle() {

    gameWindow.ClearEventQueue();

    while (!receivedInput)
        gameWindow.GetInput();

    GetRemoteMove();

    // ...
}

void OnlineGame::OnDirectionSelected(direction) { // subscribes to NotifyOnDirectionSelected()

    if (!receivedInput) {
        // do player move...

        receivedInput = true;
    }
}

void OnlineGame::OnExit() { // subscribes to NotifyOnExit()
    gameEnd = true;
    receivedInput = true; // if desired...
    // close the window after the game loop ends
}

void GameWindow::GetInput() {

    Event event{};

    while (pollEvent(event)) {
        if (event.type == Event::KeyPressed){
            switch (event.key.code) {
                case Keyboard::Up:
                    NotifyOnDirectionSelected(up);
                case Keyboard::Down:
                    NotifyOnDirectionSelected(down);
                case Keyboard::Right:
                    NotifyOnDirectionSelected(right);
                case Keyboard::Left:
                    NotifyOnDirectionSelected(left);
                case Keyboard::Escape:
                    NotifyOnExit();
                case Keyboard::Return:
                    NotifyOnConfirmation(true);
                default:
                    NotifyOnConfirmation(false);
            }
        }
        else if (event.type == Event::Closed) {
            NotifyOnExit();
        }
    }
}

The various notifications are also based on game specifics, and could be made more generic, e.g. SubscribeToKeyPress(keyCode, callback);.

---

Frames and Responsiveness:

Most games have the event loop running constantly (i.e. every frame) to ensure that the application stays responsive. SFML, SDL, etc. are built with this usage pattern in mind. If the local player wants to close the window, they should be allowed to do so at any time.

If GetRemoteMove() blocks while waiting for the opponent's move (contains a while loop) and the local player clicks the close button, their OS may claim the application is not responsive, since it's not checking for events.

A close event received during an opponent's turn should probably be processed and not discarded, even if we want to ignore keyboard input during that time. Clearing the events at the start of a turn prevents that from happening.

---

Main Loop and GameStates:

At the moment, you effectively have several main game loops, instead of one. i.e.:

while (true) // WaitForStartSignal() // a loop!
    // wait for start signal

while (!gameEnd) // the main loop! but turn-based
{
    // clear events

    while (true) // waiting for correct player input for this turn // another main loop!
        // poll events

    while (true) // GetRemoteMove()... // another loop!
        // wait for opponents move

    // do next turn if both players have moved
}

// I'm assuming the content of WaitForStartSignal() and GetRemoteMove() here... if this is wrong please let me know.

Each iteration of one of those inner loops is effectively one "frame". In order to handle input properly (including window close events), we would need to be polling for events inside all of those inner loops.

The idealised frame-based main loop looks something like this:

while (!gameEnd) {
    PollInput();
    UpdateGameState();
    Render();
}

UpdateGameState() would involve checking a state enum (GameState::WaitingForStart, GameState::PlayerTurn etc.) or the use of a GameState class hierarchy.