Sign in to follow this  

Strings have shattered my once-cool program.

This topic is 3743 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've been working on a game engine using NeHe's OpenGL tutorials. I few days ago, I had completed a working prototype that loaded a bunch of graphics and displayed them as texture masked quads, and it did it very well! I even felt comfortable turning it in for some independent study credits that I needed. But my point is, the thing WORKED. So what changed? I decided to implement std::string instead of using character arrays, and now my program is screwed. I've been working at it for days now, but the state of my engine is only going downhill, and I need the wisdom of Gamedev before I decide to just kill the whole project (which would be an extremely rash move, considering the amount of work I put into it thus far). Here's what is happening now... I have a Graphic class that combines a string ID and a reference to a loaded texture. The class is also responsible for loading said textures, which is when the ID and reference number are assigned. It was when I changed the ID from a character array to a string that memset started crashing my program. Here's the code: <code> bool Graphic::load (string fileName) { /* Temporary storage for the file being loaded from. */ AUX_RGBImageRec * image; memset(image, 0, sizeof(void *) * 1); /* Try loading the bitmap from file. If it fails, the operation will return false. */ bool status = false; if (image = loadBMP(fileName)) { status = true; glGenTextures(1, &texture); glBindTexture(GL_TEXTURE_2D, texture); glTexImage2D(GL_TEXTURE_2D, 0, 3, image->sizeX, image->sizeY, 0, GL_RGB, GL_UNSIGNED_BYTE, image->data); glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); } /* Free the original bitmap. */ if (image) { if (image->data) free(image->data); free(image); } return status; } </code> This piece of code worked flawlessly. When I changed ID into a string, memset started causing an Unhandled Exception fault, which did not happen at all until then. So what gives? Why is is happening now? I understand that std::string is safer than character arrays, but this is not the only problem I've had since implementing strings. This Graphic class gets inserted into a std::list, and that no longer works either... it only inserted garbage data, but that was until I started having THIS problem. I'm at my wits end.

Share this post


Link to post
Share on other sites
why would you memset a pointer??


AUX_RGBImageRec * image = NULL;



What your program is doing is trying to memset the memory pointed at by the uninitialized pointer image. the first argument is a pointer of the memory to be set, not the variable to be set.

the actual way to use memset in that case would be:

AUX_RGBImageRec * image;

memset(&image, 0, sizeof(void *) * 1);



note the extra '&' before image.

But never ever ever initialize a variable that way. it's a waste.

-me

Share this post


Link to post
Share on other sites
Oh god... I must have removed the & before image when I was screwing around. It is
somewhat operational again.
Sorry guys, looks like it was a simple typo that went unnoticed all day. SORRY.

Share this post


Link to post
Share on other sites
Yeah, I guess I read the wrong FAQ. Now I am enlightened.

So here's my previous problem. I'm trying to create a list of Graphic
References. Here's the code that creates a new Graphic, loads it and
then inserts it into the list. The problem is, it only inserts garbage.

I think it has something to do with the copy constructor or the = operator
I added, but I can't figure out what.



bool Databank::loadGraphic (string id, string fileName) {

/* Create a new Graphic object, and if it loads the requested image
properly, place it in the Graphic list. Otherwise, return false.
*/


Graphic newGraphic(id);

if (newGraphic.load(fileName)) {

graphics.push_back(newGraphic);
return true;

}

return false;

}




And the class defintiion...



class Graphic {

private:

/* The ID for this Graphic.
*/


string id;


/* A reference to a texture (The common name for an image that is used in
3D applications).
*/


GLuint texture;


public:

/* Constructors, copy Constructor, Deconstructor.
*/


Graphic ();
Graphic (string idIn) {id = idIn;}
Graphic (const Graphic &rhs);
~Graphic ();


/* Overloaded operators which are required for templated lists of classes.
*/


Graphic &operator= (const Graphic &rhs);
int operator== (const Graphic &rhs) const;
bool operator!= (const Graphic &rhs) const;
bool operator> (const Graphic &rhs) const;
int operator< (const Graphic &rhs) const;



/* Load a graphic file into memory.
*/



bool load (string fileName);


/* Accessors.
*/


string getID () {return id;}
GLuint getTexture () {return texture;}

};



And the relevant constructors and whatnot.


Graphic::Graphic () {

id = "";
texture = NULL;

}





Graphic::Graphic (const Graphic &rhs) {

id = rhs.id;
texture = rhs.texture;

}





Graphic::~Graphic () {

}


Graphic& Graphic::operator= (const Graphic &rhs) {

return *this;

}




Am I just missing something crucial here? I know that std::list makes a copy
of whatever data is added to it, and that's the need for the above elements.
But I can't figure out why this isn't working.

Share this post


Link to post
Share on other sites
Do you really want to copy that much data each time you insert a Graphic object into the list? What you should be doing is storing a list of pointers, so that when you call push_back() you're only copying the size of the pointer and not the entire contents of Graphic.

Modify loadGraphic() so that it allocates Graphic objects on the heap and keep a std::list<Graphic*> or...

Take a look at boost::ptr_list.

Share this post


Link to post
Share on other sites
Quote:
Original post by thecolonel
Do you really want to copy that much data each time you insert a Graphic object into the list? What you should be doing is storing a list of pointers, so that when you call push_back() you're only copying the size of the pointer and not the entire contents of Graphic.

Modify loadGraphic() so that it allocates Graphic objects on the heap and keep a std::list<Graphic*> or...

Take a look at boost::ptr_list.


All he's copying is a string and an int.

In any case, all that I can see is your assignment operator doesn't actually assign.

Share this post


Link to post
Share on other sites
Quote:
Original post by boejunda
Oh god... I must have removed the & before image when I was screwing around. It is
somewhat operational again.
Sorry guys, looks like it was a simple typo that went unnoticed all day. SORRY.


Once the dust has settled on your project enough to give you an hour or so to spare, look in to setting up a version control system. My favourite is Mercurial, but there are lots of others to investigate.

Share this post


Link to post
Share on other sites
Quote:
Original post by nuvem
In any case, all that I can see is your assignment operator doesn't actually assign.


QFE. this is a huge problem. It means that every time you are pushing onto that list, no valid data is actually being copied into it.

-me

Share this post


Link to post
Share on other sites
Just a word to the wise; you should really look to using something other than the glAux bitmap loading function for loading in textures. It is known to leak memory and should be avoided (yes, I know NeHe uses it but that doesn't mean it is any good).

There are numberous libs avaible, one which is simple should should suit your needs is SOIL.

(normally I'd take this change to pimp my own loading library, GTL, however as simple as the interface is I suspect compling it is over your head right now.)

Share this post


Link to post
Share on other sites
Quote:
Original post by Palidine
Quote:
Original post by nuvem
In any case, all that I can see is your assignment operator doesn't actually assign.


QFE. this is a huge problem. It means that every time you are pushing onto that list, no valid data is actually being copied into it.

-me


Yes.

It is worth nothing that in your case, the correct way to write the copy constructor, assignment operator and destructor is as follows:





I.e., nothing at all. The default behaviour - member-wise copies, assignments and cleanups - is exactly what you want.

Also, don't make 'load' functions. Do that work in the constructor, instead.

Also, use initialization lists to initialize data members where possible.

Also, it looks like you're storing a vector of "named" graphic objects, so that you can search through it, one at a time, to find the one you want later. Don't do that. Instead, use an associative container - std::map, in this case. It lets you use the graphic name directly as an "index" into the container (and then there is also no reason to store it as part of the graphic object).


class Databank {
// etc. etc.
std::map<string, Graphic> graphics; // instead of the vector
};

struct LoadFailure {};

// Since constructors can't return, our Graphic constructor will throw an
// exception on failure. Here's how we can wrap the exception to preserve the
// original interface.
bool Databank::loadGraphic(const string& id, const string& fileName) {
try {
graphics[id] = Graphic(fileName);
return true;
} catch (LoadFailure&) {
return false;
}
}

class Graphic {
GLuint id;

public:
Graphic() : id(-1) {}
Graphic(const std::string& filename);

// You should only need == and <, and they should return bool.
bool operator== (const Graphic &rhs) const { return id == rhs.id; }
bool operator< (const Graphic &rhs) const { return id < rhs.id; }

GLuint getTexture() { return id; }
};

Graphic::Graphic(const std::string& filename) {
// Fix your AUX_RGBImageRec class to have a proper destructor etc., and
// fix loadBMP to allocate it via 'new', and return a std::auto_ptr
// instead of a raw pointer.
std::auto_ptr<AUX_RGBImageRec> image = loadBMP(fileName);
if (!image) { throw LoadFailure(); }

glGenTextures(1, &id);
glBindTexture(GL_TEXTURE_2D, id);
glTexImage2D(GL_TEXTURE_2D, 0, 3, image->sizeX, image->sizeY, 0,
GL_RGB, GL_UNSIGNED_BYTE, image->data);

// Do you really want to redo this with each texture?
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
}

Share this post


Link to post
Share on other sites
Thanks Zahlman. A map is all I needed (Pun intended)! But seriously, for what I designed that'll work way better than the route I was going to go (Coulda inserted another map pun right there). And I also hadn't heard of auto pointers yet either.
Lemme take this new info and try it out, I'll get back.

Share this post


Link to post
Share on other sites

This topic is 3743 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.

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