Sign in to follow this  
Niddles

I broke the memory.

Recommended Posts

Hello everyone, development on my engine is going well, however I've broken the memory management inside of it. Now, I tripped it through the debugger, and it's showing me that there is a problem with a constructor of a class that inherits another class. Can someone take a look at it, and see if they can find the problem?
//The two classes
class KSceneGraph
{
    protected:
        KBoolean Renderable;
        KVector3<double>* Position;
        vector<KSceneGraph> Children;
        KModel* ModelHandle;
        KMatrix44<double>* TranslationMatrix;
        KQuaternion<double>* RotationQuaternion;
        KMatrix44<double>* ScalingMatrix;
        KMatrix44<double> FinalMatrix;
        KBoolean Scale;
        KBoolean Rotate;
        KBoolean Translate;
        string SceneName;
    public:
        KSceneGraph(string name);
        KSceneGraph(string name, const KVector3<double>& v);
        KSceneGraph(string name, const KVector3<double>& v, KModel a);
        ~KSceneGraph();
        string GetName();
        void SetModel(KModel a);
        void AddChild(string name);
        KSceneGraph GetChild(string name);
        void SetPositionRelativeToOrigin(const KVector3<double>& v);
        void SetPositionRelativeToParent(const KVector3<double>& v);
        void SetRotation(const double angle, const KVector3<double> axis);
        void SetScale(const KVector3<double> scalevector);
        KVector3<double> GetPosition() const;
        void SetRenderable(KBoolean a);
        KBoolean IsRenderable() const;
        void Render();
};
class KSceneGraphBase : public KSceneGraph
{
    private:
        KSkybox* ParentSkybox;
        KFog* ParentFog;
    public:
        KSceneGraphBase(string name);
        ~KSceneGraphBase();
        void SetSkybox(const double& size, const GLuint& right, const GLuint& left, const GLuint& top,
                       const GLuint& bottom, const GLuint& front, const GLuint& back);
        void SetFog(const float& density, const string& e, const KVector3<float> &color);
        void SetFog(const float& begin, const float& end, const KVector3<float> &color);
};

//SceneGraph constructors
KSceneGraph::KSceneGraph(string name)
{
    SceneName = name;
    ScalingMatrix = NULL;
    TranslationMatrix = NULL;
    RotationQuaternion = NULL;
    Position = NULL;
    ModelHandle = NULL;
    SetPositionRelativeToOrigin(KVector3<double>(0.0, 0.0, 0.0));
    Renderable = KTRUE;
    Translate = KFALSE;
    Rotate = KFALSE;
    Scale = KFALSE;
}
KSceneGraph::KSceneGraph(string name, const KVector3<double>& v)
{
    SceneName = name;
    ScalingMatrix = NULL;
    TranslationMatrix = NULL;
    RotationQuaternion = NULL;
    Position = NULL;
    ModelHandle = NULL;
    SetPositionRelativeToOrigin(v);
    Renderable = KTRUE;
    Translate = KTRUE;
    Rotate = KFALSE;
    Scale = KFALSE;
}
KSceneGraph::KSceneGraph(string name, const KVector3<double>& v, KModel a)
{
    SceneName = name;
    ScalingMatrix = NULL;
    TranslationMatrix = NULL;
    RotationQuaternion = NULL;
    Position = NULL;
    delete ModelHandle;
    ModelHandle = new KModel(a);
    SetPositionRelativeToOrigin(v);
    Renderable = KTRUE;
    Translate = KTRUE;
    Rotate = KFALSE;
    Scale = KFALSE;
}
//SceneGraphBase Constructors
KSceneGraphBase::KSceneGraphBase(string name) : KSceneGraph(name, KVector3<double>(0.0, 0.0, 0.0))
{
    SceneName = name;
    ParentSkybox = NULL;
    ParentFog = NULL;
    ModelHandle = NULL;
    delete Position;
    Position = new KVector3<double>(0.0, 0.0, 0.0);
    TranslationMatrix = NULL;
    RotationQuaternion = NULL;
    ScalingMatrix = NULL;
    Renderable = KTRUE;
    Scale = KFALSE;
    Translate = KFALSE;
    Rotate = KFALSE;
}

This is the first attempt at inheritance that I've tried, and it seems like a good reason to use it. Oh, and if anyone can give me some tips on my coding syle in this, and anything else, it would be greatly appreciated. Thanks =)

Share this post


Link to post
Share on other sites
What are the delete statements doing in the constructors? As far as I can see you delete some pointers, but you can't delete pointers before you have used "new" on yet. The compiler is probably complaining that it's deleting uninitialized memory.

EDIT: Looking at your code again I see you have a lot of member pointers in your class. Why are these pointers? It looks like a regular value would be a much better idea. In C++ you should try to avoid raw pointers as much as possible.

Share this post


Link to post
Share on other sites
What exactly is the problem? Can't help you if you don't tell us what is wrong.


That said, here's some advice:
0) namespaces are generally preferable to prefixed (K-whatever) identifiers.
1) The class named "base" is usually the base class, not the other way around.
2) There really is no need for inheritance here, based on what you've provided. Just because you can doesn't mean you should. What made you think it would be a good idea? What is the larger context?
4) Why do you have your own boolean type? Just because you can doesn't mean you should.
5) Smart pointers are your friend.
6) There isn't really a need to pass "small" variables (float, double, ints) by const reference. It gains you nothing.
7) Prefer initializer lists for most simple initialization in constructors.
8) Why do you use delete in some of your constructors? The memory (for ModelHandle) was not allocated, nor was the pointer initialized, you are deleting an uninitialized pointer. This is probably your problem. The delete of Position is safe because the call to the base constructor nulls that pointer; it will not crash but it won't do anything useful, either.

Share this post


Link to post
Share on other sites
It's not a compiler problem, it's a runtime problem.
"The instruction at "0x00443117" referenced memory at "0x71626231". The memory could not be "read"".
That's what the error said.
SceneGraphBase inherits SceneGraph, because everything in SceneGraphBase is in SceneGraph, except the Base is the top of the scene graph so it has some added features.

Share this post


Link to post
Share on other sites
Quote:
Original post by Niddles
"The instruction at "0x00443117" referenced memory at "0x71626231". The memory could not be "read"".

This is likely because if you don't assign any value to a pointer then it will just contain some random address. In this case that address happened to be "0x71626231". When you go ahead and run delete on that address you're told by the runtime that you can't even access that memory because it doesn't belong to your application.

Quote:
SceneGraphBase inherits SceneGraph, because everything in SceneGraphBase is in SceneGraph, except the Base is the top of the scene graph so it has some added features.

But why don't you just have a single class? Is there any situations where you would actually use KSceneGraph for anything? If not then this is a bad place for inheritance. Also why do you call "KSceneGraphBase" base? The base is the class which is inherited from, not the class which inherits. In your code KSceneGraph is the base class and KSceneGraphBase is the derived class.

Share this post


Link to post
Share on other sites
Hi,

I only glanced at it very quickly so this may be way off. Im not too familiar with const and reference passing but it strikes me somewhat odd that you would create a KVector3 explicitely by calling the constructor and then passing that in as a reference immediately. Perhaps the memory scope for this causes it to be deleted before it can be accessed in SetPositionRelativeToOrigin?

Im probably wrong since I never use const references.

EDIT: also you should check whether a pointer is NULL before deleting it.

-CProgrammer

Share this post


Link to post
Share on other sites
Quote:
Original post by CProgrammer
EDIT: also you should check whether a pointer is NULL before deleting it.


Why should he? According to the C++ standard invoking delete on the null pointer has no effect. The only valid reason for checking is if he expects it to have a valid value, and therefore want to make sure that is the case, but in that case he should use an assertion.

Share this post


Link to post
Share on other sites
Hi, welcome to C++ and proper OO design.

1) Everyone who tried to help you already knew it "wasn't a compilation but a runtime problem". You cannot call delete on an uninitialized pointer. Full stop. The value in the bytes of memory that the pointer is using are whatever was left over in there from the last thing that used that memory (hint: quite possibly, not your program). Therefore, it points to an effectively random memory address. (hint: quite possibly, not even memory that *exists*. Also be aware that memory addresses are "mapped" by the operating system to give each program the illusion of its own memory space.) You don't necessarily own the memory at that location, for many reasons.

2) To CProgrammer, checking for a NULL pointer before deletion is meaningless. Don't do it; the standard specifies that deleting a NULL pointer is safe and well defined. Adding such code adds a point of failure (you could write the if-condition wrong), and adds a false sense of security (there is no portable, sane way to check that a pointer is "valid", let alone whether it points to memory allocated with 'new'), for zero benefit.

3) Everything jpetrie said.

4) You actually probably shouldn't store the data members in question by pointer at all. What are you hoping to gain from this? If the data members are supposed to be "optional", consider boost::optional. If they're shared between instances, you are begging for a world of hurt without smart pointers, such as boost::shared_ptr. However, in almost every instance I've seen of this kind of thing, there is no actual meaningful reason for the extra indirection.

In your case, the proper way to handle an "optional translation" or "optional rotation" is to just use the identity translation matrix/rotation quaternion when there is no translation/rotation needed. In addition to avoiding memory management, it simplifies the code - just apply the transformation without thinking about it. Also keep in mind that these classes are probably fairly small, and usually needed; and that to save space in cases where you don't need them, you propose to add a pointer to every instance.

5) In fact, you proposed to add a pointer *and* a bool (no, sorry, a KBoolean, whatever that is), which is redundant anyway; this is what null pointers are for (on the rare occasions that you use raw pointers in modern C++).

6) Speaking of redundancy, there is probably no reason to hold the 'FinalMatrix' separately. Either calculate it on demand, or don't store its components; you don't need both. In fact, I'd just keep the position, scale etc. as vectors, and have one helper function to determine the resulting matrix.

6) Try not to include information about type names in the names of variables and data members. It tends not to actually be of any use.

7) Don't use 'using' statements in your header; it denies client code the option to un-use the namespace.

8) Pass std::strings by const reference, the same way you would with your own classes. On the other hand, there is no reason to do this with floats or unsigned ints (GLuints), and probably none to do it with doubles.

9) You cannot put the derived classes into a std::vector of Base (as found in the base class) when you build the graph and expect it to work. I'm going to echo the statement that you probably don't need polymorphism here at all.

10) A function called "AddChild" should add an actual child node. What are you hoping to do with just a name?

11) As it stands, there is no point protecting the 'renderable' member with a raw accessor and mutator like this.

12) The position-setting functions likely have reused code, so some refactoring is in order, as shown.

13) Use typedefs to simplify type names.

14) Specifying a "size" for skyboxes appears to be redundant. If that's a volume, isn't it implied by the dimensions?

15) Use initializer lists for constructors. Also, use default arguments to avoid (some) redundancy in constructors. (There is no such thing as a null reference, so we can't have everything here.)


namespace K {
// FIXME: Set up the Vector3 class so its default constructor initializes
// each value to 0.
typedef Vector3<double> Position;
typedef Vector3<double> Direction;
typedef Vector3<float> Color;

class SceneGraph {
Position position;
std::vector<SceneGraph> children;
boost::shared_ptr<Model> model;
Direction translation;
// FIXME: Set up the Quaternion class so its default constructor creates
// the identity quaternion.
Quaternion<double> rotation;
Direction scale;
std::string name;
boost::optional<Skybox> ParentSkybox;
boost::optional<Fog> ParentFog;

// Helper: determine net transformation for rendering.
Matrix44 transformation();

public:
SceneGraph(const std::string& name, const Position& p = Position()) : name(name), renderable(true), scale(Direction(1.0, 1.0, 1.0)), position(p) {}
SceneGraph(const std::string& name, const Position& p, const Model& m) : name(name), renderable(true), scale(Direction(1.0, 1.0, 1.0)), position(p) {
SetModel(m);
}
// There is no longer any need for a destructor.

const std::string& Name() { return name; }
void SetModel(const Model& m) { model.reset(&m); }
void AddChild(const SceneGraph& s);
const SceneGraph& GetChild(const string& name);

void SetPosition(const Position& v, bool relativeToParent);
void SetRotation(double angle, const Direction& axis);
void SetScale(const Direction& scale);
const Vector3<double>& GetPosition() const;

bool renderable;
void Render();

void SetSkybox(GLuint right, GLuint left, GLuint top,
GLuint bottom, GLuint front, GLuint back);
void SetFog(float density, const std::string& e, const Color& color);
void SetFog(float begin, float end, const Color& color);
};
}



Yes, it's really that simple. Cut stuff out; you almost always don't need as much code as you think you do.

Share this post


Link to post
Share on other sites
Quote:

2) To CProgrammer, checking for a NULL pointer before deletion is meaningless. Don't do it; the standard specifies that deleting a NULL pointer is safe and well defined. Adding such code adds a point of failure (you could write the if-condition wrong), and adds a false sense of security (there is no portable, sane way to check that a pointer is "valid", let alone whether it points to memory allocated with 'new'), for zero benefit.


For even more fun and games, the non-portable ways (IsBadReadPtr(), IsBadWritePtr(), in the Platform SDK) don't even work well and should be avoided.

Share this post


Link to post
Share on other sites
Ok, besides being a little mean, this is great information, and also the type of stuff I was looking for.
However is there anything good about my code?

Share this post


Link to post
Share on other sites
Quote:
Original post by Niddles
Ok, besides being a little mean, this is great information, and also the type of stuff I was looking for.
However is there anything good about my code?

Kudos for not getting mad at all the (fairly harsh) comments.
In reallity, there isn't much to recomend that particular snippet. As others have said: all those unneccessary pointers are just plain dangerous, passing small value types such as floats by const reference is bad (passes an extra pointer around internally). Eequally KModel is probably not that lightweight and *should* be passed by reference, and if you are making this const-correct (as you most definitely should) *all* functions that don't modify the instance should be const (including GetName() for instance).

Share this post


Link to post
Share on other sites
I have another question. Is it C++ standard, that when you are in a member function and you are going to modify or get a member variable, should you use this->? Here is an example.

class MyClass
{
private:
int MyInt;
public:
int GetInt() const
{
return this->MyInt;
}
};



Is this correct, and should I do it all the time?
Also, can I declare a const function if it does this?

void EnableFog() const
{
glEnable(GL_FOG);
glFogi(GL_FOG_MODE, GL_EXP);
glFogf(GL_FOG_COLOR, color);
glFogf(GL_FOG_DENSITY, density);
}
//or
void DisableFog() const
{
glDisable(GL_FOG);
}

Share this post


Link to post
Share on other sites
Quote:
Original post by Niddles
I have another question. Is it C++ standard, that when you are in a member function and you are going to modify or get a member variable, should you use this->?

You can, and whether you choose to or not doesn't matter to the compiler. Most people omit it in my experience, but if you really think it looks better then I see no reason not to. If you have a local variable of the same name as a member variable, then the this pointer is required to access to member variable, but you shouldn't have several variables with the same name anyway.

Quote:
Also, can I declare a const function if it does this?

Any member function that doesn't alter non-mutable members can be declared const and in most cases should.

Share this post


Link to post
Share on other sites
You dont have to but can, although it clutters things a bit, its more useful for resolving name problems.

class Foo
{
public:

void Function(int x, int y)
{
// .... code code code

x += 5; // do you mean member or argument "x"
}

protected:

int x, y; // could solve by doing, "int mX, mY"
};

not the best example but illustrates the idea.

Share this post


Link to post
Share on other sites
Quote:
Original post by Niddles
this->? Here is an example.
Is this correct, and should I do it all the time?


It is correct, but you should normally not bother with it. Do it when not doing it would mean something else or would not work (this can happen with advanced use of templates, or more commonly, when you want a parameter of a member function to have the same name as a data member).

Share this post


Link to post
Share on other sites
Quote:
Original post by Niddles
I have another question. Is it C++ standard, that when you are in a member function and you are going to modify or get a member variable, should you use this->?

You can, but it does clutter things a little. The only place I would use it regularly is in constructors, where it is often handy to have the argument names match the member variable names. But if you use initialiser lists it is unnecessary there as well.

Quote:
Is this correct, and should I do it all the time?
Also, can I declare a const function if it does this?
*** Source Snippet Removed ***

As long as you don't change any of those member variables, it can be declared const. How you read from them is no concern of the compiler.

Share this post


Link to post
Share on other sites
Hmmm, I have a class member function problem, and I can't find where the problem is, because I have four functions that set light properties(ambient, diffuse, etc.) and I have it stored in a map. Now the map size increases everytime the set functions are called, but when I call the enable function one call later the members inside the map dissapear. Take a look:

//These five are called right after each other
void KLight::SetPosition(const KVector3<float> &pos)
{
LightProperties.insert(std::pair<LightMode, KVector3<float> >(GL_POSITION, pos));
//LightProperties[GL_POSITION] = pos;
}
void KLight::SetAmbient(const KVector3<float> &amb)
{
LightProperties.insert(std::pair<LightMode, KVector3<float> >(GL_AMBIENT, amb));
//LightProperties[GL_AMBIENT] = amb;
}
void KLight::SetDiffuse(const KVector3<float> &dif)
{
LightProperties.insert(std::pair<LightMode, KVector3<float> >(GL_DIFFUSE, dif));
//LightProperties[GL_DIFFUSE] = dif;
}
void KLight::SetSpecular(const KVector3<float> &spc)
{
LightProperties.insert(std::pair<LightMode, KVector3<float> >(GL_SPECULAR, spc));
//LightProperties[GL_SPECULAR] = spc;
}
//And this function is where they disappear.
void KLight::Enable()
{
const GLenum lights[] = {GL_LIGHT0, GL_LIGHT1, GL_LIGHT2, GL_LIGHT3,
GL_LIGHT4, GL_LIGHT5, GL_LIGHT6, GL_LIGHT7};
const GLenum enablinglight = lights[LightNumber];
glEnable(GL_LIGHTING);
glEnable(enablinglight);
char a[20];
sprintf(a, "%d", LightProperties.size());
MessageBox(NULL, a, a, NULL);
for(std::map<LightMode, KVector3<float> >::iterator it = LightProperties.begin();
it != LightProperties.end(); it++)
{
KVector3<float> outcolor = it->second;
glLightfv(enablinglight, it->first, outcolor.GetArray());
}
}

Share this post


Link to post
Share on other sites
A couple of points:
1) Your set functions will only work the first time, because std::map.insert() only inserts if there is no current entry with the same key.
2) glLightfv typically takes a 4-component vector for those arguments, so you will need to pad your 3-element vector.

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