4
\$\begingroup\$

I started writing this game from scratch yesterday, and I wasn't completely new to C++'s future library.

I only use the std::async class to launch asynchronously the conio.h defined getch function, to get user's input without stopping the execution of the rest, which has to update the screen each 0.1s (I did it with chrono).

char choice;
while (choice != 'q') {
    auto input = std::async(std::launch::async, getch);
    while (input.wait_for(std::chrono::milliseconds(FRAME_DURATION)) != std::future_status::ready) {
        if (game::status == PAUSED) // if the game is paused, do nothing
            continue; // just wait for the next char
        
        moveAllBullets(); // move all bullets
        updateField(); // update the field
        printField(); // print the field

        for (auto& enemy: game::enemies) {
            if (!enemy.alive)
                continue;
            if (rand()%5 == 0) {
                enemy.turn();
                enemy.fireBullet();
            } else if (rand()%2 == 0) {
                enemy.movePlayer(game::random::directionalChar());
            }
        }
        if (rand()%50 == 0)
            game::random::addEnemy(rand()%48+1, rand()%18+1);

        if (game::player.auto_fire)
            game::player.fireBullet();
        game::player.points++;
    }
    printField(); // print the field

    choice = input.get();
    switch (choice) {
        case 'q': case 'Q':
            return;
        case 'p': case 'P':
            game::status = PAUSED;
            break;
        case 'r': case 'R':
            game::status = PLAYING;
            break;
        case 'w': case 'W': case 'a': case 'A': case 's': case 'S': case 'd': case 'D':
            if (game::status == PLAYING)
                game::player.movePlayer(choice);
            break;
        case 'f': case 'F':
            if (game::status == PLAYING && game::player.ammunitions > 0 && !game::player.auto_fire)
                game::player.fireBullet();
            break;
        case 'x': case 'X':
            game::player.auto_fire = !game::player.auto_fire;
            break;
    }
}

This is the content of the function mainloop, which gets called at the beginning of the program.

My doubts were about this two lines:

auto input = std::async(std::launch::async, getch);
while (input.wait_for(std::chrono::milliseconds(FRAME_DURATION)) != std::future_status::ready)

The refresh is quite slow for a 20x50 matrix, so I was wondering if the problem could be that input.wait_for, since I know that sometimes listening for an event may affect the performance. Also I don't know if it depends on the code or if printing 20×50=1000 characters is slow.

Is wait_for the "problem"? In this case is there any alternative to wait_for?

Edit


Would the use of multithreading instead of asynchronous functions improve the performance? Is there any difference in terms of what happens to the I/O?

Edit (1)


The most effective changes were (in this order):

  1. Inserting std::ios_base::sync_with_stdio(false) at the beginning of the program
  2. Replacing all the system("cls") as suggested by G. Sliepen
  3. Replacing std::endl with '\n' (just once, in the printField function, I don't need to flush in that case, right?)

I'll anyway follow some of your other suggestions, starting from that ones about multithreading and curses libraries, that I'm sure will be very useful.

Thank you so much, I'm new to this community and I've never recieved such acceptance in a SE community, and I've never learned so much with a single question too.

\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

Don't call system()

The refresh is quite slow for a 20x50 matrix, so I was wondering if the problem could be that input.wait_for(), since I know that sometimes listening for an event may affect the performance. Also I don't know if it depends on the code or if printing 20×50=1000 characters is slow.

It's impossible to see from the code you posted, but taking a peek at your GitHub repository, the problem seems to be mainly because you clear the screen by calling system("cls"). This is horribly inefficient because system() creates a new shell process, which in turn has to parse the string "cls", and then execute the code to clear the screen. Apart from being slow, it is also not portable: doing this will prevent your code from running on Linux or macOS for example.

A much better way would be to print the ANSI escape code for clearing the screen. This is much more efficient and more portable:

std::cout << "\33[2J\33[H";

That actually prints two ANSI escape codes; one to clear the screen and another to move the cursor to the top left of the screen.

Use '\n' instead of std::endl

Prefer using '\n' instead of std::endl; the latter is equivalent to the former, but also forces the output to be flushed, which can negatively impact performance. If you need to flush the output, I recommend you do this explicitly by using std::flush.

Consider using a curses library

Using std::async() to do getch() with a timeout is a kludge. It might work in this simple program, but it doesn't scale. What if you need to wait for two different things? There is unfortunately no simple way to wait_for() two futures at the same time.

Instead of trying to solve this problem yourself, I strongly recommend you use a library that takes care of this for you. If you want a pure text console game, then you should use a curses library. These all have a standard interface, so it's easy to swap one for another. For DOS and Windows, I recommend you use PDCurses.

\$\endgroup\$
1
  • \$\begingroup\$ I'll try to change that system calls and that std::endl. I think the first one affects the performance the most, right? Anyway thank you, +1 \$\endgroup\$
    – FLAK-ZOSO
    Jul 6 at 16:11
3
\$\begingroup\$

Here are some things that may help you improve your code.

Initialize variables before using them

The first two lines of the code show are these:

char choice;
while (choice != 'q') {

The first time through the loop, what is the value of choice? The answer is that it could be anything because it's not initialized.

Consider using a better random number generator

You are currently using

if (rand()%5 == 0) {

There are two problems with this approach. One is that the low order bits of the random number generator are not particularly random, so neither with random1 be. On my machine, there's a slight but measurable bias toward 0 with that. The second problem is that it's not thread safe because rand stores hidden state. A better solution, if your compiler and library supports it, would be to use the C++11 `std::bernoulli_distribution. It looks complex, but it's actually pretty easy to use. If we want something to happen 20% of the time, we just use this:

std::bernoulli_distribution d(0.2);

Eliminate "magic numbers"

The code is full of "magic numbers" -- that is raw numbers in the text that don't have obvious meaning. For example:

if (rand()%50 == 0)
    game::random::addEnemy(rand()%48+1, rand()%18+1);

You could define constexpr values for those constants and give them meaningful names, which would make understanding and maintaining this code much easier.

Rethink your use of objects

It seems clear that you have some objects for player and enemy, but it appears you're using free-standing functions for updateField() and printField() and moveAllBullets(). It seems to me that those would be better as member functions of a Game or Field object.

Separate responsibilities

The Model-View-Controller design pattern is often useful for programs like this. It would allow you to separate the game logic (the Model) from the view (with printField() for example) from the input, which is your std::async task. That way, if you wanted to, for example, port this to a graphics library such as SFML, you could do so without altering the underlying logic for the game.

Write better comments

The code contains these three lines:

moveAllBullets(); // move all bullets
updateField(); // update the field
printField(); // print the field

Do those comments really add anything useful? I'd suggest simply deleting those, and instead adding comments where the code is less obvious, such as the logic for calling addEnemy.

Rethink your use of std::async

What you're attempting to do in this code is update the screen on a periodic basis, but also respond quickly to input from the user. On my machine (once I make some adjustments to allow it to compile and run on Linux), each call to std::async spawns a new thread. This will definitely slow things down a bit as contrasted with simply launching a separate user input thread and keeping it going until the end of the game. The second observation is that the game update should be done on a periodic basis no matter what the user is or isn't doing, which suggests a simple timer rather than wait_for which ties the two together. See the following suggestion for alternatives.

Use threads to improve responsiveness

Video games can be really annoying if they seem to be slow to refresh or to respond to user input. For that reason, I'd suggest using at least two threads. One thread could handle user input, while the other could handle screen updates. It is probably not necessary in this case, but another possibility would be to have a third thread for game logic.

Use a library to improve the speed

Peeking at your github repo, calls to system(cls) on a regular basis are bad for portability and security, but they're also very bad for performance because it has to spawn a new process, find and load the cls command (from somewhere!) execute it and then shut all of that down neatly and return. For a text-based game like this, ncurses might be a much better choice and has the benefit of portability to other systems. For Windows, see this question and answer.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you so much! In particular the part about the number generator. About the use of objects I must say that I did a mess with namespaces ecc., I had never used classes and namespaces before. Thank you also for pointing out that "magic numbers": I tried to use as many constants as possible (see the first 70 lines of variables.hpp) but I missed some of them. As the last think I just wanted you to know that the comments were written by Copilot, but I agree that they're completely useless there. \$\endgroup\$
    – FLAK-ZOSO
    Jul 6 at 16:19
  • \$\begingroup\$ I'll accept your answer when I'll see an improvement with the performance, do you think it's affected the most by that system calls or by the use of std::async? \$\endgroup\$
    – FLAK-ZOSO
    Jul 6 at 16:19
  • \$\begingroup\$ I'm glad you found it useful. I would bet that it's the system calls that are mostly responsible for the performance issues. Another thing I see in your github repo is that you're including code files in other code files. See the first suggestion in this answer instead: codereview.stackexchange.com/questions/255226/… \$\endgroup\$
    – Edward
    Jul 6 at 18:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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