• Advertisement
Sign in to follow this  

Splitting up project in .h and .cpp

This topic is 3608 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I must say I still don't quite understand how to manage my code in the correct files. My code doesn't do anything yet, I just started this project, but I want to organize my files correctly from the start on. This is how I've tried it untill now: main.cpp
/**
  * PongSDL
  * Reinventing the wheel
  *
**/

#include <cstdlib>

#include "graphics.h"


int main ( int argc, char* argv[] )
{
    Gfx gfx;
    gfx.init();

    return 0;
}

/**
  * End of main.cpp
  *
**/

graphics.h
/**
  * graphics.h
  *
**/


class Gfx
{
public:

    Gfx();
    ~Gfx();

    int init();

    int draw();

private:

    SDL_Surface* screen;

};



/**
  * End of graphics.h
  *
**/

graphics.cpp
/**
  * graphics.cpp
  *
**/


#include "graphics.h"

#include <SDL/SDL.h>


const int GFX_SCREEN_W = 600;
const int GFX_SCREEN_H = 400;


Gfx::Gfx()
{
    *screen = NULL;
}

Gfx::~Gfx()
{
    SDL_FreeSurface( screen );
}


int Gfx::init()
{
    if ( SDL_Init( SDL_INIT_VIDEO ) < 0 )
    {
        printf( "Unable to init SDL: %s\n", SDL_GetError() );
        return -1;
    }

    SDL_WM_SetCaption( "PongSDL", NULL );

    *screen = SDL_SetVideoMode( GFX_SCREEN_W, GFX_SCREEN_H, 32, SDL_SWSURFACE );

    if ( !screensurface )
    {
        printf( "Unable to set video: %s\n", SDL_GetError() );
        return -1;
    }

    return 1;
}



/**
  * End of graphics.cpp
  *
**/

The error:
graphics.h:20: error: ISO C++ forbids declaration of `SDL_Surface' with no type
graphics.h:20: error: expected `;' before '*' token
Can someone tell me how to organize my files properly? Thanks. PS: I'm using CPP, but for some reason GameDev gets rid of the two plusses in my post.

Share this post


Link to post
Share on other sites
Advertisement
You need to #include <SDL/SDL.h> in your graphics.h header.

Share this post


Link to post
Share on other sites
Your graphics.h has no way of knowing what SDL_Surface means. To fix the problem, you should move the inclusion of SDL.h to the beginning of graphics.h.

File inclusion in C/C++ has some quirks though, for which reason you should really use include guards. It's very simple, really. Just do the following:
// blarg.h
// Class that does awesome stuff

#ifndef BLARG_H
#define BLARG_H

// Your code and includes goes here
#include <SDL.h>

class Blarg {
// Stuff
};

#endif // BLARG_H


This ensures that your code is only parsed once, no matter how many times the file is included from the same compilation unit.

Share this post


Link to post
Share on other sites
That error occurs in your graphics.cpp file, when you include #include "graphics.h". You are doing that before including #include <SDL/SDL.h>, so when it sees SDL_Surface, it doesn't know what it is.

You should include the standard includes before your own headers, like you've correctly done in main.cpp.

So your graphics.cpp file should look like:
/**
* graphics.cpp
*
**/

#include <SDL/SDL.h>

#include "graphics.h"

...
I suggest you read this article if you haven't already. It may not be the best, but it's the only one I know of.

http://www.gamedev.net/reference/articles/article1798.asp

Share this post


Link to post
Share on other sites
You need to put #include <SDL/SDL.h> into the beginning graphics.h, because you refer to types defined there. Also, you should add guards to your header files.

Put this right into the beginning of graphics.h, before anything else:

#ifndef GRAPHICS_H
#define GRAPHICS_H



And this comes right at the end after everything else:

#endif // GRAPHICS_H



If you do not have these guards, and you include graphics.h twice, the compiler will complain of doubly defined classes and such.

Share this post


Link to post
Share on other sites
Including SDL in the header file solved my problem.
Including SDL before the header in the cpp file did not solve my problem. I chose to just include it in the header aswell.
Thanks a lot for the tips on the guards, this will probably save me lots of trouble later on.
I remember seeing that article somewhere sometime, but I couldn't fid it anymore. I bookmarked it this time :)

Thanks for all your replies

Share this post


Link to post
Share on other sites
I still have many problems... Google made me a little wisers and taught me how to use the extern operator, but it doesn't always work.

snippet of game.h
/**
* game.h
*
**/



#ifndef GAME_H
#define GAME_H

#include "graphics.h" // The game passes commands to the graphics
#include <SDL/SDL.h> // For SDL_Event
#include "ball.h" // The game has a ball
#include <vector> // Sometimes more than one ball

// ...

struct Gamestate
{
public:

std::vector<Ball> vball;

};

// ...

#endif // GAME_H

snippet of graphics.h
/**
* graphics.h
*
**/



#ifndef GFX_H
#define GFX_H

#include <SDL/SDL.h> // For SDL_Video
#include "game.h" // Needs information about game elements to draw

// ...

class Gfx
{
public:

Gfx();
~Gfx();

int init();

void draw( Gamestate* state ); // <- Error is here

private:

SDL_Surface* screen;

void drawPixel( int x, int y );

};

// ...

#endif // GFX_H


I get the following error:
graphics.h:24: error: `Gamestate' has not been declared
graphics.h:24: error: ISO C++ forbids declaration of `state' with no type
graphics.h:24: note: candidates are: void Gfx::draw(int*)


I can't use the extern keyword for struct though! What am I doing wrong?

Share this post


Link to post
Share on other sites
Here you need a forward declaration. See the section "Fixing Problem 2" in the above article.

Share this post


Link to post
Share on other sites
Hmmm, this is going very wrong already... I don't understand the idea behind proper splitting up of files. I started programming Pong to teach myself this. Could you please look at my code and tell me how I should split things up and what needs to include what with a little explanation. Thanks in forward

main.cpp
/**
* PongSDL
* main.cpp
*
**/




#include "game.h"



int main ( int argc, char* argv[] )
{
Game pong;

if( pong.mainMenu() )
pong.play();


pong.~Game();

return 0;
}



/**
* End of main.cpp
*
**/



game.h
/**
* game.h
*
**/



#ifndef GAME_H
#define GAME_H

#include <SDL/SDL.h> // For SDL_Event
#include "graphics.h" // We need some graphics to play
#include "ball.h" // The game has a ball



struct Gamestate
{
public:

Ball ball;

};



class Game
{
public:

Game();
~Game();

bool mainMenu();
int play();

private:

Gfx gfx;
SDL_Event event;
Gamestate* current_state;
Gamestate* previous_state;


};



extern Ball newBall( int x, int y, float velocity, float angle, float rad );
extern void updateBall( Ball* b, double dt );



#endif // GAME_H

/**
* game.h
*
**/



game.cpp
/**
* game.cpp
*
**/



#include "game.h"
#include <SDL/SDL.h> // For SDL_Event
#include "timer.h"
#include "ball.h"



void process( Gamestate* state, double t, double dt )
{
// Move ball
updateBall( &state->ball, dt );

// Todo: Rest of processing, eg. collision detection
}



Game::Game()
{
gfx.init();
}

Game::~Game()
{
gfx.~Gfx();
}



bool Game::mainMenu()
{
return true;

// Placeholder for a future main menu

}


int Game::play()
{
Timer timer;
const float DELTA = 0.01;

previous_state = new Gamestate;
current_state = new Gamestate;

current_state->ball = newBall( 100, 100, 0.1, 0.0, 5.0 );


bool loop = true;

while( loop ) {

timer.fillAccu();

while( timer.pinchAccu( DELTA ) ) {

// Process here
*previous_state = *current_state;
process( current_state, timer.getTimePassed(), DELTA );

} // End of processing


while( SDL_PollEvent( &event ) ) {

if( event.type == SDL_QUIT )
loop = false;

} // End of event polling


gfx.draw( current_state );


} // End of main loop


return 0;
}



/**
* End of game.cpp
*
**/



graphics.h
/**
* graphics.h
*
**/



#ifndef GFX_H
#define GFX_H

#include <SDL/SDL.h> // For SDL_Video

struct Gamestate; // Forward declaration



class Gfx
{
public:

Gfx();
~Gfx();

int init();

void draw( Gamestate* state );

private:

SDL_Surface* screen;

void drawPixel( int x, int y );

};



const int GFX_SCREEN_W = 600;
const int GFX_SCREEN_H = 400;



#endif // GFX_H

/**
* End of graphics.h
*
**/



graphics.cpp
/**
* graphics.cpp
*
**/



#include "graphics.h"
#include <SDL/SDL.h> // For SDL_Video
#include "game.h" // To interact with game elements
#include <math.h> // For sqrt()



Gfx::Gfx()
{
screen = 0;
}

Gfx::~Gfx()
{
SDL_FreeSurface( screen );
}


int Gfx::init()
{
if( SDL_Init( SDL_INIT_VIDEO ) < 0 )
{
printf( "Unable to init SDL: %s\n", SDL_GetError() );
return -1;
}

SDL_WM_SetCaption( "PongSDL", NULL );

screen = SDL_SetVideoMode( GFX_SCREEN_W, GFX_SCREEN_H, 32, SDL_SWSURFACE );

if( !screen )
{
printf( "Unable to set video: %s\n", SDL_GetError() );
return -1;
}

return 1;
}


void Gfx::draw( Gamestate* state )
{
// WIP
}




void Gfx::drawPixel( int x, int y )
{
SDL_Rect rect;
rect.x = x;
rect.y = y;
rect.w = 1;
rect.h = 1;

SDL_FillRect( screen, &rect, SDL_MapRGB( screen->format, 0xFF, 0xFF, 0xFF ) );
}



/**
* End of graphics.cpp
*
**/



timer.h
/**
* timer.h
*
**/



#ifndef TIME_H
#define TIME_H



// Time is measured in seconds
class Timer
{
public:

Timer();
//~Timer();

void fillAccu();
bool pinchAccu( float dt );
float getTimePassed();

private:

float time_passed;
float current_time;
float accumulator;
float new_time;
float delta_time;

};



#endif // TIME_H

/**
* End of timer.h
*
**/



timer.cpp
/**
* timer.cpp
*
**/



#include "timer.h"
#include <SDL/SDL.h> // For SDL_GetTicks()



Timer::Timer()
: time_passed(0.00),
accumulator(0.00),
new_time(0.00),
delta_time(0.00)
{
current_time = (float)SDL_GetTicks() / 100;
}



void Timer::fillAccu()
{
new_time = (float)SDL_GetTicks() / 100;
delta_time = new_time - current_time;
current_time = new_time;

accumulator+= delta_time;
}


bool Timer::pinchAccu( float dt )
{
if( accumulator >= dt ) {
time_passed+= dt;
accumulator-=dt;

return true;
}

return false;
}


float Timer::getTimePassed()
{
return time_passed;
}



/**
* End of timer.cpp
*
**/



ball.h
/**
* ball.h
*
**/



#ifndef BALL_H
#define BALL_H



struct Ball
{
public:

float x;
float y;
float velx;
float vely;
float r;

};



#endif // BALL_H

/**
* End of ball.h
*
**/



ball.cpp
/**
* ball.cpp
*
**/



#include "ball.h"
#include "math.h"



Ball newBall( int x, int y, float velocity, float angle, float radius )
{
Ball b;

b.x = x;
b.y = y;

// Set velocities in px/s.
// 180.0*3.14 converts the angle from ° into rad
b.velx = velocity * cos( angle / 180.0 * 3.14 );
b.vely = velocity * sin( angle / 180.0 * 3.14 );

b.r = radius;

return b;
}


void updateBall( Ball* b, double dt )
{
b->x+= dt * b->velx;
b->y+= dt * b->vely;
}



/**
* End of ball.cpp
*
**/


Share this post


Link to post
Share on other sites
It's not usually a good idea to split up code unless you need to. Well organised code is a balance - 3000 line main files makes people cry, but having 20 or so files makes for irritating reading.

What error is your program kicking up? It might be a good idea to start with everything in main.cpp then just build up extra .h/.cpp files one at a time as needed (thats how I program anyway, and when it's just a one man project that tends to work fine).

'Picturing' a whole system of interconnected files and objects before implimenting it can be a whole job in itself (I think thats what software archietects do, but don't quote me on that), and when your task doesn't require you to do it, don't bother! Just take little steps. If it's something reasonably straightforward like pong maybe you coudl write it all in main(), then move components out into other files once you see groupings of functions or objects that can be put together logically.

Share this post


Link to post
Share on other sites
Important Rule: Never explicitly call a destructor, unless you know what you are doing. People posting in "For Beginners" do not fall under this category. It is extremely rare thing to have to do, even if you happen to be an experienced programmer.

Other than that you have a few errors in your code. Here is my take on it. These are only suggestions.


#include "game.h"

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

Game pong;

if( pong.mainMenu() ) {
// you declared Game::play to return an integer
// either use the return value, or don't return a value
return pong.play();
}

return 0;
}




#ifndef GAME_H
#define GAME_H

// The preferred way to include SDL is like this
// This allows games to be source portable among systems where SDL is not
// in a subdirectory called SDL. Instead, set your additional include path
// to include /path/to/your/include/SDL
#include "SDL.h"
#include "graphics.h"
#include "ball.h"

struct Gamestate
{
public:
Gamestate(const Ball &ball)
:
ball(ball)
{
}

void process(double t, double dt);

// anticipating future need
void draw(Gfx &gfx);

private:
Ball ball;
};

const int SCREEN_W = 600;
const int SCREEN_H = 400;

class Game
{
public:
// don't write empty constructors or destructors
Game()
:
gfx(SCREEN_W,SCREEN_H)
{
}
// ~Game();

bool mainMenu();
int play();

private:
// these can be local to play
// SDL_Event event;
// Gamestate* current_state;
// Gamestate* previous_state;

// at the moment, this could be made a local too.
// it is kept as a member because your mainMenu function is likely to
// make use of it.
Gfx gfx;
};

// these can be made part of the ball class
// extern Ball newBall( int x, int y, float velocity, float angle, float rad );
// extern void updateBall( Ball* b, double dt );

#endif




#include "game.h"
#include "timer.h"

// these are included in "game.h"
// include "ball.h"
// include "SDL.h"

void Gamestate::process(double t, double dt )
{
// Move ball
ball.update( dt );

// Todo: Rest of processing, eg. collision detection
}

bool Game::mainMenu()
{
return true; // Placeholder for a future main menu
}

int Game::play()
{
Timer timer;
const float DELTA = 0.01;

// prefer values to pointers
// previous_state = new Gamestate;
// current_state = new Gamestate;
// I have removed the "previous" state, as it doesn't appear
// to be used in any meaningful way
Gamestate current(Ball( 100, 100, 0.1, 0.0, 5.0 ));

bool loop = true;
SDL_Event event;
while( loop ) {

timer.fillAccu();

while( timer.pinchAccu( DELTA ) ) {

current.process( timer.getTimePassed(), DELTA );
}


while( SDL_PollEvent( &event ) ) {

if( event.type == SDL_QUIT )
loop = false;

}

current.draw( gfx );
}
return 0;
}




#ifndef GFX_H
#define GFX_H

#include "SDL.h"

class Gfx
{
public:
// by taking the screen dimensions as arguments, this class
// is easier to re-use in future projects
Gfx(int width, int height);

~Gfx();

// do "init" work in constructor
// note: you used the return value to signal success or failure
// however, your game class never checked the result!
// int init();

// the graphics objects shouldn't really care what they are drawing
// they shouldn't access the game state
// instead, other objects should tell it what to draw
// and where

// void draw( Gamestate* state );

private:

SDL_Surface* screen;

void drawPixel( int x, int y );
};

#endif




#include "graphics.h"

// include "game.h"

// C++ standard headers have no extension
#include <cmath>
// we include this so we can use
// the standard C++ exception types
#include <stdexcept>

// exceptions are the easiest way to signal such errors
// this is a fatal error usually.
Gfx::Gfx(int width, int height)
{
screen = 0;
if( SDL_Init( SDL_INIT_VIDEO ) < 0 )
{
throw std::runtime_error(SDL_GetError());
}

SDL_WM_SetCaption( "PongSDL", NULL );

screen = SDL_SetVideoMode( width, height, 32, SDL_SWSURFACE );

if( !screen )
{
// we need to ensure SDL_Quit is called on all code paths
// if the graphics object is successfully constructed, its
// constructor will call it. If this error occurs though
// we must call it here
SDL_Quit();
throw std::runtime_error(SDL_GetError());
}
}

Gfx::~Gfx()
{
// SDL says you shouldn't free the screen surface
// SDL_Quit does this for you
SDL_Quit();
}

void Gfx::drawPixel( int x, int y )
{
SDL_Rect rect;
rect.x = x;
rect.y = y;
rect.w = 1;
rect.h = 1;

SDL_FillRect( screen, &rect, SDL_MapRGB( screen->format, 0xFF, 0xFF, 0xFF ) );
}




#ifndef BALL_H
#define BALL_H

// balls are now smart objects
// they can set themselves up, and can update themselves
struct Ball
{
public:
Ball(float x, float y, float velocity, float angle, float radius );

void update(float dt);
private:
float x;
float y;
float velx;
float vely;
float radius;
};

#endif




#include "ball.h"
#include <cmath>

// use helper functions to increase code clarity

float to_radians(float degrees)
{
return degrees / 180.0f * 3.14f;
}

float cos_degrees(float angle)
{
return cos(to_radians(angle));
}

float sin_degrees(float angle)
{
return sin(to_radians(angle));
}

Ball::Ball( float x, float y, float velocity, float angle, float radius )
:
x(x),
y(y),
velx(cos_degrees(angle)),
vely(sin_degrees(angle)),
radius(radius)
{
}

void Ball::update( float dt )
{
x += dt * velx;
y += dt * vely;
}


Share this post


Link to post
Share on other sites
Thanks so much for the detailed reply! I used almost all your tips. I changed my structs into classes, because I somehow feel wrong defining constructors and functions to structs. I'm running into a problem though.

I feel I shouldn't make my Gfx object local, because it is rather big, and it would improve loading times coming from the main menu to the game if it only needs to be declared once. (I know, we're talking about Pong, but it could be a very big fancy game hypothetically)

I removed the init() function and put it in the constructor as said:

class Gfx
{
public:

Gfx();
Gfx( int screen_w, int screen_h );
~Gfx();

void drawPixel( int x, int y );
void drawCircle( int x, int y, int r );


private:

SDL_Surface* screen;

};

Gfx::Gfx()
{
screen = 0;
}

Gfx::Gfx( int screen_w, int screen_h )
{
if( SDL_Init( SDL_INIT_VIDEO ) < 0 )
throw std::runtime_error( SDL_GetError() );

SDL_WM_SetCaption( "PongSDL", NULL );

screen = SDL_SetVideoMode( screen_w, screen_h, 32, SDL_SWSURFACE );

if( !screen ) {
SDL_Quit();
throw std::runtime_error( SDL_GetError() );
}
}

Gfx::~Gfx()
{
SDL_Quit();
}



The problem is now that my gfx object in my Game class gets constructed with the default constructor automatically when I declare it, and I can't seem to call on the fancy constructor anymore in the Game's constructor.

Game::Game()
{
gfx.Gfx( GAME_SCREEN_W, GAME_SCREEN_H );
}

error: invalid use of `class Gfx'

Game::Game()
{
gfx = Gfx( GAME_SCREEN_W, GAME_SCREEN_H );
}

No compilation errors, the program runs according to my taskmanager, but after a short flash of a window, the window dissappears as if the video initiation fails.


How can I now use the fancy constructor for my non-local gfx object?

Share this post


Link to post
Share on other sites
I actually showed the code required to do that in my example. You need to use an initialiser list. See my version of your "Game" class for an example.

However, you appear to have added a default constructor to your Gfx object. This constructor however does not initialise the object properly (because you cannot use a default-constructed Gfx object, it has no screen). You must either change your default constructor to provide reasonable defaults for the screen dimensions, or (my suggestion) remove it.

Another issue that I did not mention is the Rule Of Three. The rule of three is a C++ rule and it states that if you have any of the following, a destructor, copy constructor or operator= overload, then you need all three.

However, there is a "get out" clause. Instead of implementing the copy constructor and assignment operator, we can disable copying for our object. We can do this by declaring both functions, but not implementing them. Also, we place them private. This prevents the compiler from automatically generating them for us (which is wrong, objects with pointers tend not to be copyable by default). If you ever accidentally try to copy a Gfx object, the compiler will spit out an error.


{
public:
Gfx( int screen_w, int screen_h );
~Gfx();

void drawPixel( int x, int y );
void drawCircle( int x, int y, int r );


private:
// These are deliberately left private and unimplemented.
// Do not copy Gfx instances!
Gfx(const Gfx &);
Gfx &operator=(const Gfx &);

SDL_Surface* screen;

};


Share this post


Link to post
Share on other sites
class Gfx
{
public:

//Gfx();
Gfx( int screen_w, int screen_h );
~Gfx();

// Draws a single white pixel
void drawPixel( int x, int y );
// Draws a circle with radius r and M(x;y)
void drawCircle( int x, int y, int r );


private:

// These are deliberately left private and unimplemented.
Gfx( const Gfx & );
Gfx &operator=( const Gfx & );


SDL_Surface* screen;

};

Game::Game()
: gfx(Gfx( GAME_SCREEN_W, GAME_SCREEN_H ))
{
}
graphics.h:31: error: `Gfx::Gfx(const Gfx&)' is private
game.cpp:33: error: within this context
graphics.h:31: error: `Gfx::Gfx(const Gfx&)' is private
game.cpp:33: error: within this context


For now, I've commented those unimplemented lines out, that fixed the problem.

Share this post


Link to post
Share on other sites
You still don't understand. That error is the very error I was talking about when I said that the compiler will warn you when you try and copy a Gfx instance.

Again, I will ask you to look carefully at the Game class in my post. Can you see how it differs from what you are trying to compile?

Share this post


Link to post
Share on other sites
Ah, now I see, I feel stupid for not noticing that myself... Thanks a lot though! I think I finally understand the idea behind proper splitting. I can finish my game now :)

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement