//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;
}
I broke the memory.
Author
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?
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 =)
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.
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.
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.
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.
Author
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.
"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.
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.
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
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
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.
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.)
Yes, it's really that simple. Cut stuff out; you almost always don't need as much code as you think you do.
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.
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.
Author
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?
However is there anything good about my code?
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement