SDL/C++ Pong clone

Question

This is my first time writing a game in C++/SDL and I decided to post the code here so you can tell me what I am doing wrong.

main.cpp

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

int screenWidth = 640;
int screenHeight = 480;
const char* title = "Pong Clone";

Game game;

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

game.init(title, SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED, screenWidth, screenHeight, false);

while (game.running())
{
    game.eventHandler();
    game.render();
}
return 0;
}

Game.h

#ifndef GAME_H
#define GAME_H

#include <SDL.h>
#include <iostream>
#include <string>
#include "GameObject.h"

class Game
{
public:
bool init(const char* title, int xPos, int yPos, int width, int height,     bool flags);
void eventHandler();
void render();
void clean();
void collision();
void reset();

bool running() { return m_Running; }

private:
SDL_Window* window;
SDL_Renderer* renderer;

int screenWidth;
int screenHeight;

int xSpeed, ySpeed;

bool m_Running;

GameObject* paddle;
GameObject* ball;
};

#endif

Game.cpp

#include "Game.h"
#include <stdlib.h>
#include <time.h>

//player speed
int playerVelocity = 0;
int playerSpeed = 15;


bool Game::init(const char* title, int xPos, int yPos, int width, int     height, bool flags)
{
// screen and renderer initialization
screenWidth = width;
screenHeight = height;
window = SDL_CreateWindow(title, xPos, yPos, screenWidth, screenHeight, flags);
if (window == nullptr)
{
    std::cout << "SDL_CreateWindow Error: " << SDL_GetError() << std::endl;
}

renderer = SDL_CreateRenderer(window, -1, SDL_RENDERER_ACCELERATED | SDL_RENDERER_PRESENTVSYNC);
if (renderer == nullptr)
{
    std::cout << "SDL_CreateRenderer Error: " << SDL_GetError() << std::endl;
}
SDL_SetRenderDrawColor(renderer, 0, 0, 0, 255);

//game objects
ball = new GameObject();
paddle = new GameObject();

//image load
ball->load("Assets/ball.png", "ball", renderer);
paddle->load("Assets/paddle.png", "paddle", renderer);

//starting speed and directions
xSpeed = rand() % 8 + 5;
ySpeed = rand() % 8 + 5;

reset();

m_Running = true;

return true;
}

void Game::reset()
{
// center screen position
ball->setPosition((screenWidth - (ball->getW() / 2)) / 2, (screenHeight - (ball->getH() / 2)) / 2);

}

void Game::eventHandler()
{
// ball speed
ball->setX(ball->getX() + xSpeed);
ball->setY(ball->getY() - ySpeed);

// game loop
SDL_Event event;
if (SDL_PollEvent(&event))
{
    switch(event.type)
    {
    case SDL_QUIT:
        {
            m_Running = false;
            break;
        }

    case SDL_KEYDOWN:
        {
            switch( event.key.keysym.sym )
            {
            case SDLK_UP:
                {
                    playerVelocity = -playerSpeed;
                    break;
                }

            case SDLK_DOWN:
                {
                    playerVelocity = playerSpeed;
                    break;
                }

            default:
                break;
            }
            break;
        }

    case SDL_KEYUP:
        {
            switch( event.key.keysym.sym )
            {
            case SDLK_UP:
                {
                    if (playerVelocity < 0)
                        playerVelocity = 0;
                    break;
                }

            case SDLK_DOWN:
                {
                    if (playerVelocity > 0)
                        playerVelocity = 0;
                    break;
                }

            default:
                break;
            }
            break;
        }

    default:
        break;
    }
}
paddle->setY(paddle->getY() + playerVelocity);

collision();
}

void Game::render()
{
// render game objects
SDL_RenderClear(renderer);
ball->draw(renderer, "ball", ball->getX(), ball->getY());
paddle->draw(renderer, "paddle", paddle->getX(), paddle->getY());
SDL_RenderPresent(renderer);
}

void Game::collision()
{
// top collision
if (ball->getY() < 0)
{
    ySpeed = -ySpeed;
}

// paddle collision
else if (ball->getX() <= paddle->getX() + paddle->getW() && 
    (ball->getY() >= paddle->getY() && ball->getY() <= paddle->getY() + paddle->getH()))
{

    xSpeed = -xSpeed;
}

// bottom collision
else if (ball->getY() >= (screenHeight - ball->getH()))
{
    ySpeed = -ySpeed;
}

// player collision
else if (ball->getX() >= (screenWidth - ball->getW()))
{
    xSpeed = -xSpeed;
}

// reset ball location
else if (ball->getX() < 0)
{
    xSpeed = rand() % 8 + 5;
    ySpeed = rand() % 8 + 5;
    reset();
}
}

GameObject.h

#ifndef GAMEOBJECT_H
#define GAMEOBJECT_H

#include <SDL.h>
#include <SDL_image.h>
#include <string>
#include <iostream>

class GameObject
{
public:
bool load(std::string filename, std::string id, SDL_Renderer* renderer);
void draw(SDL_Renderer* renderer, std::string id, int x, int y);
void setX(int x);
void setY(int y);
int getX();
int getY();
int getW();
int getH();

void setPosition(int x, int y);

private:
    SDL_Renderer* renderer;
    SDL_Surface* tempImage;
    SDL_Texture* texture;
    SDL_Rect srcRect;
    SDL_Rect dstRect;
};

#endif

GameObject.cpp

#include "GameObject.h"

bool GameObject::load(std::string filename, std::string id, SDL_Renderer* renderer)
{
    // image load manager
    tempImage = IMG_Load(filename.c_str());
    texture = SDL_CreateTextureFromSurface(renderer, tempImage);
    SDL_FreeSurface(tempImage);

    SDL_QueryTexture(texture, NULL, NULL, &srcRect.w, &srcRect.h);

    dstRect.w = srcRect.w;
    dstRect.h = srcRect.h;

    return true;
}


void GameObject::draw(SDL_Renderer* renderer, std::string id, int x, int y)
{
    // image draw
    dstRect.x = x;
    dstRect.y = y;

    SDL_RenderCopy(renderer, texture, &srcRect, &dstRect);
}

void GameObject::setX(int x)
{
    dstRect.x = x;
}

void GameObject::setY(int y)
{
    dstRect.y = y;
}

int GameObject::getX()
{
    return dstRect.x;
}

int GameObject::getY()
{
    return dstRect.y;
}

int GameObject::getW()
{
    return dstRect.w;
}

int GameObject::getH()
{
    return dstRect.h;
}

void GameObject::setPosition(int x, int y)
{
    dstRect.x = x;
    dstRect.y = y;
}

I'm still working on AI/2^nd player and score.

My main problem right now is: how do I correctly randomize the starting direction of the ball? How can I increase the ball speed after each bounce?

Answer 1

This is just a quick partial review. I can't comment on the SDL parts because I know very little about SDL.

main.cpp

Avoid global mutable state. All of the variables shown below could be made local.

int screenWidth = 640;
int screenHeight = 480;
const char* title = "Pong Clone";

Game game;

If for some reason, you want them in a global area, at least make them constants. Since these are in a .cpp file and not a .h, you can mark them to be static. This gives them internal linkage, which basically keeps them 'private' to the file. Of course, if you want to extern them, then do not do this.

static const int SCREEN_WIDTH = 640;
static const int SCREEN_HEIGHT = 480;
static const char* TITLE = "Pong Clone";

An alternative to static global variables, is putting them into an unnamed namespace.

namespace {
    const int SCREEN_WIDTH = 640;
    const int SCREEN_HEIGHT = 480;
    const char* TITLE = "Pong Clone";
}

If you ever need to put these values into a header file, I would place them into a class or named-namespace. If you do the former (put them in a class), it's okay to make them static. If you do the latter (put them in a named-namespace), do not make them static or else every translation unit will have its own private copy of the variable.

Game.h

Remove #include <iostream> and #include <string>. This particular header file doesn't use them at all.

Two-step initialization is old-fashioned (though admittingly still used). Your init() function could be turned into a constructor instead.

Game (const char* title, int xPos, int yPos, int width, int height, bool flags);

The const char* title should also be changed to const std::string &title, but I'll talk about that later.

I'm not seeing any reason why the objects below are pointers.

GameObject* paddle;
GameObject* ball;

Save yourself the headache of pointless memory management in this case and just make them objects.

GameObject paddle;
GameObject ball;

Game.cpp

Let's go back to my previous point of using std::string instead of char*.

bool Game::init(const char* title,//...
// ... More code
window = SDL_CreateWindow(title,//...

Since you are not checking to see if title is nullptr, it would be better to use const std::string &title instead.

If you choose to keep init() instead of creating a constructor, then I'm guessing these two if-statements should return false.

window = SDL_CreateWindow(title, xPos, yPos, screenWidth, screenHeight, flags);
if (window == nullptr)
{
    std::cout << "SDL_CreateWindow Error: " << SDL_GetError() << std::endl;
    return false; // probably?
}

renderer = SDL_CreateRenderer(window, -1, SDL_RENDERER_ACCELERATED | SDL_RENDERER_PRESENTVSYNC);
if (renderer == nullptr)
{
    std::cout << "SDL_CreateRenderer Error: " << SDL_GetError() << std::endl;
    return false; // probably?
}

Checking error codes is also somewhat old fashioned. I would recommend throwing exceptions instead.

I see that you are using rand() right here.

//starting speed and directions
xSpeed = rand() % 8 + 5;
ySpeed = rand() % 8 + 5;

I do not see you using srand() anywhere though. Without srand(), your random number generator seed will always be the same and you will always get the same results.

Even better, since you are using C++11, I would recommend using functions from the new <random> header. Since you are looking for a random number in the range [5,12], here is a rough example:

// Be sure to #include <chrono>
auto seed = std::chrono::system_clock::now ().time_since_epoch ().count () ;

// Be sure to #include <random> 
typedef std::default_random_engine::result_type seed_type ;
std::default_random_engine generator (static_cast <seed_type> (seed)) ;
std::uniform_int_distribution <int> distribution (5, 12) ;

xSpeed = distribution (generator) ;
ySpeed = distribution (generator) ;

You would probably want to create the std::default_random_engine only once though, or else you'll be constantly reseeding it.

Answer 2

General comments:

• You have inconsistent indentation. The statements in the outermost block scope in the functions in GameObject.cpp are indented four spaces, while everywhere else they're not indented at all. The declarations in the private section of GameObject.h are indented, but the public section isn't.

File main.cpp:

• You're not using the arguments to main(), so you could use the alternative form that doesn't declare them:

  int main()
  

• The function Game::init() returns a bool (it always returns true, but presumably it's intended to indicate success or failure), but you're not checking the return value here. However, I'd go even more C++ and use a constructor for Game and use exceptions to indicate an error during construction.

• I don't see a reason for having the Game object defined outside of main(); I'd make it a local variable within your main function.

File Game.h:

• The member function running() doesn't modify the instance in any way, so you should add a const specifier to it:

  bool running() const { return m_Running; }
  

File Game.cpp:

• You have two global variables, playerSpeed and playerVelocity. Since they're used exclusively within the Game class' methods, they should be members of the class. playerSpeed is never changed, so it should be declared as a const int. Also consider different names, since the words "speed" and "velocity" are very close to being synonyms; perhaps maximumSpeed and actualSpeed respectively?

• Consider adding a constructor for the Game class, instead of having a separate init() method that initializes all the class-level variables. The compiler will generate a constructor for you if you don't provide one, but that means that you get default initialization of all the member variables, followed by re-initialization when you call the init() method. If you switch to using a constructor, you should probably also look into using exceptions to report errors to the constructor.

• In Game::init() you have this code:

  window = SDL_CreateWindow(title, xPos, yPos, screenWidth, screenHeight, flags);  
  if (window == nullptr)
  {
      std::cout << "SDL_CreateWindow Error: " << SDL_GetError() << std::endl;
  }
  
  renderer = SDL_CreateRenderer(window, -1, SDL_RENDERER_ACCELERATED | SDL_RENDERER_PRESENTVSYNC);
  if (renderer == nullptr)
  {
      std::cout << "SDL_CreateRenderer Error: " << SDL_GetError() << std::endl;
  }
  SDL_SetRenderDrawColor(renderer, 0, 0, 0, 255);
  

  1. By convention, errors should go to std::cerr; normal output should go to std::cout.
  2. Even though you check if the SDL_CreateWindow call fails, you still go on to use the returned value in the renderer = ... line. Same for the call to SDL_CreateRenderer, where you use the value in the SDL_SetRenderDrawColor color. I'm not familiar with SDL so I don't know if it will handle nullptr argument values gracefully, but this looks suspicious.

• You're using nullptr, so you must be using C++11. You're using new to allocate a couple of instances of your GameObject class; in C++11, the recommendation is to use std::unique_ptr to manage instances of raw pointers. So you would have:

  // In Game.h:
  std::unique_ptr<GameObject> ball; 
  // In Game.cpp
  ball = std::unique_ptr<GameObject>(new GameObject());
  

  The unique_ptr will automatically call delete the objects they manage when the containing class is destroyed. If you don't want to use unique_ptr, you need to define a destructor for the Game class and delete the GameObject objects there:

  Game::~Game()
  {
      delete ball;
      delete paddle;
  }
  

• In Game::event_handler(), you have code like:

  // ball speed
  ball->setX(ball->getX() + xSpeed);
  ball->setY(ball->getY() - ySpeed);
  

  Consider changing GameObject's interface to updateX() and updateY() that takes just the offset, and let the GameObject class worry about the details of changing its X- or Y-coordinates.

File GameObject.h:

• Member functions GameObject::SetX(), SetY(), GetX(), GetY(), GetW(), GetH() are all one-liner functions. For simple setters and getters like these, it's often better to define them in the header file so that (a) you don't have to look for them in two places, and (b) the compiler can inline them wherever they're used, meaning that instead of making a function call, it can simply insert the function's code directly at the point of use.

• The getters should be declared const (like Game::running() above):

  class GameObject 
  {
      // ...code elided...
      int GetX() const { return dstRect.x; }
      // ...remaining getters...
  };
  

File GameObject.cpp:

• Consider adding a constructor for GameObject instead of having a separate load() method; see the equivalent comment for Game.cpp above for the reason why.

• In function GameObject::load(), the variable tempImage is used. It is declared at class scope, but it doesn't seem to be used outside of this function. If that's true, you could move the declaration into this function, keeping it closer to its use. It also means that you don't have a class-level variable that isn't valid outside of the one function where it's used.

• The same applies for the class-level srcRect which is only used in this function. You also initialize srcRect in the call to SDL_QueryTexture(), then copy the values into dstRect immediately afterwards; could you use dstRect in the call, thus eliminating the need for srcRect?

• Functions GameObject::load() and GameObject::draw() take an id argument that they never use.

• Move all the setters and getters into the header file, as mentioned above.

Answer 3

Please keep your indentation consistent. You are correctly indenting within many curly brace blocks, but not all of them. You should especially indent all code within functions, classes, and structs, so that any indented code blocks within the function can be distinguished. It also makes it harder to line up the curly braces. One example of this inconsistency is in main(). If two or more closing braces are lined up vertically, that's a good indication that something is not correctly indented.