🔒 Locked

std::vector push_back causes program crash =(

Started by silverphyre673 Sep 16, 2005 at 2:07 AM 10 replies 5.4k views

Original Post

Sep 16, 2005 at 2:07 AM

I'm working on an art storage and retrieval system for my GUI system. So far all it is is a vector of a class DGUI_Image. The DGUI_Image class loads a bitmap that it is supposed to store and keeps it on an SDL_Surface (this is specifically for SDL). Anyways, the loading appears to go fine, but I keep getting these wierd crashing bugs whenever I try to push_back them in the art engine thingy. Here is some relevant code:



class DGUI_ArtEngine
{
    public:
    
    /*Loads up the image and returns the offset the image is at in image_list*/
    int AddImage(std::string path);
    
    /*Renders an image at offset image_id*/
    void Render(int image_id, int x, int y);
    
    int Size() const {return image_list.size();}
    
    private:
    std::vector<DGUI_Image> image_list;
};

class DGUI_Image
{
    public:
    DGUI_Image():image(0){}
    DGUI_Image(std::string path, bool auto_alpha = true):image(0){Load(path, auto_alpha);}
    ~DGUI_Image(){SDL_FreeSurface(image);}
    
    bool operator () () const {return image != 0;}
    
    bool Load(const std::string & filename, bool auto_alpha = true);

    void Render(int x, int y) const;
    
    private:
    SDL_Surface * image;
};


int DGUI_ArtEngine::AddImage(std::string path)
{
    /*Creates a DGUI_Image and loads the image from file at location "path".*/
    DGUI_Image image(path);
    if (image())
    {
        image_list.push_back(image);  //Crash!!! (segmentation default)
        return image_list.size() - 1;
    }
    else
    {
        return -1;
    }
}

bool DGUI_Image::Load(const std::string & filename, bool auto_alpha)
{
    if (image)
        SDL_FreeSurface(image);
    image = SDL_LoadBMP(filename.c_str());
    if (image == 0)
        return false;
    if (auto_alpha)
        SDL_SetColorKey(image, SDL_SRCCOLORKEY, SDL_MapRGB(image->format, 255, 0, 255));
    return true;
}

Any insights are much-appreciated, as always! Thanks.

dave

322

dave

Sep 16, 2005 at 2:13 AM

Hi,

Right, well firstly why are you reducing the size of the vector by one once you have added something to it?

Apart from that it is difficult to tell what is wrong without stepping through it. Have you stepped through it yourself?

ace

Porphiron

Sep 16, 2005 at 2:38 AM

Hmmm, the image gets copied when you put it into the vector by the default copyctor(the pointer of the copy points to the same SDL_Surface)(right?) and the local version will be destroyed taking the SDL_Surface with it in its destructor. I think. ;) Not sure if that's the problem.

[Edited by - Matthias Lange on September 16, 2005 3:38:58 AM]

Dmytry

264

Dmytry

Sep 16, 2005 at 3:45 AM

Quote:
Original post by Matthias Lange
Hmmm, the image gets copied when you put it into the vector by the default copyctor(the pointer of the copy points to the same SDL_Surface)(right?) and the local version will be destroyed taking the SDL_Surface with it in its destructor. I think. ;) Not sure if that's the problem.

i think you are right. probably by copyctor you mean copy constructor - good abbrevation actually.

Possible solution is to New the DGUI_Image and store pointer in the vector. Prior to destruction of vector, You will have to manually delete everything vector points to. Or use reference-counted smartpointers to have it deleted automatically.

Rancorb

Sep 16, 2005 at 5:23 AM

Hi,

I saw something which looked odd to me but I'm not sure if it's a problem or not, so I thought I would just ask...

In your DGUI_ArtEngine::AddImage method you've got an "if" statement checking image() (the default / empty constructor method?) while in the rest of the code you check the image itself. Is this correct, and if so what exactly is it doing?

I thought the image was already constructed in the previous line, so when the "if" statement calls the constructor you would lose your previously loaded image.

Porphiron

Sep 16, 2005 at 5:37 AM

Quote:
Original post by Rancorb
In your DGUI_ArtEngine::AddImage method you've got an "if" statement checking image() (the default / empty constructor method?) while in the rest of the code you check the image itself. Is this correct, and if so what exactly is it doing?

It checks if the image pointer is 0, because he overloaded it: "bool operator () () const {return image != 0;}". In the rest of the code where he checked that, he was able to access the image pointer directly. Weird, if you ask me. ;)

edit: I feel like saying a bit more to that: Why don't you just make a function an give it a descriptive name? Or just bind the lifetime of the DGUI_Image to the lifetime of its SDL_Surface? I mean, making it impossible to change the SDL_Surface of the DGUI_Image so it's always valid and when you want to load a different picture just create a new instance of DGUI_Image?

Another thing you should do in any case: Either forbid copying of DGUI_Images or make your own copy-constructor that creates a new SDL_Surface for the copy.

Enigma

188

Enigma

Sep 16, 2005 at 6:28 AM

Rather than storing pointers in the vector I would advise making the image member of DGUI_Image a smart pointer, since otherwise the error is likely to recur anywhere else a DGUI_Image is passed by value. The alternative would be to implement a proper copy constructor and copy assignment operator, but it's a lot easier just to make the raw pointer a smart pointer:

class DGUI_Image{    public:    DGUI_Image():image(0){}    DGUI_Image(std::string path, bool auto_alpha = true):image(0){Load(path, auto_alpha);}        bool operator () () const {return image}        bool Load(const std::string & filename, bool auto_alpha = true);    void Render(int x, int y) const;        private:    boost::shared_ptr< SDL_Surface > image;};bool DGUI_Image::Load(const std::string & filename, bool auto_alpha){    image.reset(SDL_LoadBMP(filename.c_str()), SDL_FreeSurface);    if (!image)        return false;    if (auto_alpha)        SDL_SetColorKey(image.get(), SDL_SRCCOLORKEY, SDL_MapRGB(image->format, 255, 0, 255));    return true;}

Enigma

Dmytry

264

Dmytry

Sep 16, 2005 at 7:05 AM

^^^ hmmm it is necessary to do something with shared_ptr so it doesn't "delete" what it points to, but instead call SDL_FreeSurface , right? or it is going to crash when smartpointer will try to delete what it points to. edit: whoops, haven't noticed you made it call SDL_FreeSurface.

I would just disallow copying of DGUI_Image (by making copy constructor be private or protected) and would use vector holding smartpointers, i.e. std::vector< shared_ptr< DGUI_Image > > there, and use smartpointer to DGUI_Image instead of passing it by value.

[Edited by - Dmytry on September 16, 2005 7:05:18 AM]

Enigma

188

Enigma

Sep 16, 2005 at 7:22 AM

Quote:
Original post by Dmytry
^^^ hmmm it is necessary to do something with shared_ptr so it doesn't "delete" what it points to, but instead call SDL_FreeSurface , right? or it is going to crash when smartpointer will try to delete what it points to.

Quote:
Original post by Enigma
image.reset(SDL_LoadBMP(filename.c_str()), SDL_FreeSurface);

I did do something with shared_ptr.

Enigma

Dmytry

264

Dmytry

Sep 16, 2005 at 7:36 AM

ops, sorry, haven't noticed.

silverphyre673

Author

silverphyre673

Sep 16, 2005 at 2:37 PM

Well, thanks guys. I never would have thought of that =) That's probably what it is - I can't check till I get home from school. Also, if you're wondering, I parse a text file for information on what images to load and stuff. The whole parsing system assigns integer IDs to each piece of art, each control, etc. and the art is assigned an ID corresponding to its offset in the vector, if you understand. If not, then just know that it works =)

Thanks for your help everyone!

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.