Project broken beyond repair

[bugmenot]

You also have memory leaks:

Code: Select all

#ifndef IMAGE_H_INCLUDED
#define IMAGE_H_INCLUDED

#include <string>
#include "SDL/SDL.h"

class Image{
    private:
        SDL_Surface *image;
    public:
        int Load( std::string filename );
        SDL_Surface *GetSurface() { return image; }
};

#endif // IMAGE_H_INCLUDED

How does the SDL_Surface that image points to ever get freed?

[dandymcgee]

From a very quick glance:

Code: Select all

    Level *test;
    test->Load( mapname, tilesheet );
    test->Draw();

Doesn't need to be a pointer

Fixed, I'm not sure why I had that as a pointer? My bad.

Always pass objects as references and const whenever possible unless there is a design reason not to. It is cheaper and faster to pass by reference then by value for non-basic datatypes and the use of const is so you don't accidently overwrite the data:

Code: Select all

int Level::Load( const std::string & file_map, const std::string & file_tilesheet )

Never thought of passing as a const, and as far as reference now I see what it's used for

Code: Select all

    file_tilesheet = "images\\" + file_tilesheet;
    tileSheet->Load( file_tilesheet, TILE_WIDTH, TILE_HEIGHT, TILE_SPRITES );

tileSheet (which is currently a pointer) is not initialised or pointing to a valid memory address. Again it has no reason to be a pointer.

That's one ugly piece of code.. So you're saying I should pass by reference rather than passing a pointer to an object where it's possible?

You also have memory leaks:

Code: Select all

...

How does the SDL_Surface that image points to ever get freed?

Code: Select all

Level::~Level()
{
    SDL_FreeSurface( tileSheet.GetSurface() );
    ...
}

Doesn't that free it?

Thanks so much for your constructive criticism, I'll do my best to fix the problems you've presented to me, at which point if I still cannot debug the application myself I will post again

EDIT: The project works again Since tilesheet is passed to so many times before it ever reaches render it's now a non-pointer. When it gets to render I just create a temporary pointer to use for blitting:

Code: Select all

SDL_Surface *tileSheetSurface = tileSheet.GetSurface() //Returns pointer to the tileSheet surface;

[bugmenot]

Yeah, it does free it. However, since Image class allocates the memory for the SDL_Surface in the first place and takes ownership of the pointer to it, it should also be responsible to free it as well.

Code: Select all

tileSheet->Load( file_tilesheet, TILE_WIDTH, TILE_HEIGHT, TILE_SPRITES );

tileSheet is a member variable of Level which is never initialised and therefore pointing to garbarge.Most of your problems are caused by dereferencing invalid pointers. What I am saying is that don't us pointers unless you are sharing or don't 'own' an object. In this case, tileSheet is owned by Level and is currently not a shared resource so it shouldn't be a pointer or a reference.

Your Level header should look something like:

Code: Select all

#ifndef LEVEL_H_INCLUDED
#define LEVEL_H_INCLUDED

#include <vector>
#include <string>
#include "Tile.h"
#include "Image.h"

/*class Level {
public:
int layer1[200][200];
int layer2[200][200];
void Draw();
void Load();
void Clear();
void Update();
};
*/

//class Tile;

class Level{
    private:
        int levelWidth;
        int levelHeight;
        int tileWidth;
        int tileHeight;
        Tilesheet tileSheet;
        //int mapData[20][15];
        std::vector<Tile*> mapData;
        //Tile *mapData[300];
    public:
        //Level();
        ~Level();
        int Load( const std::string & file_map, const std::string & file_tilesheet );
        int Draw();
};

It is worth reading Thinking in C++ Vol 1 which is a free eBook for more about areas like this.

[dandymcgee]

Yeah, it does free it. However, since Image class allocates the memory for the SDL_Surface in the first place and takes ownership of the pointer to it, it should also be responsible to free it as well.

That's what I figured you had meant, it shouldn't hurt to leave it both places, correct? Or should it only be in Image?

Code: Select all

tileSheet->Load( file_tilesheet, TILE_WIDTH, TILE_HEIGHT, TILE_SPRITES );

tileSheet is a member variable of Level which is never initialised and therefore pointing to garbarge.Most of your problems are caused by dereferencing invalid pointers. What I am saying is that don't us pointers unless you are sharing or don't 'own' an object. In this case, tileSheet is owned by Level and is currently not a shared resource so it shouldn't be a pointer or a reference.

Your Level header should look something like:

Code: Select all

#ifndef LEVEL_H_INCLUDED
...

My header looks exactly like that right now. (see comment below)

EDIT: The project works again Since tilesheet is passed to so many times before it ever reaches render it's now a non-pointer. When it gets to render I just create a temporary pointer to use for blitting:

Code: Select all

SDL_Surface *tileSheetSurface = tileSheet.GetSurface() //Returns pointer to the tileSheet surface;

We posted about the same time, I had just added this to my previous post. I had changed tileSheet so that it was no longer a pointer except where it had to be (in the render function).

Thanks again, I'll check that book out.

[bugmenot]

That's what I figured you had meant, it shouldn't hurt to leave it both places, correct? Or should it only be in Image?

Should only be in Image.

[dandymcgee]

That's what I figured you had meant, it shouldn't hurt to leave it both places, correct? Or should it only be in Image?

Should only be in Image.

Okay.