SFML community forums

Help => General => Topic started by: Gaius on July 25, 2014, 07:21:33 pm

Title: Pong code review request
Post by: Gaius on July 25, 2014, 07:21:33 pm

So, I wrote a simple Pong game in C++ with SFML and Visual Studio 2013 and was wondering if somebody could tell me if there is anything I could improve upon in the code.
 
Noteworthy:
    - The collision between the ball and the top and bottom of the player panel seems to be slightly bugged
    - The full solution for visual studio can be found here: https://www.dropbox.com/s/558as04io6jr2b5/Pong.zip
    - To quit the game, press escape
 
Game:
 
(http://i.imgur.com/vt8HFLN.png)
 
 
Files:
 
(http://i.imgur.com/YnINnbA.png)
 
Code: (main.cpp)

#include <iostream>
#include <string>
#include <vector>
#include <SFML\Graphics.hpp>
#include <SFML\Audio.hpp>
#include <Windows.h>
#pragma warning(disable: 4244)

#define SCRW    1000    // Screen width
#define SCRH    600             // Screen height
#define BALLW   20              // Ball width
#define BALLH   20              // Ball height 
#define PADDLEW 20              // Paddle width
#define PADDLEH 100             // Paddle height
#define BSPEED  -0.1f   // Ball origin speed
#define ESPEED  0.08f   // Enemy origin speed (player is mouse controlled)

sf::RenderWindow window(sf::VideoMode(SCRW, SCRH), "Pong");
sf::RectangleShape ball(sf::Vector2f(BALLW, BALLH));
sf::RectangleShape player(sf::Vector2f(PADDLEW, PADDLEH));
sf::RectangleShape enemy(sf::Vector2f(PADDLEW, PADDLEH));
float ballSpeedX = BSPEED;
float ballSpeedY = BSPEED;
float enemySpeed = ESPEED;
bool gameOver = false;
int playerPoints = 0;
int enemyPoints = 0;
sf::Font font;
sf::Text plScoreTxt;
sf::Text enScoreTxt;

void draw();
void moveBall();
void enemyAI();
bool intersect(sf::Vector2f, sf::Vector2f, sf::Vector2f);
float intersectPos(float, sf::RectangleShape);
void reset();

int main(){
        // Font and text stuff
        std::string fontname = "segoeuil.ttf";
        if (!font.loadFromFile(fontname))
        {
                std::cerr << "Font " << "\"" << fontname << "\" couldn't be/wasn't loaded. Please try again.\n";
        }
        plScoreTxt.setFont(font);
        plScoreTxt.setCharacterSize(200);
        plScoreTxt.setPosition(sf::Vector2f(window.getSize().x / 2 - 250, window.getSize().y / 2 - 150));
        enScoreTxt.setFont(font);
        enScoreTxt.setColor(sf::Color(255, 255, 255, 32));
        enScoreTxt.setCharacterSize(200);
        enScoreTxt.setPosition(sf::Vector2f(window.getSize().x / 2 + 150, window.getSize().y / 2 - 150));

        // Setting up the window ant entities
        window.setMouseCursorVisible(false);
        ball.setPosition(sf::Vector2f(window.getSize().x / 2 - BALLW / 2, window.getSize().y / 2 - BALLH / 2));
        enemy.setPosition(sf::Vector2f(window.getSize().x - PADDLEW, window.getSize().y / 2 - PADDLEH / 2));

        // Mouse confinement stuff
        RECT rect;
        rect.bottom = window.getPosition().y + window.getSize().y + 32 - PADDLEH / 2;
        rect.top = window.getPosition().y + 31 + PADDLEH / 2;
        rect.left = window.getPosition().x + 50;
        rect.right = window.getPosition().x + window.getSize().x - 42;

        ClipCursor(&rect);

        // Game loop
        while (window.isOpen()){
                sf::Event event;
                while (window.pollEvent(event)){
                        if (event.type == sf::Event::Closed){
                                window.close();
                        }
                        if (event.type == sf::Event::KeyPressed){
                                // Press escape to close the window
                                if (event.key.code == sf::Keyboard::Escape)
                                        window.close();
                        }
                }

                moveBall(); // Move the ball and test for collision
                player.setPosition(sf::Vector2f(player.getPosition().x, sf::Mouse::getPosition(window).y - PADDLEH / 2)); // Move the player paddle to mouse position
                if (ballSpeedX > 0) // Perform enemy AI stuff when the ball is heading in it's direction
                        enemyAI();
                if (gameOver){ // if someone scored, reset the game
                        reset();
                }

                // Text stuff
                plScoreTxt.setString(std::to_string(playerPoints));
                enScoreTxt.setString(std::to_string(enemyPoints));
                plScoreTxt.setColor(sf::Color(ball.getFillColor().r, ball.getFillColor().g, ball.getFillColor().b, 32));
                enScoreTxt.setColor(sf::Color(ball.getFillColor().r, ball.getFillColor().g, ball.getFillColor().b, 32));
                player.setFillColor(ball.getFillColor());
                enemy.setFillColor(ball.getFillColor());

                draw(); // draw everything to the screen

                // If somebody wins, the window closes (the window can also be closed by pressing escape)
                if (playerPoints > 9 || enemyPoints > 9)
                        window.close();
        }
        return 0;
}

void reset(){
        ball.setPosition(sf::Vector2f(window.getSize().x / 2 - BALLW / 2, window.getSize().y / 2 - BALLH / 2));
        enemy.setPosition(sf::Vector2f(window.getSize().x - PADDLEW, window.getSize().y / 2 - PADDLEH / 2));
        if (ballSpeedX < 0)
                ballSpeedX = BSPEED;
        else
                ballSpeedX = BSPEED * -1;
        ballSpeedY = BSPEED;
        enemySpeed = ESPEED;
        gameOver = false;
}

void draw(){
        window.clear();
        window.draw(plScoreTxt);
        window.draw(enScoreTxt);
        window.draw(player);
        window.draw(enemy);
        window.draw(ball);
        window.display();
}

void increaseSpeed(){
        if (ballSpeedY > 0)
                ballSpeedY += (rand() % 15 + 1) / 1000.0;
        else
                ballSpeedY -= (rand() % 15 + 1) / 1000.0;
        if (ballSpeedX > 0)
                ballSpeedX += (rand() % 20 + 1) / 1000.0;
        else
                ballSpeedX -= (rand() % 20 + 1) / 1000.0;
}

void moveBall(){
        ball.setPosition(sf::Vector2f(ball.getPosition().x + ballSpeedX, ball.getPosition().y + ballSpeedY));

        // Ball -> Wall
        if (ball.getPosition().x < 0){
                enemyPoints++;
                gameOver = true;
                return;
        }
        if (ball.getPosition().x + BALLW > window.getSize().x){
                playerPoints++;
                gameOver = true;
                return;
        }
        if (ball.getPosition().y < 0 || ball.getPosition().y + BALLH > window.getSize().y){
                ballSpeedY *= -1;
                return;
        }

        // Ball -> Both paddles (Side)
        if (intersect(sf::Vector2f(ballSpeedX, ballSpeedY), sf::Vector2f(player.getPosition().x + PADDLEW, player.getPosition().y),
                sf::Vector2f(player.getPosition().x + PADDLEW, player.getPosition().y + PADDLEH)) ||
                intersect(sf::Vector2f(ballSpeedX, ballSpeedY), enemy.getPosition(),
                sf::Vector2f(enemy.getPosition().x, enemy.getPosition().y + PADDLEH)))
        {
                ballSpeedX *= -1;
                ball.setFillColor(sf::Color(rand() % 156 + 100, rand() % 156 + 100, rand() % 156 + 100));
                increaseSpeed();
                return;
        }

        // Player - Top
        if (intersect(sf::Vector2f(ballSpeedX, ballSpeedY), player.getPosition(),
                sf::Vector2f(player.getPosition().x + PADDLEW, player.getPosition().y)))
        {
                ballSpeedY += 0.02f;
                if (ballSpeedX < 0)
                        ballSpeedX *= -1;
                if (ballSpeedY > 0)
                        ballSpeedY *= -1;
                return;
        }

        // Player - Bottom
        if (intersect(sf::Vector2f(ballSpeedX, ballSpeedY),
                sf::Vector2f(player.getPosition().x, player.getPosition().y + PADDLEH),
                sf::Vector2f(player.getPosition().x + PADDLEW, player.getPosition().y + PADDLEH)))
        {
                if (ballSpeedX < 0)
                        ballSpeedX *= -1;
                if (ballSpeedY < 0)
                        ballSpeedY *= -1;
                ballSpeedY += 0.02f;
        }
}

void enemyAI(){
        float Ypos = intersectPos(ballSpeedY, ball);
        // Move enemy to optimal location
        if (enemy.getPosition().y + PADDLEH / 2 > Ypos){
                enemy.setPosition(sf::Vector2f(enemy.getPosition().x, enemy.getPosition().y - enemySpeed));
        }
        else if (enemy.getPosition().y + PADDLEH / 2 < Ypos){
                enemy.setPosition(sf::Vector2f(enemy.getPosition().x, enemy.getPosition().y + enemySpeed));
        }
        // Check if enemy collided with the walls
        if (enemy.getPosition().y < 0)
                enemy.setPosition(enemy.getPosition().x, 0);
        else if (enemy.getPosition().y + PADDLEH > window.getSize().y)
                enemy.setPosition(enemy.getPosition().x, window.getSize().y - PADDLEH);
}

bool lineIntersect(sf::Vector2f LinePointA, sf::Vector2f LinePointB, sf::Vector2f pointMoving, float diam){
        if (LinePointA.x == LinePointB.x && pointMoving.x >= LinePointA.x - diam / 2 && pointMoving.x <= LinePointA.x + diam / 2 &&
                pointMoving.y >= LinePointA.y && pointMoving.y <= LinePointB.y)
                return true;
        if (LinePointA.y == LinePointB.y && pointMoving.y >= LinePointA.y - diam / 2 && pointMoving.y <= LinePointA.y + diam / 2 &&
                pointMoving.x >= LinePointA.x && pointMoving.x <= LinePointB.x)
                return true;
        return false;
}

bool intersect(sf::Vector2f vel, sf::Vector2f LinePointA, sf::Vector2f LinePointB){
        double m, b;
        m = ((ball.getPosition().y + ball.getSize().y / 2.0 + vel.y) - (ball.getPosition().y + ball.getSize().y / 2.0)) /
                ((ball.getPosition().x + ball.getSize().x / 2.0 + vel.x) - (ball.getPosition().x + ball.getSize().x / 2.0));
        b = (ball.getPosition().y + ball.getSize().y / 2.0 + vel.y) - m * (ball.getPosition().x + ball.getSize().x / 2.0 + vel.x);

        // If the line is on the y axis, solve for y in order to have highest accuracy rate
        if (vel.x > 0 && LinePointA.x == LinePointB.x){
                for (double x = 0; x <= vel.x; x += 0.1){
                        if (lineIntersect(LinePointA, LinePointB, sf::Vector2f(x + ball.getPosition().x + ball.getSize().x / 2, (int)(m * (x + ball.getPosition().x + ball.getSize().x / 2.0) + b)), ball.getSize().x))
                                return true;
                }
        }
        else if (vel.x < 0 && LinePointA.x == LinePointB.x){
                for (double x = 0; x >= vel.x; x -= 0.1){
                        if (lineIntersect(LinePointA, LinePointB, sf::Vector2f(x + ball.getPosition().x + ball.getSize().x / 2, (int)(m * (x + ball.getPosition().x + ball.getSize().x / 2.0) + b)), ball.getSize().x))
                                return true;
                }
        }

        // if the line is on the x axis, solve for x in order to have highest accuracy rate
        else if (vel.y > 0 && LinePointA.y == LinePointB.y){
                for (double y = 0; y <= vel.y; y += 0.1){
                        if (lineIntersect(LinePointA, LinePointB, sf::Vector2f((int)((y + ball.getPosition().y + ball.getSize().y / 2.0 - b) / m), y + ball.getPosition().y + ball.getSize().y / 2.0), ball.getSize().y))
                                return true;
                }
        }
        else if (vel.y < 0 && LinePointA.y == LinePointB.y){
                for (double y = 0; y >= vel.y; y -= 0.1){
                        if (lineIntersect(LinePointA, LinePointB, sf::Vector2f((int)((y + ball.getPosition().y + ball.getSize().y / 2.0 - b) / m), y + ball.getPosition().y + ball.getSize().y / 2.0), ball.getSize().y))
                                return true;
                }
        }
        return false;
}

// This function is for the enemy to determine where it has to go
// It basically returns a 'y' coordinate of where the ball is going to end up for the enemy's 'x' coordinate
// If the ball hits either the ceiling or the floor before it reaches it's goal, the function is called recursively until the ball stays within bounds
float intersectPos(float tmpBallSpeedY, sf::RectangleShape tmpBall){
        float x = window.getSize().x - PADDLEW;
        float m = (((tmpBall.getPosition().y + BALLH / 2) + tmpBallSpeedY) - (tmpBall.getPosition().y + BALLH / 2)) /
                (((tmpBall.getPosition().x + BALLW / 2) + ballSpeedX) - (tmpBall.getPosition().x + BALLW / 2));
        float b = (tmpBall.getPosition().y) - m * (tmpBall.getPosition().x);
        float y = m * x + b;

        if (y <= 0){
                tmpBall.setPosition(sf::Vector2f((0 - b) / m, 0));
                y = intersectPos(tmpBallSpeedY*-1, tmpBall);
        }
        else if (y >= window.getSize().y - BALLH){
                tmpBall.setPosition(sf::Vector2f(((window.getSize().y - BALLH) - b) / m, window.getSize().y - BALLH));
                y = intersectPos(tmpBallSpeedY*-1, tmpBall);
        }

        return y;
}

Title: Re: Pong code review request
Post by: Strelok on July 25, 2014, 07:39:10 pm

I see where you're coming from. You learned C then moved to C++. Without delving problems in your code, two things stand out:

• You don't use classes
• You use global variables

This is NOT the way to code in C++, classes are a really powerful tool for programmers and they are the cornerstone of C++. As long as you don't grasp this concept there's no use in writing C++ code. As for global variables, they hold the same problem in C, so you can google it.

Title: Re: Pong code review request
Post by: Gaius on July 25, 2014, 08:06:01 pm

Quote from: Strelok on July 25, 2014, 07:39:10 pm

• You don't use classes
• You use global variables

I thought, since this is a relatively small scale game, I could use global variables and one .cpp file without much consequence. Wouldn't it just over complicate the code to separate it into different files if there are only near 300 lines?

Title: Re: Pong code review request
Post by: dabbertorres on July 25, 2014, 08:08:07 pm

Yes, but it scales better. You can take that idea and design with you to more complex programs. There's the saying: perfect practice makes perfect.

I'd strongly advise you to take the code you have here, and improve it with a design using classes.

Other than that, here's my comments:

• You're not coding in C, you're coding in C++: I would use const variables instead of #define's
• I noticed you disabled a warning, it's best to treat warnings as errors.
• Good use of multiple functions to simplify your main loop
• Speaking of loops, I'd read this article (http://gafferongames.com/game-physics/fix-your-timestep/) and see if you can implement a fixed-timestep game loop. It improves your game immensely.
• Careful mixing real-time input (sf::Mouse::getPosition()) and events. Personally, I would check for the MouseMove event and move the paddle with those values.

That's all I have to say on top of Strelok's post. Good job!

Title: Re: Pong code review request
Post by: Nexus on July 25, 2014, 10:51:55 pm

Quote from: Gaius on July 25, 2014, 08:06:01 pm

I thought, since this is a relatively small scale game, I could use global variables and one .cpp file without much consequence.

You asked for code review, and bad habits aren't much better in small scale ;)

Pong is an ideal project to learn how to do things the right way. Since the logic is very simple to implement, you have an opportunity to focus more on code design -- maybe you can even overengineer a bit, just to learn different approaches. It will certainly be a benefit for future projects, where design truly matters.

Other points:

• Backslashes in paths are non-portable
• Global SFML variables are not only bad style, they can lead to serious issues
• Outputting an error message but continuing the program is a bad idea
• Do you really need <Windows.h> and the ClipCursor? If so, you could put it in a separate file and compile conditionally, to keep portability
• I wouldn't use identifiers like "plScoreTxt". If the few additional letters are problematic to type, you need a better IDE
• sf::Vector2f(player.getPosition().x + PADDLEW, player.getPosition().y) is player.getPosition() + sf::Vector2f(PADDLEW, 0)
  
• In general, you have a lot of expressions like ball.getPosition().x. By using vectors instead of separate components and storing them into variables, things get much more readable.
  
• Pass parameters such as sf::RectangleShape tmpBall by const-reference

Title: Re: Pong code review request
Post by: Gaius on July 26, 2014, 12:35:48 am

Quote from: dabbertorres on July 25, 2014, 08:08:07 pm

I'd strongly advise you to take the code you have here, and improve it with a design using classes.

Will do.

Quote from: dabbertorres on July 25, 2014, 08:08:07 pm

Speaking of loops, I'd read this article (http://gafferongames.com/game-physics/fix-your-timestep/) and see if you can implement a fixed-timestep game loop. It improves your game immensely.

I tried to implement that, but I can't seem to find a default high resolution timer for c++.
Edit: It looks like sf::Time could be what I'm looking for.

Quote from: dabbertorres on July 25, 2014, 08:08:07 pm

Careful mixing real-time input (sf::Mouse::getPosition()) and events. Personally, I would check for the MouseMove event and move the paddle with those values.

Was I mixing? I'll check out the MouseMove event.

Quote from: Nexus on July 25, 2014, 10:51:55 pm

• Do you really need <Windows.h> and the ClipCursor? If so, you could put it in a separate file and compile conditionally, to keep portability

How would I compile a file conditionally?

Title: Re: Pong code review request
Post by: Strelok on July 26, 2014, 01:18:40 am

Quote from: Gaius on July 26, 2014, 12:35:48 am

I tried to implement that, but I can't seem to find a default high resolution timer for c++.
Edit: It looks like sf::Time could be what I'm looking for.

How would I compile a file conditionally?

What you need is a sf::Clock (http://www.sfml-dev.org/documentation/2.1/classsf_1_1Clock.php)
and

#ifdef _WIN32 //for visual C++ compiler
//code
#endif
 

or

#if defined(somemacro) || defined(somemacro)
//code
#endif