0

I'm writing code to fill a texture class with data from an image file. As far as I can tell, the code and implementation works, however it causes a segmentation fault on entering the main loop of my program. These 4 lines, when removed, remove the segmentation fault:

Texture* texture = new Texture(); // Dynamically allocate texture object to texture pointer
bool success = loadTexture(texture, "C:/pathToImage/image.png"); // Function that gets image data
cout << success << endl; // Print out success or not
textures.push_back(*texture); // put texture in a vector of textures

Edit: texture.h

class Texture {
public:
    Texture();
    Texture(const Texture&);
    ~Texture();
    Texture& operator=(const Texture&);
public:
    void init();
    int width, height;
    std::vector<unsigned char> pixmap;
    GLuint id;
};

texture.cpp:(The init function is edited out, as it doesn't pertain to the error, it isn't even called.)

Texture::Texture() : width(0), height(0), pixmap(), id(int(-1)){}
Texture::Texture(const Texture& other) : width(other.width), height(other.height), pixmap(other.pixmap), id(other.id){}
Texture::~Texture()
{
width = 0;
height = 0;
delete &pixmap;
id = int(-1);
}
Texture& Texture::operator=(const Texture& other) {
width = other.width;
height = other.height;
pixmap = other.pixmap;
id = other.id;
}

I'm assuming it has something to do with the texture pointer, however I've tried several methods of doing this same thing, and they've all caused the same segmentation fault. Could someone explain what's causing this?

eightShirt
  • 1,457
  • 2
  • 15
  • 29
Colin Moore
  • 25
  • 5
  • More code please. What you have shown so far is not enough to see the problem. – Kyurem Apr 28 '13 at 21:30
  • 1
    I would guess that Texture is not following the rule of three. – john Apr 28 '13 at 21:31
  • @Kyurem What would you like? This is the only code I have that causes the error- removing it removes the error. – Colin Moore Apr 28 '13 at 21:31
  • 1
    Kill the pointer. It's useless here. – chris Apr 28 '13 at 21:31
  • Something about the Texture class would help, does it have a destructor, does it have a copy constructor, what are it's data members etc. – john Apr 28 '13 at 21:32
  • 1
    It is odd, although not necessary incorrect, that you are dynamically allocating a Texture, but then putting a copy of it into textures. – Vaughn Cato Apr 28 '13 at 21:32
  • I'll edit in the texture class. – Colin Moore Apr 28 '13 at 21:33
  • Looks like @john is right. Again, the pointer in the class can also be replaced with something that manages the memory properly. – chris Apr 28 '13 at 21:34
  • @VaughnCato Originally I simply instantiated it as Texture texture() and passed a pointer to it in loadTexture(), however that didn't work, so I tried this. – Colin Moore Apr 28 '13 at 21:36
  • @ColinMoore, `Texture texture;`. Yours declares a function. – chris Apr 28 '13 at 21:43
  • Going off of john's post, I edited the texture class, still the same error. – Colin Moore Apr 28 '13 at 21:55
  • @ColinMoore Your copy constructor is not correct, neither is your assignment operator. You are just making explicit what was implicit before. Your copy constructor must allocate some memory for the pixmap, otherwise you have exactly the same problem you had before. – john Apr 28 '13 at 22:04
  • @ColinMoore I'll update my answer with one possiblity for the copy constructor. – john Apr 28 '13 at 22:06

2 Answers2

0

Your error is because the Texture class is not following the rule of three. 'Any class which has one of a destructor, copy constructor or assignment operator most likely needs all three'.

What is The Rule of Three?

Specifcally what is probably happening is that because you have not defined a copy constructor your object is being shallow copied into the vector. This results in two Texture objects sharing the same pixmap data. When one of those objects is destructed, this invalidates the pixmap data in the other object.

For instance one possibility for the copy constructor is this

Texture::Texture(const Texture& other) : width(other.width), height(other.height), pixmap(NULL), id(other.id)
{
    pixmap = new unsigned char[width*height];
    memcpy(pixmap, other.pixmap, width*height);
}

Only one way to do it. cHao is suggesting other possibilities.

Community
  • 1
  • 1
john
  • 85,011
  • 4
  • 57
  • 81
  • Depending on whether the `pixmap` array should be shared, you could also use a smart pointer to it (requires C++11 or Boost, or some other library with decent shared pointers), or a `vector`, which would be copied for you. Either would avoid even needing a destructor at all, let alone the other two. – cHao Apr 28 '13 at 21:39
  • Working on implementing aforementioned "Rule of Three". I wasn't aware of it until now, thanks for the heads up. – Colin Moore Apr 28 '13 at 21:44
  • Same error still applies, even after implementing the rule of three. See the updated post above for the new texture class – Colin Moore Apr 28 '13 at 21:52
  • @ColinMoore: Do the constructors and assignment operator allocate a new array for `pixmap`? Just copying the pointer would give you the same problem as the default functions. – cHao Apr 28 '13 at 21:53
  • `Texture operator=(Texture);` should be `Texture& operator=(const Texture&);` Perhaps you could show us the code for the destructor, copy constructor etc. – john Apr 28 '13 at 21:54
  • I changed it, still the same error, but thanks for the fix. I also added the bodies of the constructor, copy constructor, destructor, and assignment operator, as you asked. – Colin Moore Apr 28 '13 at 21:58
  • @ColinMoore: And where do you ever set a non-null value for `pixmap`...? – cHao Apr 28 '13 at 22:16
0

(Note: I'm assuming Texture() properly allocates an array, or at least zeroes out the pointer. If it doesn't do either, that's your problem. If it only does the latter, any attempt to use the array would cause UB, but at least the destructor wouldn't.)

As has been mentioned, pixmap isn't being handled properly. If even a temporary object were destroyed, the array it points to would be deallocated, leaving you with dangling pointers in every other instance that had that same pointer in pixmap.

You could add a copy constructor and assignment operator that copy the pointed-to array for you, fulfilling the "rule of three". But in my opinion, a cleaner solution would be to follow the "rule of zero". :) Use a type that does the copying and destruction automatically, and you obviate the need for special functions to handle copying, assignment, and destruction.

A std::vector or std::array could easily do this, if the array should be copied. (Which one you'd use may depend on whether the array needs to be resizable. std::vector will always work, but std::array may be better for a few odd cases with static-sized arrays.)

If you need the data to be shared, a std::shared_ptr (C++11) or boost::shared_ptr would keep track of how many pointers there are. Only when all the shared_ptrs to it are destroyed, would the array be deallocated.

Note that with any of these solutions, the array's memory would be taken care of for you. If this is the only reason for the destructor etc, then you no longer need one, and thus the "rule of three" no longer comes into play.

Glorfindel
  • 21,988
  • 13
  • 81
  • 109
cHao
  • 84,970
  • 20
  • 145
  • 172
  • I applied what you said, replacing pixmap with a vector. The same error occurs, this time earlier in the program. – Colin Moore Apr 28 '13 at 22:19
  • @ColinMoore: Do you init it with `NULL`, like you did with the pointer? That won't fly. Just let it create itself, and before you use it, `resize` it to a size that will hold the data you need. To me this seems to be your primary issue; i don't see where you ever filled even the pointer `pixmap` with a usable value. – cHao Apr 28 '13 at 22:23
  • If you look at the updated constructor in my post, I think it will answer you question. I initialize it with a default constuctor. And you don't see where I filled it with a usable value because that is done in the function which a pointer to the texture is being passed into. – Colin Moore Apr 28 '13 at 22:26
  • @ColinMoore You don't delete vectors. If you use a vector then you can remove your destructor, copy constructor and assignment operator. I think you should take the time to read that link on the rule of three I posted. Or read a book, this subject will be covered extensively in any good C++ book, and you really can't program C++ properly without understanding the issues of resource management and object copying. – john Apr 28 '13 at 22:28
  • @ColinMoore: Please don't change the code that's already in your question. It invalidates all the answers; it looks now like i'm telling you to do stuff you're already doing. If you want to mention your changes, include an "Update:" or something after the existing content of the question. – cHao Apr 28 '13 at 22:28
  • @cHao I'll keep that in mind, however if you could do the same, it would be nice. I have had to read through you post several times looking for what you edited in order to find any helpful advice. – Colin Moore Apr 28 '13 at 22:34
  • @ColinMoore: The function this Texture is being passed to...how is it setting or filling `pixmap`? – cHao Apr 28 '13 at 22:37
  • @john Are you saying that the error is that the texture variable is being deleted after the end of the end of the code block is located in, causing an error with the data contained? If so, could you give me quick explaination of how I would avoid that? – Colin Moore Apr 28 '13 at 22:37
  • @ColinMoore, Write a proper copy constructor, not any copy constructor, one that works, that's all. I've shown you one way, cHao has been showing another. Ditto for assignment operator. Unless you can copy Texture objects correctly you are never going to be able to use them in a std::vector. – john Apr 28 '13 at 22:40
  • @ColinMoore This is fundamental C++, if you want to be a C++ programmer you have to learn this stuff. A forum maybe isn't the best teaching environment, so read your C++ book, or look it up on the internet. – john Apr 28 '13 at 22:44
  • @cHao here's the pertinent line of code(tempBuffer is a unsigned char array filled with the texture data): texture->pixmap.insert(texture->pixmap.begin(), tempBuffer, tempBuffer + (texture->width * texture->height)); – Colin Moore Apr 28 '13 at 22:44
  • @ColinMoore: That should be fine. Personally, i'd consider `std::copy` (either to `std::back_inserter(texture->pixmap)` as is, or to `texture->pixmap.begin()` after resizing `pixmap`). But that's just personal preference; as long as the `width` and `height` fields and `tempBuffer` variable have appropriate values in them before the copy, and `tempBuffer` actually contains `width * height` entries, it ought to work either way. – cHao Apr 28 '13 at 22:57
  • @john I guess that's what i'll have to do. Thanks to you and cHao both. – Colin Moore Apr 28 '13 at 23:01