Multithreaded console-based monster battle with earliest-deadline-first scheduler

Question

In working on this answer it occurred to me that it might be interesting to further expand the idea.

The game

This is an extremely simple (and boring!) text-based game that creates a few monsters which attack the player and which the player can attack. The goal is for player to defeats all of the monsters before dying. To fight monsters, the player types the generic name of the monster, e.g "Orc" and if there are multiple Orcs, the program will register a hit on each of them. The game is very boring, to play but it was intended as a proof-of-concept rather than a fully fleshed out game. With that said, it is complete and runs without error.

Features

Some features of the game are that it's multi-threaded with the monsters being driven in one thread and the user I/O in another. It uses a std::priority_queue to keep track of the timing of the monster attacks on the player. That is, the monsters act in real time and autonomously, independent of the player.

It can use the C++20 std::osyncstream if available, but has a substitute for C++11 or above which is why it has both tags.

Questions

I'm particularly interested in:

1. Is the Game object design easy to understand?
2. Does the scheduling mechanism make sense?
3. I could have used std::initializer_list<Monster> as the argument instead of std::initializer_list<std::pair<std::string, unsigned>> for the constructor. I chose not to because I didn't want temporary copies of Monsters made. What do you think of that choice?
4. Are there any flaws in the multithreading?
5. The game currently ends via an exit call to kill all threads. Is there a more elegant way to approach this?

Game.h

#ifndef GAME_H
#define GAME_H
#include <atomic>
#include <initializer_list>
#include <memory>
#include <mutex>
#include <string>
#include <vector>


class Game {
    struct Monster {
        static unsigned serial;
        Monster(std::string name, unsigned interval);
        Monster(const Monster &other) = delete;
        std::string name;
        unsigned interval;
        unsigned deadline;
        int health = 4;
        unsigned id;
    };

    std::vector<std::shared_ptr<Monster>> Monster_list;
    std::mutex monster_lock;
    std::atomic_uint enemy_count{0};
    std::atomic_uint player_health{10};
    void listEnemies();
    unsigned hit(std::shared_ptr<Monster> victim);
public:    
    void monsters();
    void player();
    Game(std::initializer_list<std::pair<std::string, unsigned>> init);
};


#endif // GAME_H

Game.cpp

#include "Game.h"
#include <atomic>
#include <chrono>
#include <initializer_list>
#include <iostream>
#include <memory>
#include <mutex>
#include <queue>
#include <string>
#include <thread>
#include <vector>

#if __has_include(<syncstream>)
#include <syncstream>
std::osyncstream sync_out{std::cout};
#else 
class syncstr {
    std::mutex cout_lock;
    std::ostream& out;
public:
    syncstr(std::ostream& out) : out{out} {}
    syncstr& operator<<(const char *item) {
        std::lock_guard<std::mutex> lock(cout_lock);
        out << item;
        out.flush();
        return *this;
    }
    syncstr& operator<<(const std::string &item) {
        std::lock_guard<std::mutex> lock(cout_lock);
        out << item;
        out.flush();
        return *this;
    }
    syncstr& operator<<(char item) {
        std::lock_guard<std::mutex> lock(cout_lock);
        out << item;
        out.flush();
        return *this;
    }
    syncstr& operator<<(unsigned item) {
        std::lock_guard<std::mutex> lock(cout_lock);
        out << item;
        out.flush();
        return *this;
    }
    syncstr& operator<<(int item) {
        std::lock_guard<std::mutex> lock(cout_lock);
        out << item;
        out.flush();
        return *this;
    }
    syncstr& operator<<(long int item) {
        std::lock_guard<std::mutex> lock(cout_lock);
        out << item;
        out.flush();
        return *this;
    }
};

syncstr sync_out{std::cout};
#endif

Game::Monster::Monster(std::string name, unsigned interval) : 
    name{name}, 
    interval{interval}, 
    deadline{interval}, 
    id{++serial} 
{ }

Game::Game(std::initializer_list<std::pair<std::string, unsigned>> init) {
    std::cin.tie(nullptr);
    for (auto &temp : init) {
        Monster_list.emplace_back(std::make_shared<Monster>(temp.first, temp.second));
        ++enemy_count;
    }
}

void Game::listEnemies() {
    std::lock_guard<std::mutex> mlock(monster_lock);
    sync_out << "Surrounding you are " << enemy_count << " enemies:\n";
    for (const auto &m: Monster_list) {
        if (m->health) {
            sync_out << m->name << m->id << '\n';
        }
    }
    sync_out << "What would you like to attack? ";
}

unsigned Game::hit(std::shared_ptr<Monster> victim) {
    if (victim->health) { 
        if (--victim->health == 0) {
            sync_out << victim->name << victim->id << " defeated!\n";
            --enemy_count;
        }
    }
    return victim->health;
}

void Game::monsters() {
    static auto compare_deadlines = [](std::shared_ptr<Monster> a, std::shared_ptr<Monster> b){
        return a->deadline > b->deadline;
    };
    std::priority_queue<std::shared_ptr<Monster>, std::vector<std::shared_ptr<Monster>>, decltype(compare_deadlines)> monster{compare_deadlines};
    for (auto m : Monster_list) {
        monster.push(m);
    }
    auto start = std::chrono::system_clock::now();
    while (!monster.empty()) {
        std::this_thread::sleep_for(std::chrono::seconds(monster.top()->deadline));
        std::unique_lock<std::mutex> mlock(monster_lock);
        // if the moster is already dead, don't let it attack
        if (hit(monster.top())) {
            sync_out << std::chrono::duration_cast<std::chrono::seconds>(std::chrono::system_clock::now() - start).count() << "  " 
                << monster.top()->name << monster.top()->id << " attacks!  Health = " << monster.top()->health << '\n'
                << "Your health = " << --player_health << '\n';
        }
        // is the game over?
        if (player_health == 0 || enemy_count == 0) {
            mlock.unlock();
            if (enemy_count) {
                sync_out << "You have died -- Game over!\n";
            } else {
                sync_out << "All enemies are defeated!!\n";
            }
            // this also kills the other thread
            exit(0);
        }
        // adjust the priority queue
        auto elapsed = monster.top()->deadline;
        decltype(monster) m2{std::move(monster)};
        while (!m2.empty()) {
            auto current = m2.top();
            m2.pop();
            if (current->deadline > elapsed) {
                current->deadline -= elapsed;
            } else {
                current->deadline = current->interval;
            }
            if (current->health) {
                monster.push(current);
            } 
        }
    }
}

void Game::player() {
    while (enemy_count) {
        listEnemies();
        std::string enemy;
        std::cin >> enemy;
        unsigned hitcount{0};
        for (auto &m: Monster_list) {
            std::lock_guard<std::mutex> mlock(monster_lock);
            if (m->name == enemy && m->health) {
                sync_out << "Hacking away at " << m->name << m->id << '\n';
                hit(m);
                ++hitcount;
            }
        }
        if (hitcount == 0) {
            sync_out << "No living enemy named " << enemy << '\n';
        }
    }
}

unsigned Game::Monster::serial{0};

main.cpp

#include "Game.h"
#include <thread>
#include <functional>

int main() {
    Game game{ {"Dragon",8}, {"Orc", 3}, {"Wumpus", 5}, {"Edward", 7}, {"Orc", 4} };
    auto m = std::thread(&Game::monsters, std::ref(game));
    game.player();
    m.join();
}

Answer 1

Answers to your questions

1. Is the Game object design easy to understand?

It's not too hard. But I would have expected a struct Player (even if there is only one instance of it, so no need to put it in a container), and I also expected class Game to manage the monster thread.

2. Does the scheduling mechanism make sense?

Yes, but it is a bit too complicated. Instead of storing intervals, you can store absolute times, and use std::this_thread::sleep_until(). This avoids having to adjust all the deadlines every time a monster gets to do something. Then you can just do:

while (!monster.empty()) {
    auto current = monster.top();
    std::this_thread::sleep_until(current->deadline);

    // let the monster do its thing

    monster.pop();

    if (current->health) {
        current->deadline += current->interval;
        monster.push(current);
    }
}

It helps if you declare deadline and interval with the correct std::chrono types, so you avoid a lot of casts.

3. I could have used std::initializer_list<Monster> as the argument instead of std::initializer_list<std::pair<std::string, unsigned>> for the constructor. I chose not to because I didn't want temporary copies of Monsters made. What do you think of that choice?

But now it has to create temporary std::pair<std::string, unsigned>s. I would rather use an initializer list that takes Monsters. This will also make it more future proof, for example if you add more overloads to the constructor of Monster.

4. Are there any flaws in the multithreading?

Not that I can see. Of course, if you use an event loop that handles both timeouts and keyboard input, then you wouldn't need threads at all, and you would avoid having to use mutexes.

5. The game currently ends via an exit call to kill all threads. Is there a more elegant way to approach this?

It depends on what you think is elegant. exit() is not very nice, but on the other hand it is just a simple, small statement that takes care of your problems.

One approach, again, is to use an event loop, which is terminated as soon as all monsters are dead or when the player is dead. There are no delays this way, neither when the monsters are killed or the player is killed. I would consider this the most elegant.

You can still use multiple threads, but then use an event loop in player() that checks both cin and a self pipe. When the monsters kill the player, they send something over the self pipe so the player thread can react immediately. You still have a potential delay if the player kills all monsters.

If you can use C++20, then also consider using std::jthread for a little extra elegantness.

You don't need the monster id

You already have a std::vector of Monsters, so the index of the monster in the array is already a unique identifier. And if you use the index as the identifier, you no longer need to scan the array to find the monster.

If you don't want to use indices into an array or vector as an identifier, then I would store the Monsters in a std::map or std::unordered_map.

Naming things

There are some inconsistencies in how you name things. For example, Monster_list is a variable but it starts with an upper case. This makes it easier to confuse it for a type name. Also, don't encode the type of container in the name. It's not a list in any case. I would just call this vector monsters.

Prefer using nouns for variable names, and verbs for function names. So instead of the function monsters(), name it do_monsters() or process_monsters(). The same goes for player().

Add const where appropriate

Some member variables of Monster can be made const, and some member functions of Game can be made const.