HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Collision-detection for an SDL game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
sdlcollisiongamefordetection

Problem

I am working on a simple SDL game in C++. I want to make my collision detection implementation better.

Here's the complete project:

```
#include
#include
#include
#include

const int SCREEN_WIDTH = 512;
const int SCREEN_HEIGHT = 512;

bool initialize_game();
bool load_game();
void close_game();
SDL_Texture* load_texture(std::string path);

bool check_collision(SDL_Rect a, SDL_Rect b);

SDL_Texture* playerOneTexture = NULL;
SDL_Window* globalWindow = NULL;
SDL_Renderer* globalRenderer = NULL;

class Timer
{
public:
Timer();

void start_timer();
void stop_timer();
bool is_started();

Uint32 get_ticks();

private:
Uint32 startTicks;
Uint32 pausedTicks;

bool isPaused;
bool isStarted;
};

class Player
{
public:
const int PLAYER_WIDTH = 32;
const int PLAYER_HEIGHT = 32;
const int PLAYER_VELOCITY = 160;

Player();

void move_player(float timeStep, SDL_Rect objectWeHit);
void render_player(float positionX, float positionY, SDL_Texture texture, SDL_Rect clip = NULL, double angle = 0.0, SDL_Point* center = NULL, SDL_RendererFlip flip = SDL_FLIP_NONE);
void handle_player_event(SDL_Event &event);

float positionX, positionY;
float velocityX, velocityY;

private:
SDL_Rect playerCollider;
};

Timer::Timer()
{
startTicks = 0;
pausedTicks = 0;

isPaused = false;
isStarted = false;
}

void Timer::start_timer()
{
isStarted = true;
isPaused = false;

startTicks = SDL_GetTicks();
pausedTicks = 0;
}

void Timer::stop_timer()
{
isStarted = false;
isPaused = false;

startTicks = 0;
pausedTicks = 0;
}

Uint32 Timer::get_ticks()
{
Uint32 time = 0;

if(isStarted){
time = SDL_GetTicks() - startTicks;
}
return time;
}

bool Timer::is_started()
{
return isStarted;
}

Player::Player()
{
positionX = 0.0;
positionY = 0.0;

velocityX = 0.

Solution

You have implemented your own rectangle collision check routine when you really didn't have to. SDL provides the function SDL_IntersectRect() which tests two SDL_Rects for intersection and also returns the intersection rect of A and B.

Now lets talk about coding practices a little bit.

It looks like you are an adept of the single return statement. In the initialize_game() function, for instance, it has lead to a lot of juggling and flag checking to ensure the single return. In that case, it only made the code more complex. You should have just done it like so:

if(SDL_Init(SDL_INIT_VIDEO) < 0) {
    return false;
}

... stuff ...

if(globalWindow == NULL) {
    return false;
}


Prefer to return early and avoid deep nesting. It makes logic easier to follow. The single return technique is hardly applicable to modern C++. The RAII idiom covers dynamic resource management inside functions.

Other minor things:

-
NULL is not C++! NULL is a C library macro. Modern C++ should use nullptr.

-
Did you code everything up in the same file? If so, it is probably time to move each class to it's own module.

-
Give std::cout a try. You might end up liking it ;).

Code Snippets

if(SDL_Init(SDL_INIT_VIDEO) < 0) {
    return false;
}

... stuff ...

if(globalWindow == NULL) {
    return false;
}

Context

StackExchange Code Review Q#67887, answer score: 7

Revisions (0)

No revisions yet.