Jump to content

  • Log In with Google      Sign In   
  • Create Account


SDL and Copy Constructors


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
15 replies to this topic

#1 LatchGameDev   Members   -  Reputation: 165

Like
1Likes
Like

Posted 05 December 2013 - 02:17 PM

So I've been wrestling with this problem for a few days now. So I have a entity class and I thought I would use a vector to make it so i could have a dynamic array of the entity's. It crashed horribly and i learned that in my SmartImage class (located inside the entity class) was only making a shallow copy. So i'm trying to make a deep copy using the copy constructor. My program compiles but it crashes as soon as it launches. It seems even initializing the SmartImage class will cause it to crash. What am i doing wrong?

 

SmartImage.h

class SmartImage
{
    private:
            bool image_loaded;
    public:
           SmartImage();
           ~SmartImage();
           // copy constructor:
           SmartImage (const SmartImage& param);
           
           SDL_Surface *surface;
           void load(std::string filename);
           void copy_surface(SDL_Surface *target_surface);
};
#include"SmartImage.cpp"

SmartImage.cpp

SmartImage::SmartImage()
{
 surface=NULL;
 image_loaded = false;     
}

SmartImage::~SmartImage()
{
 SDL_FreeSurface(surface);      
}

// copy constructor
SmartImage::SmartImage (const SmartImage& param)
{         
 surface = SDL_DisplayFormat(param.surface);          
}

void SmartImage::load(std::string filename)
{
    if(image_loaded == false)
    {
     surface = load_image(filename);
     image_loaded = true;
    }    
    else
    {
     SDL_FreeSurface(surface);
     surface = load_image(filename);
    }  
}

void SmartImage::copy_surface(SDL_Surface *temp_surface)
{
     if(image_loaded == true)
     {
      SDL_FreeSurface(surface);               
     }
 surface = SDL_DisplayFormat(temp_surface); 
 image_loaded = true;     
}

Edited by LatchGameDev, 05 December 2013 - 02:19 PM.


Sponsor:

#2 Álvaro   Crossbones+   -  Reputation: 12848

Like
1Likes
Like

Posted 05 December 2013 - 02:59 PM

Do you know about the rule of three? It looks like you didn't define an operator=, so you probably don't. Read about it and you may have an easier time understanding your problem.



#3 LatchGameDev   Members   -  Reputation: 165

Like
1Likes
Like

Posted 05 December 2013 - 03:44 PM

I know of the rule of three but i'm having trouble implementing it. I'm looking at a tutorial and trying to implement the copy constructor,copy assignment operator, and the destructor. This is what i'm looking at http://www.cplusplus.com/doc/tutorial/classes2/ and i'm about half way down the page.

 

Modified the .h to try and include the assignment operator. It more matches what the tutorial has but I don't think it's going to do what i want. I don't exactly understand how to use the assignment operator tho. Still crashes when I try to make a SmartImage object which means something is majorly wrong right?

class SmartImage
{
    private:
            bool image_loaded;
            
    public:
           SmartImage();
           ~SmartImage();
           // copy constructor:
           SmartImage (const SmartImage& param) : surface(new SDL_Surface(param.content())) {}
           //access content
           const SDL_Surface& content() const {return *surface;}
           SDL_Surface *surface;
           void load(std::string filename);
           void copy_surface(SDL_Surface *target_surface);
};

#include"SmartImage.cpp"

the .cpp

SmartImage::SmartImage()
{
 surface=NULL;
 image_loaded = false;     
}

SmartImage::~SmartImage()
{
 SDL_FreeSurface(surface);      
}

void SmartImage::load(std::string filename)
{
    if(image_loaded == false)
    {
     surface = load_image(filename);
     image_loaded = true;
    }    
    else
    {
     SDL_FreeSurface(surface);
     surface = load_image(filename);
    }  
}

void SmartImage::copy_surface(SDL_Surface *temp_surface)
{
     if(image_loaded == true)
     {
      SDL_FreeSurface(surface);               
     }
 surface = SDL_DisplayFormat(temp_surface); 
 image_loaded = true;     
}


#4 exOfde   Members   -  Reputation: 809

Like
1Likes
Like

Posted 05 December 2013 - 03:59 PM

You should check if SDL_DisplayFormat return NULL is that the case perhaps there is your error because should a other function need to access your surface but found only NULL could cause your problem. you should in general check if got SDL_Surface notequal NULL from the SDL functions.


Edited by exOfde, 05 December 2013 - 04:02 PM.


#5 Ludus   Members   -  Reputation: 970

Like
1Likes
Like

Posted 05 December 2013 - 04:20 PM

The constructor function is called as soon as that object is created, so depending on where your object is created, this may occur before the SDL_Init of your program is even called. Depending on how you've set up your program, your constructor may be calling an SDL function before SDL has been initialized, which may be the source of your problem.

 

Edited for clarification due to a miswording.


Edited by Ludus, 07 December 2013 - 11:05 AM.


#6 Álvaro   Crossbones+   -  Reputation: 12848

Like
2Likes
Like

Posted 05 December 2013 - 07:04 PM

#include"SmartImage.cpp"

 

What is that?



#7 LatchGameDev   Members   -  Reputation: 165

Like
1Likes
Like

Posted 05 December 2013 - 07:25 PM

The constructor function is called as soon as that object is created, so if your object is not dynamically allocated to memory during run time this will be before the main function of your program is even called. Depending on how you've set up your program, your constructor may be calling an SDL function before SDL has been initialized.

 

 

OMG I had no idea that copy constructor would run before main. I changed it to a pointer and now use the new keyword when I get into main. It stopped the crashing and after a few more hours of poking around I got it to work how I wanted to. So thank you, I feel like I need to research pointers more. I get the concept behind them but actually using them is giving me trouble. It seems I only run into problems with SDL_Surface tho.

 

 

#include"SmartImage.cpp"

What is that?

 

 

 

It's the .cpp file to the .h I don't see a problem with that line of code. I also listed the .cpp in my posts soooooooo what's up?



#8 Brother Bob   Moderators   -  Reputation: 8010

Like
1Likes
Like

Posted 05 December 2013 - 07:37 PM

 


#include"SmartImage.cpp"

What is that?

 

 

 

It's the .cpp file to the .h I don't see a problem with that line of code. I also listed the .cpp in my posts soooooooo what's up?

 

The problem is that you cannot use your SmartImage header in two or more files across your project. Each time you include SmartImage.h, it will also include SmartImage.cpp and all its definitions. If you include SmartImage.h in more than one file in your project, you will consequently have more than one definition of each of the class member functions and the linker will fail to do its job.

 

Add the file SmartImage.cpp to your project instead and compile it as a separate function. And, as the file is shown at the moment, it has to include the necessary headers as well in order to compile properly.



#9 LatchGameDev   Members   -  Reputation: 165

Like
1Likes
Like

Posted 05 December 2013 - 08:01 PM

Is it common practice to include the header file more then once? In my main file i just have .h after .h listed. I always thought this way was better since I used to do it like

a.h

a.cpp

b.h

b.cpp

c.h

c.cpp

 

Now a days I do

 

a.h

b.h

c.h

 

with the .cpp included from the .h



#10 Ludus   Members   -  Reputation: 970

Like
1Likes
Like

Posted 05 December 2013 - 08:02 PM


OMG I had no idea that copy constructor would run before main. I changed it to a pointer and now use the new keyword when I get into main. It stopped the crashing and after a few more hours of poking around I got it to work how I wanted to. So thank you, I feel like I need to research pointers more. I get the concept behind them but actually using them is giving me trouble. It seems I only run into problems with SDL_Surface tho.

 

Just some advice, I recommend avoiding any use of SDL functions within constructors or destructors (the same issue will occur if you quit SDL before your destructors are called when the program closes). Simply create an initializing function and a clean up function which fulfill the same purpose that your constructors and destructors do now. Call the initializing function after you initialize SDL and before your game loop, and call the clean up function after your game loop and before you call SDL_Quit();

This way you can avoid needing to dynamically allocate your object after initializing SDL. You should avoid dynamically allocating anything unless it's absolutely required.



#11 Pink Horror   Members   -  Reputation: 1133

Like
0Likes
Like

Posted 05 December 2013 - 08:44 PM

Is it common practice to include the header file more then once? In my main file i just have .h after .h listed. I always thought this way was better since I used to do it like

a.h

a.cpp

b.h

b.cpp

c.h

c.cpp

 

Now a days I do

 

a.h

b.h

c.h

 

with the .cpp included from the .h

 

If you really wanted to compile everything together in one big file, why not put just the cpp files into that one big file? Each cpp file should include all the headers it needs.

 

However... you can compile a bunch of cpp files separately and let the linker do the linking.

 

Unfortunately, I work somewhere that does "bulk builds", and I work on one particular piece of software that basically includes every relevant cpp file into one big cpp file, so my "professional" environment is almost exactly how you work, though it doesn't sound like you knew there was a choice. The main difference I guess it the build step that makes the one file that includes all the cpp files for me. After a few years of neglect, none of the cpp files have a meaningful set of headers on them any more, and static is practically meaningless as a storage type or as a way to throw a simple helper function into a file. Trust me, it's more fun to have separate compile units.

 

It's common practice for a header file to include what needs to be shared with other cpp files, while the cpp file includes only what it needs to compile all by itself. For example, SmartImage.cpp would include SmartImage.h, because that cpp needs to know the definition of the SmartImage class in order to compile. In your situation, it doesn't even look like you really use header files for anything, and if you got into some circular dependency situation where you really needed headers, you probably wouldn't be able to fix it without changing how you include files.

 

So, how do you compile your code? Depending on what tools you use, I'm sure someone here can help you set up your project.



#12 SeanMiddleditch   Members   -  Reputation: 5069

Like
0Likes
Like

Posted 05 December 2013 - 08:47 PM

Use std::unique_ptr for your SDL_Surface. This will ensure that compiler-generated copy constructor/assignment operators cannot be used. If you write movement constructor/assignment (or use a recent compiler that knows how to auto-generate them), the type will still work in a std::vector.

If you're using a recent Clang/GCC or the latest Visual Studio then this gets even easier as you can probably used defaulted movement operators. VS 2010/2011 will require you to write them manually, however.

Moving works much better than copying when a vector is resized or when elements are put into them. It avoids any of the nastiness inherent with trying to share ownership or deep copying resources.

#13 EarthBanana   Members   -  Reputation: 870

Like
0Likes
Like

Posted 06 December 2013 - 04:53 AM

As a poster previously said - you gotta make sure you do SDL initializations before you try and create any copies of your class instances

because you call an SDL function in your copy constructor

 

As for the comment on your including the cpp - usually you would include your files in the following manner

 

 

SmartImage.h

#ifndef SMARTIMAGE_H     // Include header guards (so your declarations wont be included more than once)
#define SMARTIMAGE_H

class SmartImage
{
    public:
           // your class declarations
    private:
           // your class declarations
};

#endif

SmartImage.cpp

#include "SmartImage.h"   // Include your header file from your .cpp file - only include cpp files if there is no other way

SmartImage::SmartImage()
{
     // your code    
}

SmartImage::~SmartImage()
{
     // your code  
}

// copy constructor
SmartImage::SmartImage (const SmartImage& param)
{         
    // your code      
}

void SmartImage::load(std::string filename)
{
   // your code
}

void SmartImage::copy_surface(SDL_Surface *temp_surface)
{
     // your code
}

main.cpp

#include "SmartImage.h"     // now you can use SmartImage here

int main()
{
    // your code - make sure you initialize SDL before you make any instance copies of your image class
}

Edited by EarthBanana, 06 December 2013 - 04:54 AM.


#14 Bohdan Kornienko   Members   -  Reputation: 122

Like
0Likes
Like

Posted 07 December 2013 - 06:48 AM

Here is my realization of your class.

 

/*
/*
 * SmartImage.h
 *
 *  Created on: Dec 7, 2013
 *      Author: Bohdan Kornienko
 */
 
#ifndef SMARTIMAGE_H_
#define SMARTIMAGE_H_
 
#include <SDL/SDL.h>
#include <SDL/SDL_image.h>
#include <string>
#include <iostream>
 
class SmartImage
{
private:
    SDL_Surface *surface;
    bool image_loaded;
    SDL_Surface* load_image(std::string file_name);
 
public:
    SmartImage();
    SmartImage(const SmartImage& param);
    virtual ~SmartImage();
 
    SDL_Surface* content() const
    {
        return surface;
    }
 
    void load(std::string filename);
    void copy_surface(SDL_Surface *target_surface);
    bool is_image_loaded();
    bool clean_up();
};
 
 
 
#endif /* SMARTIMAGE_H_ */
 

 

/*
/*
 * SmartImage.cpp
 *
 *  Created on: Dec 7, 2013
 *      Author: Bohdan Kornienko
 */
 
#include "SmartImage.h"
 
SmartImage::SmartImage()
        : surface(NULL), image_loaded(false)
{
}
 
SmartImage::SmartImage(const SmartImage& param)
        : image_loaded(true)
{
    surface = SDL_DisplayFormat(param.content());
 
    std::cout << &surface << std::endl;
}
 
SmartImage::~SmartImage()
{
    // TODO: put something here when is need
}
 
SDL_Surface* SmartImage::load_image(std::string file_name)
{
    SDL_Surface* loadedImage = NULL;
    SDL_Surface* optimizedImage = NULL;
 
    loadedImage = IMG_Load(file_name.c_str());
 
    if(loadedImage == NULL)
    {
        std::cerr << "Image [" << file_name << "] is not loaded\n";
        image_loaded = false;
 
        return NULL;
    }
 
    optimizedImage = SDL_DisplayFormat(loadedImage);
    SDL_FreeSurface(loadedImage);
 
    if(optimizedImage == NULL)
    {
        image_loaded = false;
        return NULL;
    }
 
    std::cout << "Image [" << file_name << "] loaded\n";
 
    image_loaded = true;
    return optimizedImage;
}
 
void SmartImage::load(std::string filename)
{
    if(image_loaded == false)
    {
        surface = load_image(filename);
    }
    else
    {
        SDL_FreeSurface(surface);
        surface = load_image(filename);
    }
 
    std::cout << &surface << std::endl;
}
 
void SmartImage::copy_surface(SDL_Surface *temp_surface)
{
    if(image_loaded == true)
    {
        SDL_FreeSurface(surface);
    }
    surface = SDL_DisplayFormat(temp_surface);
    image_loaded = true;
}
 
bool SmartImage::is_image_loaded()
{
    return image_loaded;
}
 
bool SmartImage::clean_up()
{
    SDL_FreeSurface(surface);
    image_loaded = false;
 
    return true;
}
 

 

And here some of using this class example:

 

/*
/*
 * smartimg.cpp
 *
 *  Created on: Dec 7, 2013
 *      Author: Bohdan Kornienko
 */
 
#include <SDL/SDL.h>
#include <SDL/SDL_image.h>
#include <string>
#include "SmartImage.h"
 
void apply_surface(int x, int y, SDL_Surface* source, SDL_Surface* dest)
{
    SDL_Rect offset;
 
    offset.x = x;
    offset.y = y;
 
    SDL_BlitSurface(source, NULL, dest, &offset);
}
 
int main(int argc, char **argv)
{
    if(SDL_Init(SDL_INIT_EVERYTHING) == -1)
    {
        return -1;
    }
    const int SCREEN_WIDTH = 640;
    const int SCREEN_HEIGHT = 480;
    const int SCREEN_BPP = 32;
 
    SDL_Surface* screen = 0;
 
    screen = SDL_SetVideoMode(
            SCREEN_WIDTH, SCREEN_HEIGHT, SCREEN_BPP, SDL_SWSURFACE);
 
    if(screen == NULL)
    {
        return 1;
    }
 
    SDL_WM_SetCaption("Hello world", NULL);
 
    SmartImage smart_image;
    smart_image.load("hello.bmp");
 
    if(!smart_image.is_image_loaded())
    {
        return -2;
    }
 
    SmartImage second_smart_image(smart_image);
    smart_image.clean_up();
 
    apply_surface(0, 0, second_smart_image.content(), screen);
 
    if(SDL_Flip(screen) == -1)
    {
        return -2;
    }
 
    SDL_Delay(3000);
 
    second_smart_image.clean_up();
 
    SDL_Quit();
 
 
    std::cout << SDL_GetError() << std::endl;
 
    return 0;
}
 

 

I put here some of debug information. So if u don't need it. U can delete it.

I hope my solution will help you.


Edited by Bohdan Kornienko, 07 December 2013 - 06:53 AM.


#15 Bregma   Crossbones+   -  Reputation: 4965

Like
0Likes
Like

Posted 07 December 2013 - 07:33 AM


Here is my realization of your class.
  1. Why do you use a separate boolean variable to indicate if the surface member is NULL or not?
  2. You do not provide an assignment operator, thus violating the rule of three.  You're going to be in for a bad time.
  3. Separate resource acquisition from construction is very 1970s C-retro and all, but what possible advantage does it convey in this scenario?  It means you need to explicitly acquire and release resources separately from variable scope, which means each end every use of the object has to check to see if the object is valid.  It means each and every possible scope exit needs to explicitly check if the object is valid and then release the resources, otherwise resources leak.  It means more code and more opportunity for coding errors.  It means harder to find bugs.  It means your project will probably never ship.

Other than that, it looks fine.


Stephen M. Webb
Professional Free Software Developer

#16 Bohdan Kornienko   Members   -  Reputation: 122

Like
0Likes
Like

Posted 07 December 2013 - 11:13 AM

 


Here is my realization of your class.
  1. Why do you use a separate boolean variable to indicate if the surface member is NULL or not?
  2. You do not provide an assignment operator, thus violating the rule of three.  You're going to be in for a bad time.
  3. Separate resource acquisition from construction is very 1970s C-retro and all, but what possible advantage does it convey in this scenario?  It means you need to explicitly acquire and release resources separately from variable scope, which means each end every use of the object has to check to see if the object is valid.  It means each and every possible scope exit needs to explicitly check if the object is valid and then release the resources, otherwise resources leak.  It means more code and more opportunity for coding errors.  It means harder to find bugs.  It means your project will probably never ship.

Other than that, it looks fine.

 

 

All right! 

You got me ;)

 

Here class with "rules of three":

/*
 * SmartImage.h
 *
 *  Created on: Dec 7, 2013
 *      Author: Bohdan Kornienko
 */
 
#ifndef SMARTIMAGE_H_
#define SMARTIMAGE_H_
 
#include <SDL/SDL.h>
#include <SDL/SDL_image.h>
#include <string>
 
class SmartImage
{
private:
    SDL_Surface *surface;
 
    SDL_Surface* load_image(std::string file_name);
 
public:
    SmartImage();
    SmartImage(const SmartImage& param);
    virtual ~SmartImage();
 
    SmartImage& operator= (const SmartImage& rhd);
 
    SDL_Surface* content() const;
    bool load(std::string filename);
    void copy_surface(SDL_Surface *target_surface);
    bool is_image_loaded();
    bool clean_up();
};
 
#endif /* SMARTIMAGE_H_ */

 

/*
 * SmartImage.cpp
 *
 *  Created on: Dec 7, 2013
 *      Author: Bohdan Kornienko
 */
 
#include "SmartImage.h"
 
SmartImage::SmartImage()
        : surface(NULL)
{
}
 
SmartImage::SmartImage(const SmartImage& param)
{
    surface = SDL_DisplayFormat(param.content());
}
 
SmartImage::~SmartImage()
{
    // TODO: put something here when is need
}
 
SDL_Surface* SmartImage::load_image(std::string file_name)
{
    SDL_Surface* loadedImage = NULL;
    SDL_Surface* optimizedImage = NULL;
 
    loadedImage = IMG_Load(file_name.c_str());
 
    optimizedImage = SDL_DisplayFormat(loadedImage);
    SDL_FreeSurface(loadedImage);
 
    return optimizedImage;
}
 
bool SmartImage::load(std::string filename)
{
    if(surface == NULL)
    {
        surface = load_image(filename);
    }
    else
    {
        SDL_FreeSurface(surface);
        surface = load_image(filename);
    }
 
    return (surface != NULL ? true : false);
}
 
void SmartImage::copy_surface(SDL_Surface *temp_surface)
{
    if(surface != NULL)
    {
        SDL_FreeSurface(surface);
        surface = NULL;
    }
    surface = SDL_DisplayFormat(temp_surface);
}
 
bool SmartImage::is_image_loaded()
{
    return (surface != NULL ? true : false);
}
 
SDL_Surface* SmartImage::content() const
{
    return surface;
}
 
SmartImage& SmartImage::operator =(const SmartImage& rhd)
{
    if(this != &rhd)
    {
        SDL_FreeSurface(surface);
 
        SDL_Surface* new_surface = NULL;
 
        new_surface = SDL_DisplayFormat(rhd.surface);
 
        surface = new_surface;
    }
 
    return *this;
}
 
bool SmartImage::clean_up()
{
    SDL_FreeSurface(surface);
    surface = NULL;
 
    return true;
}
 


Edited by Bohdan Kornienko, 07 December 2013 - 11:18 AM.





Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS