Page 2 of 2
Re: Project broken beyond repair
Posted: Sun Dec 28, 2008 2:05 am
by 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?
Re: Project broken beyond repair
Posted: Sun Dec 28, 2008 9:58 am
by dandymcgee
bugmenot wrote: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.
bugmenot wrote:
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
bugmenot wrote:
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?
bugmenot wrote:You also have memory leaks:
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;
Re: Project broken beyond repair
Posted: Sun Dec 28, 2008 10:41 am
by 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.
Re: Project broken beyond repair
Posted: Sun Dec 28, 2008 11:47 am
by dandymcgee
bugmenot wrote: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?
bugmenot wrote:
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:
My header looks exactly like that right now. (see comment below)
dandymcgee wrote:
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.
Re: Project broken beyond repair
Posted: Sun Dec 28, 2008 2:08 pm
by 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.
Re: Project broken beyond repair
Posted: Sun Dec 28, 2008 2:45 pm
by dandymcgee
bugmenot wrote: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.
