Sign in to follow this  
danielGame

Pointer help please!!!

Recommended Posts

Hi all, I have an AABB class that has four float pointers to store the box. I want them as pointers so that when the object they are bounding changes/moves, the box changes too. This works fine for my character class that stores four floats, and an AABB which is constructed from the four floats, i.e, x = 2; x1 = 2; y = 1; y1 = 2; box = AABB(&x,&x1,&y,&y1); This works for the character class so when the character moves (by changing x, x1, y and y1) the values that the box point to change too. However, it doesn't work for my CollectableObject class which does exactly the same thing. The box is created in the correct place but when I change the x and y values in the CollectableObject class, the values that the box points to do not change. Im very stuck. If anybody has any ideas as to what I should try I will greatly appreciate it. Thanks in advance, Daniel.

Share this post


Link to post
Share on other sites
Quote:
Original post by danielGame
Hi all,
I have an AABB class that has four float pointers to store the box. I want them as pointers so that when the object they are bounding changes/moves, the box changes too. This works fine for my character class that stores four floats, and an AABB which is constructed from the four floats, i.e,

x = 2;
x1 = 2;
y = 1;
y1 = 2;
box = AABB(&x,&x1,&y,&y1);

This works for the character class so when the character moves (by changing x, x1, y and y1) the values that the box point to change too. However, it doesn't work for my CollectableObject class which does exactly the same thing. The box is created in the correct place but when I change the x and y values in the CollectableObject class, the values that the box points to do not change. Im very stuck. If anybody has any ideas as to what I should try I will greatly appreciate it.
Thanks in advance,
Daniel.


That code is OK. Try sending us a little bit more code and info.

Share this post


Link to post
Share on other sites
Ok, I create a Collectable instance using this constructor:

Collectable::Collectable(float newX, float newY, float xSize, float ySize, char* texName,Action act)
{
this->x = newX;
this->x1 = x+xSize;
this->y = newY;
this->y1 = y+ySize;
this->type = act;
box = AABB(&x,&x1,&y,&y1);
visible = true;
texture = loadTexture(texName);
}
In a Level class I create an array of these objects by having:

pCollectables[i] = *(new Collectable(rand()%50,rand()%20,1,1,"Data/Sheep/sheep.tga",Collectable::FRIEND));

If I don't use 'new' then the AABB pointers have values of -3e0004 for example.

I then loop over all the collectables and if any are colliding with a platform (AABB to AABB test), I move the collectable instance up using;

void Collectable:: moveUp(float by)
{
this->y += by;
this->y1 += by;
}
This does change the value of y and y1 correctly but the pointers in the AABB class do not seem to be updated if that makes sense?? I.e. to start of, the values of y and y1 are 4 and 5 respectively, the values the the AABB points to are correct, then I call pCollectables[i].moveUp(2) and the values of y and y1 change to 6 and 7 but the values in the AABB instance are still 4 and 5!
P.S. Sorry if the code is not in the correct format, this is my first time posting so I'm not sure how to make in format it correctly!
Thanks,
Daniel

Share this post


Link to post
Share on other sites
Thanks Kaysa, I have done it the same way as you posted. I have just tried making a pointer to a Collectable object and then calling 'moveUp' on that and it works! Slightly confused as to why it works on a pointer to a COllectable but not an object that is in an array!



pCollectables = new Collectable[numFriends];

for(int i =0; i < numFriends; ++i)
pCollectables[i] = *(new Collectable(rand()%50,rand()%20,1,1,"Data/Sheep/sheep.tga",Collectable::FRIEND));

pCollectables[2].moveUp(2);



pCollectables is defined in the header file as; Collectable* pCollectables;
The object in pCollectbles[2] now has an AABB that is not in the correct place.



temp = new Collectable(3,3,1,1,"Data/Sheep/sheep.tga",Collectable::FOOD);
temp->moveUp(2);





temp is defined in the header file as; Collectable* temp;
This however works and the AABB is in the correct place after.
Should I make pCollectables a Collectable** or is there a better way to store an array of pointers?
Thanks,
Daniel

Share this post


Link to post
Share on other sites
You need to implement a copy constructor and assignment operator, which update the pointers in the AABB. The default compiler generated ones are causing your problems.

Share this post


Link to post
Share on other sites
Thanks very much Adam_42, That worked! Any ideas as to why it worked when I used a pointer to a Collectable object rather than an object?
Thanks to everybody for very speedy replies! I very much appreciate it!
Daniel

Share this post


Link to post
Share on other sites
this is because when you are creating objects you are already creating instance of it. just think of that:



Collectable * collector;

collector->moveUp(2);





will that work? No. This will give you runtime error. because you haven't created it yet. Now look at this:



Collectable collector;
collector.moveUp(2);




This will work. so in the array of objects all objects are already initialized, so you have to assign values differently. For example:



void Collectable::Init(float newX, float newY, float xSize, float ySize, char* texName,Action act) {

this->x = newX;
this->x1 = x+xSize;
this->y = newY;
this->y1 = y+ySize;
this->type = act;
box = AABB(&x,&x1,&y,&y1);
visible = true;
texture = loadTexture(texName);

}

pCollectables = new Collectable[numFriends];

for(int i =0; i < numFriends; ++i)
pCollectables[i].Init(rand()%50,rand()%20,1,1,"Data/Sheep/sheep.tga",Collectable::FRIEND));

pCollectables[2].moveUp(2);





IMO assigning with new is not correct because you are creating 2 objects and assigning one to another while keeping the other one (which is new Collectable(...)). And here it comes the memory leak because you don't use the other created instance of Collectable. And removing the leaked instance will eat performance if you want to use thousands of collectable objects (i bet you do).

Share this post


Link to post
Share on other sites
Thanks alot! I had to read up on overloading the assignment operator and it has taught me why most of my destrcutors cause crashes when deleting pointer arrays(mainly due to not having an overloaded assignment operator)!!!
So thanks to everyone!!
Thread solved!!
Daniel

Share this post


Link to post
Share on other sites
Please do not try to just work around this. The design is fundamentally wrong.

I will explain a proper fix at the bottom of the post. But first, you really need to read through a bunch of stuff and make sure you totally understand what has been going on. Because as it stands, you're talking about pointers in a way that suggests a lack of understanding, or at least some imprecise thinking. This won't cut it for programmers.


Collectable::Collectable(float newX, float newY, float xSize, float ySize, char* texName,Action act)
{
this->x = newX;
this->x1 = x+xSize;
this->y = newY;
this->y1 = y+ySize;
this->type = act;
box = AABB(&x,&x1,&y,&y1);
visible = true;
texture = loadTexture(texName);
}




Are 'box', 'visible' and 'texture' actually members of Collectable? I ask because you qualified the other ones with 'this->' but not these ones. (You only need to write 'this->' when there's a name confict with the parameters, and there's no name conflict here. So I guessed that your style is to always write it for members, which would mean you don't think the other ones are members...)

Quote:

If I don't use 'new' then the AABB pointers have values of -3e0004 for example.


This is just plain wrong. First off, the AABB pointers don't have floating-point values at all, because they aren't floating-point variables, they're pointers. And second, you absolutely do not have to write it that way.

In fact, you should never write something like 'x = *(new X(...))'. The correct form for this process is 'x = X(...)'. Using 'new', dereferencing and assigning accomplishes nothing except to leak memory. The variable 'x' has a specific location in memory no matter how you allocate the temporary X instance, and the '=' copies the value over. If you allocate the temporary X with 'new', then the only way to deallocate it is by calling 'delete' on the pointer returned from 'new'; but here there is no way to get that pointer again - it was a temporary and is now thrown away. By comparison, if you just allocate a temporary X, it will be deallocated automatically after the copying is done.

Quote:
This does change the value of y and y1 correctly but the pointers in the AABB class do not seem to be updated


The pointers should not be updated, because 'y' and 'y1' are still in the same place in memory. If you dereference the pointers, you should get the updated values.

Quote:
but the values in the AABB instance are still 4 and 5!


There are no y and y1 values in the AABB instance. There are just pointers to those values.




What is happening is that your Collectible instance gets copied, or assigned, somewhere along the lines. That means that the copy is in a different location in memory. Since you didn't write a copy constructor or assignment operator, the variables in the AABB struct get copied as values. That means that the pointers in the AABB struct point in the same place in the copy as they did in the original.

This is not what you want, because now the AABB struct in the copy points to the data in the original.




To fix the code, what we realize is that what we want is for the AABB struct to always represent the data that's part of the object. The way to do that is to use the AABB struct to represent that data. That is, instead of having the 4 variables x, x1, y, y1, we replace them with the AABB instance.

First, let's define the AABB class. We'll give it a constructor, so that we can create it easily, and the desired functionality for updating it - in our case, the only kind of "updating" we're interested in (so far) is just movement, so we'll add a 'move' function that updates everything. Notice that there is no point in making separate functions to move in each direction; we can generalize easily. It'll have the function to detect intersections, too.


// Please notice how the variable names are chosen.
class AABB {
float left, top, right, bottom; // for example.
// Oops, did they actually represent left, top, *width*, *height*? Oh, well,
// you should be able to figure out how to fix it. But at least with names
// like this, we can be clear about what we mean.

public:
AABB(float left, float top, float right, float bottom): left(left), top(top), right(right), bottom(bottom) {}
void move(float dx, float dy) {
left += dx; right += dy;
top += dy; bottom += dy;
}
bool intersects(const AABB& other); // left as an exercise.
};




Notice how the constructor is defined with an initialization list. This is the preferred way to initialize things in the constructor. It's cleaner*, it may be faster (and can't be slower)**, it lets you work around name conflicts easily***, and sometimes you have to use it****. So get familiar with it.

Next, we implement the Collectable, giving it an AABB instead of separate variables:


class Collectable {
AABB bounds;
bool visible;
Action type;
Texture* texture;

public:
// In C++, use std::string to represent text.
// Anyway, let's see what the initialization list can do for us:
Collectable::Collectable(float left, float top, float right, float bottom, const std::string& texName, Action act):
AABB(left, top, right, bottom), visible(true), type(act), texture(loadTexture(texName)) {}
// Whee, that was fun.
// If loadTexture is your own function, you'll need to adapt it to accept
// a const std::string& as well. If it's someone else's code (or at the point
// where you call code that *needs* a const char*), use the .c_str() member
// function of the string to get it.
// Again, I changed the parameter names for the box.

// And we'll want some functionality:
void move(float dx, float dy) {
// This is easy to implement:
bounds.move(dx, dy);
}
};





* It's idiomatic in C++. Everything actually gets initialized - at least theoretically; the compiler might be able to optimize it out - before the constructor body runs, whether you mention it in the initialization list or not. The initialization list lets you say how members will be initialized, instead of using the defaults. The constructor body is for any extra work that's needed to initialize the instance as a whole, rather than its parts.

** It could be faster because you don't have to default-initialize something and then overwrite it with a custom-initialized version.

*** Within the initialization list, names inside the parentheses refer to the parameters by default - since they're in a tighter scope - while names outside the parentheses must refer to members and base classes - since they're the only names legal in that position. Therefore, there is never any ambiguity, and you don't need to write 'this->' anywhere.

**** If you have a data member of a class type, or a base class, and that class doesn't have a default constructor (which happens if you write a constructor that takes arguments, and don't also write one which doesn't take any), then you will get a compile-time error if you don't use the initialization list for that base/member. This is because it has to get initialized, and if you don't say how to initialize it, C++ will assume the zero-argument constructor - which doesn't exist, so it complains about not being able to find that constructor.

Share this post


Link to post
Share on other sites
Thanks Zahlman,
I think some confusion has arisen from my poorly named variables and my rubbish explanation!
I wanted to have pointers in my AABB class to avoid having to call a function to update the values in the AABB class. I de-reference them whenever I want to get the values that they point to (for testing against other AABBs). The x, x1, y and y1 variables (which I have changed so are named appropriately) in the Collectable class are used for rendering the object. I have a draw function that renders a quad using these four points. I then change these four variables to move the object around.
I was hoping that by using pointers in my AABB class, they should always point to the values in the Collectable class and (provided I have done it correctly) should always have the same values when they are de-referenced as the values used to render the quad. I thought this would be a faster way and not allow for the values to differ from the Collectable class and the AABB class, hence keep the AABB 'around' the object.
Since writing this, I have realised that I could just store the values in the AABB class only (as values, not pointers), not the Collectable class and then render the quad using the AABB variables.

Quote:

Are 'box', 'visible' and 'texture' actually members of Collectable?


Yes they are. I used 'this->' to resolve naming issues, I then changed the names of the variables passed to the constructor so it now appears odd. I will be changing it!

Quote:

In fact, you should never write something like 'x = *(new X(...))'. The correct form for this process is 'x = X(...)'.


I have realised this and now do it the correct way.

Quote:

AABB(float left, float top, float right, float bottom): left(left), top(top), right(right), bottom(bottom) {}


I have never seen this way of initializing variables before but it does seem to make sense and if it stops errors then I'm all for using it! Is this done in the header file where you prototype the constructor or in the cpp file?
Thanks for your help,
Daniel

Share this post


Link to post
Share on other sites

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