double free from Shaders

[furudbat]

When loading the Models with Mesh the ownership is not clear and the mash(es)(?) got double freed.

Logs

INFO: TEXTURE: [ID 2] Texture loaded successfully (128x128 | GRAY_ALPHA | 1 mipmaps)
INFO: FONT: Default font loaded successfully (224 glyphs)
INFO: FILEIO: [resources/cubicmap.png] File loaded successfully
INFO: IMAGE: Data loaded successfully (32x16 | R8G8B8 | 1 mipmaps)
INFO: TEXTURE: [ID 3] Texture loaded successfully (32x16 | R8G8B8 | 1 mipmaps)
INFO: VAO: [ID 2] Mesh uploaded successfully to VRAM (GPU)
INFO: FILEIO: [resources/cubicmap_atlas.png] File loaded successfully
INFO: IMAGE: Data loaded successfully (256x256 | R8G8B8A8 | 1 mipmaps)
INFO: TEXTURE: [ID 4] Texture loaded successfully (256x256 | R8G8B8A8 | 1 mipmaps)
INFO: TIMER: Target time per frame: 16.667 milliseconds
INFO: TEXTURE: [ID 2] Unloaded texture data from VRAM (GPU)
INFO: SHADER: [ID 3] Default shader unloaded successfully
INFO: TEXTURE: [ID 1] Default texture unloaded successfully
double free or corruption (!prev)
INFO: Window closed successfully
INFO: TEXTURE: [ID 4] Unloaded texture data from VRAM (GPU)
INFO: VAO: [ID 2] Unloaded vertex array data from VRAM (GPU)
INFO: VAO: [ID 2] Unloaded vertex array data from VRAM (GPU)

models example

    // Define the camera to look into our 3d world
    raylib::Camera camera({ 0.2f, 0.4f, 0.2f }, { 0.0f, 0.0f, 0.0f }, { 0.0f, 1.0f, 0.0f }, 45.0f);

    raylib::Texture cubicmap;
    raylib::Model model;
    raylib::Texture texture;

    // load from imMap
    {
        raylib::Image imMap;
        imMap.Load("resources/cubicmap.png");      // Load cubicmap image (RAM)
        cubicmap.Load(imMap);                    // Convert image to texture to display (VRAM)
        raylib::Mesh mesh = raylib::Mesh::Cubicmap(imMap, Vector3{1.0f, 1.0f, 1.0f});
        model.Load(mesh);

        // NOTE: By default each cube is mapped to one part of texture atlas
        texture.Load("resources/cubicmap_atlas.png");    // Load map texture
        model.materials[0].maps[MATERIAL_MAP_DIFFUSE].texture = texture;     // Set map diffuse texture
    }

// Error happen at the end of the main
    ~Model() {
        Unload(); ///< error here ::UnloadModel(*this);
    }

rmodel.c

// Unload model (meshes/materials) from memory (RAM and/or VRAM)
// NOTE: This function takes care of all model elements, for a detailed control
// over them, use UnloadMesh() and UnloadMaterial()
void UnloadModel(Model model)
{
    // Unload meshes
    for (int i = 0; i < model.meshCount; i++) UnloadMesh(model.meshes[i]); ///< here

...

// Unload mesh from memory (RAM and VRAM)
void UnloadMesh(Mesh mesh)
{
    // Unload rlgl mesh vboId data
    rlUnloadVertexArray(mesh.vaoId);

    if (mesh.vboId != NULL) for (int i = 0; i < MAX_MESH_VERTEX_BUFFERS; i++) rlUnloadVertexBuffer(mesh.vboId[i]);
    RL_FREE(mesh.vboId);

    RL_FREE(mesh.vertices); ///< error here

[furudbat]

For Context, LoadModelFromMesh from rmodels.c:

// Load model from generated mesh
// WARNING: A shallow copy of mesh is generated, passed by value,
// as long as struct contains pointers to data and some values, we get a copy
// of mesh pointing to same data as original version... be careful!
Model LoadModelFromMesh(Mesh mesh)
{
    Model model = { 0 };

    model.transform = MatrixIdentity();

    model.meshCount = 1;
    model.meshes = (Mesh *)RL_CALLOC(model.meshCount, sizeof(Mesh));
    model.meshes[0] = mesh; ///< here

Seems like the mesh ownership is shared with the model.

[RobLoach]

Good catch :+1: What would be the best way around this? Something similar to the TextureUnmanaged approach we took?

[furudbat]

~~I'm honestly not sure, my first though would be a Mesh and SharedMesh class, the SharedMesh can only been used by the Model. (Maybe something like a shared_ptr), but idk. The SharedMesh has a "weak-ptr" to the Model(s) and know when the Model has been unloaded, so it didn't need to unload (itself). (but that's just to much shared memory management :skull: IMO)~~

Maybe a MeshUnmanaged can be fine for the first solution.

I also had look into RenderTexture and they have similar ownership problem, I changed my own code to this:

    void SetTexture(const ::Texture& newTexture) = delete;
    void SetTexture(::Texture&& newTexture) {
        texture = newTexture;
        newTexture = NullTexture;
    }

rtextures.c

// Unload render texture from GPU memory (VRAM)
void UnloadRenderTexture(RenderTexture2D target)
{
    if (target.id > 0)
    {
        if (target.texture.id > 0)
        {
            // Color texture attached to FBO is deleted
            rlUnloadTexture(target.texture.id);
        }

        // NOTE: Depth texture/renderbuffer is automatically
        // queried and deleted before deleting framebuffer
        rlUnloadFramebuffer(target.id);
    }
}

texture in RenderTexture gets unloaded.

[DenisBelmondo]

Hello, I noticed this could happen with Shaders as well, as seen in Raylib's rmodels.c:2110:

// Unload material from memory
void UnloadMaterial(Material material)
{
    // Unload material shader (avoid unloading default shader, managed by raylib)
    if (material.shader.id != rlGetShaderIdDefault()) UnloadShader(material.shader);
    ...

I suppose a ShaderUnamanged class may also be required.

[RobLoach]

Good catch, @DenisBelmondo! Let's re-work this issue for Shaders too.

[furudbat]

https://github.com/RobLoach/raylib-cpp/blob/master/include/Material.hpp#L54

    GETTERSETTER(::Shader, Shader, shader)
    GETTERSETTER(::MaterialMap*, Maps, maps)
    // TODO(RobLoach): Resolve the Material params being a float[4].
    // GETTERSETTER(float[4], Params, params)

Seem like the getter for shader needs to return ShaderUnamanged OR just an ::Shader* (?, keep in mind the owner is still the Material, so beware of dangling-pointer...) and the setter needs to move the ownership of Shader&& (or Owner<::Shader>&&) into the Material.

void SetShader(const Shader&) = delete;
void SetShader(::Shader*) { ... } // keep the old API ?, risk of dangling pointer, who is the owner of this pointer ???
//void SetShader(const ShaderUnamanged&) = delete;  // not sure about setter for ShaderUnamanged
void SetShader(Shader&&) { ... }
void SetShader(::Shader&&) { ... }

I guess you load the Shader via raylib (C API) or use the Shader class and move it into the Material:

raylib::Material mat;

{
    raylib::Shader shr (...);
    mat.SetShader(shr);
}

{
   mat.SetShader(LoadShader(...))
}

EDIT: @RobLoach void SetShader(::Shader*) = delete This can to be a Breaking Change for the Shader class

[furudbat]

https://github.com/RobLoach/raylib-cpp/blob/master/include/Material.hpp#L75

Maybe when resetting the shader in Material, the id should be set to rlGetShaderIdDefault() and shader.locs = nullptr; (just to be safe).