When a 'cloned' struct is changed it changes all the others

Question

I have the following statements:

#define SCALE 4
#define PADDING 1.1

#define CUBES 27
#define CUBE_VERTICES 8
#define CUBE_FACES 6
#define FACE_VERTICES 4

typedef struct {
  float angle;
  float x;
  float y;
  float z;
} Rotate;

typedef struct {
  float x;
  float y;
  float z;
} Vertice;

typedef struct {
  int vertices[4];
  float red;
  float green;
  float blue;
} Face;

typedef struct {
  Vertice vertices[8];
  Face faces[6];
  Rotate rotate;
} Cube;

float rotate_y = 0;
float rotate_x = 0;
int mode = 1;
float rotate_angle = 5;
Cube cubes[27];

Where I define some variables where the most important ones are the structs and the array of Cube.

I will quickly describe what I am doing so it becomes easier for everyone to understand, (Cube) consists of 8 vertices or points (Vertice), six-sided(Face) and a rotation(Rotate), in turn, each Vertice has coordinates (x,y,z), each Face has a coloring and a set of vertex indicators, which will show which vertices of the Cube forms that face, and each Rotate has the rotation angle and which axis will rotate.

Having this cube information I can pass this structure to the openGL draw it on the screen. My project is to make a Rubik’s Cube, so it would have 27 cubes, so the array cube.

So I thought I’d just create the first cube, and then clone the rest by just changing the coordinates of its vertices. Let’s go through the process of creating the first cube:

Cube createCube(Cube c) {
  // criando vertices de cima
  // vertice          cria         X      Y    Z
  c.vertices[0] = createVertice(-0.5, 0.5, 0.5);
  c.vertices[1] = createVertice(0.5, 0.5, 0.5);
  c.vertices[2] = createVertice(0.5, 0.5, -0.5);
  c.vertices[3] = createVertice(-0.5, 0.5, -0.5);

  // criando vertices de baixo
  // vertice          cria         X      Y    Z
  c.vertices[4] = createVertice(-0.5, -0.5, 0.5);
  c.vertices[5] = createVertice(0.5, -0.5, 0.5);
  c.vertices[6] = createVertice(0.5, -0.5, -0.5);
  c.vertices[7] = createVertice(-0.5, -0.5, -0.5);

  // cima e baixo
  // face       cria          determina vertices         R  G  B
  c.faces[0] = createFace(verticesFromFace(1, 2, 3, 4), 0.9, 0.9, 0.9);
  c.faces[1] = createFace(verticesFromFace(5, 6, 7, 8), 0.8, 1.0, 0.2);

  // direita e esquerda
  // face       cria          determina vertices         R  G  B
  c.faces[2] = createFace(verticesFromFace(1, 4, 8, 5), 0.2, 0.4, 0.8);
  c.faces[3] = createFace(verticesFromFace(2, 3, 7, 6), 0.2, 0.8, 0.4);

  // fundo e frente
  // face       cria          determina vertices         R  G  B
  c.faces[4] = createFace(verticesFromFace(1, 2, 6, 5), 0.9, 0.0, 0.2);
  c.faces[5] = createFace(verticesFromFace(4, 3, 7, 8), 1.0, 0.3, 0.1);

  c.rotate = createRotate(0.0f, 0.0f, 0.0f, 0.0f);

  return c;
}

In this method I just create a cube, passing data of each vertex, each face and its rotation. The methods called inside these are:

Vertice createVertice(float x, float y, float z) {
  Vertice v;
  v.x = x;
  v.y = y;
  v.z = z;
  return v;
}

Face createFace(int vertices[4], float red, float green, float blue) {
  Face f;
  f.vertices[0] = vertices[0];
  f.vertices[1] = vertices[1];
  f.vertices[2] = vertices[2];
  f.vertices[3] = vertices[3];
  free(vertices);
  f.red = red;
  f.green = green;
  f.blue = blue;
  return f;
}

Rotate createRotate(float angle, float x, float y, float z) {
  Rotate r;
  r.angle = angle;
  r.x = x;
  r.y = y;
  r.z = z;
  return r;
}

They create a certain struct, passing the data to it and returning it.

With the first cube created, I will clone the others:

void createAndCloneCubes() {
  cubes[0] = createCube(cubes[0]);
  cubes[1] = cloneCube(cubes[0], PADDING, 0, 0);
  cubes[2] = cloneCube(cubes[0], -PADDING, 0, 0);

  cubes[9] = cloneCube(cubes[0], 0, 0, -PADDING);
  cubes[10] = cloneCube(cubes[1], 0, 0, -PADDING);
  cubes[11] = cloneCube(cubes[2], 0, 0, -PADDING);

  cubes[18] = cloneCube(cubes[0], 0, 0, PADDING);
  cubes[19] = cloneCube(cubes[1], 0, 0, PADDING);
  cubes[20] = cloneCube(cubes[2], 0, 0, PADDING);

  cubes[3] = cloneCube(cubes[0], 0, PADDING, 0);
  cubes[4] = cloneCube(cubes[1], 0, PADDING, 0);
  cubes[5] = cloneCube(cubes[2], 0, PADDING, 0);

  cubes[12] = cloneCube(cubes[3], 0, 0, -PADDING);
  cubes[13] = cloneCube(cubes[4], 0, 0, -PADDING);
  cubes[14] = cloneCube(cubes[5], 0, 0, -PADDING);

  cubes[21] = cloneCube(cubes[3], 0, 0, PADDING);
  cubes[22] = cloneCube(cubes[4], 0, 0, PADDING);
  cubes[23] = cloneCube(cubes[5], 0, 0, PADDING);

  cubes[6] = cloneCube(cubes[0], 0, -PADDING, 0);
  cubes[7] = cloneCube(cubes[1], 0, -PADDING, 0);
  cubes[8] = cloneCube(cubes[2], 0, -PADDING, 0);

  cubes[15] = cloneCube(cubes[6], 0, 0, -PADDING);
  cubes[16] = cloneCube(cubes[7], 0, 0, -PADDING);
  cubes[17] = cloneCube(cubes[8], 0, 0, -PADDING);

  cubes[24] = cloneCube(cubes[6], 0, 0, PADDING);
  cubes[25] = cloneCube(cubes[7], 0, 0, PADDING);
  cubes[26] = cloneCube(cubes[8], 0, 0, PADDING);
}

The first I create, and the other I clone, pass by parameter the cube to be cloned, and then the distance between one and the other on each axis(x,y,z).

The method for cloning the cube is as follows:

Cube cloneCube(Cube input, float appX, float appY, float appZ) {
  int i;
  Cube output;
  for (i = 0; i < CUBE_VERTICES; i++)
    output.vertices[i] =
        createVertice(input.vertices[i].x + appX, input.vertices[i].y + appY,
                      input.vertices[i].z + appZ);

  for (i = 0; i < CUBE_FACES; i++) {
    output.faces[i] = createFace(
        verticesFromFace(input.faces[i].vertices[0], input.faces[i].vertices[1],
                         input.faces[i].vertices[2],
                         input.faces[i].vertices[3]),
        input.faces[i].red, input.faces[i].green, input.faces[i].blue);
  }

  output.rotate = createRotate(input.rotate.angle, input.rotate.x,
                               input.rotate.y, input.rotate.z);

  return output;
}

Where I create a new cube, parrying the information from the old one to it, with observation that in the vertices I add the spacing between a cube and another.

Well, finally, after explaining all my program flow, I’ll tell you the real problem, after all this code, I draw the cubes with openGL, so far so good, the cube is generated, but at certain times I need to change the rotation of a cube, so try the following code:

if (key == '9' && mode == 2) {
    Rotate r;
    r.angle = -90;
    r.x = 0;
    r.y = 0;
    r.z = 1;
    cubes[16].rotate = r;
} else if (key == '0' && mode == 2) {
    cubes[16].rotate = createRotate(90, 0, 0, 1);
}

Above, I have two equivalent attempts to change the rotation of cube number 16, but when changing the rotation of the cube, all cloned cubes of this or that has been cloned(7) are also changing the value of their rotation. For example, cube 16 was created like this:

cubes[16] = cloneCube(cubes[7], 0, 0, -PADDING);

So by changing its rotation, the rotation of the Ubes[7], Ubes[25], Ubes[1] (because the 7 was cloned from it) and others are also changing. Probably the rotation of all cloned cubes are at the same address as their parent cube. But I’m not finding the error, as I do so that each cube has its own rotation, even if it has been cloned from another cube?

P.S.: I’m sorry the question is huge, but I tried to do my best so you can understand everything and help me.

Answer 1

The problem is that C, unlike higher-level languages with native object orientation support, does not "create" independent objects for you just with language syntax - you have to do this manually.

So when you declare your data structures static within their functions, as in:

Vertice createVertice(float x, float y, float z) {
  Vertice v;
  ...
  return v;
}

The compiler reserves memory space for a single "Vertice" structure in the body of the createvertice function. When the function is called again, the same structure is used in memory, in the same place - and data changed in it, will change data in other vertices that you have returned in previous invocations.

The C language even supports copying structs and arrays of limited size when they are passed as parameters "by value" and used on both sides of the equal sign, as you are (ab)using in your code. This only prevents your behavior from being even crazier, and causes your program to actually run - otherwise it would probably catch up before that.

I couldn’t really identify at what point in your program vpode is happening something different that causes one of the structures not to be assigned in the normal way by the compiler - who copies each field of the structure - But you can bet your ass the problem is related to that. Possibly there is some problem since you mix malloc usage in the middle and m at least one place - and the error may be there. (Alias, its function createRotate is completely redundant for cases where you will copy the data from another struct Rotate - the assignment with = already copy the fields of the source struct)

Good - basically, what you have to be aware of is the following: the data types directly supported by the CPU architecture are the ones that can and should normally be used "by value" in function calls in C - and are automatically copied when they "arrive" within a function and in return.

Apart from these native types, the correct thing in any code is to allocate the space for your data structures manually, with malloc and pass at all times its objects by reference.

Yes, but other things, will bring the code you see in your program much closer than machine code real created by the C compiler - which is exactly the killer advantage of coding in C: you know what the CPU is doing. In code like yours, when you do something like cubes[0] = createCube(...) - when returning the function Cube, the compiler C has to put "inline" in machine code, the code to copy each field of each structure nested inside the cube - are hundreds of machine code instructions, repeated for each of the 27 cubes you clone. So, although the impact of this on code size on modern machines is deniable, and in performance even less, it’s rather "inelegant". Similarly, when you pass a structure by value to a function, the compiler has to generate code to recursively copy all fields.

So - the "normal" is you, yes, create your objects with malloc and always pass as reference to a function - each function that "create" a new object passes allocates NEW memory to the object in another memory position, and returns a single value that is a unique address of that object.

You can even use your nested structs "by value" as they are (that is, a Cube actually has a 6-face array inside, not six pointers to six face objects) - but then your cube constructor has to take care of it. But it can be more consistent and easier to write everything in this style:

typedef struct {
  float x;
  float y;
  float z;
} Vertice;

... 

#define vertices_per_cube 8
typedef struct {
  Vertice *vertices[8];
  Face faces[6];
  Rotate rotate;
} Cube;


Vertice * createVertice(float x, float y, float z) {
    Vertice * output = malloc(sizeof(Vertice));
    if (output == NULL) {
        printf("Not enough memory - program will crash :-p  ");
    }
    output->x = x; output->y = y; output->z = z;
    return output;
}

Vertice cloneVertice(Vertice * input) {
    Vertice * output = malloc(sizeof(Vertice));
    if (output == NULL) {
        printf("Not enough memory - program will crash :-p  ");
    }
    output->x = input->x; output->y = input->y; output->z = input->z;
    return output;
}

void * destroyVertice(Vertice * vertice) {
    free(vertice);
}

...

Cube * createCube(Vertice * vertices, Faces * faces, Rotate * rotate) {
    int i;
    Cube * output = malloc(sizeof(Cube));
    if (output == NULL) {
        printf("Not enough memory - program will crash :-p  ");
    }

    for (i=0; i < vertices_per_cube; i++) {
        output->vertices[i] = cloneVertice(vertices[i]);
    }
    ...
    return output;

}
...
void destroyCube(Cube * cube) {
    int i;
    for (i=0; i < vertices_per_cube; i++) {
        destroyVertice(cube->vertices[i]);
    }
    // code to destroy Faces
    ...
    destroyRotate(cube->Rotate);
    free(cube);
}

And then, for every object you create, you destroy it when you’re done using it - usually within the same function that was responsible for your creation. The fact that you in your code have a malloc on one side, to create your vertex vector, and a free on the other hand, completely asymmetrical, is something that indicates a code that is very difficult to write correctly and extremely prone to errors. An alternative is to have in all your objects a count of how many references to it there are - and only when the number of references reaches zero, you destroy it - Note that - in a very short time - you want to have generic functions to create, allocate memporia, manage life time, and destroy your objects - it is from this need that languages with built-in object orientation support were born - or even, C frameworks for working with objects, where much of this work is done by macros. (one example is GLIB)

In short - if you want to do this kind of thing in C, you better do it "the right way" - even if it means sweating your shirt. On the other hand, it’s only worth doing this in C to (1) precisely understand and train these mechanisms (didactic purposes) - or if you’re going to run the code on very limited hardware (Iot).