Raylib: [models] 2 models using same mesh causes double-free

[chriscamacho]

Please, before submitting a new issue verify and check:

• [x ] I tested it on latest raylib version from master branch
• [ x] I checked there is no similar issue already reported
• [x ] My code has no errors or misuse of raylib

Issue description

    Mesh mesh = GenMeshTorus(.3, 1, 16, 32);
    models[0] = LoadModelFromMesh(mesh);
    models[4] = LoadModelFromMesh(mesh);
    ....
    UnloadModel(models[0]);
    UnloadModel(models[4]); <--- boom!

Environment

Linux

Issue Screenshot

N/A

Code Example

see above

I you don't want to force the end user to free all meshes before unloading models, how about NULLing the mesh pointer when free-ing and only free when its a NULL pointer ?

[chriscamacho]

I have a pending PR, I can add this to it if you like ? (NULL check before free)

[raysan5]

@chriscamacho thanks! you can send a new PR!

[chriscamacho]

actually this is not a straight forward as I thought, bit difficult to describe what I mean, can you take a look?

[raysan5]

@chriscamacho I see, a bit tricky issue, not easy solution... probably it should be left AS-DESIGN.

[chriscamacho]

I think its worth spending some time on, as it is its not really ideal.....

[chriscamacho]

one thought, might be to have an array of mesh pointers rather than array of mesh....

[Ushio]

Hi, How about the option?

typedef struct Model {
    Matrix transform;       // Local transform matrix

    int meshCount;          // Number of meshes
    Mesh *meshes;           // Meshes array
    bool automaticUnloadMesh; // <- New! If the option is ture, UnloadMesh() will call for the meshes

    int materialCount;      // Number of materials
    Material *materials;    // Materials array
    int *meshMaterial;      // Mesh material number

    // Animation data
    int boneCount;          // Number of bones
    BoneInfo *bones;        // Bones information (skeleton)
    Transform *bindPose;    // Bones base transformation (pose)
} Model;

And the following code will work.

    Mesh mesh = GenMeshTorus(.3, 1, 16, 32);
    models[0] = LoadModelFromMesh(mesh);
    models[4] = LoadModelFromMesh(mesh);
    models[0].automaticUnloadMesh = false;
    models[4].automaticUnloadMesh = false;
    ....
    UnloadModel(models[0]);
    UnloadModel(models[4]); <--- ok
    UnloadMesh(mesh);

It is not the best solution. But I think it is controllable and simple and easy.

[raysan5]

I think problem is on the design and the API itself, Models are complex structures, also keep in mind they could be made of multiple meshes...

[Ushio]

I agree... it is complicated.

[chriscamacho]

There is one possible solution, using reference counting, this can be done by storing a reference count just before the allocated memory (you actually allocate slightly more memory and pass a modified pointer from your own alloc routine) I have a working mockup of this technique if you are interested....

[raysan5]

Some simple (and suboptimal) solutions that pop into my mind:

1. UnloadModel() never unloads meshes, they should be unloaded by user.
2. UnloadModel() receives Model by reference and sets *meshes = NULL when unloaded.
3. LoadModelMesh() makes a shallow copy of the mesh for the model, original mesh should be managed by user.

Those are the simples ones I think...

[raysan5]

Still trying to find a solution for this... at least to avoid the crash with the minimum changes...

[chriscamacho]

you might not be too keen on reference counting, but it can be implemented quite easily by using pointer maths and malloc...

http://bedroomcoders.co.uk/reference-counting-with-c/

I would suggest only doing this for mesh... as each model uses or free's a mesh it will only actually be free'd when no other models are using it...

[chriscamacho]

the above url has some example source code, which is probably more illustrative ....

[raysan5]

After long thinking I decided to leave this issue as-design. Right (and simples) way to use it should be:

models[0] = LoadModelFromMesh(GenMeshTorus(3, 1, 16, 32));
models[4] = LoadModelFromMesh(GenMeshTorus(4, 1, 16, 32));

UnloadModel(models[0]);
UnloadModel(models[4]);

Hopefully anyone else struggling with this problem will find this issue with this answer.