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.