Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): add nullptr check to mesh destructor to avoid segfault on mesh deletion #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

george-cosma
Copy link

@george-cosma george-cosma commented Nov 7, 2024

Bug overview

If the user attempts to delete a mesh using the delete keyword, the program will always segfault if InitFromScene() was not called. This issue is prevalent if the user implements scene-changes, or has short-lived objects that need their own mesh (e.g. bullets with random colors).

The cause of this issue is that the mesh destructor attempts to call ClearAnimations and ClearRootNode even if these two objects have not been initialized (currently they are only set in the InitFromScene() function). The simple fix for this is to check before-hand if these functions need to be called.

The reason this might not have popped up as an issue before is because the default scene behaviour isn't to delete all values from the meshes hash map when the scene's destructor is called. For the lab assignments, this memory leak isn't a big issue.

How to replicate

A simple way to replicate this is to modify lab1.cpp as such:

void Lab1::Init()
{
    // ...
    {
        Mesh* mesh = new Mesh("box");
        mesh->LoadMesh(PATH_JOIN(window->props.selfDir, RESOURCE_PATH::MODELS, "primitives"), "box.obj");
        meshes[mesh->GetMeshID()] = mesh;
    }

    // -=-=-=-=-= ADD THIS =-=-=-=-=-=-
    {
        Mesh* mesh = new Mesh("box2");
        delete mesh;
    }
    // -=-=-=-=-= ADD THIS =-=-=-=-=-=-

   // ...
}

And use this scene in main.cpp:

    // Create a new 3D world and start running it
    World *world = new m1::Lab1();
   

I have tested this bug and the fix only on windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant