Sign in to follow this  
Miles Lombardi

Sprite' does not name a type

Recommended Posts

Hey, I'm getting an error for whenever I try to create an instance of the class Sprite (within my other class). The three lines are: Sprite spr_helmaroc_left; Sprite spr_helmaroc_right; Sprite sprite_index; And the three errors are: helmaroc.h:20: error: `Sprite' does not name a type helmaroc.h:21: error: `Sprite' does not name a type helmaroc.h:22: error: `Sprite' does not name a type I'm not entirely sure what this means, so could someone tell me, and then help me fix this, please.

Share this post


Link to post
Share on other sites
The includes for the erroring file:
#include "sprite.h"

Sprite.h:

//sprite.h
#ifndef SPRITE_H_INC
#define SPRITE_H_INC
#include <allegro.h>
#include "vardec.h"
extern int safe_sprite_ids;

class Sprite {
public:
int offset_x;
int offset_y;
int max_frames;
int width;
int height;
int id;
bool operator == (Sprite &class1);
BITMAP ** frames;
Sprite(BITMAP * load_sprite, int load_width, int load_height, int frame_num, int load_offset_x = 0, int load_offset_y = 0, int offset_x = 0, int offset_y = 0);
~Sprite();
void draw(int x, int y, int frame, BITMAP * destination);

};

void mirrorSprite(Sprite &mirror);

int get_new_sprite_id();

#endif //SPRITE_H_INC



Sprite.cpp:

//sprite.cpp
#ifndef SPRITE_INC
#define SPRITE_INC
#include <allegro.h>
#include <math.h>
#include "sprite.h"
#include "vardec.h"

int safe_sprite_ids = 0;

int get_safe_id(){
safe_sprite_ids++;
return safe_sprite_ids;
}

Sprite::Sprite(BITMAP* load_sprite, int load_width, int load_height, int frame_num, int load_offset_x, int load_offset_y, int draw_offset_x, int draw_offset_y ) {
if (load_sprite != NULL){
id = get_safe_id();
frames = new BITMAP* [frame_num];
width = abs(load_width);
height = load_height;
max_frames = frame_num;
offset_x = draw_offset_x;
offset_y = draw_offset_y;
for (int a = 0; a < frame_num; a++){
frames[a] = create_bitmap(width, height);
blit(load_sprite, frames[a], load_offset_x + a*width, load_offset_y, 0, 0, width, height);
}
destroy_bitmap(load_sprite);
}
else allegro_message("Error loading a certain sprite");
}

Sprite::~Sprite(){
for (int a = 0; a< max_frames; a++)
destroy_bitmap(frames[a]);
}



void mirrorSprite(Sprite &mirror){
for (int a = 0; a< mirror.max_frames; a++){
BITMAP * temp = create_bitmap(mirror.width, mirror.height);
clear_to_color(temp, makecol(255, 0, 255) );
draw_sprite_h_flip(temp, mirror.frames[a], 0, 0);
blit(temp, mirror.frames[a], 0, 0, 0, 0, mirror.width, mirror.height);
destroy_bitmap(temp);
}
}

bool Sprite::operator==(Sprite &class1){
return (id == class1.id);
}

void Sprite::draw(int x, int y, int frame, BITMAP * destination = sbuffer){
masked_blit(frames[frame], destination, 0, 0, x - offset_x, y - offset_y, width, height);
}

#endif //Sprite_INC

Share this post


Link to post
Share on other sites
the only thing i can think of is some place, maybe in the #includes of sprite.h, thee is a missing '}' at the end of a function/class or ';' at the end of a class.

look at vardec.h for things like that, and also in the code that creates the sprites (helmaroc.h)

and i guess ace_lovegrove didnt see my or your replys before he replied himself...

Share this post


Link to post
Share on other sites
A quick google search suggests that that error can occur if you have code like:
class Sprite
{
};

int main()
{
int Sprite;
Sprite spr_helmaroc_left;
}

So check that you're not using Sprite as a variable name anywhere in your code at the same scope as your error-causing declarations.
A few other comments on your code:
  1. #include <math.h> is deprecated in C++. You should be using #include <cmath> instead. This also puts all the symbols into namespace std (i.e. std::abs).

  2. There should be no need to wrap Sprite.cpp (or indeed any cpp file unless it contains template definitions and is included by a header) in header guards. There's no harm in doing it (unless you mistakenly reuse the header's guard) but it's not really neccessary.

  3. It looks like get_safe_id is only used by Sprite. If this is true then it should not be exposed to the rest of your program in the header file. Instead consider putting it and safe_sprite_ids into the unnamed namespace in Sprite.cpp, i.e.:
    namespace
    {
    int safe_sprite_ids = 0;
    int get_safe_id()
    {
    // ...
    }
    }
    and remove the corresponding declarations from Sprite.h.

  4. Your data members should certainly not all be public. As it stands anyone can change max_frames and cause the Sprite class to access invalid memory. Encapsulation is good.

  5. You leak frames. You allocate it with new[] but you never delete[] it.

  6. Sprites are not safe to copy. Obey the law of three (if you need one of the copy constructor, destructor and copy assignment operator you generally need all three). I would advise a std::vector< boost::shared_ptr< BITMAP > > or a boost::shared_array< BITMAP >. In both cases you would need to provide the correct deleter to the boost smart pointer.

  7. In the event that your Sprite constructor fails you leave the instance with uninitialised data, but do not signal the failure to the calling code. You should either initialise all data to default values, set a fail flag or (my preference) throw an exception.

Enigma

Share this post


Link to post
Share on other sites
Erm wow Enigma, it's gonna take me a while to get through all of that.
So I'll just start with this:
Quote:

A quick google search suggests that that error can occur if you have code like:

class Sprite

{

};



int main()

{

int Sprite;

Sprite spr_helmaroc_left;

}


So check that you're not using Sprite as a variable name anywhere in your code at the same scope as your error-causing declarations.


I cannot find any evidence of me doing that, nor do I remember me doing that.
I almost did it with my room class, but then I realised that that would happen, so I pre-empted myself there, so I would think I would preempt myself here.

Quote:


1. #include <math.h> is deprecated in C++. You should be using #include <cmath> instead. This also puts all the symbols into namespace std (i.e. std::abs).

Done, didn't know this. I assume everything that has a .h after it is a C header, rather than a C++?

Done 2.

Quote:

It looks like get_safe_id is only used by Sprite. If this is true then it should not be exposed to the rest of your program in the header file. Instead consider putting it and safe_sprite_ids into the unnamed namespace in Sprite.cpp, i.e.:

namespace

{

int safe_sprite_ids = 0;

int get_safe_id()

{

// ...

}

}

and remove the corresponding declarations from Sprite.h.

I don't know how to set up my own namespaces, how would I do this?

Quote:


4 Your data members should certainly not all be public. As it stands anyone can change max_frames and cause the Sprite class to access invalid memory. Encapsulation is good.

I kept them like this so that people could check them themselves, so that they could cycle through frames. Is there any way to make them "read only"?

Quote:


5 You leak frames. You allocate it with new[] but you never delete[] it.


Fair point, I've probably done this in a lot of other places as well, though only with arrays, I'll have to check that out.
Fixed.

Quote:

Sprites are not safe to copy. Obey the law of three (if you need one of the copy constructor, destructor and copy assignment operator you generally need all three). I would advise a std::vector< boost::shared_ptr< BITMAP > > or a boost::shared_array< BITMAP >. In both cases you would need to provide the correct deleter to the boost smart pointer.

Unfortunately I don't know anything about boost, i should probably start learning about it.
I'm pretty lazy so I didn't make a copy constructor myself, and the programme got on fine without it. Seems to copy stuff on it's own.

Quote:


# In the event that your Sprite constructor fails you leave the instance with uninitialised data, but do not signal the failure to the calling code. You should either initialise all data to default values, set a fail flag or (my preference) throw an exception.

Which bit would you expect to fail? The load_bitmap, yes, but I have throw up an exception, well sorta. It brings up an error message, though it doesn't halt the programme.

I think that's all of it, thanks for your criticism. But it hasn't fixed the actual problem, lol.


Edit: Yes ace helmaroc.h is my own. It's also where the problem crops up.

Share this post


Link to post
Share on other sites
read-only means using accessor methods. either return a copy of the value, or for large types const references, or for pointers, const pointers.

the idea though is that the class should be encapsulated and people don't need to check. the class should ideally report back when something goes wrong, via exceptions, or return values. if the class is in a valid state it stays silent.

and as to actually helping your problem, try post the code for the various header files

vardec.h ,helmaroc.h

namespaces. just type :

namespace /*OPTIONAL*/NAME{

// all stuff here
void function();
class Class{/*...*/};

};

//to access the things
NAME::function();
NAME::Class instance;



if you dont give a namespace a name, you can only access its members in the same file. it is hidden to everything else. its like private: in a class

Share this post


Link to post
Share on other sites
Some nice headers for you.

vardec.h

//vardec.h
#ifndef VARDEC_H_INC
#define VARDEC_H_INC
#define ROOM_EXIT 0
#define ROOM_TITLE 1
#define ROOM_HELMAROC 2
#include <allegro.h>
#include "link.h"
#include "helmaroc.h"
#include "room.h"

extern bool B2S;
extern bool run;

extern BITMAP *title_screen;
extern BITMAP *press_start;
extern BITMAP *sbuffer;
extern BITMAP *tower;
extern BITMAP *rock;

extern int stepcounter;
extern int room;

extern int heartcontainers;
extern int hearts;

extern int view_x;
extern int view_y;

extern bool col_tower[67][54];

extern OBJ_LINK *link;
extern OBJ_HELMAROC *helmaroc;
extern clROOM *rmtower;

#endif




link.h

//link.h
#ifndef LINK_H_INC
#define LINK_H_INC
#include <allegro.h>
#include "sprite.h"

#define MODE_STAND 1
#define MODE_WALK 2
#define MODE_SWORD 3

#define LINK_DIR_UP 0
#define LINK_DIR_DOWN 1
#define LINK_DIR_LEFT 2
#define LINK_DIR_RIGHT 3

class OBJ_LINK {
private:
unsigned int key_length_up;
unsigned int key_length_down;
unsigned int key_length_left;
unsigned int key_length_right;

int x;
int y;
int mode;
int direction;
int frame;
int speed;

SPRITE * sprite_index;

void all_minus();

void move();
void draw();
void setupsprite();
public:
SPRITE spr_link_up_walk;
SPRITE spr_link_down_walk;
SPRITE spr_link_left_walk;
SPRITE spr_link_right_walk;
SPRITE spr_link_up_stand;
SPRITE spr_link_down_stand;
SPRITE spr_link_left_stand;
SPRITE spr_link_right_stand;
SPRITE spr_link_up_sword;
SPRITE spr_link_down_sword;
SPRITE spr_link_left_sword;
SPRITE spr_link_right_sword;
OBJ_LINK();
void exec();
};

#endif //LINK_H_INC




helmaroc.h

//helmaroc.h
#ifndef HELMAROC_H_INC
#define HELMAROC_H_INC

#define NO_STATUS 0
#define MOVE_UP 1
#define SWOOP 2
#define HURT 3

#include "sprite.h"
class OBJ_HELMAROC{
private:
int x;
int y;
int status;
int alarm[8];
int health;
double frame;
void draw();
void move();
void alarms();
SPRITE spr_helmaroc_left;
SPRITE spr_helmaroc_right;
SPRITE sprite_index;
public:
~OBJ_HELMAROC();
OBJ_HELMAROC();
void execute();

};
#endif //HELMAROC_H_INC




You already have sprite.h

Share this post


Link to post
Share on other sites
#include "sprite.h"

- #include "vardec.h"

- - #incldue "helmaroc.h"

- - - #include "sprite.h" ---> nothing happens SPRITE_H_INC already defined
---> helmaroc doesnt know what a sprite is...


i think you dont need to #include "vartec.h" in sprite.h

i think you would be able to #include it in sprite.cpp

this is just what i see though. i'm not positive that will fix everything, or whether thats the main problem

Share this post


Link to post
Share on other sites
That actually helped, and I've managed to get down to a bunch of linker errors now. Yey!

Wait wait.
Project rebuild!
YEY!
It works.
Thanks guys. I guess it wasn't necessary to include that vardec.h twice in the same loop, I guess. And that screwed it up.

Oh but there's another thing, a problem with Enigma's advice.
I did delete[] frames.
But that doesn't work.
You see I already did this:
for (int a = 0; a< max_frames; a++)
destroy_bitmap(frames[a]);

And that seems to clear the pointers, but I don't know if it clears the array.

Share this post


Link to post
Share on other sites
Quote:
Original post by Miles Lombardi
That actually helped, and I've managed to get down to a bunch of linker errors now. Yey!

Wait wait.
Project rebuild!
YEY!
It works.
Thanks guys. I guess it wasn't necessary to include that vardec.h twice in the same loop, I guess. And that screwed it up.

Oh but there's another thing, a problem with Enigma's advice.
I did delete[] frames.
But that doesn't work.
You see I already did this:
for (int a = 0; a< max_frames; a++)
destroy_bitmap(frames[a]);

And that seems to clear the pointers, but I don't know if it clears the array.


thats good news.

on the delete[] issue, you need to do both.

you allocated some memory for the pointers with new[]

so you must delete[] it.

you also need to do destroy_bitmap also, before the delete[] call.

with those two done you have cleared all dynamically allocated memory.

Share this post


Link to post
Share on other sites
The code you previously posted results in the following:
{ // new scope
// create a sprite
Sprite sprite1(aBitmap, 32, 32, 4); // [1]
} // leave scope, sprite1 is destroyed [2]

At the end of [1] sprite1 looks like (in memory):
[0x00000000] // offset_x
[0x00000000] // offset_y
[0x00000004] // max_frames
[0x00000020] // width
[0x00000020] // height
[0x00000001] // id
[PointerNo0] // frames

where PointerNo0 points to a structure in memory that looks like:
[PointerNo1] // frames[0]
[PointerNo2] // frames[1]
[PointerNo3] // frames[2]
[PointerNo4] // frames[3]

and each of PointerNo1-4 point to a BITMAP in memory.
At [2] the Sprite is destroyed, which frees the BITMAPs and the sprite but leaves the above array around. So you have a chunk of memory hanging around containing four pointers, but none of them are or even can ever be used again.

Things get worse if you copy your Sprites:
{ // new scope
// create a sprite
Sprite sprite2(aBitmap, 32, 32, 4); // [3]
{ // new scope
Sprite sprite3 = sprite1; // [4]
} // leave scope, sprite3 is destroyed [5]
} // leave scope, sprite2 is destroyed [6]

At the end of [3] sprite2 again looks like (in memory):
[0x00000000] // offset_x
[0x00000000] // offset_y
[0x00000004] // max_frames
[0x00000020] // width
[0x00000020] // height
[0x00000001] // id
[PointerNo5] // frames

where PointerNo5 points to a structure in memory that looks like:
[PointerNo6] // frames[0]
[PointerNo7] // frames[1]
[PointerNo8] // frames[2]
[PointerNo9] // frames[3]

and each of PointerNo6-9 point to a BITMAP in memory.
At the end of [4] sprite3 looks like (in memory):
[0x00000000] // offset_x
[0x00000000] // offset_y
[0x00000004] // max_frames
[0x00000020] // width
[0x00000020] // height
[0x00000001] // id
[PointerNo5] // frames

Where PointerNo5 points to the same structure as sprite2.
At [5] sprite3 is destroyed, which frees the BITMAPs and the Sprite but leaves the array of pointers around. However, sprite2 still points to that array of pointers, which now point to nothing and any attempt to access what they used to point to is undefined behaviour.

At [6] sprite2 is destroyed, which again tries to free the BITMAPs pointed to by PointerNo6-9 even though they have already been freed. This again is undefined behaviour which, in the words of Scott Meyers, means "it works during development, it works during testing, and it blows up in your most important customers' faces".
If you properly delete[] the array of pointers then that memory too will also get destroyed multiple times if you copy your Sprites.

What you need to do to properly handle this is to implement a copy constructor and a copy assignment operator that either deep copy the bitmaps when a copy is made, so that each Sprite points to different copies of the same BITMAPs, or else keep a reference count and only free the BITMAPs when no more Sprites point to them. The former results in the following memory layout at the end of [4]:
sprite2:
[0x00000000] // offset_x
[0x00000000] // offset_y
[0x00000004] // max_frames
[0x00000020] // width
[0x00000020] // height
[0x00000001] // id
[PointerNoA] // frames
sprite3:
[0x00000000] // offset_x
[0x00000000] // offset_y
[0x00000004] // max_frames
[0x00000020] // width
[0x00000020] // height
[0x00000001] // id
[PointerNoB] // frames
array pointed to be PointerNoA:
[PointerNoC] // frames[0]
[PointerNoD] // frames[1]
[PointerNoE] // frames[2]
[PointerNoF] // frames[3]
array pointed to be PointerNoB:
[PointerNoG] // frames[0]
[PointerNoH] // frames[1]
[PointerNoI] // frames[2]
[PointerNoJ] // frames[3]

The latter results in something like:
sprite2:
[0x00000000] // offset_x
[0x00000000] // offset_y
[0x00000004] // max_frames
[0x00000020] // width
[0x00000020] // height
[0x00000001] // id
[PointerNoK] // frames
sprite3:
[0x00000000] // offset_x
[0x00000000] // offset_y
[0x00000004] // max_frames
[0x00000020] // width
[0x00000020] // height
[0x00000001] // id
[PointerNoK] // frames
array pointed to be PointerNoK:
[0x00000002] // reference count
[PointerNoL] // frames[0]
[PointerNoM] // frames[1]
[PointerNoN] // frames[2]
[PointerNoO] // frames[3]

When using a reference count (as my previous suggestions involving boost do) when sprite3 leaves scope and is destroyed it decrements the reference count but does not free the Bitmaps or the array (since the reference count is non-zero, which means another Sprite is still using them). This leaves memory looking like:
sprite2:
[0x00000000] // offset_x
[0x00000000] // offset_y
[0x00000004] // max_frames
[0x00000020] // width
[0x00000020] // height
[0x00000001] // id
[PointerNoK] // frames
array pointed to be PointerNoK:
[0x00000001] // reference count
[PointerNoL] // frames[0]
[PointerNoM] // frames[1]
[PointerNoN] // frames[2]
[PointerNoO] // frames[3]

Now when sprite2 leaves scope and is destroyed it again decrements the reference count and, since the reference count is now zero, frees the BITMAPs and the array.

Enigma

Share this post


Link to post
Share on other sites
Wow Enigma.
To be honest that's really really REALLY confusing for a guy new to this. But it's comprehensive.
I'm gonna read that a few more times.
But I think you were warning me about problems with Copy Constructors and the = operator, right?
I have actually already come across that problem, and I bodge fixed it, and only for that instance. I set all of the copied one's frames to NULL, which fixed the bug. But I daresay I'm leaking memory there. Lol, I say I like control but god does it come at a price.

Anyway that wasn't much of a reply to that huge novel you wrote there, but I can't think of anything to say.

And to rip-off the code I'm using is:

for (int a = 0; a< max_frames; a++)
destroy_bitmap(frames[a]);

delete[] frames;



And that crashes. With a windows error code of 0xc0000005, aka pointer.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this