Ownership of Models + Double Frees

Question

Ownership of Models + Double Frees

Opened this issue 10 months ago · 11 comments

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

Answer 1 · 2024-03-01T18:10:54.000Z

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.

Answer 2 · 2024-03-01T21:14:02.000Z

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

Answer 3 · 2024-03-01T21:25:07.000Z

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 💀 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.

Answer 4 · 2024-07-02T20:27:27.000Z

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.

Answer 5 · 2024-07-03T01:06:46.000Z

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

Answer 6 · 2024-07-03T19:55:07.000Z

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

Answer 7 · 2024-07-03T20:06:16.000Z

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).~~
https://github.com/raysan5/raylib/blob/master/examples/shaders/shaders_basic_pbr.c#L266

    // Unbind (disconnect) shader from car.material[0] 
    // to avoid UnloadMaterial() trying to unload it automatically
    car.materials[0].shader = (Shader){ 0 };

Answer 8 · 2024-08-30T18:31:37.000Z

Rather than making a new issue, let's just keep this rolling to keep the history in place. Thanks a lot, @furudbat, for the work on cleaning up some of the Shader work.

Answer 9 · 2024-09-01T02:07:25.000Z

It would be really awesome to "automate" or RAII the ownership of resources. (IMHO it should be one of THE feature of this project)

We already try to use move-semantic and dtor to free resources and memory.
Some resources are really simple like Image or bit more complex like RenderTexture, but other like Model feels more like a mess.
Would be nice to gather some information on HOW the Load/Unload C raylib functions works.

Image

This one is very simple, Unload with the dtor,
beware of data (pointer) ownership (moved).

AudioStream and Wave

Like Image, it's self contained.
Used by Sound and Music.

Font

Self contained, but a bit special, only Unload non-default fonts, see UnloadFont.

Sound and Music

When using Sound or Music, you can use AudioStream to construct the classes, the ownership of AudioStream gets moved into Sound/Music, see UnloadMusicStream.

// Unload music stream
void UnloadMusicStream(Music music)
{
    UnloadAudioStream(music.stream);

make AudioStream only movable into Music/Sound ???

Texture and Shader

Manages TextureUnmanaged, calls UnloadTexture.
- unload texture (id)
Manages ShaderUnmanaged, calls UnloadShader.
- unload shader program (id)
- free locs

Be aware when constructing Shader with locs, locs ownership get transfer into Shader

RenderTexture

This one holds an TextureUnmanaged (indirectly), unload frame buffer and texture (UnloadRenderTexture ).
Be aware when using ::Texture to construct the RenderTexture (if you are not the owner).
Maybe forbid calls with const-ref class and only use move:

    RenderTexture(unsigned int id, ::Texture&& texture, const ::Texture& depth)
        : ::RenderTexture{id, texture, depth} {
        texture = { 0 };
    }

Not sure about depth, but the frame buffer (id) and texture gets unloaded, but only IF target.id > 0:

// 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);
    }
}

Material

Shader and maps (pointer) can be set (beware of ownership).

// 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);

    // Unload loaded texture maps (avoid unloading default texture, managed by raylib)
    if (material.maps != NULL)
    {
        for (int i = 0; i < MAX_MATERIAL_MAPS; i++)
        {
            if (material.maps[i].texture.id != rlGetTextureIdDefault()) rlUnloadTexture(material.maps[i].texture.id);
        }
    }

    RL_FREE(material.maps);
}

only unload non-default Shader, make Shader only movable into Material ???
Unload non-default textures (who is owner of this textures, the material ?)
free maps

Mesh

???

Model

???

Value Classes

Color, Vector, etc. ... no RAII needed.

"Global" Management Classes

Some classes like AudioDevice or Window, SHOULD be initialized and closed once.
But I personally would not recommend make this classes singletone, the user should be responsible for using this classes once, like in main or in some sort of Game class, it's more of a "Guard" class.

Answer 10 · 2024-12-08T02:42:39.000Z

Hi there! Sorry, didn't look through issues first and created a pull request for a related problem: #340

Answer 11 · 2024-12-08T03:42:07.000Z

Great research here, @furudbat. Raylib could certainly clean up the memory management of some of these objects a bit. Having it automated would be a bit of a challenge, but I'm open to ideas.

Thanks again for the push, @GaniAliguzhinov. Glad to see some clean up of the memory.