Code Review - Pong

Started by
5 comments, last by W84M3 8 years, 8 months ago

I don't know if this is allowed, but since i read in this archive that it was recommended so we beginners can improve here is my code for the game Pong:

I've watched some video tutorials about how to make pong.

I've used C++ with SDL2.


#include "SDL.h"
#include "SDL_ttf.h"
#include <iostream>
#include <stdlib.h>
#include <ctime>
#include <string>

using namespace std;

// Default positions for the Rects
const int BALL_X = 390;
const int BALL_Y = 290;

const int Player_X = 20;
const int Player_Y = 250;

const int AI_X = 770;
const int AI_Y = 250;

int score_Player = 0;
int score_AI = 0;

bool running;

//SDL STUFF
SDL_Window *window = NULL;
SDL_Renderer *renderer= NULL;
SDL_Event event;

SDL_Rect rect_Player;
SDL_Rect rect_AI;
SDL_Rect rect_Ball;

//SDL_TTF STUFF
SDL_Color White = { 255, 255, 255 , 0};
TTF_Font* Sans;

// Ball Velecities
int xVel;
int yVel;

//Get random number between high and low
int GetRandomNumber(int high, int low)
{
return rand() % high + low;
}

//Reset the ball and the players to the default position and gives teh inicial boost to the ball
void ResetGame()
{
rect_Player.x = Player_X;
rect_Player.y = Player_Y;

rect_AI.x = AI_X;
rect_AI.y = AI_Y;

rect_Ball.x = BALL_X;
rect_Ball.y = BALL_Y;
xVel = GetRandomNumber(2, -2);
yVel = GetRandomNumber(2, -2);
//Horizontal speed is always 2 ou -2
if (xVel >= 2 || xVel >=0)
{
xVel = 2;
}
if (xVel < 0)
{
xVel = -2;
}
}

//Test if a point (a,b) is inside rect
bool PointInRect(int a, int b, SDL_Rect rect)
{
if (a > rect.x && b > rect.y && a < rect.x + rect.w && b < rect.y + rect.h)
{
return true;
}
return false;
}

//Test colision between rect1 and rect2
bool TestColision(SDL_Rect rect1, SDL_Rect rect2)
{
if (PointInRect(rect1.x, rect1.y, rect2) || PointInRect(rect1.x + rect1.w, rect1.y, rect2) || PointInRect(rect1.x, rect1.y + rect1.h, rect2) || PointInRect(rect1.x + rect1.w, rect1.y + rect1.h, rect2))
{
return true;
}
return false;
}

//Starts the window, the renderer and sets the position of the PLayer, Ball and AI and loads the font for the score
void LoadGame()
{
if (SDL_Init(SDL_INIT_EVERYTHING) != 0){
std::cout << "SDL_Init Error: " << SDL_GetError() << std::endl;
return;
}

if (TTF_Init()!= 0){
std::cout << "SDL_Init Error: " << TTF_GetError << std::endl;
return;
}

window = SDL_CreateWindow("Pong!", SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED, 800, 600, SDL_WINDOW_OPENGL);
if (window == nullptr){
std::cout << "SDL_CreateWindow Error: " << SDL_GetError() << std::endl;
SDL_Quit();
return;
}

renderer = SDL_CreateRenderer(window, -1, SDL_RENDERER_ACCELERATED );
if (renderer == nullptr){
SDL_DestroyWindow(window);
std::cout << "SDL_CreateRenderer Error: " << SDL_GetError() << std::endl;
SDL_Quit();
return;
}

SDL_SetRenderDrawColor(renderer, 0, 0, 0, SDL_ALPHA_OPAQUE);

SDL_RenderClear(renderer);

SDL_RenderPresent(renderer);

rect_Player.h = 100;
rect_Player.w = 10;

rect_AI.h = 100;
rect_AI.w = 10;

rect_Ball.h = 20;
rect_Ball.w = 20;
rect_Ball.x = BALL_X;
rect_Ball.y = BALL_Y;

srand((int)time(0));

Sans = TTF_OpenFont("Sans.ttf", 19);
ResetGame();
}

//Keeps tabs on score x=1 - AI Scored! x=2 - Player Scored!
void Score(int x)
{
if (x == 1)
{
score_Player += 1;
}
if (x == 2)
{
score_AI += 1;
}
}

//Game logic - Movement of the ball and the players...colision tests and score update
void GameLogic()
{
//Delay so the game doesnt run too fast on my PC
SDL_Delay(10);

//Event Manager
SDL_PollEvent(&event);
switch (event.type)
{
case SDL_KEYDOWN:
/* Player Movement */
switch (event.key.keysym.sym)
{
case SDLK_w:
rect_Player.y -= 10;
break;
case SDLK_s:
rect_Player.y += 10;
break;
default:
break;
}
break;
case SDL_QUIT:
running = false;
break;
}

//Ball Movement
rect_Ball.x += xVel;
rect_Ball.y += yVel;

//AI Movement
//AI starts moving only when ball gets to his half of the map
if (rect_Ball.x > 400)
{
if (rect_AI.y + 0.5*rect_AI.h > rect_Ball.y + 0.5*rect_Ball.h)
{
rect_AI.y -= 3;
}

if (rect_AI.y + 0.5*rect_AI.h < rect_Ball.y + 0.5*rect_Ball.h)
{
rect_AI.y += 3;
}
}

//Player Top and Bottom limits
if (rect_Player.y < 1)
{
rect_Player.y = 1;
}

if (rect_Player.y + rect_Player.h > 599)
{
rect_Player.y = 599 - rect_Player.h;
}

//Ball Top and Bottom limits
if (rect_Ball.y < 1)
{
yVel = -yVel;
}

if (rect_Ball.y + rect_Ball.h > 599)
{
yVel = -yVel;
}

//Ball Left and Right limits
//When the ball touches the limits someone scored!
//1 - The player
//2 - The AI
if (rect_Ball.x + rect_Ball.w > 801)
{
ResetGame();
Score(1);
}

if (rect_Ball.x < -1)
{
ResetGame();
Score(2);
}

//AI Top and Bottom limits

if (rect_AI.y < 1)
{
rect_AI.y = 1;
}

if (rect_AI.y + rect_AI.h > 599)
{
rect_AI.y = 599 - rect_AI.h;
}

// Test colision Ball-Player
if (TestColision(rect_Ball, rect_Player))
{
xVel = -xVel;
}

// Test colision Ball-AI
if (TestColision(rect_Ball, rect_AI))
{
xVel = -xVel;
}
}

//Renders the Rects into the window
void DrawScreen()
{
//Renders the Ball the Player and the AI
SDL_SetRenderDrawColor(renderer, 0, 0, 0, 255);

SDL_RenderClear(renderer);

SDL_SetRenderDrawColor(renderer, 255, 255, 255, 255);

SDL_RenderFillRect(renderer, &rect_Player);

SDL_RenderPresent(renderer);

SDL_RenderFillRect(renderer, &rect_AI);

SDL_RenderPresent(renderer);

SDL_RenderFillRect(renderer, &rect_Ball);

SDL_RenderPresent(renderer);

//PLAYER_SCORE

string PScore = "Player: " + to_string(score_Player);

SDL_Surface* scorePlayerMessage = TTF_RenderText_Solid(Sans, PScore.c_str() , White); 

SDL_Texture* playerMessage = SDL_CreateTextureFromSurface(renderer, scorePlayerMessage);

int Pw = scorePlayerMessage->w;
int Ph = scorePlayerMessage->h;

SDL_FreeSurface(scorePlayerMessage);

SDL_Rect rect_PlayerScore = { 145, 10, Pw, Ph };

SDL_RenderCopy(renderer, playerMessage, NULL, &rect_PlayerScore);
SDL_RenderPresent(renderer);

SDL_DestroyTexture(playerMessage);

//AI_SCORE

std::string AScore = "AI: " + std::to_string(score_AI);

SDL_Surface* scoreAIMessage = TTF_RenderText_Solid(Sans, AScore.c_str(), White);

SDL_Texture* AIMessage = SDL_CreateTextureFromSurface(renderer, scoreAIMessage);

int Aw = scoreAIMessage->w;
int Ah = scoreAIMessage->h;

SDL_FreeSurface(scoreAIMessage);

SDL_Rect rect_AIScore = { 390+145, 10, Aw, Ah };

SDL_RenderCopy(renderer, AIMessage, NULL, &rect_AIScore);
SDL_RenderPresent(renderer);

SDL_DestroyTexture(AIMessage); 
}

//Update method... too fast
void Update()
{
while (running) //game loop
{
GameLogic(); //update stuff;
DrawScreen(); //render your stuff
}
}

//Quits the game
void Quit()
{
SDL_DestroyRenderer(renderer);
SDL_DestroyWindow(window);
TTF_Quit();
SDL_Quit();
}

//Main
int main(int argc, char* args[])
{

LoadGame();
running = true;
Update();
Quit();

return 0;
}

Its a bit rough around the edges. I made this in less than 24 hours with lots of google. There is stuff here i don't understand 100% mainly some SDL stuff on the renderer (DrawScreen() method).

Some questions i have:

  1. How can improve the update method? Because now it is updating way to fast even with a SDL_Delay(10); the rendering is flickering a bit.
  2. I didn't fully understand the renderer code. It was somewhat of try and error where i ended up placing SDL_RenderPresent(renderer); after each thing i wanted to render. (feels stupid and unnecessary, maybe i'm the one who is stupid)
  3. What is the idea behind implementing sound? (i didn't feel like googling this one)
  4. How can i improve upon my code?
  5. What is a good way to make the AI to lose? AT this moment is quit impossible for him to lose... I've made him move only when the ball gets into his side of the map, and even slowed him down quite a bit (relative to the player speed) but he looks almost like he teleports to the ball location,

Tomorrow i'll start with my worm game, this time without (hopefully) any video tutorial.

Advertisement
2. Definitely don't do that. Only Present once per update/frame. That is why you get flickering; you're copying incomplete images to the screen while drawing out the scene.

Sean Middleditch – Game Systems Engineer – Join my team!

Some questions i have:
How can improve the update method? Because now it is updating way to fast even with a SDL_Delay(10); the rendering is flickering a bit.

Measure time instead of moving every frame. "Speed" in defined as distance divided by time, e.g. miles per hour, meters per second, etc.

Your game logic is adding/removing 10 to the position without accounting for time. You shoudl just _measure_ time, and then use that to calculate how far you should move every time your GameLogic function runs.

See http://gameprogrammingtutorials.blogspot.com/2010/01/sdl-tutorial-series-part-5-dealing-with.html (first hit on Google I found for "SDL measure time" - it seems to show how to use SDL_GetTicks to measure elapsed time).

By measuring time, it doesn't matter if your game runs fast or slow; the speed will be consistent. If the player is supposed to move 1000 pixels every second, multiply that speed by the time since the last movement. 1000 pixels / 1 second * X elapsed seconds = 1000 * X pixels. X will is time in seconds, which for a game will be well under 1, so you'll move only some fraction of 1000 pixels that frame. As the frame speed varies, so will the distance moved, giving you the illusion of smooth continuous motion.

I didn't fully understand the renderer code. It was somewhat of try and error where i ended up placing SDL_RenderPresent(renderer); after each thing i wanted to render. (feels stupid and unnecessary, maybe i'm the one who is stupid)

As per my first answer, just do that once. Present copies what you've drawn onto the actual screen (normally it's in off-screen temporary buffer somewhere). You only want to copy complete images up, as otherwise the user's eye will be able to see very brief glimpses of the incomplete scene. That reuslts in the flickering, because you first show the user a scene with one paddle, then with both paddles, etc., instead of only showing the user a screen with everything.

What is the idea behind implementing sound? (i didn't feel like googling this one)

1. Make a sound file.

2. Ask SDL to load the sound file into a sound object.

3. Tell SDL to play the sound object when you want the sound to start playing.

Repeat steps 1-2 for each unique sound in your game (ball hits paddle, ball hits wall, player wins, player loses, etc.). Repeat step 3 every time you want a sound to play.

You're going to have to feel like Googling it if you want more information. SDL's sound capabilities are very low-level and not the best if you're just looking for fast and easy. You might need to use an add-on library like SDL_mixer to do more complex sound.

What is a good way to make the AI to lose? AT this moment is quit impossible for him to lose... I've made him move only when the ball gets into his side of the map, and even slowed him down quite a bit (relative to the player speed) but he looks almost like he teleports to the ball location,

As above, use speed. The player moves at a certain speed. The AI can have a differnet speed. Easier AI can move slower, meaning that if the player bounces the ball far away from where the AI currently has its paddle, it simply may not be able to move the paddle fast enough to stop the ball.

The speed use here is the same as above. If the player can move 1000 pixels per second, perhaps the AI can only move 700 pixels per second. Multiply by the time and that is as far as the AI is allowed to move in a single GameLogic update.

Sean Middleditch – Game Systems Engineer – Join my team!


How can improve the update method? Because now it is updating way to fast even with a SDL_Delay(10)

In addition to Sean's post above -- fix your timestep

Hello to all my stalkers.


1. Make a sound file.

2. Ask SDL to load the sound file into a sound object.

3. Tell SDL to play the sound object when you want the sound to start playing.

Repeat steps 1-2 for each unique sound in your game (ball hits paddle, ball hits wall, player wins, player loses, etc.). Repeat step 3 every time you want a sound to play.

You're going to have to feel like Googling it if you want more information. SDL's sound capabilities are very low-level and not the best if you're just looking for fast and easy. You might need to use an add-on library like SDL_mixer to do more complex sound.

its not that i didn't feel like googling... it was more of a tired thing... i was a bit tired yesterday and i wanted like a explanation first before i started messing with sound :D

About the other responses i really appreciate them...

My review by going through your code, and presenting an alternative way of writing it. Your code was retained as commented out sections, and I've sprinkled some notes throughout to (hopefully) explain my reasoning.
#include "SDL.h"
#include "SDL_ttf.h"
#include <iostream>
#include <stdlib.h>
#include <ctime>
#include <string>

using namespace std;

/*
rip-off: when you have a bunch of variables that share a name, consider grouping
         them into a structure or class.
 
const int Player_X = 20;
const int Player_Y = 250;

const int AI_X = 770;
const int AI_Y = 250;

int score_Player = 0;
int score_AI = 0;

SDL_Rect rect_Player;
SDL_Rect rect_AI;
*/

struct Player
{
    string name;
    int positionX;
    int positionY;
    int score;
    int velocity;

    Player()
    : 
        name("Unknown"),
        positionX(0),
        positionY(0),
        score(0),
        velocity(0)
    {
    }
};

/* 
const int BALL_X = 390;
const int BALL_Y = 290;

SDL_Rect rect_Ball;

rip-off: consider ballVelX and ballVelY rather than less specific names with an
         explanatory comment. A couple of screens down you won't be able to see
         the comment, but the variable name will always remind you what it's for

// Ball Velecities
int xVel;
int yVel;

*/
struct Ball
{
    int positionX;
    int positionY;
    int velocityX;
    int velocityY;

    Ball()
    :
        positionX(0),
        positionY(0),
        velocityX(0),
        velocityY(0)
    {
    }
};


const int SCREEN_WIDTH = 800;
const int SCREEN_HEIGHT = 600;

const int NUM_PLAYERS = 2;
const int HUMAN_PLAYER_INDEX = 0;
const int COMPUTER_PLAYER_INDEX = 1;

const int PADDLE_WIDTH = 10;
const int PADDLE_HEIGHT = 100;
const int PLAYER_SPEED = 10;

const int BALL_SIZE = 20;

/*
rip-off: Try to avoid global variables. They are convenient, but as your code
         base grows it will get harder and harder to understand exactly how and
         where they are used.

bool running;

//SDL STUFF
SDL_Window *window = NULL;
SDL_Renderer *renderer= NULL;
SDL_Event event;

//SDL_TTF STUFF
SDL_Color White = { 255, 255, 255 , 0};
TTF_Font* Sans;
*/

struct Game
{
    SDL_Window *window;
    SDL_Renderer *renderer;
    TTF_Font *font;
    Ball ball;
    Player players[NUM_PLAYERS];

    Game()
    :
        window(nullptr),
        renderer(nullptr),
        font(nullptr)
    {
    }
};

int Middle(int screen, int objectSize)
{
    return (screen - objectSize) / 2;
}

//Get random number between high and low
int GetRandomNumber(int high, int low)
{
    return rand() % high + low;
}

bool RandomBool()
{
    return rand() > (RAND_MAX / 2);
}

int RandomSign() {
    return RandomBool() ? 1 : -1;
}

//Reset the ball and the players to the default position and gives teh inicial boost to the ball
void ResetGame(Game &game)
{
    /*
    rect_Player.x = Player_X;
    rect_Player.y = Player_Y;
    
    rect_AI.x = AI_X;
    rect_AI.y = AI_Y;
    */

    // rip-off: Now that you have named variables, these values can be computed
    //          The meaning is much clearer: 390 looks arbitrary
    const int PADDLE_OFFSET_FROM_SIDE = 20;
    SDL_Point resetPositions[NUM_PLAYERS] = {
        { PADDLE_OFFSET_FROM_SIDE, Middle(SCREEN_HEIGHT, PADDLE_HEIGHT) },
        { SCREEN_WIDTH - PADDLE_OFFSET_FROM_SIDE - PADDLE_WIDTH, Middle(SCREEN_HEIGHT, PADDLE_HEIGHT) }
    };

    for (int i = 0 ; i < NUM_PLAYERS ; ++i)
    {
        Player &player = game.players[i];
        player.positionX = resetPositions[i].x;
        player.positionY = resetPositions[i].y;
    }

    /*
    rect_Ball.x = BALL_X;
    rect_Ball.y = BALL_Y;
    */
    Ball &ball = game.ball;
    ball.positionX = Middle(SCREEN_WIDTH, BALL_SIZE);
    ball.positionY = Middle(SCREEN_HEIGHT, BALL_SIZE);

    /*
    rip-off: Here, "2" is a magic number. Consider extracting it into a named
             variable to clarify.

    xVel = GetRandomNumber(2, -2);
    yVel = GetRandomNumber(2, -2);

    rip-off: Might be simpler to try do this directly rather than fix it later.
             Also the check for xVel >= 2 is unnecessary, as xVel >= 0 already
             handles such cases.
    
    //Horizontal speed is always 2 ou -2
    if (xVel >= 2 || xVel >=0)
    {
        xVel = 2;
    }
    if (xVel < 0)
    {
        xVel = -2;
    }
    */

    const int MAX_BALL_SPEED = 2;
    ball.velocityX = RandomSign() * MAX_BALL_SPEED;
    ball.velocityY = GetRandomNumber(MAX_BALL_SPEED, -MAX_BALL_SPEED);
}

// rip-off: It would be clearer if it were (x, y) rather than (a, b)
//
//Test if a point (a,b) is inside rect
bool PointInRect(int a, int b, SDL_Rect rect)
{
    if (a > rect.x && b > rect.y && a < rect.x + rect.w && b < rect.y + rect.h)
    {
        return true;
    }
    return false;
}

// rip-off: Minor, but it is "collision", not "colision".
//
//Test colision between rect1 and rect2
bool TestCollision(SDL_Rect rect1, SDL_Rect rect2)
{
    if (PointInRect(rect1.x, rect1.y, rect2) || PointInRect(rect1.x + rect1.w, rect1.y, rect2) || PointInRect(rect1.x, rect1.y + rect1.h, rect2) || PointInRect(rect1.x + rect1.w, rect1.y + rect1.h, rect2))
    {
        return true;
    }
    return false;
}

/*
rip-off: You've checked for errors, but you don't handle them correctly. You
         don't let the calling function know about the error, so the game
         will try to continue anyway, and probably crash...

         I've changed the function to return a boolean indicating success.
*/
//Starts the window, the renderer and sets the position of the PLayer, Ball and AI and loads the font for the score
bool LoadGame(Game &game)
{
    if (SDL_Init(SDL_INIT_EVERYTHING) != 0)
    {
        std::cout << "SDL_Init Error: " << SDL_GetError() << std::endl;
        return false;
    }
    // rip-off: This is a simple way to try ensure the SDL cleanup functions are
    //          always called.
    atexit(&SDL_Quit);
    
    if (TTF_Init() != 0)
    {
        std::cout << "TTF_Init Error: " << TTF_GetError() << std::endl;
        // rip-off: Like here, you didn't call "SQL_Quit()".
        return false;
    }
    atexit(&TTF_Quit);
    
    game.window = SDL_CreateWindow("Pong!", SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED, SCREEN_WIDTH, SCREEN_HEIGHT, SDL_WINDOW_OPENGL);
    if (game.window == nullptr)
    {
        std::cout << "SDL_CreateWindow Error: " << SDL_GetError() << std::endl;
        // rip-off: You remembered to SQL_Quit, but not to TTF_Quit!
        // SDL_Quit();
        return false;
    }
    
    game.renderer = SDL_CreateRenderer(game.window, -1, SDL_RENDERER_ACCELERATED );
    if (game.renderer == nullptr)
    {
        SDL_DestroyWindow(game.window);
        std::cout << "SDL_CreateRenderer Error: " << SDL_GetError() << std::endl;
        // SDL_Quit();
        return false;
    }
    
    // rip-off: "Sans" was a poor variable name. What if you wanted to change
    //          the font later, you probably don't want to change all the code 
    //          that uses it. So just call it something like "font" is simpler.
    //
    //            Also, all your other variables start with lowercase, so this was
    //          inconsistent...
    //
    //          Finally, there doesn't appear to be any error checking here.
    game.font = TTF_OpenFont("Sans.ttf", 19);
    if (game.font == nullptr)
    {
        SDL_DestroyWindow(game.window);
        std::cout << "TTF_OpenFont Error: " << TTF_GetError() << std::endl;
        // SDL_Quit();
        return false;
    }

    // rip-off: I've centralised the call to SDL_RenderPresent, as others have
    //          recommended

    SDL_SetRenderDrawColor(game.renderer, 0, 0, 0, SDL_ALPHA_OPAQUE);
    
    SDL_RenderClear(game.renderer);
    
    // SDL_RenderPresent(renderer);


    /*
    rip-off: more magic numbers, I've moved them to named constants above 
   
    rect_Player.h = 100;
    rect_Player.w = 10;
    
    rect_AI.h = 100;
    rect_AI.w = 10;
    
    rect_Ball.h = 20;
    rect_Ball.w = 20;
    rect_Ball.x = BALL_X;
    rect_Ball.y = BALL_Y;
    */
    
    srand((int)time(0));
    
    ResetGame(game);
    return true;
}

/* 
rip-off: More magic numbers here. Normally we would extract this into a named
         variable, but later we'll see how using an array has handled this for
         us!

//Keeps tabs on score x=1 - AI Scored! x=2 - Player Scored!
void Score(int x)
{
    if (x == 1)
    {
        score_Player += 1;
    }
    if (x == 2)
    {
        score_AI += 1;
    }
}
*/

// rip-off: extracting this makes it easier to avoid global variables
void HandleInput(Player &humanPlayer, bool &running)
{
    // rip-off: Here is an example of eliminating a global variable, this isn't
    //          used anywhere else!
    SDL_Event event;
    /*    
    rip-off: You need to check SDL_PollEvent's return value, otherwise you will
             run your event logic when nothing happened.

             Also, you should call SDL_PollEvent() in a loop, to consume all the 
             events since the previous frame. Mouse motion in particular can
             generate lots of events, and if you run the rest of the game logic
             in between, your game can feel sluggish to respond.
    */
    while(SDL_PollEvent(&event))
    {
        switch (event.type)
        {
            /*
            rip-off: As mentioned above, this code was running whether or not an
                     event had occurred. This probably worked to your advantage
                     before. Now that we're only reacting to events, we need to
                     store the player's intention to move so that the game logic
                     will continue to apply it beween frames...
            */
            case SDL_KEYDOWN:
                switch (event.key.keysym.sym)
                {
                    case SDLK_w:
                        // rect_Player.y -= 10;
                        humanPlayer.velocity -= PLAYER_SPEED;
                        break;
                    case SDLK_s:
                        // rect_Player.y += 10;
                        humanPlayer.velocity += PLAYER_SPEED;
                        break;
                }
                break;
            /*
            rip-off: Here, we need to "undo" the changes applied above. This
                     isn't the only way to do this, but this way avoids certain
                     bugs where the user presses "up" and without releasing it,
                     presses "down", then releases "up".
            */
            case SDL_KEYUP:
                switch (event.key.keysym.sym)
                {
                    case SDLK_w:
                        humanPlayer.velocity += PLAYER_SPEED;
                        break;
                    case SDLK_s:
                        humanPlayer.velocity -= PLAYER_SPEED;
                        break;
                }
                break;
            case SDL_QUIT:
                running = false;
                break;
        }
    }
}

SDL_Rect GetRect(const Player &player)
{
    SDL_Rect rect = { 
        player.positionX, 
        player.positionY, 
        PADDLE_WIDTH,
        PADDLE_HEIGHT
    };
    return rect;
}

SDL_Rect GetRect(const Ball &ball)
{
    SDL_Rect rect = { 
        ball.positionX, 
        ball.positionY, 
        BALL_SIZE, 
        BALL_SIZE 
    };
    return rect;
}

// rip-off: The original implementation of this function had a *lot* of magic 
//          numbers...
//
//Game logic - Movement of the ball and the players...colision tests and score update
void GameLogic(Game &game)
{
    // rip-off: as others have mentioned, Fix Your Timestep!
    //Delay so the game doesnt run too fast on my PC
    SDL_Delay(10);
    
    // rip-off: in your original code, you had the following order:
    //          1. Ball movement
    //          2. A.I. decisions & movement
    //          3. Player clamping
    //          4. Ball clamping
    //          5. A.I. clamping
    //
    //          It is easier to read and understand code if related things are
    //          grouped together

    //Ball Movement
    Ball &ball = game.ball;
    ball.positionX += ball.velocityX;
    ball.positionY += ball.velocityY;

    /*
    //Ball Top and Bottom limits
    if (rect_Ball.y < 1)
    {
        yVel = -yVel;
    }
    
    if (rect_Ball.y + rect_Ball.h > 599)
    {
        yVel = -yVel;
    }
    */
    if (ball.positionY <= 0 || ball.positionY + BALL_SIZE >= SCREEN_HEIGHT)
    {
        ball.velocityY *= -1;
    }

    for (int i = 0 ; i < NUM_PLAYERS ; ++i)
    {
        Player &player = game.players[i];
        player.positionY += player.velocity;

        /*
        //Player Top and Bottom limits
        if (rect_Player.y < 1)
        {
            rect_Player.y = 1;
        }
        
        if (rect_Player.y + rect_Player.h > 599)
        {
            rect_Player.y = 599 - rect_Player.h;
        }

        //AI Top and Bottom limits
        
        if (rect_AI.y < 1)
        {
            rect_AI.y = 1;
        }
        
        if (rect_AI.y + rect_AI.h > 599)
        {
            rect_AI.y = 599 - rect_AI.h;
        }
        */
        if (player.positionY <= 0)
        {
            player.positionY = 0;
        }
        else if (player.positionY + PADDLE_HEIGHT >= SCREEN_HEIGHT)
        {
            player.positionY = SCREEN_HEIGHT - PADDLE_HEIGHT;
        }
    }

    /*
    // Test colision Ball-Player
    if (TestColision(rect_Ball, rect_Player))
    {
        xVel = -xVel;
    }
    
    // Test colision Ball-AI
    if (TestColision(rect_Ball, rect_AI))
    {
        xVel = -xVel;
    }
    */
    SDL_Rect ballRect = GetRect(ball);
    for (int i = 0 ; i < NUM_PLAYERS ; ++i)
    {
        SDL_Rect playerRect = GetRect(game.players[i]);
        if (TestCollision(ballRect, playerRect))
        {
            ball.velocityX *= -1;
        }
    }
    
    /*
    //Ball Left and Right limits
    //When the ball touches the limits someone scored!
    //1 - The player
    //2 - The AI
    if (rect_Ball.x + rect_Ball.w > 801)
    {
        ResetGame();
        Score(1);
    }
    
    if (rect_Ball.x < -1)
    {
        ResetGame();
        Score(2);
    }
    */

    if (ball.positionX + BALL_SIZE >= SCREEN_WIDTH)
    {
        ResetGame(game);
        game.players[HUMAN_PLAYER_INDEX].score += 1;
    }
    
    if (ball.positionX <= 0)
    {
        ResetGame(game);
        game.players[COMPUTER_PLAYER_INDEX].score += 1;
    }
}

void HandleAI(Player &computer, const Ball &ball)
{
    /*
    rip-off: Maybe you wanted this, but it could be a bug due to having magic
             numbers, but the AI player had a speed of "3" whereas the player
             had a speed of "10"

    //AI Movement
    //AI starts moving only when ball gets to his half of the map
    if (rect_Ball.x > 400)
    {
        if (rect_AI.y + 0.5*rect_AI.h > rect_Ball.y + 0.5*rect_Ball.h)
        {
            rect_AI.y -= 3;
        }
        
        rip-off: repeating expressions like this makes the code long and could
                 be a source of bugs where you update one but not the other.

        if (rect_AI.y + 0.5*rect_AI.h < rect_Ball.y + 0.5*rect_Ball.h)
        {
            rect_AI.y += 3;
        }
    }
    */ 

    computer.velocity = 0;
    if (ball.positionX > SCREEN_WIDTH / 2)
    {
        int computerMidpoint = computer.positionY + (PADDLE_HEIGHT / 2);
        int ballMidpoint = ball.positionY + (BALL_SIZE / 2);

        if (computerMidpoint > ballMidpoint)
        {
            computer.velocity = -PLAYER_SPEED;
        }
        
        if (computerMidpoint / 2 < ballMidpoint)
        {
            computer.velocity = PLAYER_SPEED;
        }
    }
}

// rip-off: When you find yourself repeating code, then you have a candidate for
//          a new function!
void DrawScore(Game &game, Player &player, SDL_Point position)
{
    string text = player.name + ": " + to_string(player.score);
    
    // rip-off: Another global variable turned local!
    SDL_Color colour = { 255, 255, 255, 0};
    SDL_Surface* textSurface = TTF_RenderText_Solid(game.font, text.c_str() , colour);
    // rip-off: Remember to check for errors, particular in cases here where the
    //          resulting null pointer could crash your program if not noticed.
    //          Here we just log the error...
    if (!textSurface)
    {
        std::cerr << "Error creating texture from surface: " << SDL_GetError() << std::endl;
        return;
    }

    SDL_Texture* textTexture = SDL_CreateTextureFromSurface(game.renderer, textSurface);
    SDL_FreeSurface(textSurface);

    if (textTexture)
    {
        SDL_Rect drawLocation = { position.x, position.y, textSurface->w, textSurface->h };
        SDL_RenderCopy(game.renderer, textTexture, NULL, &drawLocation);
        SDL_DestroyTexture(textTexture);
    } 
    else
    {
        std::cerr << "Error creating texture from surface: " << SDL_GetError() << std::endl;
    }
}

//Renders the Rects into the window
void DrawScreen(Game &game)
{
    //Renders the Ball the Player and the AI
    SDL_SetRenderDrawColor(game.renderer, 0, 0, 0, 255);
    SDL_RenderClear(game.renderer);
    SDL_SetRenderDrawColor(game.renderer, 255, 255, 255, 255);

    /*    
    SDL_RenderFillRect(renderer, &rect_Player);
    
    SDL_RenderPresent(renderer);
    
    SDL_RenderFillRect(renderer, &rect_AI);
    
    SDL_RenderPresent(renderer);
    */
    for (int i = 0 ; i < NUM_PLAYERS ; ++i)
    {
        SDL_Rect playerRect = GetRect(game.players[i]);
        SDL_RenderFillRect(game.renderer, &playerRect);
    }

    /*
    SDL_RenderFillRect(renderer, &rect_Ball);
    
    SDL_RenderPresent(renderer);
    */
    SDL_Rect ballRectangle = GetRect(game.ball);
    SDL_RenderFillRect(game.renderer, &ballRectangle);

    /*
    
    //PLAYER_SCORE
    
    string PScore = "Player: " + to_string(score_Player);
    
    SDL_Surface* scorePlayerMessage = TTF_RenderText_Solid(Sans, PScore.c_str() , White);
    
    SDL_Texture* playerMessage = SDL_CreateTextureFromSurface(renderer, scorePlayerMessage);
    
    int Pw = scorePlayerMessage->w;
    int Ph = scorePlayerMessage->h;
    
    SDL_FreeSurface(scorePlayerMessage);
    
    SDL_Rect rect_PlayerScore = { 145, 10, Pw, Ph };
    
    SDL_RenderCopy(renderer, playerMessage, NULL, &rect_PlayerScore);
    // SDL_RenderPresent(renderer);
    
    SDL_DestroyTexture(playerMessage);
    
    //AI_SCORE
    
    std::string AScore = "AI: " + std::to_string(score_AI);
    
    SDL_Surface* scoreAIMessage = TTF_RenderText_Solid(Sans, AScore.c_str(), White);
    
    SDL_Texture* AIMessage = SDL_CreateTextureFromSurface(renderer, scoreAIMessage);
    
    int Aw = scoreAIMessage->w;
    int Ah = scoreAIMessage->h;
    
    SDL_FreeSurface(scoreAIMessage);
    
    SDL_Rect rect_AIScore = { 390+145, 10, Aw, Ah };
    
    SDL_RenderCopy(renderer, AIMessage, NULL, &rect_AIScore);
    // SDL_RenderPresent(renderer);
    
    SDL_DestroyTexture(AIMessage);
    */

    // rip-off: I'm not sure where the "390" offset came from, it looks related
    //          to SCREEN_WIDTH / 2 but there is an additional 10 pixels that 
    //          still need explaining...
    const int SCORE_TEXT_OFFSET_X = 145;
    const int SCORE_TEXT_OFFSET_Y = 10;
    SDL_Point scorePositions[NUM_PLAYERS] = {
        { SCORE_TEXT_OFFSET_X, SCORE_TEXT_OFFSET_Y },
        { 390 + SCORE_TEXT_OFFSET_X, SCORE_TEXT_OFFSET_Y }
    };

    for (int i = 0 ; i < NUM_PLAYERS ; ++i)
    {
        DrawScore(game, game.players[i], scorePositions[i]);
    }

    // rip-off: As others have mentioned, try to have a single place where this
    //          is performed.
    SDL_RenderPresent(game.renderer);
}

//Update method... too fast
void Update(Game &game)
{
    bool running = true;
    while (running) //game loop
    {
        HandleInput(game.players[HUMAN_PLAYER_INDEX], running);
        HandleAI(game.players[COMPUTER_PLAYER_INDEX], game.ball);

        GameLogic(game); //update stuff;
        DrawScreen(game); //render your stuff
    }
}

//Quits the game
void Quit(Game &game)
{
    SDL_DestroyRenderer(game.renderer);
    SDL_DestroyWindow(game.window);
    /*
    rip-off: these are now automatically called

    TTF_Quit();
    SDL_Quit();
    */
}

//Main
int main(int argc, char* args[])
{   
    Game game;
    game.players[HUMAN_PLAYER_INDEX].name = "Player";
    game.players[COMPUTER_PLAYER_INDEX].name = "AI";

    if (!LoadGame(game))
    {
        // rip-off: It is good practise to signal to the external environment if
        //          an error occurred.
        return 1;
    }
    Update(game);
    Quit(game);
}
Note that while the code should compile (it did on my machine), I've not run it so I may have introduced bugs or surprising changes in behaviour.

Many thanks rip-off!

That's exactly what i wanted to hear. i'll read and study your notes tomorrow and i'll come back if i have any doubts about anything.

Just wanted to say that the collision typo has occurred because i'm not an English native speaker .

This topic is closed to new replies.

Advertisement