Archived

This topic is now archived and is closed to further replies.

wolf_10

CopyConstructor

Recommended Posts

wolf_10    122
Hi, i have a Question about my CopyConstructor, anyone knows why that dont work ???? The Program crasged when it''s call the Destructor CMSModel(const CMSModel &par) { m_position=par.m_position; m_timedelta=par.m_timedelta; m_velocity=par.m_velocity; m_speed=par.m_speed; m_numMeshes=par.m_numMeshes; m_pMeshes=new Mesh[m_numMeshes]; m_pMeshes=par.m_pMeshes; m_numMaterials=par.m_numMaterials; m_pMaterials=new Material[m_numMaterials]; m_pMaterials=par.m_pMaterials; m_numTriangles=par.m_numTriangles; m_pTriangles=new Triangle[m_numTriangles]; m_pTriangles=par.m_pTriangles; m_numVertices=par.m_numVertices; m_pVertices= new Vertex[m_numVertices]; m_pVertices=par.m_pVertices; Texture=par.Texture; } CMSModel *Model=new CMSModel(); Here is the Code to Copy CMSModel *newModel=new CMSModel(*Model); wolf

Share this post


Link to post
Share on other sites
Sneftel    1788
You aren''t copying the arrays properly; use memcpy, not the assignment operator. What your code currently does is copy over the pointer, not the array.


How appropriate. You fight like a cow.

Share this post


Link to post
Share on other sites
Wildfire    154
There's quite a few new's in there, got all the appropriate deletes?
Edit: Yup, Sneftel is right.

[edited by - Wildfire on August 11, 2003 9:42:20 AM]

Share this post


Link to post
Share on other sites
Sphax    122

The problem is your allocations and assignations lines, like :

m_pMeshes=new Mesh[m_numMeshes];
m_pMeshes=par.m_pMeshes;

I''m sure that in your destructor you call: delete [] m_pMeshes,
and so you''ll delete multiple time the same memory area ...

What you really want to do in your constructor is probably this :

m_pMeshes=new Mesh[m_numMeshes];
for(int m=0; m < m_numMeshes ;++m)
{
m_pMeshes[m] = par.m_pMeshes[m]
}

Do the same thing for Materials, Triangles and vertices

Share this post


Link to post
Share on other sites
wolf_10    122
I have try this befor

memcpy(m_pVertices, par.m_pVertices, sizeof(Vertex));

but this dosent works too :-(

My Destructor:
.
.
m_numMeshes = 0;
if ( m_pMeshes != NULL )
{
delete[] m_pMeshes;
m_pMeshes = NULL;
}
.
.
.

Share this post


Link to post
Share on other sites
Wildfire    154
quote:

memcpy(m_pVertices, par.m_pVertices, sizeof(Vertex));


shouldn't that be

memcpy(m_pVertices, par.m_pVertices, par.m_numVertices *sizeof(Vertex));

You want to copy all of the vertices, not just a single one?

edit: Checking the pointer for null, and setting the pointer/counter to null/0 in the destructor is pretty much unneeded. As soon the the object is destroyed no one has access to those anymore.

[edited by - Wildfire on August 11, 2003 9:56:14 AM]

Share this post


Link to post
Share on other sites
wolf_10    122
Ok,
the Problem is here

glBegin( GL_TRIANGLES );
{
for ( int j = 0; j < m_pMeshes.m_numTriangles; j++ )
{
int triangleIndex = m_pMeshes[i].m_pTriangleIndices[j];

const Triangle* pTri = &m_pTriangles[triangleIndex];

for ( int k = 0; k < 3; k++ )
{
--------->int index = pTri->m_vertexIndices[k];<-------------
glNormal3fv( pTri->m_vertexNormals[k] );
glTexCoord2f( pTri->m_s[k], pTri->m_t[k] );
glVertex3fv( m_pVertices[index].m_location );
}
}
}
glEnd();

with the Copyconstructor
''index'' have a wrong Value

:-((

Share this post


Link to post
Share on other sites
desertcube    514
did you first assign the memory before calling memcopy? I agree with the rest of them that your problem is here. In the destructor, it doesn''t matter calling delete on NULL it will not crash your program, but calling delete on memory that has already been freed will crash.
If you don''t like memcopy, use the for loop as suggested by Sphax.

Share this post


Link to post
Share on other sites
wolf_10    122
Wildfire,
yes your right, but this dosent work

m_pMeshes=new Mesh[m_numMeshes];
memcpy(m_pMeshes, par.m_pMeshes, par._numMeshes*sizeof(Mesh));


Share this post


Link to post
Share on other sites
wolf_10    122
desertcube,

that''s exactly is my Problem, the Destructor try delete Memory that dosent exist.
But i dont no how avoid that.
I belief the Copyconstructor works wrong, thats the Problem.

Share this post


Link to post
Share on other sites
Sneftel    1788
define "doesn''t work". What line does it crash on?


How appropriate. You fight like a cow.

Share this post


Link to post
Share on other sites
wolf_10    122
Ok,
here my Code

Copyconstructor

CMSModel(const CMSModel &par)
{
m_position=par.m_position;

m_timedelta=par.m_timedelta;

m_velocity=par.m_velocity;

m_speed=par.m_speed;

m_numMeshes=par.m_numMeshes;

m_pMeshes=new Mesh[m_numMeshes];
memcpy(m_pMeshes, par.m_pMeshes, par.m_numMeshes*sizeof(Mesh));

m_numMaterials=par.m_numMaterials;


m_pMaterials=new Material[m_numMaterials];
memcpy(m_pMaterials, par.m_pMaterials, par.m_numMaterials*sizeof(Material));

m_numTriangles=par.m_numTriangles;

m_pTriangles=new Triangle[m_numTriangles];
memcpy(m_pTriangles, par.m_pTriangles, par.m_numTriangles*sizeof(Triangle));

m_numVertices=par.m_numVertices;

m_pVertices= new Vertex[m_numVertices];
memcpy(m_pVertices, par.m_pVertices, par.m_numVertices*sizeof(Vertex));

Texture=par.Texture;
}


My assignment operator:



CMSModel& operator=(const CMSModel &par)
{
if(this==&par)
return *this;

m_position=par.m_position;

m_timedelta=par.m_timedelta;

m_velocity=par.m_velocity;

m_speed=par.m_speed;

m_numMeshes=par.m_numMeshes;

delete[] m_pMeshes;
m_pMeshes=new Mesh[m_numMeshes];
memcpy(m_pMeshes, par.m_pMeshes, par.m_numMeshes*sizeof(Mesh));

m_numMaterials=par.m_numMaterials;

delete[] m_pMaterials;
m_pMaterials=new Material[m_numMaterials];
memcpy(m_pMaterials, par.m_pMaterials, par.m_numMaterials*sizeof(Material));

m_numTriangles=par.m_numTriangles;

delete[] m_pTriangles;
m_pTriangles=new Triangle[m_numTriangles];
memcpy(m_pTriangles, par.m_pTriangles, par.m_numTriangles*sizeof(Triangle));

m_numVertices=par.m_numVertices;

delete[] m_pVertices;
m_pVertices= new Vertex[m_numVertices];
memcpy(m_pVertices, par.m_pVertices, par.m_numVertices*sizeof(Vertex));

Texture=par.Texture;

return *this;
}

the Destructor (works well without Copyconstructor ;-) )

CMSModel::~CMSModel()
{
int i;
for ( i = 0; i < m_numMeshes; i++ )
delete[] m_pMeshes.m_pTriangleIndices;


for ( i = 0; i < m_numMaterials; i++ )
delete[] m_pMaterials[i].m_pTextureFilename;


if ( m_pMeshes != NULL )
{
delete[] m_pMeshes;
}

if ( m_pMaterials != NULL )
{
delete[] m_pMaterials;
}

if ( m_pTriangles != NULL )
{
delete[] m_pTriangles;
}

if ( m_pVertices != NULL )
{
delete[] m_pVertices;
}
}

The Program crashed when its call the Destructor for the Copy Object



Share this post


Link to post
Share on other sites
Sneftel    1788
quote:
Original post by wolf_10
The Program crashed when its call the Destructor for the Copy Object


Yes, we know that. On what line in the destructor?


How appropriate. You fight like a cow.

Share this post


Link to post
Share on other sites
desertcube    514
What you are doing in your copy constructor is creating whats called a shallow copy and is exactly what the default copy constructor does. The problem with this is when you start using pointers. Say you have a pointer that points to A. In your copy constructor, you simply tell the new class to point to A aswell, what you need to do is tell it to point to B, but make sure B holds the same data as A.
So, here's a short example: (note, i haven't had time to test it)


class MyClass
{
public:
MyClass():itsPointer(0) {} //Constructor set pointers to null

MyClass(const MyClass &); //Copy constructor

~MyClass() {delete itsPointer;} //Destructor


int GetData() const {return *itsPointer;}
void SetData(int newData) {*itsPointer = newData;}
private:
int * itsPointer;
};

MyClass::MyClass(const MyClass & rhs)
{
itsPointer = new int;
if (!itsPointer) //Couldn't get any memory

return; //Error


*itsPointer = *rhs.itsPointer; //Set itsPointer to the VALUE of rhs's itsPointer

}



In your example though you have an array, so you'd need to go through each element in a for loop.
Hope this helps

[edited by - desertcube on August 11, 2003 11:09:15 AM]

Share this post


Link to post
Share on other sites
Sneftel    1788
Ah, okay. The problem here is that you have arrays of arrays. These sub-arrays must have space allocated for their copies with new and be moved with a nested for-loop, as before.


How appropriate. You fight like a cow.

Share this post


Link to post
Share on other sites