4
\$\begingroup\$

This is an attempt at a multithreaded model-view-controller based engine for 2d console games (board games, roguelikes that sort of thing.) The code below will provide a fully working example but is missing a lot of bits as I am specifically interested in a critique of the multithreading and MVC bits though any other advice you may have will be gratefully received. You will need a c++17 capable compiler and the ncurses library to compile it. There are multiple source files:

model.h

#ifndef MODEL_H
#define MODEL_H

#include <functional>
#include <memory>
#include <mutex>
#include <queue>

constexpr inline const int MAXROWS = 10;
constexpr inline const int MAXCOLS = 10;
struct Command;

class Model {
public:
    Model();

    std::function<void()> render;
    std::function<void()> shutdownController;

    bool at(int, int) const;
    void gameloop();
    void move(int, int);
    void receiveCommand(Command*);
    void quit();
private:
    void update();
    std::queue<std::unique_ptr<Command>> commands_;
    std::mutex mutex_;
    std::array<std::array<bool, MAXCOLS>, MAXROWS> board_;
    int row_;
    int col_;
};

#endif

model.cc

#include <algorithm>
#include <chrono>
#include <csignal>
#include <cstdlib>
#include "command.h"
#include "model.h"

constexpr const double TICK = 1E6 / 60.0;

volatile static std::sig_atomic_t endflag = 0;

static void end(int sig) {
    switch (sig) {
        case SIGINT:
        case SIGTERM:
            endflag = 1;
            break;
        case SIGHUP:
            exit(EXIT_FAILURE);
            break;
        default:
            break;
    }
}

Model::Model() : render{}, shutdownController{}, commands_{}, mutex_{},
board_{}, row_{3}, col_{7} {
    board_[row_][col_] = true;
}

bool Model::at(int row, int col) const {
    return board_[row][col];
}

void Model::gameloop() {
    struct sigaction act;
    act.sa_handler = end;
    sigemptyset (&act.sa_mask);
    act.sa_flags = 0;
    sigaction(SIGHUP, &act, NULL);
    sigaction(SIGINT, &act, NULL);
    sigaction(SIGTERM, &act, NULL);

    std::chrono::steady_clock clock;
    auto previous = clock.now();
    double lag = 0.0;

    while (!endflag) {

        auto current = clock.now();
        auto elapsed = current - previous;
        previous = current;
        lag += elapsed.count();

        while (lag >= TICK) {
            lag -= TICK;
            update();
        }

        render();
    }

    shutdownController();
}

void Model::move(int row, int col) {
    board_[row_][col_] = false;
    row_ = std::clamp(row_ + row, 0, MAXROWS - 1);
    col_ = std::clamp(col_ + col, 0, MAXCOLS - 1);
    board_[row_][col_] = true;
}

void Model::receiveCommand(Command* command) {
    std::lock_guard<std::mutex> lock(mutex_);
    commands_.emplace(command);
}

void Model::quit() {
    endflag = 1;
}

void Model::update() {
    std::lock_guard<std::mutex> lock(mutex_);
    while (!commands_.empty()) {
        auto& nextCommand = commands_.front();
        nextCommand->execute(*this);
        delete nextCommand.release();
        commands_.pop();
    }
}

view.h

#ifndef VIEW_H
#define VIEW_H

#include <memory>
#include <ncurses.h>

class View {
public:
    View();
    ~View();
    bool draw(const int, const int, const chtype);
    WINDOW* window() const;

private:
    std::shared_ptr<WINDOW> window_;
};

#endif

view.cc

#include <clocale>
#include "view.h"

struct WindowDeleter {
    void operator()(WINDOW* window) {
        delwin(window);
    }
};

View::View() : window_{nullptr} {
    setlocale(LC_ALL, "");

    initscr();
    cbreak();
    noecho();
    intrflush(stdscr, FALSE);
    keypad(stdscr, TRUE);

    window_.reset(subwin(stdscr, 0, 0, 0, 0), WindowDeleter());
    curs_set(0);
}

View::~View() {
    curs_set(1);
    endwin();
    clear();
}

bool View::draw(const int row, const int col, const chtype ch) {
    const auto& win = window_.get();
    if (wmove(win, row, col) == ERR) {
        return false;
    }

    if (waddch(win, ch) == ERR) {
        return false;
    }

    return true;
}

WINDOW* View::window() const {
    return window_.get();
}

controller.h

#ifndef CONTROLLER_H
#define CONTROLLER_H

#include <functional>
#include <unordered_map>
#include <ncurses.h>
#include "command.h"

class Controller {
public:
    explicit Controller(WINDOW*);
    Controller(const Controller&)=delete;
    Controller(const Controller&&)=delete;
    bool operator=(const Controller&)=delete;
    bool operator=(const Controller&&)=delete;

    std::function<void(Command*)> sendCommand;

    void handleEvents();
    void shutdown();

protected:
    std::unordered_map<chtype, std::function<Command*()>>  keymap_;

private:
    WINDOW* window_;
    bool finished_;
    std::mutex mutex_;
};

#endif

controller.cc

#include "controller.h"

Controller::Controller(WINDOW* window) : sendCommand{}, keymap_{
    { 'h', [](){ return new MoveCommand( 0, -1); }  },
    { 'j', [](){ return new MoveCommand(-1,  0); }  },
    { 'k', [](){ return new MoveCommand( 1,  0); }  },
    { 'l', [](){ return new MoveCommand( 0,  1); }  },
    { 'q', [](){ return new QuitCommand();      }  }
}, window_{window}, finished_{false}, mutex_{} {
    nodelay(window_, TRUE);
}

void Controller::handleEvents() {
    int c;

    while (!finished_) {
        if ((c = wgetch(window_)) != ERR) {
            auto input = keymap_.find(c);
            if (input != keymap_.end()) {
                sendCommand(std::invoke(input->second));
            }
        }
    }
}

void Controller::shutdown() {
    std::lock_guard<std::mutex> lock(mutex_);
    finished_ = true;
}

command.h

#ifndef COMMAND_H
#define COMMAND_H

#include "model.h"

struct Command {
    virtual ~Command() {}
    virtual void execute(Model&)=0;
};


struct MoveCommand : public Command {
    virtual ~MoveCommand() {}

    explicit MoveCommand(int row, int col) : row_{row}, col_{col} {
    }

    void execute(Model& model) override {
        model.move(row_, col_);
    }
private:
    int row_, col_;
};

struct QuitCommand : public Command {
    virtual ~QuitCommand() {}

    void execute(Model& model) override {
        model.quit();
    }
};

#endif

main.cc

#include <cstdlib>
#include <thread>
#include "model.h"
#include "view.h"
#include "controller.h"

int main() {
    Model model;
    View view;
    Controller controller(view.window());

    controller.sendCommand = [&model](Command* command) -> void {
        model.receiveCommand(command);
    };

    model.shutdownController = [&controller]() -> void {
        controller.shutdown();
    };

    model.render = [&view, &model]() -> void {
        for (auto row = 0; row < MAXROWS; ++row) {
            for (auto col = 0; col < MAXCOLS; ++col) {
                view.draw(row, col, model.at(row, col) ? '@' : '-');
            }
        }
    };

    auto controllerThread = std::thread(&Controller::handleEvents, &controller);
    auto modelThread = std::thread(&Model::gameloop, &model);

    modelThread.join();

    controllerThread.join();
 
    return EXIT_SUCCESS;
}

You can compile it like this:

g++ -std=c++17 -Wall -Wextra -Weffc++ -O2 -g -o game main.cc model.cc view.cc controller.cc -lncurses

As I mentioned, the idea is that the game should follow the Model-View-Controller pattern. Each of the big 3 components should be unaware of and independent of the others. I couldn't see a way to make the Controller not have a reference to the window which is a member of the View object. And should the Model really be shutting down the Controller? It seems that should happen in main() but when I tried it I had thread synchronization problems which froze the game.

Can the std::functions and lambdas I have to connect up the classes (like Qt's "signals and slots") be made more generic?

Speaking of threads, I have two (for now) one listens for input and the other runs the game loop, updating the Model 60 times a second and rendering the View as necessary. Am I doing this right?

These are some questions that come to mind but, again, any critique or guidance will be gratefully received.

\$\endgroup\$
0

1 Answer 1

Reset to default
1
\$\begingroup\$

About the MVC pattern

As I mentioned, the idea is that the game should follow the Model-View-Controller pattern. Each of the big 3 components should be unaware of and independent of the others.

The goal of the MVC pattern is not that each of the three components are unaware and independent of each other. Rather, the goal is to separate responsibilities, and to make it easier to replace components with different implementations, without affecting the other components.

So it is fine if the components depend on each other, as long as they depend on a well-defined interface, and not on implementation details. For example, you could have created base classes ModelBase, ViewBase and ControllerBase, which the concrete classes derive from, and pass a pointer to ModelBase to Controller, and have move() and quit() be public virtual member functions of ModelBase.

I couldn't see a way to make the Controller not have a reference to the window which is a member of the View object.

You could create a class Window which represents a curses window. You could pass a reference to that to both the Controller and the View. This removes the responsibility of creating a window from View, the latter is now only responsible for drawing the state of the Model.

Can the std::functions and lambdas I have to connect up the classes (like Qt's "signals and slots") be made more generic?

Probably, but why do you need it to be more generic? In this case, you could even argue that they are already too generic, and they can be made more specific.

About the threads

And should the Model really be shutting down the Controller? It seems that should happen in main() but when I tried it I had thread synchronization problems which froze the game.

If I had to design it I would probably have the Controller be in charge of stopping things, but it's not the only way to do it. You could also start the controller thread after the model thread, and instead of sending a quit command to the model, the controller will just exit handleEvents() when you press the quit key. Then in main(), you could write something like:

auto modelThread = std::thread(&Model::gameloop, &model);
auto controllerThread = std::thread(&Controller::handleEvents, &controller);

controllerThread.join();
model.quit();
modelThread.join();

This way, Model::shutdownController is no longer necessary.

Speaking of threads, I have two (for now) one listens for input and the other runs the game loop, updating the Model 60 times a second and rendering the View as necessary. Am I doing this right?

No. You never sleep, so a lot of CPU cycles are wasted between updates to Model. Use std::this_thread::sleep_for() or std::this_thread::sleep_until() to sleep. Also, you unconditionally call render(), not just when there is an update, again wasting CPU cycles.

Thread safety issues

You should worry more about thread safety. Is your curses library thread safe? You have two threads calling curses functions possiby simulatenously.

Model::endflag is read by one thread and modified by another. You made it volatile, but that is not the same as atomic. std::sig_atomic_t is a type meant for POSIX signal handlers, not for threads. While it most likely will do what you want, it is much better to use a std::atomic_flag, which is guaranteed to do what you want. Also, why is this a global variable? It should be a private member variabel of Model.

Controller::finished_ looks like it serves a similar purpose as Model::endflag, but it is implemented in a completely different way. You do lock mutex_ inside shutdown(), but you don't use the mutex in handleEvents(). That's definitely wrong; if you lock a mutex to guard a variable at one place, you should almost always do that in every other place as well. But again, std::atomic_flag is a much better solution here.

Handing commands

keymap_ is unnecessarily complex. It stores lambdas which in turn create objects that derive from Command, and those in turn have a virtual execute() function which does the actual work. Why not just use std::function<void(Model&)> to store commands?

using Command = std::function<void(Model&)>;
…
std::unordered_map<chtype, Command> keymap_;
…
Controller::Controller(WINDOW* window) : sendCommand{}, keymap_{
    { 'h', [](Model& model){ model.move( 0, -1); }  },
    …
    { 'q', [](Model& model){ model.quit(); }  },
}, …

And then in Model:

std::queue<Command> commands_;
…
void Model::receiveCommand(Command command) {
    std::lock_guard<std::mutex> lock(mutex_);
    commands_.emplace(command);
}

void Model::update() {
    std::lock_guard<std::mutex> lock(mutex_);
    while (!commands_.empty()) {
        commands_.front()(*this);
        commands_.pop();
    }
}

It gives you exactly the same flexibility and decoupling, but now you no longer need the Command class hierarchy, there is less indirection, and if the commands don't have any captures, it avoids heap allocations.

I also wonder why it is necessary to use a queue to pass commands from the Controller to the Model. Why not call the commands directly in the Controller's thread? You can do this if you do proper locking in Model's move(), quit() and gameloop(). This avoids passing Command objects around.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.