12
\$\begingroup\$

Here is a project that I have been working on for the past few days.

I have used the SFML library in C++ to make a flappy bird game of my own. I made this as a step towards learning GUI in C++.


The program is Object-Oriented as I believe this made it a little easier to maintain. Although this isn't my first time learning SFML, I am pretty rusty since I never tried to make something serious with it.


Game.h

#pragma once

#include <SFML/Graphics.hpp>
#include "Bird.h"
#include "Obstacle.h"


class Game
{
    public:
        Game(const char*);
        ~Game();
        
        int score;
        void mainloop();
    

    private:
        sf::RenderWindow window;
        Bird bird;
        Obstacle obstacle; 

        sf::Texture background_texture;
        sf::Sprite background;

        void handle_events(const sf::Event&);

        inline void draw_objects();
        inline void update_object_positions();
        inline bool detect_loss();
};


inline bool Game::detect_loss()
{
    const auto& bird_bounds = bird.body.getGlobalBounds();
    if (bird_bounds.intersects(obstacle.top_obstacle.getGlobalBounds()))
        return true;

    if (bird_bounds.intersects(obstacle.bottom_obstacle.getGlobalBounds()))
        return true;

    return false;
}

inline void Game::update_object_positions()
{
    bird.update_bird();
    obstacle.update_obstacle();

    if (obstacle.bottom_obstacle.getPosition().x < -89)
    {
        ++score;
        obstacle.new_rand_obstacle();
    }
}

inline void Game::draw_objects()
{
    window.draw(background);
    window.draw(bird.body);
    window.draw(obstacle.bottom_obstacle);
    window.draw(obstacle.top_obstacle);
}

Game.cpp

#include "Game.h"
#include <iostream>


Game::~Game()
{
    std::cout << "Well played ! Score : " << score << '\n';
}

Game::Game(const char* title)
    : score{ 0 }
{
    window.create(sf::VideoMode(800, 800), title);
    
    

    if (!background_texture.loadFromFile("images//background.png"))
        std::cout << "Failed to load background image\n";

    background.setTexture(background_texture);
}

void Game::handle_events(const sf::Event& event)
{
    switch (event.type)
    {

        case sf::Event::Closed:
            window.close();
            break;

        case sf::Event::KeyPressed:
            if (event.key.code == sf::Keyboard::Space || event.key.code == sf::Keyboard::Up)
                bird.fly();
            if (event.key.code == sf::Keyboard::N)
                obstacle.new_rand_obstacle();
            break;
        
    }
}


void Game::mainloop()
{
    while (window.isOpen())
    {
        sf::Event event;
        
        while (window.pollEvent(event))
        {
            handle_events(event);
        }

        if (detect_loss())
            break;

        update_object_positions();
        window.clear();
        draw_objects();
        window.display();
    }
}

Bird.h

#pragma once

#include <SFML/Graphics.hpp>


class Bird
{
    public:
        sf::Texture texture_wing_up;
        sf::Texture texture_wing_down;
        sf::Sprite body;
        sf::Vector2f acceleration;
        sf::Vector2f velocity;
    

        Bird();
        void fall();
        void fly();
        void reset();
        void update_bird();

    private:
        int start_fall;
};

Bird.cpp

#include "Bird.h"
#include <iostream>

namespace
{
    const sf::Vector2f fly_acc(0, -0.01f);
    const sf::Vector2f fall_acc(0, 0.001f);
    const float fly_rot{ -30.5f };
    const float fall_rot{ 0.06f }; // offset is applied to current rotation
    const sf::Vector2f middle(35,29);
    const sf::Vector2f initial_bird_pos(320, 300);
    const float max_fall_vel = 0.4f;
    const float max_fly_vel = -0.5f;

}


void Bird::fly()
{
    acceleration = ::fly_acc;
    start_fall = static_cast<int>(body.getPosition().y-7);
    body.setRotation(::fly_rot);
    body.setTexture(texture_wing_down);
}

void Bird::fall()
{
    acceleration = ::fall_acc;
    body.rotate(::fall_rot);
    body.setTexture(texture_wing_up);

}

void Bird::reset()
{
    acceleration = { 0,0 };
    velocity = { 0,0 };
    body.setPosition(320, 300);
    body.setRotation(0);

    start_fall = 0;
}

void Bird::update_bird()
{

    velocity += acceleration;


    if (velocity.y > ::max_fall_vel) velocity.y = ::max_fall_vel;
    if (velocity.y < ::max_fly_vel) velocity.y = ::max_fly_vel;

    body.move(velocity);

    const auto& position = body.getPosition().y;
    if (position < start_fall) fall();
}

Bird::Bird()
{
    if (!texture_wing_up.loadFromFile("images//bird_wing_up.png"))
        throw std::runtime_error("Failed to load images//bird_wing_up.png\n");


    if (!texture_wing_down.loadFromFile("images//bird_wing_down.png"))
        throw std::runtime_error("Failed to load images//bird_wing_down.png");


    body.setTexture(texture_wing_up);
    body.setPosition(initial_bird_pos);

    acceleration = { 0,0 }; 
    velocity = { 0,0 };

    body.setOrigin(middle); // Imporant as it also sets the point where the bird rotates at
    start_fall = 0;

}

Obstacle.h

#pragma once

#include <SFML/Graphics.hpp>



class Obstacle
{
    public:
        sf::Texture texture;
        sf::Sprite bottom_obstacle;
        sf::Sprite top_obstacle;
        sf::Vector2f velocity;

        Obstacle();

        void update_obstacle();
        void new_rand_obstacle();

};

Obstacle.cpp

#include "Obstacle.h"
 
#include <stdlib.h>    
#include <iostream>



Obstacle::Obstacle()
{
    velocity = { -0.15f,0 };
    if (!texture.loadFromFile("images//obstacle.png"))
        throw std::runtime_error("Failed to load images//obstacle.png\n");


    bottom_obstacle.setTexture(texture);
    bottom_obstacle.setPosition(720, 300);

    top_obstacle = bottom_obstacle;
    top_obstacle.rotate(180);

    
    const auto&  bottom_position = bottom_obstacle.getPosition();
    top_obstacle.setPosition(bottom_position.x+89, bottom_position.y - 250);

    srand((unsigned)time(0));
}

void Obstacle::update_obstacle()
{
    bottom_obstacle.move(velocity);
    auto bottom_position = bottom_obstacle.getPosition();
    top_obstacle.setPosition(bottom_position.x+89, bottom_position.y - 250);
}

void Obstacle::new_rand_obstacle()
{
    const auto new_pos = rand() % 600 + 200;
    bottom_obstacle.setPosition(800, (float)new_pos);
    const auto& bottom_position = bottom_obstacle.getPosition();
    top_obstacle.setPosition(bottom_position.x+89, bottom_position.y - 250);
}

main.cpp

#include "Game.h"

int main()
{
    Game* game = new Game("Flappy Bird");
    game->mainloop();


    delete game;
    game = nullptr;


    return 0;
}

The physics for the bird

  • The bird's physics was the part I took time to code, not because it was tough but I tried to perfect how the bird fall() and fly(). I used acceleration that would modify velocity. The values given to acceleration are quite small, but each frame it adds up so the overall movement of the bird looks really good. Every time fly() is called, it sets a point above the bird at which the bird will start to de-celerate. Hence start_fall() .I am happy with how the bird flew at last 😁

  • There are two images, one with the bird's wings flapped and one normal. When fly() is called I switch to the flapped wings, and when it starts falling I switch back to the normal ones, this also adds to the effect and gives a better look.

  • The bird also rotates according to its velocity.

The obstacles are pretty straight forward.

  • One obstacle at the bottom has a constant velocity and is placed randomly on the y-axis every new generation.

  • Top obstacle is rotated 180 ° and aligned with the bottom obstacle.

What I expect from a review

  • General coding aspects

  • Things like acceleration, origin, positions are all constant, and bird.cpp has many of them. At first, I decided to just use the plan floating-constants, but then the magic numbers didn't look very nice.
    Hence, I decided to keep them in an anonymous namespace as they are only used in bird.cpp. Is this a better way to do this? Also, what way do you usually prefer to store stuff like this?

Bird Bird2

Game

\$\endgroup\$
  • 2
    \$\begingroup\$ What do you expect that the reviewer looks at? General coding, specific aspects? \$\endgroup\$ – JVApen Oct 20 at 10:56
  • 1
    \$\begingroup\$ Yes anything and everything \$\endgroup\$ – Aryan Parekh Oct 20 at 11:25
5
\$\begingroup\$

Prefer member variables to be private if possible

There are a lot of member variables that are public that are not used outside the class itself. For example, Game::score, Bird::texture_wing_up, and many more. These should all be private, as this prevents other classes from accidentily accessing these member variabels.

Avoid premature inlining of functions

Why are detect_loss(), update_object_positions() and draw_objects() declared as inline functions in Game.h? I do not see any reason why these would be performance critical. Declare them as regular member functions, and define them in Game.cpp. Note that the compiler itself can still decide to inline those functions when they are called from mainloop().

Who is responsible for what

Your game is quite simple, and Game::draw_objects() looks perfectly reasonable. But what it actually does is having class Game reaching into class Bird and class Obstacle, and accessing their member variabels body and bottom/top_obstacle. This means that there is now quite a tight coupling between those classes. But consider now that drawing the bird would be much more complex than just drawing a single sf::Sprite. Maybe you have many separate sprites, for example one for the wings, one for the body, one for the head and so on, that all animate independently. Do you want class Game to be responsible for drawing a Bird in that case?

There are several ways to address this issue. You could simply add a member function that does all the drawing in class Bird:

void Game::draw_objects() {
    ...
    bird.draw(window);
    ...
}

void Bird::draw(sf::RenderTarget &target) {
    target.draw(body);
}

I used the fact that sf::Window derives from sf::RenderTarget, so Bird::draw() is now more generic than if you would pass a reference to an sf::Window. Alternatively, with SFML, you could make Bird become an sf::Drawable, like so:

void Game::draw_objects() {
    ...
    window.draw(bird);
    ...
}

class Bird: public sf::Drawable {
    ...
    Bird::draw(sf::RenderTarget &target, sf::RenderStates states) final;
    ...
};

void Bird::draw(sf::RenderTarget &target, sf::RenderStates states) {
    target.draw(body);
}

With either technique, you can make the sf::Sprite variabels private, and while it doesn't look like much of an improvement for Bird, it already becomes more interesting for Obstacle, where in Game::draw_objects() you should only have to call window.draw(obstacle) to have the Obstacle itself draw both its bottom and top parts.

(For games with much more objects you might want to look into using an Entity Component System like EnTT, where one of the components would be the drawable part of entities like the bird and the obstacle, but that is obviously complete overkill for this game.)

Have Game::handle_events() implement the while-loop

Despite the name, Game::handle_events() only handles a single event, the while-loop that ensures all queued events are handled is in Game::mainloop(). Consider moving the while-loop to handle_events().

Create a function render() to further simplify mainloop()

Create one high-level function to do the rendering. This keeps mainloop() nice and clean:

void Game::render() {
    window.clear();
    draw_objects();
    window.display();
}

void Game::mainloop() {
    while (running)
    {
        handle_events();
        update_object_positions();
        render();
    }
}

Create a member variable running that can be set to false in handle_events() if the window is closed, or by update_object_positions() if a collision between the bird and the obstacle is detected.

Make global constants constexpr

It's very good that you avoided magic constants in your code, and gave them clear names. The anonymous namespace does the same as static, and ensures they don't have external linkage. But even better is to make them constexpr instead of const. This makes it clear to the compiler that this is not meant to be used as a variable (of which an address can be taken for example), but really just as a literal constant.

There still are magic constants left

There are still lots of magic constants in your code. For example, the window size, the initial position of the obstacle, and so on.

Call reset() from the constructor

You have some unnecessary code duplication, since you are initializing member variables manually in the constructor of Bird and Obstacle, that you also set from Bird::reset() and Obstacle::new_rand_obstacle(). Consider calling the latter two functions from the constructors of those classes.

Use C++'s random number generator functions

C++ has much better random number generator facilities than C. Use then instead of rand(). For example, you can use std::uniform_int_distribution in Obstacle.

Use std::min and std::max

For example:

void Bird::update_bird()
{

    velocity += acceleration;
    velocity.y = std::min(std::max(velocity.y, max_fly_vel), max_fall_vel);
    ...
}

Or even better, with C++17 you can write:

    velocity.y = std::clamp(velocity.y, max_fly_vel, max_fall_vel);

Avoid allocating objects on the heap for no good reason

There is no need to use new in main(), you can just write:

int main()
{
    Game game("Flappy Bird");
    game.mainloop();
}

If you do want or need to avoid allocating an object on the stack, then you should still avoid using new and delete directly, but instead use something like std::unique_ptr to manage the lifetime for you automatically. like so:

int main()
{
    auto game = std::make_unique<Game>("Flappy Bird");
    game->mainloop();
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Amazing insights! I even told this on the other review since It was pointed out there too, I have read that I shouldn't allocate large objects on the stack, and even visual studio suggests me to move Game into the heap if I initialize it on the stack. Is this wrong? \$\endgroup\$ – Aryan Parekh Oct 21 at 3:23
  • 1
    \$\begingroup\$ Also, std::clamp didn't seem to work, the bird flew straight up never came down, I think you have switched max_fall_vel and max_fly_vel \$\endgroup\$ – Aryan Parekh Oct 21 at 3:38
  • \$\begingroup\$ Ah, I indeed had the velocities the wrong way around. As for Game on the stack: it's not large at all, and even if it was in the order of kilobytes, I still wouldn't worry about it if it's just a single object. However, the moment you start declaring an array of objects on the stack you should be careful, as it's easy to use a lot of memory that way, and indeed the stack is a limited resource. But on most desktop operating systems it's in the order of megabytes for the main thread. \$\endgroup\$ – G. Sliepen Oct 21 at 6:35
  • \$\begingroup\$ Alright, I understand that, so for this case, it shouldn't be an issue to have Game on the stack right? \$\endgroup\$ – Aryan Parekh Oct 21 at 6:42
  • 1
    \$\begingroup\$ Correct, in this case it's fine. \$\endgroup\$ – G. Sliepen Oct 21 at 6:59
4
\$\begingroup\$

I'm not an expert in SFML, so I can't really give any advice about that. Though, let me look at the code as is.

Let's start with Game.h:

  • Your Game is taking a const char * as argument, I would recommend std::string_view if you would be compiling with C++17. It has a lot of features of std::string and it behaves as const char *
  • I like how you encapsulate several of your members behind relevant functions, though, why is score public?
  • I can understand the need for a few functions to be inline. However, why would you implement those functions in the same header if they are only callable from within your other methods (which are all implemented in the cpp). It would increase compile times (especially on large projects) and it puts the private details in the public file.
  • In one of the functions you compare x with -89, this is a bit strange to me as a reader, what's this number? What does it represent. Putting it in a constant would help a lot in understanding why every value from -inf to (and including) -90 would be accepted.

Game.cpp:

  • Again you have some magic constants, in this case: "images//background.png". Here it makes sense to put this in a constant, that way, you later on could use a code generator to create these constants based on the actual pictures (or even embed them) and you get compilation failures if they go missing.
  • Failed to load ... sounds like an error, yet, you stream this to std::cout instead of std::cerr. As a result, the console can't collor this differently.
  • Looking at the same message, your user is gonna be puzzled, how should they solve this? It could help if you mention to them where you expected the image to be so they can put a new picture there.

Bird.h:

  • start_fall is not initialized, by writing int start_fall{0}; you can say this needs to be zero. That way, you can't forget about it in the cpp.

Bird.cpp:

  • Let's look at layout, in update_bird you put the if and the code on 1 line, in the Ctor, you put it on 2.
  • In the same update_bird function, you seem to be correcting velocity.y, I would write something like: velocity.y = std::clamp(velocity.y, ::max_fly_vel, ::max_fall_vel); Much easier to read what's going on, less chance to write something wrong.
  • In the Ctor, you throw exceptions, yet I don't see any mention of noexcept or in this case noexcept(false) to inform your users when to expect exceptions.

Obstacle.cpp:

main.cpp:

  • What's the point of allocating a game if you can put it on the stack? I've once explained this in more detail, see stackoverflow

So in general:

  • Really good code with a few remarks
  • It's obvious that you ain't familiar with the details of C++1 or more recent, using those things could help making this code easier to read
  • I didn't really mention anything about structure, let me fix that: This looks like really nice OO!

And to answer your question about the constants: I usually use constexpr constants. Whether it's in unnamed namespace, constants in a separate header or static constants in the class depends on the use case.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Brilliant review, I have to clarify a few things though. I have read that I shouldn't allocate large objects on the stack, and even visual studio suggests me to move Game into the heap if I initialise it on the stack. Is this wrong? \$\endgroup\$ – Aryan Parekh Oct 21 at 3:20
  • \$\begingroup\$ Also, declaring any sf::Vector2f variable constexpr throws an error instantly saying that the object isn't constant, is this related to how the constructor is designed in class Vector2f?' \$\endgroup\$ – Aryan Parekh Oct 21 at 3:57
  • 2
    \$\begingroup\$ Yes, you are right, though what's a big object? I only see 5 members of which I expect most of them to do allocations to store their data (they have the big data in it). Best to do a sizeof(Game) and check. Normally you should be having 1MB of stack with visual studio compiler and you can change this if needed. \$\endgroup\$ – JVApen Oct 21 at 6:35
  • 1
    \$\begingroup\$ Constexpr has its limitations, c++20 resolves a few of them again making std::vector and std::string constexpr. Instead use inline static const for ease of use. That said, when constexpr is possible, use that. \$\endgroup\$ – JVApen Oct 21 at 6:38
  • \$\begingroup\$ Yes I did do sizeof(), but I assumed that since Game also uses Bird and Obstacle which also use some extra variables, it would be big, but since you've explained it now, I think I will be okay if I continue to use the stack \$\endgroup\$ – Aryan Parekh Oct 21 at 6:45

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.