Hi everyone, it's my first post, and don't really know if this is the right section, but here it goes:
I'm new in C++, made some simples programs and now I'm trying to make something with C++ and openGL, today i tried to create a TriangleMesh class, which creates a mesh of triangles giving the number of triangles of each side, the inicial point and the size of each catete, until now everything went great, but today i got a bug and can't resolve it
I allocate a new array of Vector3f and Triangles (all done by me) and then I start some loops to create the Vectors and after that the triangles inside the array, the problem is,at some point, the Vectors get deleted without me calling a destructor
Is it legal to post here my code so you guys can try to find the problem?
problem with memory allocation in C++
It is legal, and normal, for people to post code. Please produce a minimal example if you are going to post code.
From your description I imagine it will be a rule of three violation. Using std::vector<> instead of new[] and delete[] will probably solve it.
From your description I imagine it will be a rule of three violation. Using std::vector<> instead of new[] and delete[] will probably solve it.
Yes please. The code will be very helpful with trying to find your bug. Also please make sure you use the [ source ] [ /source ] tags but without the spaces.
Author
It is legal, and normal, for people to post code. Please produce a minimal example if you are going to post code.
From your description I imagine it will be a rule of three violation. Using std::vector<> instead of new[] and delete[] will probably solve it.
Can you tell me those rules you talk about, please?
And yes, I haven't made that, since I come from java thse language specifications are new to me
ok, here it goes:
Vectors header
[source]
#ifndef VECTORS_H_INCLUDED
#define VECTORS_H_INCLUDED
class Vector3f {
public:
Vector3f();
Vector3f(float x, float y, float z);
~Vector3f();
float getX();
float getY();
float getZ();
void setX(float x);
void setY(float y);
void setZ(float z);
void add(Vector3f * v3f);
void sub(Vector3f * v3f);
void multiply(float f);
void divide(float f);
void crossProduct(Vector3f * v);
float dot(Vector3f * v);
private:
float _x;
float _y;
float _z;
void normalize();
};
#endif
[/source]
Vectors implementation
[source]
#include "Vectors.h"
#include <math.h>
Vector3f::Vector3f(){
_x = 0;
_y = 0;
_z = 0;
}
Vector3f::Vector3f(float x, float y, float z){
_x = x;
_y = y;
_z = z;
}
Vector3f::~Vector3f(){}
float Vector3f::getX(){
return _x;
}
float Vector3f::getY(){
return _y;
}
float Vector3f::getZ(){
return _z;
}
void Vector3f::setX(float x){
_x = x;
}
void Vector3f::setY(float y){
_y = y;
}
void Vector3f::setZ(float z){
_z = z;
}
void Vector3f::add(Vector3f * v3f){
_x+=v3f->getX();
_y+=v3f->getY();
_z+=v3f->getZ();
}
void Vector3f::sub(Vector3f * v3f){
_x-=v3f->getX();
_y-=v3f->getY();
_z-=v3f->getZ();
}
void Vector3f::multiply(float f){
_x*=f;
_y*=f;
_z*=f;
}
void Vector3f::divide(float f){
_x/=f;
_y/=f;
_z/=f;
}
void Vector3f::crossProduct(Vector3f * v){
float tempX = _y*v->getZ() - _z*v->getY();
float tempY = _z*v->getX() - _x*v->getZ();
_z = _x*v->getY() - _y*v->getX();
_x = tempX;
_y = tempY;
normalize();
}
void Vector3f::normalize(){
float length = sqrtf( (_x*_x) + (_y*_y) + (_z*_z) );
_x/=length;
_y/=length;
_z/=length;
}
float Vector3f::dot(Vector3f * v){
return _x * v->getX() + _y * v->getY() + _z * v->getZ();
}
[/source]
Triangle Header
[source]
#ifndef TRIANGLE_H_INCLUDED
#define TRIANGLE_H_INCLUDED
#include "Vectors.h"
#include <Windows.h>
#include <GL\gl.h>
#include <GL\glu.h>
#include <glut.h>
//represents a triangle
class Triangle {
public:
Triangle();
Triangle(Vector3f *p0, Vector3f *p1, Vector3f *p2);
~Triangle();
Vector3f * _p0;
Vector3f * _p1;
Vector3f * _p2;
Vector3f * _v1;
Vector3f * _v2;
Vector3f * _normalVec;
float forDist;
void draw();
};
#endif
[/source]
Triangle implementation
[source]
#include "Triangle.h"
#include <iostream>
using namespace std;
Triangle::Triangle(){ }
Triangle::Triangle(Vector3f *p0, Vector3f *p1, Vector3f *p2){
_p0 = p0;
_p1 = p1;
_p2 = p2;
_v1 = new Vector3f(p1->getX() - p0->getX(), p1->getY() - p0->getY(), p1->getZ() - p0->getZ());
_v2 = new Vector3f(p2->getX() - p0->getX(), p2->getY() - p0->getY(), p2->getZ() - p0->getZ());
_normalVec = new Vector3f(p1->getX() - p0->getX(), p1->getY() - p0->getY(), p1->getZ() - p0->getZ());
_normalVec->crossProduct(_v2);
forDist = -(_normalVec->getX() * p0->getX() + _normalVec->getY() * p0->getY() + _normalVec->getZ() * p0->getZ());
}
Triangle::~Triangle(){
delete _p0;
delete _p1;
delete _p2;
delete _v1;
delete _v2;
delete _normalVec;
}
void Triangle::draw(){
//TODO - AO ALTERAR A POSICAO DO TRIANGULO E NECESSARIO FAZER UPDATE A ESTA VARIAVEL!
forDist = -(_normalVec->getX() * _p0->getX() + _normalVec->getY() * _p0->getY() + _normalVec->getZ() * _p0->getZ());
cout<<_p0->getX()<<" "<< _p0->getY()<<" "<< _p0->getZ()<<endl;
cout<<_p1->getX()<<" "<< _p1->getY()<<" "<< _p1->getZ()<<endl;
cout<<_p2->getX()<<" "<< _p2->getY()<<" "<< _p2->getZ()<<endl<<endl;
glBegin(GL_TRIANGLES);
glVertex3f(_p0->getX(), _p0->getY(), _p0->getZ());
glVertex3f(_p1->getX(), _p1->getY(), _p1->getZ());
glVertex3f(_p2->getX(), _p2->getY(), _p2->getZ());
glEnd();
}
[/source]
TriangleMesh header
[source]
#ifndef IMAGEUTILS_H_INCLUDED
#define IMAGEUTILS_H_INCLUDED
#include "Triangle.h"
class TriangleMesh{
public:
bool color;
int nOfTriangles;
Vector3f *vertexes;
TriangleMesh(int nTrianglesBySide, float cateteSize, float bottomLeftStartXPosfloat,float bottomLeftStartZPos);
~TriangleMesh();
void Draw();
protected:
private:
Triangle * triangles;
void fillTrianglePointer(int nTrianglesBySide);
void fillVectorPointer(int nTrianglesBySide, float cateteSize, float bottomLeftStartXPosfloat,float bottomLeftStartZPos);
};
#endif
[/source]
TriangleMesh implementation
[source]
#include "TriangleMesh.h"
#include <iostream>
#include <GL\gl.h>
using namespace std;
TriangleMesh::TriangleMesh(int nTrianglesBySide, float cateteSize,float bottomLeftStartXPos,float bottomLeftStartZPos){
color = true;
nOfTriangles = nTrianglesBySide*nTrianglesBySide*2;
fillVectorPointer(nTrianglesBySide, cateteSize,bottomLeftStartXPos,bottomLeftStartZPos);
fillTrianglePointer(nTrianglesBySide);
}
TriangleMesh::~TriangleMesh(){ }
void TriangleMesh::Draw(){
for(int index = 0; index <nOfTriangles;index++){
if(color) glColor3f(1,0,0);
else glColor3f(0,1,0);
color = !color;
triangles[index].draw();
}
}
void TriangleMesh::fillVectorPointer(int nTrianglesBySide, float cateteSize,float bottomLeftStartXPosfloat,float bottomLeftStartZPos){
int totalNumOfVertexes = (nTrianglesBySide+1)*(nTrianglesBySide+1) ;
vertexes = new Vector3f[totalNumOfVertexes];
float xPos = bottomLeftStartXPosfloat;
float zPos = bottomLeftStartZPos;
for(int index = 0; index < totalNumOfVertexes; index++){
if(index!=0 && (index%4) == 0){
xPos = bottomLeftStartXPosfloat;
zPos -=cateteSize;
}
vertexes[index] = Vector3f(xPos, 0, zPos);
xPos += cateteSize;
}
}
void TriangleMesh::fillTrianglePointer(int nTrianglesBySide){
int totalNumOfTriangles = (nTrianglesBySide*nTrianglesBySide*2) ;
triangles = new Triangle[totalNumOfTriangles];
int auxIndex0 = 0;
int auxIndex1 = 1;
int auxIndex2 = nTrianglesBySide+1;
int auxIndex3 = nTrianglesBySide+2;
int vertexIndex0 = 0;
int vertexIndex1 = 1;
int vertexIndex2 = nTrianglesBySide+1;
int vertexIndex3 = nTrianglesBySide+2;
int numOfTrianglesPerLine = nTrianglesBySide*2;
for(int index = 0; index < totalNumOfTriangles; index+=2){
if(index>numOfTrianglesPerLine){
auxIndex0 = auxIndex2;
auxIndex1 = auxIndex3;
auxIndex2 = auxIndex0+nTrianglesBySide+1;
auxIndex3 = auxIndex2;
vertexIndex0 = auxIndex0;
vertexIndex1 = auxIndex1;
vertexIndex2 = auxIndex2;
vertexIndex3 = auxIndex3;
}
triangles[index] = Triangle(&vertexes[vertexIndex0],&vertexes[vertexIndex1],&vertexes[vertexIndex2]);
triangles[index+1] = Triangle(&vertexes[vertexIndex1],&vertexes[vertexIndex3],&vertexes[vertexIndex2]);
vertexIndex0++;
vertexIndex1++;
vertexIndex2++;
vertexIndex3++;
}
}
int main(){
TriangleMesh tm(3,1,0,0);
cin.get();
return 0;
}
[/source]
Ouch, I see we have a little culture shock coming from Java. No worries though! =]
In C++, the Rule of Three states that if you have a class with a destructor, copy constructor or assignment operator then you probably need all three. We can come back to that in a bit.
First off, you can lose the Vector3f destructor, the compiler will generate one implicitly that does nothing anyway. Then we change from using pointers in the interface to const references. This is almost like passing by value, except we avoid making a copy. It might not be necessary for such as simple type but I generally use const references when passing any non-primitive type by value (conceptually):
Something like this, with the appropriate changes to the declarations in the header file.
Your Triangle class is the likely cause of the problem. You've defined a destructor, that deletes the vertices. However you haven't defined a copy constructor or assignment operator. These will be implicitly generated, and will do shallow copies of the pointer values, i.e. the wrong thing. In C++ pointers are a rarity, not the norm. So your Vector3f should be stored by value in the Triangle class:
You'll note I use const references for the constructor parameters. This just avoids making an additional copy when passing them to the function. Again we can remove the destructor as the default, compiler generated version will simply destroy all the members, which aren't pointers so everything will be correctly cleaned up.
We would implement the constructor like so:
I'm using the functions you've defined to simplify this a little. If the functions were pure it would be even easier.
The triangle mesh now needs a list of triangles. In Java you might have used List<Foo> l = new ArrayList<Foo>(); In C++ we have std::vector<>, which is like ArrayList<>:
There are various member functions for manipulating vector<>, there is push_back(), which is analogous to ArrayList#add(). You can access elements using the array bracket syntax, e.g. vertices[42]. You can get the length using size(). There is also a nested iterator type for blazing through the array in a loop.
See how far you get with the approach I've outlined above. You can see that by removing the pointers, we've removed the destructors, which also means that we don't need to worry about the Rule of Three just yet.
In C++, the Rule of Three states that if you have a class with a destructor, copy constructor or assignment operator then you probably need all three. We can come back to that in a bit.
First off, you can lose the Vector3f destructor, the compiler will generate one implicitly that does nothing anyway. Then we change from using pointers in the interface to const references. This is almost like passing by value, except we avoid making a copy. It might not be necessary for such as simple type but I generally use const references when passing any non-primitive type by value (conceptually):
void Vector3f::add(const Vector3f &v){
_x += v3f.getX();
_y += v3f.getY();
_ z+= v3f.getZ();
}
void Vector3f::sub(const Vector3f &v){
_x -= v3f.getX();
_y -= v3f.getY();
_z -= v3f.getZ();
}
Something like this, with the appropriate changes to the declarations in the header file.
Your Triangle class is the likely cause of the problem. You've defined a destructor, that deletes the vertices. However you haven't defined a copy constructor or assignment operator. These will be implicitly generated, and will do shallow copies of the pointer values, i.e. the wrong thing. In C++ pointers are a rarity, not the norm. So your Vector3f should be stored by value in the Triangle class:
class Triangle {
public:
Triangle();
Triangle(const Vector3f &p0, const Vector3f &p1, const Vector3f &p2);
Vector3f _p0;
Vector3f _p1;
Vector3f _p2;
Vector3f _v1;
Vector3f _v2;
Vector3f _normalVec;
float forDist;
void draw();
};
You'll note I use const references for the constructor parameters. This just avoids making an additional copy when passing them to the function. Again we can remove the destructor as the default, compiler generated version will simply destroy all the members, which aren't pointers so everything will be correctly cleaned up.
We would implement the constructor like so:
Triangle::Triangle(const Vector3f &p0, const Vector3f &p1, const Vector3f &p2)
:
_p0(p0),
_p1(p1),
_p2(p2),
{
_v1 = p1;
_v1.sub(p0);
_v2 = p2;
_v2.sub(p0);
_normalVec = p1;
_normalVec.sub(p0);
_normalVec->crossProduct(_v2);
forDist = -(_normalVec.getX() * p0.getX() + _normalVec.getY() * p0.getY() + _normalVec.getZ() * p0.getZ());
}
I'm using the functions you've defined to simplify this a little. If the functions were pure it would be even easier.
The triangle mesh now needs a list of triangles. In Java you might have used List<Foo> l = new ArrayList<Foo>(); In C++ we have std::vector<>, which is like ArrayList<>:
#include <vector>
class TriangleMesh{
public:
bool color;
TriangleMesh(int nTrianglesBySide, float cateteSize, float bottomLeftStartXPosfloat,float bottomLeftStartZPos);
void Draw();
private:
std::vector<Vertex3f> vertices;
std::vector<Triangle> triangles;
void fillTrianglePointer(int nTrianglesBySide);
void fillVectorPointer(int nTrianglesBySide, float cateteSize, float bottomLeftStartXPosfloat,float bottomLeftStartZPos);
};
There are various member functions for manipulating vector<>, there is push_back(), which is analogous to ArrayList#add(). You can access elements using the array bracket syntax, e.g. vertices[42]. You can get the length using size(). There is also a nested iterator type for blazing through the array in a loop.
See how far you get with the approach I've outlined above. You can see that by removing the pointers, we've removed the destructors, which also means that we don't need to worry about the Rule of Three just yet.
Author
ok dude, I understand most things you have done
the "[font="Consolas,"]const Vector3f &v" was somehow new to me,but it's like you said, it's a little optimization in the code, optimizations are always good x)[/font]
[font="Consolas,"]
[/font]
[color="#1C2837"]copy constructor or assignment operator are news to me, never heard of them, but will check it out soon
[color="#1C2837"]
[color="#1C2837"]and about the vector<>, I head about them and ye, in this case they're really useful, will try your changes later and try to go more on this "C++ way"
[color="#1C2837"]
[color="#1C2837"]by the way, after all that i still haven't understood the reason for my dynamic memory allocation failures, with new ... [] or malloc (tried this before the new), but anyway thanks for the reaply, will try to post something as soon as I am done with the changes
[color="#1c2837"]
[color="#1C2837"]and by the way, I used so many pointers because like that I can change one of the vertices and that way there is no need to change the triangle Vector3fs, with your solution will I still get that?
the "[font="Consolas,"]const Vector3f &v" was somehow new to me,but it's like you said, it's a little optimization in the code, optimizations are always good x)[/font]
[font="Consolas,"]
[/font]
[color="#1C2837"]copy constructor or assignment operator are news to me, never heard of them, but will check it out soon
[color="#1C2837"]
[color="#1C2837"]and about the vector<>, I head about them and ye, in this case they're really useful, will try your changes later and try to go more on this "C++ way"
[color="#1C2837"]
[color="#1C2837"]by the way, after all that i still haven't understood the reason for my dynamic memory allocation failures, with new ... [] or malloc (tried this before the new), but anyway thanks for the reaply, will try to post something as soon as I am done with the changes
[color="#1c2837"]
[color="#1C2837"]and by the way, I used so many pointers because like that I can change one of the vertices and that way there is no need to change the triangle Vector3fs, with your solution will I still get that?
ok dude, I understand most things you have done
[/quote]
Good stuff!
the "const Vector3f &v" was somehow new to me,but it's like you said, it's a little optimization in the code, optimizations are always good x)
[/quote]
I don't really think of it as an optimisation. It expresses my intent: did I intend to make a copy here? If so, pass by value. I not pass by const reference. In the case of primitives I always pass by value because the distinction is blurred, and it is idiomatic in most languages to copy primitives even when no copy is strictly necessary.
In fact, it is only small types like Vector3 where you even have to think about it. It is a border line type between a primitive and a complex class, because it is so simple. If you are passing a std::string instance, or std::vector<>, or a TriangleMesh, you almost certainly do not intend to make an extra copy in memory.
I suppose the fact that it is an optimisation makes it a little easier to force yourself to write all those nasty "const &" cruft though.
by the way, after all that i still haven't understood the reason for my dynamic memory allocation failures, with new ... [] or malloc (tried this before the new), but anyway thanks for the reaply, will try to post something as soon as I am done with the changes
[/quote]
Let us examine this line:
triangles[index] = Triangle(&vertexes[vertexIndex0],&vertexes[vertexIndex1],&vertexes[vertexIndex2]);
What does it do? It creates a temporary triangle from the three pointers specified. This triangle is copied into the array. The temporary is implicitly destroyed, which calls ~Triangle(). The triangle destructor calls delete on the pointers (which isn't valid because the pointers weren't individually allocated by new).
At this point* we have undefined behaviour: anything might happen. As a Java programmer, you might be used to having exceptions thrown when something goes wrong. C++ makes no such guarantees. If anything, it goes out of its way to do the opposite, it tries to make it such that all runtime correctness checks can be elided in Release mode, unless explicitly sought by the programmer. In this case, a complicated Debug runtime would recognise that there is a mismatch between the allocation and deallocation and would generate some kind of debug error. However, it may choose to do nothing. Or it might not have any checks at all and this can result in memory corruption in your process.
Which is one of many reasons not to manage memory yourself. std::vector<> manages its own memory, so unless you are doing something extremely wrong it will always allocate enough space and clean up after itself. It generally comes with some nice runtime checks in Debug mode too.
I used so many pointers because like that I can change one of the vertices and that way there is no need to change the triangle Vector3fs, with your solution will I still get that?
[/quote]
No, you will not. One solution is to move to an indexed model, so you have one authoritative array of vertices and the triangles are made up of integer indices into this array. This avoids storing duplicate vertices where triangles meet.
* [size="1"]I haven't exhaustively analysed the code, there might be a point earlier where undefined behaviour is invoked.
By the way, your Vector class is in Java style, but it is idiomatic to do things differently in C++.
For example, instead of having getters and setters for everything, usually you just have functions to return references to a member. Assuming there's any point to encapsulating it at all. In a pure data class like a vector, you probably just want to make all the members public.
Also, instead of having addition, multiplication, etc. functions, you probably want to overload the operators.
Also, in C++ you can have functions outside of a class.
As long as the code is functional, it's not too important, but it does help other people to read it if code is written in a more idiomatic style.
Here's an example of a 2d vector class I made. It may not be the best example, but it does show operator overloading and public members. It also uses Boost Operator to automatically generate most of the operators.
For example, instead of having getters and setters for everything, usually you just have functions to return references to a member. Assuming there's any point to encapsulating it at all. In a pure data class like a vector, you probably just want to make all the members public.
Also, instead of having addition, multiplication, etc. functions, you probably want to overload the operators.
Also, in C++ you can have functions outside of a class.
As long as the code is functional, it's not too important, but it does help other people to read it if code is written in a more idiomatic style.
Here's an example of a 2d vector class I made. It may not be the best example, but it does show operator overloading and public members. It also uses Boost Operator to automatically generate most of the operators.
#pragma once
#include <cmath>
#include "BOOST/operators.hpp"
//Boost operators used to produce arithmatic operators based on the compound assignment ones, e.g. a + b from +=
//To reduce object size, the templates are chained
struct Vec2d // note: private inheritance is OK here!
: boost::additive< Vec2d // point + point and point - point
, boost::dividable2< Vec2d, double // point / T
, boost::multipliable2< Vec2d, double // point * T, T * point
> > >
{
double x,y;
Vec2d(): x(0), y(0) {}
Vec2d(double newx, double newy): x(newx), y(newy) {}
Vec2d operator-() const
{return Vec2d(-x,-y);}
Vec2d& operator+= (const Vec2d& arg)
{
x+=arg.x; y+=arg.y;
return *this;
}
Vec2d& operator-= (const Vec2d& arg)
{
x-=arg.x; y-=arg.y;
return *this;
}
Vec2d& operator/= (double arg)
{
x/=arg; y/=arg;
return *this;
}
Vec2d& operator*= (double arg)
{
x*=arg; y*=arg;
return *this;
}
};
inline double operator* (const Vec2d& lhs, const Vec2d& rhs)
{
return (lhs.x * rhs.x) + (lhs.y * rhs.y);
}
inline double Norm(const Vec2d& arg)
{
return sqrt( arg * arg );
}
inline Vec2d Normalize(const Vec2d& arg)
{
return arg / Norm(arg);
}
The const & is a great technique and I use it a lot in my personal C++ code as well it is almost like a defacto best practice when working with complex types in C++ as rip-off stated. As for the use of pointer I have a general rule I follow which might help you write better C++ code and help avoid these types of errors in the future. Before you use a pointer ask your self if it is necessary. In c++ the only time this answer is typically yes is when you are dealing with something of arbitrary size where you have no clue what size it is going to be or if you are directly interfacing with C code that forces you to pass in a pointer for some reason. The final reason that SOMETIMES is a answer yes to that question is when you need to return more then 1 value from a function. The only way to return more then one value from a function in C++ is typically with a pointer or via a structure of some sort. The pointer is a lot cheaper in some cases.
Edit: Oh and yes try to lose the bad Java habits everyone will call you a loony. Sometimes objects are not the answer and sometimes they are. That is a key advantage to C++ is that it does not force you to use OOP. Getters and Setters are not always the proper way to do things. If you ever wondered why some Android applications are so slow on a Android phone it is because people are coding in Java with a Java mentality. For some devices and in some cases function calls can be very expensive and what do you think getters and setters are.....
Edit: Oh and yes try to lose the bad Java habits everyone will call you a loony. Sometimes objects are not the answer and sometimes they are. That is a key advantage to C++ is that it does not force you to use OOP. Getters and Setters are not always the proper way to do things. If you ever wondered why some Android applications are so slow on a Android phone it is because people are coding in Java with a Java mentality. For some devices and in some cases function calls can be very expensive and what do you think getters and setters are.....
^ If you're returning multiple values, you should probably use references. Pointers is more like C style than C++
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
