3
\$\begingroup\$

Mainly I am looking for a better way to control the user input in the game loop which is in the first file. Since the nested switch with many case statements are a headache and error-prone.

Also, a way to extract the drawing process out of the Game class. I am not looking for performance, but extendability.

Minor problems are also described in some of the comments.

The quick version of the logic: You get a menu with 3 options (Play, Options, Exit), move with the arrows. The Options allow you to change the size of the window and the board. When you select Play or done with the settings you can LMB to make a cell alive, and RMB to pause/unpause.

Do you have any advice on handling user input and decoupling drawing logic?

// game.hpp
#include <cmath>

#include "board.hpp"
#include "menu.hpp"
#include "settings.hpp"
#include "SFML/Window.hpp"

class Game
{
    Settings settings;
    Board board = Board(&settings);
    Menu menu = Menu(&settings);
    
public:
    void runLoop()
    {
        // Some initial steps before starting the game
        // also sometimes SFML forces you to use floating point numbers, so I have to cast often
        float CELL_WIDTH = settings.WINDOW_WIDTH / static_cast<float>(settings.BOARD_WIDTH);
        float CELL_HEIGHT = settings.WINDOW_HEIGHT / static_cast<float>(settings.BOARD_HEIGHT);
        // flags like these feel like they belong as member variables, but also they are only used in this function so feels like a waste
        bool rightClickPressed = false; // flag indicating weather to accept mouse inputs (left clicks); works like a pause button
        bool playSelected = false;

        // create the window
        sf::RenderWindow window(sf::VideoMode(settings.WINDOW_WIDTH, settings.WINDOW_HEIGHT), "Game of Life");
        window.setVerticalSyncEnabled(true);
        window.clear(sf::Color::Black);

        // draw the menu
        menu.draw(window);
        window.display();

        // The actual game loop
        /*
         * 1. handle user input
         * 2. update game state
         * 3. draw
         */
        while (window.isOpen())
        {
            // check all the window's events that were triggered since the last iteration of the loop
            sf::Event event;
            while (window.pollEvent(event))
            {
                switch (event.type)
                {
                    // window closed
                case sf::Event::Closed:
                    window.close();
                    break;
                case sf::Event::KeyReleased:
                    switch (event.key.code)
                    {
                    case sf::Keyboard::Up:
                        menu.moveUp();
                        window.clear(sf::Color::Black);
                        menu.draw(window);
                        window.display();
                        break;

                    case sf::Keyboard::Down:
                        menu.moveDown();
                        window.clear(sf::Color::Black);
                        menu.draw(window);
                        window.display();
                        break;

                    case sf::Keyboard::Enter:
                        switch (menu.getPressedItem())
                        {
                        case 0:
                            // Play button pressed
                            playSelected = true;
                            window.clear(sf::Color::Black);
                            window.display();
                            break;
                        case 1:
                        {
                            // Option button pressed -> waiting for new window size
                            window.clear(sf::Color::Black);
                            window.display();
                            int resolutionWidth;
                            int resolutionHeight;
                            std::cin >> resolutionWidth >> resolutionHeight;
                            int numCellsRow;
                            int numCellsCol;
                            std::cin >> numCellsRow >> numCellsCol;
                            if (numCellsRow < resolutionWidth && numCellsCol < resolutionHeight)
                            {
                                settings.WINDOW_HEIGHT = resolutionHeight;
                                settings.WINDOW_WIDTH = resolutionWidth;
                                settings.BOARD_HEIGHT = numCellsCol;
                                settings.BOARD_WIDTH = numCellsRow;
                                CELL_WIDTH = settings.WINDOW_WIDTH / static_cast<float>(settings.BOARD_WIDTH); // recalculating. narrowing conversion from float -> int
                                CELL_HEIGHT = settings.WINDOW_HEIGHT / static_cast<float>(settings.BOARD_HEIGHT);
                            }

                            window.setSize(sf::Vector2u(settings.WINDOW_WIDTH, settings.WINDOW_HEIGHT));
                            window.display();

                            board = Board(&settings);

                            window.clear(sf::Color::Black);
                            playSelected = true;
                            // with this library you have to reset this view object, when you change the size of the window
                            sf::View view;
                            sf::Vector2<float> position(0, 0);
                            sf::Vector2<float> size(settings.WINDOW_WIDTH, settings.WINDOW_HEIGHT);
                            sf::Rect<float> re(position, size);
                            view.reset(static_cast<const sf::Rect<float> &>(re));
                            window.setView(view);

                            window.display();
                            break;
                        }
                        case 2:
                            // Exit selected
                            window.close();
                            break;

                        default:
                            break;
                        }
                        break;
                    default:
                        break;
                    }

                // mouse pressed
                case sf::Event::MouseButtonPressed:
                {
                    if (playSelected)
                    {
                        if (event.mouseButton.button == sf::Mouse::Right)
                        {
                            rightClickPressed = !rightClickPressed;
                            break;
                        }
                        if (event.mouseButton.button == sf::Mouse::Left && !rightClickPressed)
                        {
                            window.clear(sf::Color::Black);
                            window.display();
                            auto n = static_cast<int>(floor(event.mouseButton.x / CELL_WIDTH));
                            auto m = static_cast<int>(floor(event.mouseButton.y / CELL_HEIGHT));

                            board.arr[m][n] = true; // the change !!!

                            // draw the cells
                            window.clear(sf::Color::Black);

                            for (auto i = 0; i < settings.BOARD_HEIGHT; ++i)
                            {
                                for (auto j = 0; j < settings.BOARD_WIDTH; ++j)
                                {
                                    sf::RectangleShape rectangle(sf::Vector2f(CELL_WIDTH, CELL_HEIGHT));

                                    rectangle.setPosition(sf::Vector2f(CELL_WIDTH * static_cast<float>(j), CELL_HEIGHT * static_cast<float>(i)));
                                    
                                    if (!board.getCellValue(i, j))
                                        rectangle.setFillColor(sf::Color(0, 0, 0)); //else it's white by default
                                    window.draw(rectangle);
                                }
                            }
                            window.display();
                            break;
                        }
                        break;
                    }
                    break;
                }
                default:
                    break;
                }
            }
            // game should be calculating/changing state
            if (rightClickPressed && playSelected)
            {
                board.applyRulesOnce();

                window.clear(sf::Color::Black);

                // draw the cells
                for (auto i = 0; i < settings.BOARD_HEIGHT; ++i)
                    for (auto j = 0; j < settings.BOARD_WIDTH; ++j)
                    {
                        sf::RectangleShape rectangle(sf::Vector2f(CELL_WIDTH, CELL_HEIGHT));
                        rectangle.setPosition(sf::Vector2f(CELL_WIDTH * static_cast<float>(j), CELL_HEIGHT * static_cast<float>(i)));

                        if (!board.getCellValue(i, j)) // if is dead set color black (0, 0, 0)
                            rectangle.setFillColor(sf::Color(0, 0, 0)); //else it's white by default
                        window.draw(rectangle);
                    }
                window.display();
                // end the current frame
            }
        }
    }
};

I am alright with the files below, but still here for completeness

// board.hpp
#ifndef BOARD_HPP
#define BOARD_HPP
#include <vector>
#include <iostream>

#include "settings.hpp"

class Board
{
    friend class Game;
    Settings* settings;
    std::vector<std::vector<bool>> arr;

private:
    //  whether the value of the cell should change
    bool checkNeighbours(const int& i, const int& j, const bool& value)
    {
        auto aliveCount = 0;

        for (auto ii = i - 1; ii <= i + 1; ++ii)
        {
            for (auto jj = j - 1; jj <= j + 1; ++jj)
            {
                if (ii == i && jj == j) // don't check the cell itself
                    continue;
                if (isSafe(ii, jj))
                {
                    if (arr[ii][jj])
                        ++aliveCount;   // you can break early if > 3
                }
            }
        }
        if (value && (aliveCount == 2 || aliveCount == 3)) // rule 1
            return false;
        if (!value && (aliveCount == 3))                   // rule 2
            return true;
        return value; // else change if alive or stay if dead
    }
public:
    Board(Settings* settings) : settings(settings), arr(std::vector<std::vector<bool>>(settings->BOARD_HEIGHT, std::vector<bool>(settings->BOARD_WIDTH, false)))
    {}

    bool getCellValue(const int& i, const int& j) const
    {
        return arr.at(i).at(j);
    }

    /*
     *  1. Any live cell with two or three live neighbours survives.
        2. Any dead cell with three live neighbours becomes a live cell.
        3. All other alive cells die in the next generation. Similarly, all other dead cells stay dead
     */
    void applyRulesOnce()
    {
        Board nextState(settings);

        for (auto i = 0; i < settings->BOARD_HEIGHT; ++i)
        {
            for (auto j = 0; j < settings->BOARD_WIDTH; ++j)
            {
                if (checkNeighbours(i, j, arr[i][j]))
                {
                    if (nextState.arr[i][j] == arr[i][j])
                        nextState.arr[i][j] = !nextState.arr[i][j];
                }
                else if (nextState.arr[i][j] != arr[i][j])
                    nextState.arr[i][j] = !nextState.arr[i][j];
            }
        }

        *this = std::move(nextState);
    }

    bool isSafe(const int& i, const int& j)
    {
        return (i >= 0 && i < settings->BOARD_HEIGHT &&
                j >= 0 && j < settings->BOARD_WIDTH);
    }
};
#endif // BOARD_HPP
//menu.hpp
#ifndef MENU_HPP
#include <array>
#include <iostream>

#include "settings.hpp"
#include "SFML/Graphics.hpp"
constexpr int MAX_NUMBER_OF_ITEMS = 3;

class Menu
{
public:
    Menu(Settings* settings)
    {
        if (!font.loadFromFile("../../../fonts/arial.ttf"))
        {
            std::cout << "font error\n";
        }
        data[0].setFont(font);
        data[0].setFillColor(sf::Color::Red);
        data[0].setString("Play");
        data[0].setPosition(sf::Vector2f(settings->WINDOW_WIDTH / 2.F, settings->WINDOW_HEIGHT / (MAX_NUMBER_OF_ITEMS + 1) * 1.F));

        data[1].setFont(font);
        data[1].setFillColor(sf::Color::White);
        data[1].setString("Options");
        data[1].setPosition(sf::Vector2f(settings->WINDOW_WIDTH / 2.F, settings->WINDOW_HEIGHT / (MAX_NUMBER_OF_ITEMS + 1) * 2.F));

        data[2].setFont(font);
        data[2].setFillColor(sf::Color::White);
        data[2].setString("Exit");
        data[2].setPosition(sf::Vector2f(settings->WINDOW_WIDTH / 2.F, settings->WINDOW_HEIGHT / (MAX_NUMBER_OF_ITEMS + 1) * 3.F));

        selectedItemIndex = 0;
    }

    void draw(sf::RenderWindow& window)
    {
        for (auto& el : data)
            window.draw(el);
    }
    void moveUp()
    {
        if (selectedItemIndex - 1 >= 0)
        {
            data[selectedItemIndex].setFillColor(sf::Color::White);
            selectedItemIndex--;
            data[selectedItemIndex].setFillColor(sf::Color::Red);
        }
    }
    void moveDown()
    {
        if (selectedItemIndex + 1 < MAX_NUMBER_OF_ITEMS)
        {
            data[selectedItemIndex].setFillColor(sf::Color::White);
            selectedItemIndex++;
            data[selectedItemIndex].setFillColor(sf::Color::Red);
        }
    }
    int getPressedItem() const { return selectedItemIndex; }

private:
    sf::Font font;
    std::array<sf::Text, MAX_NUMBER_OF_ITEMS> data;
    int selectedItemIndex;
};
#endif // MENU_HPP

A simple Settings class

#ifndef SETTINGS_HPP
#define SETTINGS_HPP
struct Settings
{
    int WINDOW_WIDTH = 800;
    int WINDOW_HEIGHT = 800;

    int BOARD_WIDTH = 10;
    int BOARD_HEIGHT = 10;
};
#endif // SETTINGS_HPP

And this is how I run it.

#include "game.hpp"

int main()
{
    Game game;
    game.runLoop();
    return 0;
}

Update: Here is a link to a repository (the branch - "latest" is this version of the project)

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Hi, it would be great if you could upload this to GitHub and include a Makefile or something so we don't have to guess your build process! Uploading to GitHub and sharing the link would make it much easier to grab the code and test changes on it. \$\endgroup\$ Apr 11, 2022 at 13:54
  • 1
    \$\begingroup\$ @TamoghnaChowdhury i've added a link \$\endgroup\$ Apr 12, 2022 at 12:19

2 Answers 2

1
\$\begingroup\$

Frames

Simulations or games usually have the concept of a "frame" within their main loop, which is used interchangeably to refer to the process of preparing a single image, and for that image itself. Each frame involves executing the following stages:

while (true)
{
    getInput();
    update();
    render();
}

Both getInput() and update() update the simulation data in preparation for rendering a frame, and there might be some overlap between the two, but the general idea there is to separate "update logic responding to user input" from "update logic that happens every frame, or at a set time".

To be as responsive as possible, we generally want to avoid blocking on input (e.g. waiting for std::cin), and we want to process input events as soon as we get them. For the Game of Life, we probably want to do the update logic at fixed timesteps (i.e. check a timer every frame, and then update when necessary), and do the input processing and rendering every frame.


States

We also want to be able to switch between different modes or "states", in which we do quite different things at each stage of the main loop. So in the code above, we'd have three separate states, each with their own main loop:

void doMainMenu()
{
    while (true)
    {
        // ... do frame
    }
}

void doOptions()
{
    while (true)
    {
        // ... do frame
    }
}

void doGameOfLife()
{
    while (true)
    {
        // ... do frame
    }
}

We then need a way to switch between the states, without nesting or introducing extra complexity. One way of doing this is to write a "state factory" class. Each state returns a "state factory", which is just a function that, when called, begins the next state. This is an indirect way of calling the next state function, so we don't have an ever-increasing stack of function calls.

So our factory class looks like this:

struct StateFactory
{
    std::function<StateFactory(sf::RenderWindow&)> m_function;
};

and our main() function would then do this:

auto factory = StateFactory{ [] (sf::RenderWindow& window) { return doMainMenu(window); } };

while (factory.m_function)
    factory = factory.m_function(window);

So to switch from the main menu state to the game state, we'd return a function that calls the game state:

StateFactory doMainMenu(sf::RenderWindow& window)
{
    auto menu = Menu(window.getSize()); // we can use your `Menu` class as a local variable here.

    while (true)
    {
        // get input
        {
            auto event = sf::Event();

            while (window.pollEvent(event))
            {
                if (event.type == sf::Event::Closed)
                    return { }; // done! return a state factory with no "next state" function
                
                if (event.type == sf::Event::MouseButtonPressed) // or whatever...
                    return { [] (sf::RenderWindow& window) { return doGameOfLife(window); } }; // when mouse is pressed, go to game state
            }
        }
        
        // ... update
        // ... draw
    }
}

StateFactory doGameOfLife(sf::RenderWindow& window)
{
    // ...
}

In this way, we can keep the logic for our different "states" entirely separate, which makes things much simpler.

(We might want to pass the settings between states as well as the window).


Review

So to look at the actual code!

Game:

  • As mentioned, the menu, options, and game logic should be split up.

  • Member variables should also be split up: we don't need the menu to exist when we're running the game, and we don't need the board to exist when we're in the menu.

  • The game logic could be placed in a function doGameOfLife(), with the board as a local variable inside it - we don't really need a class here at all.

  • We could add a timer to run the update logic at fixed intervals, so the simulation is easier to see, e.g.:

     using clock = std::chrono::steady_clock;
    
     auto lastUpdate = clock::now();
     auto const updatePeriod = std::chrono::milliseconds(500);
    
     while (true)
     {
         // ... get input
    
         // update
         {
             auto const now = clock::now();
             if (now - lastUpdate >= updatePeriod)
             {
                 lastUpdate = now;
    
                 // UPDATE STEP!
                 std::cout << "update!\n";
             }
         }
    
         // ... render
     }
    
  • The code is much cleaner if we do the rendering / drawing as a separate phase every frame, instead of mixing it in with the update logic (we could perhaps use a boolean flag to avoid inefficiency and redraw only when necessary).


Board:

  • We should avoid using friend classes. It looks like we need a setCellValue function to go with getCellValue. The normal C++ approach for something like this is to use overloading:

     bool& cell(std::size_t x, std::size_t y) { return arr.at(x).at(y); }
     bool const& cell(std::size_t x, std::size_t y) const { return arr.at(x).at(y); }
    

    But somehow the C++ Standards Comittee still haven't fixed vector<bool>... So we need separate get and set functions:

     void setCell(std::size_t x, std::size_t y, bool value) { arr.at(x).at(y) = value; }
     bool getCell(std::size_t x, std::size_t y) { return arr.at(x).at(y); }
    

    Note the use of the correct index type for std::vector, which is std::size_t. We should also pass small POD types like int or std::size_t by value - passing by reference is just an unnecessary complication (even if it probably gets optimized to the same thing by the compiler). Note that the names x and y convey more meaning than i and j.

  • We generally want to avoid "vectors of vectors" - it adds unnecessary overhead (storing an extra size with each inner vector), and may spread data out in memory, which can make accessing it slower. Instead, we can store a flat vector of (width * height) size:

     std::size_t width, height;
     std::vector<bool> board;
    

    and calculate the indices of the cells using the formula: index = y * width + x inside the setCell and getCell functions.


Menu:

  • We don't need to pass the Settings class in the constructor, just the window size passed in as an sf::Vector2f. Thus we avoid including the settings header.

  • The positioning could definitely be neater. I'm just going to point out that we can get the size of an sf::Text using text.getLocalBounds().width and text.getLocalBounds().height. We can also get a reasonable height for a line of text by doing font.getLineSpacing(text.getCharacterSize()). Either of these could help to lay out the buttons more neatly.


Options:

  • Using std::cin is bad! It makes our app unresponsive to normal input and means we can't render anything.

  • Could provide a simple widget with a plus and minus button to change the size, e.g.: "Simulation size: [-] 20 [+]".

  • This could be part of the menu state, or a similar but separate state.


One other thing: it's a good idea to put all your code into a namespace. :)

\$\endgroup\$
2
+25
\$\begingroup\$

Agreed too much nesting. Here's a couple of simple rules of thumb for you to follow:

  1. if a case is longer than one line plus the break, factor it out into a function,
  2. more generally, a good maximum length for any function is about one screen-size. It means you can look at the whole thing at once and understand it, without scrolling,
  3. function names should be readable, so that when anyone (including you) is reading your case statement, they understand what the case does, just from reading the function name, rather than having to go and look at the contents of the function to understand it and then back to the case statement... and if at some point you then change the code in the function, check the name still makes sense, and update it if not. If you pick really good function names you'll find you rarely need to add comments to your code, but I think you have perhaps already discovered that :-)

I hope that helps.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Good first or second answer. You might want to give examples of the function names that you think are unreadable. When I discuss function length I also discuss the Single Responsibility Principle \$\endgroup\$
    – pacmaninbw
    Apr 15, 2022 at 13:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.