Why doesn't this work?

Question

I'v tried to solve a memory leak in the GLU callback by creating a global variable but now it dos not draw anything:

GLdouble *gluptr = NULL;

void CALLBACK combineCallback(GLdouble coords[3], GLdouble *vertex_data[4],
                              GLfloat weight[4], GLdouble **dataOut)
{
    GLdouble *vertex;
    if(gluptr == NULL)
    {
        gluptr = (GLdouble *) malloc(6 * sizeof(GLdouble));
    }
    vertex = (GLdouble*)gluptr;
    vertex[0] = coords[0];
    vertex[1] = coords[1];
    vertex[2] = coords[2];


    for (int i = 3; i < 6; i++)
    {

        vertex[i] = weight[0] * vertex_data[0][i] +
            weight[1] * vertex_data[0][i] +
            weight[2] * vertex_data[0][i] +
            weight[3] * vertex_data[0][i];
    }


    *dataOut = vertex;


}

basically instead of doing malloc each time in the loop (thus the memory leak) im using a global pointer, but this doesn't work (drawing to the screen not working) which means dataOut is not receiving the vertex data pointed to by my pointer. Why would using malloc to a pointer created in the function work any different than a global variable?

Thanks

Answer 1

You allocate the data only once -- but GLUtesselator needs more than one set of data at a time!

What you do here, is putting all the vertex data into a single place in memory, where in the original code, you had memory per vertex. GLUtesselator needs more then one vertex to funtion properly.

You do call

void gluDeleteTess(GLUtesselator *tessobj);

...afterwards, do you?

Answer 2

Most likely the reason is that something outside of your callback is holding on to the returned data across calls to combineCallback(), and subsequent calls to combineCallback() clobber now the data from the older calls.

Answer 3

The only difference between allocating within the function and having a global variable is that the latter will not allow re-entrant, threaded or recursive calls to combineCallback since the global will be overwritten.

By allocating within the function, you avoid that problem.

By the way, I wouldn't have used a global, I simply would have done:

static GLDouble vertex[6]; // why 6 when you only use 3?

inside the function. The lifetime of the variable would still then be the same but it wouldn't be accessible from outside that function.

But, if you need multiple allocations, that won't help either.

---

As I mentioned in one of the comments, the responsibility for freeing memory passes along with the pointer. There should be a means for you to free all that memory at some point (or GLU will do it itself). Otherwise, I would consider that a bug with GLU and treat it appropriately (I think Kornel has probably hit the nail on the head with his answer).

Answer 4

Looking at the code, you need to rewrite this, there's a few things wrong which shows the inherent lack of understanding pointers and using call-by-reference parameter such as dataOut, secondly, there is no checking on the call to malloc which can fail and WILL fail, the code blindly assumes that the memory is available, thirdly, you have redundant pointer variables used such as vertex and gluptr for a reason. You are actually trying to build up a block of memory by copying the contents from gluptr to vertex, and use the coords pointer-to-block of data type of 'GLDouble', then build up the vertex block of memory... and finally assign it back to dataOut...forgive me if I misunderstand but read on...

This is the code that has removed redundant variables as shown below and fixes up the lack of checking for a NULL pointer...

GLdouble *gluptr = NULL;

void CALLBACK combineCallback(GLdouble coords[3], GLdouble *vertex_data[4],
                              GLfloat weight[4], GLdouble **dataOut)
{
    if((*dataOut) == NULL)
    {
        (*dataOut) = (GLdouble *) malloc(6 * sizeof(GLdouble));
    }
    if (*dataOut != NULL){
       /* PASSED MEMORY ALLOC! */
       (*dataOut)[0] = coords[0];
       (*dataOut)[1] = coords[1];
       (*dataOut)[2] = coords[2];

       for (int i = 3; i < 6; i++)
       {

        (*dataOut)[i] = weight[0] * vertex_data[0][i] +
            weight[1] * vertex_data[0][i] +
            weight[2] * vertex_data[0][i] +
            weight[3] * vertex_data[0][i];
       }
    }
}

The last parameter when calling this function combineCallback is a call-by-reference parameter, hence the usage of the double asterisk..

I must ask this, is dataOut definitely a fixed size of 6 elements? if so then the parameter would need to be tweaked up...to make it look like *(*dataOut[6])... looking at it top off my head (it's late and past my bedtime...)