PDA

View Full Version : Variable reset after constructor



JohnnyKPL
04-29-2015, 02:22 PM
I have a class on a header file but the privates variables on its reset after exiting from the constructor... and, I don't know why they do that! :mad:

Here's the code:
texture.h


#pragma once

#include "global.h"

class Texture
{
private:
static unsigned int staticId;
unsigned int id; // variable that reset

GLuint texture; // variable that reset
int width, height;

protected:
public:
Texture(const char* fileName, int type);
~Texture();

void bind();
};


texture.cpp


#include "texture.h"

unsigned int Texture::staticId = 0;

Texture::Texture(const char* fileName, int type)
{
// Initialize variable id.
id = staticId++;

int _type;
switch (type)
{
case GL_RGB:
_type = SOIL_LOAD_RGB;
break;
case GL_RGBA:
_type = SOIL_LOAD_RGBA;
break;
}

glGenTextures(1, &texture);

glActiveTexture(GL_TEXTURE0 + id);
glBindTexture(GL_TEXTURE_2D, texture);

unsigned char* image = SOIL_load_image(fileName, &width, &height, 0, _type);
glTexImage2D(GL_TEXTURE_2D, 0, type, width, height, 0, type, GL_UNSIGNED_BYTE, image);
SOIL_free_image_data(image);

glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
}

Texture::~Texture()
{
glDeleteTextures(1, &texture);
}

void Texture::bind()
{
// Here the variable "id" and "texture" are resetting and the value is
// something like: 32897154, 43241344, 21214123...
glActiveTexture(GL_TEXTURE0 + id);

glBindTexture(GL_TEXTURE_2D, texture);

glUniform1i(Uniform::SAMPLER, id);
}


P.S: Sorry for my bad english! :p

Alfonse Reinheart
04-29-2015, 05:00 PM
Well, you don't say how you're using this class. I'll assume that you properly initialized OpenGL and have a current context and all that (https://www.opengl.org/wiki/Common_Mistakes#The_Object_Oriented_Language_Probl em).

What's probably happening is that you're copying a class that, by all rights, should not be copied. Your class claims RAII-style ownership (https://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization) of a texture object. The constructor creates it, and the destructor destroys it. But you neglected to implement a copy constructor or otherwise prevent your class from automatically generating one.

And the copy constructor will result in multiple copies of the same class that own the same texture. Which means, sooner or later, one of these classes will execute their destructor, which deletes the texture.

The correct way to handle this is to make the class non-copyable. Which means that you'll have to rely on moving the class instances, or you must dynamically allocate them (possibly sticking them in a reference-counted smart pointer like std::shared_ptr).

You'd do this as follows:



class Texture
{
private:
static unsigned int staticId;
unsigned int id;

GLuint texture;
int width, height;

void DestroyTexture();

public:
Texture(const char* fileName, GLenum type);
~Texture() { DestroyTexture(); }

Texture(const Texture&) = delete; //No copy constructor.
Texture(Texture &&other) : id(other.id), texture(other.texture), width(other.width), height(other.height)
{
other.texture = 0;
other.id = 0;
}

Texture &operator=(const Texture&) = delete; //No copy-assignment.
Texture &operator=(Texture &&other)
{
DestroyTexture();
id = other.id;
texture = other.texture;
width = other.width;
height = other.height;
}

};


This of course uses C++11. If your compiler is developmentally challenged, then you'll have to use something like boost::noncopyable (http://www.boost.org/doc/libs/master/libs/core/doc/html/core/noncopyable.html).

JohnnyKPL
05-01-2015, 11:44 AM
Thank you for your reply but your answer doesn't help me... I've tried to "apply" your header class sample but I get the same error D:

Oh sorry, I've forgot to post how I use the textures, here a piece of my code:




enum
{
BACKGROUND,
GRID
};

Game::Game()
{
glEnable(GL_TEXTURE_2D);

glEnable(GL_BLEND);
glBlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA);

for (unsigned int i = 0; i < (sizeof(keys) / sizeof(keys[0])); i++)
{
keys[i] = 0;
}

Texture textures[2] =
{
Texture("sky.png", GL_RGBA),
Texture("frame.png", GL_RGBA)
};

background = new Background(textures[BACKGROUND], 1.0f);

grid = new Grid(5, textures[GRID], 0.5f);

shader = new Shader();

shader->activate();
}

Alfonse Reinheart
05-01-2015, 12:14 PM
Thank you for your reply but your answer doesn't help me... I've tried to "apply" your header class sample but I get the same error D:

I don't know what those quotes are intended to mean. So it's not clear if you used it correctly or not.


Oh sorry, I've forgot to post how I use the textures, here a piece of my code:

If you are not using C++11, then the code you posted will fail. Guaranteed. You cannot allow copying of Texture objects (at least, as you've written them), and your code is copying Texture objects.

If you're using a proper, move-only C++11-style Texture object, I don't recall exactly whether aggregate initialization like that will copy or move from the braced-init-list. However, if the Texture class properly removes the copy constructor, if the compiler tried to use it, you'd get an error. So if it compiles, you should get a good, moved-from value.

That all being said, there's also a much more basic issue at play here. You still need to fix what I just talked about. But you also need to fix this:



Texture textures[2] =
{
Texture("sky.png", GL_RGBA),
Texture("frame.png", GL_RGBA)
};


textures is a local variable. That means that it, and its contents will be destroyed upon exiting its scope. Namely, the end of Game::Game.

I see that you pass the contents of this array to the constructors of Background and Grid. If those functions are taking the Texture objects by value... that would require a copy, so see above about why that's bad. So I have to assume they're taking them by (non-const) reference.

In which case, those objects now hold references to local variables. Therefore, at the exit of Game::Game, those objects will hold references to objects that have been destroyed.

This is functionally no different from you doing this:



std::string &Stuff()
{
std::string local("foo");
return local;
}


This is always wrong.

You need to define ownership for your Texture objects. Some code needs to be responsible for owning them (which means deleting them). Since Background and Grid seem to need Texture to still exist while those object instances exist, then they must claim ownership of whatever Texture object is given to them.

If you want those objects to uniquely own their Textures (so that no other object shares ownership rights with them), then you should either pass a Texture by-value (in which case, your Background/Grid constructors should move those Texture objects into member variables), or you should pass them a std::unqiue_ptr to the Texture. In both cases, the destructor of Background/Grid will handle destroying the Texture automatically.

If you want shared ownership of Textures, such that the object is only destroyed when all of its owners have been destroyed, then you need to use a std::shared_ptr<Texture>.

JohnnyKPL
05-04-2015, 08:42 AM
I still don't understand where I'm wrong...
In Java for example, if I do something like:


public class Person {

private String name;
private String surname;

public Person(String name, String surname) {
this.name = name;
this.surname = surname;
}

public void print() {
print(name + " " + surname);
}
}

And then I create two object "Person":


void main() {
Person mark = new Person("Mark", "McDonald");
Person jack = new Person("Jack", "Ford");
mark.print();
jack.print();
}

The output would be:


Mark McDonald
Jack Ford

So, I mean, WHY in C++ it doesn't work!? :mad:
And how can I resolve that?

I don't know where I've copied the object "Texture", I've only created two object and set them parameters D;

P.S: Sorry, but I'm only 14, mind me please :(
Alfonse thank you for your help ;)

JohnnyKPL
05-04-2015, 09:07 AM
Wait.... oooooh, now it works!!! Thank you <3<3

Alfonse Reinheart
05-04-2015, 10:02 AM
So, I mean, WHY in C++ it doesn't work!?

You didn't show the C++ equivalent. And the C++ equivalent (http://ideone.com/mzHVJo) seems to work fine for me.


I don't know where I've copied the object "Texture", I've only created two object and set them parameters D;

That's because you're a Java programmer learning C++. You're conditioned to think in terms of references to objects, while C++ is a value-oriented language.

In Java, a variable isn't an object; it is a reference to an object. In C++, unless a variable is explicitly a pointer or reference, that variable actually is an object.

Take this lines:



var = SomeFunc(...);


In Java, this will stick a new reference into `var`. In C++, unless `var` is a pointer, this will always copy the value returned from `SomeFunc`. So whatever data `SomeFunc` returned must be copied by some process into the object currently stored by `var`.

The same is true of:



SomeFunc(..., var, ...);


In Java, `SomeFunc`'s parameter is initialized with the same reference as `var`. And thus, `SomeFunc` in Java will reference the same object that the caller is referencing. If `SomeFunc` modifies this parameter, then the caller will see the modification of that object.

In C++, unless the parameter is explicitly a reference, this will copy `var`into `SomeFunc`'s parameter. Which means that if `SomeFunc` modifies the parameter, the caller will not see the modification. Because what's being modified is a copy, not the original.

So what does it mean to copy an object? Well, that depends. In C++, by default, all objects are copyable. But because there is a default, it has no idea what the objects internals actually mean. Therefore, default copying simply does a member-wise copy operation.

However, C++ does offer a way to explicitly specify how a copy operation works. And here's where we get into your Java example and the problem you're having in code.

When initializing your objects, your Java example did this:


Person mark = new Person("Mark", "McDonald");

If you did an exact replica of this in C++, you'd get a compile error. Obviously, that's because `new` returns a pointer, while `Person mark` is an object variable, not a pointer. So you remove the `new` and the compiler shuts up:


Person mark = Person("Mark", "McDonald");

And this would work just like your Java code. (http://ideone.com/s6YDdl)

(Pedantic note: I am now about to say something that is completely untrue of C++. However, I'm lying for a very good reason: the truth is complicated and only muddles the explanation of the problem.)

Given what I've told you, this creates a new `Person` object and then copies it into `mark` (see pedantic note above). However, if you change the Ideaone code to initialize the two variables like this, rather than the way I did originally, you'll find that your code works just fine.

So why does this example work when your Texture object copying doesn't? Well, that has to do with a concept called "ownership". And this concept is why copying is something you have to think about in C++.

Deep down, you probably know that a string type has to allocate memory. And because C++ doesn't have a garbage collector to "save" you from memory leaks, someone has to both allocate that memory and deallocate that memory when you're finished with it. This is the primary job of `std::string`.

The constructor of `std::string` allocates memory. The destructor of `std::string` deallocates any memory previously allocated by the constructor. So that means, somewhere inside std::string, it stores a pointer to some array of characters. One of its member variables is a `char*`. It's allocated in the constructor, and deallocated in the destructor.

The `std::string` instance is said to own this array of characters. It created it, and it claims responsibility for destroying it.

So... what happens when you copy a string?

If `std::string` used the default copy constructor (which just does a member-wise copy operation), what would this do:



std::string obj = std::string("A String");


This creates a string, then copies that string into `obj` (remember the pedantic note). Which means that there are now two std::string objects: the one created by `std::string("A String")` and `obj`. So... what happens to the first object?

The first object is called a "temporary"; it is so named because it is... temporary. It will last only for as long as it needs to. And this temporary does not need to last longer than it takes to copy its data into `obj`. Therefore, the final step of this line is to destroy the temporary.

But wait. We just said that std::string's destructor will deallocate its array of characters. And we just copied a pointer to that array of characters into `obj`. If the temporary is destroyed, then the array will be deallocated. But `obj` still has a pointer to it.

Which would mean that `obj` has a pointer to deallocated memory. Therefore, any attempt to use `obj` will fail.

And yet, this code works (again, ignoring the lie); it is legal to copy std::string to your heart's content. Why?

Because std::string implements "value semantics".

See, every std::string instance knows that it is responsible for its own memory. Therefore, it overrides the default copying mechanics, to ensure that every instance remains independent on copying. When you copy a std::string, the destination string will allocate its own array of characters, then copy the characters from the incoming string into that block. After the copy, both the source and destination strings have an array of characters, but they're a separate array. Thus, you can delete one array without affecting the other.

The ability to copy an object such that the resulting object si completely separate from the original is called "value semantics". It works just like this:



int val1 = 1;
int val2 = val1;
val2 = 5;
printf("%i", val1);


This will always print 1. Because `val2` is a completely separate value from `val1`; changing `val2` will never affect `val1`'s value. The copy stored only the current value of `val1` into `val2`; any subsequent changes to one will not affect the other.

`std::string` is designed to work the same way. Value semantics, for types that own some resource, means that copying them must copy the resource as well. Not merely copying the reference/pointer to that resource. But the resource itself.

`Person` inherits this because the default copy mechanism copies all of the elements member-wise. And this member-wise copy can use the overridden copy mechanism of `std::string`, even though `Person` itself uses the default.

Which brings us at last to your `Texture` class. This has the exact same problem that `std::string` would. Just like `std::string`, it stores a "pointer" to some other data. And therefore, its destructor needs to delete that data. `Texture` owns the OpenGL texture object it creates.

Which means, just like `std::string`, you can no longer use the default copy semantics. Otherwise, after a copy, you will have two objects that own the same OpenGL texture object. One of them will be destroyed first; it doesn't matter which one, because once it happens, the other will have an OpenGL texture object that has been destroyed.

My suggestion above was to implement something called "move semantics". This is where you are allowed to "copy" the object, but only by stealing the data. This represents a transfer of ownership from one object to another. First, object A owned it, but then you transfer it to object B. After the transfer, object B alone owns the memory; object A is left with nothing.

This represents "unique ownership" of the memory. At any time, only one object ever claims ownership of the resource.

----

Pedantic truth: The reality is that a statement of the form `Typename varName = Typename(...);` never copies anything. It initializes `varName` directly in place, without calling any copy operation. Now, in order for this statement to work, `varName` must have a copy constructor which can be called from this location. But it won't actually use the copy constructor. Only one object will be created, no temporary will be made, and thus no errors could come from it.

However, it's easier to talk about that case than `Typename varName = ...; varName = Typename(...);`, which would legitimately be copying the value.

JohnnyKPL
05-04-2015, 11:30 AM
Thanks ;)
Finally, the problem is in the game.cpp that I've construct two Texture object but they destroyed just came out from the Game constructor. So I've made a vector in the header and saved the variable on it.
Thank you for your precious help :D

JohnnyKPL
05-04-2015, 11:30 AM
Thanks ;)
Finally, the problem is in the game.cpp that I've construct two Texture object but they destroyed just came out from the Game constructor. So I've made a vector in the header and saved the variable on it.
Thank you for your precious help :D