raylib::Model Causing a Malloc error on Copy

[Raelr]

Hey guys!

I've run into a segfault that only seems to be caused by the destructor of both raylib::Model and raylib::Texture. I'm not sure where the full issue actually lies, however I thought I'd make it known here.

The context here is that I'm trying to build a resource manager that stores and tracks all Models and Textures that were loaded into the project. To allow multiple different types of objects, I'm using a std::variant class to store either a texture or Model, with a string acting as the key. The data structure is as follows:

std::map<std::string, std::variant<raylib::Model, raylib::Texture2D>>

The issue is, once I insert an element to this map, my program crashes with the following error:

app(31394,0x10fc30e00) malloc: *** error for object 0x7f9100000000: pointer being freed was not allocated
app(31394,0x10fc30e00) malloc: *** set a breakpoint in malloc_error_break to debug
make: *** [execute] Abort trap: 6

To test this further, I set up a few test cases to see whether I could isolate this issue. Here's my code for storing a standard Raylib Model is a std::variant class:

auto model = LoadModel("assets/models/cube/cube.obj");
std::variant<Model, Texture> variant = std::variant<Model, Texture>();
variant = model;

When running this, I get no issues and the program runs normally. I can even add this variant to a map with no issue:

std::map<std::string, std::variant<Model, Texture>> map = std::map<std::string, std::variant<Model, Texture>>();
map.insert({"model", variant});

Doing this has no issues at all with any part of the program. It runs correctly and closes correctly.

When trying the same tests with raylib::Model, I get a very different outcome:

raylib::Model model2 = raylib::Model("assets/models/cube/cube.obj");
std::variant<raylib::Model, raylib::Texture> variant2 = std::variant<raylib::Model, raylib::Texture>(model2);

Running the following causes no errors initially, however closing the program results in the following segfault:

app(31580,0x1115eae00) malloc: *** error for object 0x9000000000000000: pointer being freed was not allocated
app(31580,0x1115eae00) malloc: *** set a breakpoint in malloc_error_break to debug
make: *** [execute] Abort trap: 6

When I then try and add the variant to a map of variants:

std::map<std::string, std::variant<raylib::Model, raylib::Texture>> map = std::map<std::string, std::variant<raylib::Model, raylib::Texture>>();
map.insert({"variant", variant});

This causes an instant error:

INFO: MODEL: model has 1 material meshes
INFO: VAO: [ID 2] Mesh uploaded successfully to VRAM (GPU)
INFO: VAO: [ID 2] Unloaded vertex data from VRAM (GPU)
INFO: MODEL: Unloaded model from RAM and VRAM
app(31580,0x1115eae00) malloc: *** error for object 0x9000000000000000: pointer being freed was not allocated
app(31580,0x1115eae00) malloc: *** set a breakpoint in malloc_error_break to debug
make: *** [execute] Abort trap: 6

I'll also note that this issue seems to persist whenever a raylib::Model is added to any container type. For example:

std::vector<raylib::Model> vector = std::vector<raylib::Model>();
vector.push_back(raylib::Model("assets/models/cube/cube.obj"));

This will run alright, but when the application is closed another crash will occur:

app(31634,0x11b0fee00) malloc: *** error for object 0x7ffedfe0b918: pointer being freed was not allocated
app(31634,0x11b0fee00) malloc: *** set a breakpoint in malloc_error_break to debug
make: *** [execute] Abort trap: 6

The reason why I made all these cases was to try and illustrate that there seems to be some sort of issue with the way the raylib::Model class is being destroyed. Looking over Raylib's logs, I can see the following sequence of events:

INFO: MODEL: [assets/models/cube/cube.obj] OBJ data loaded successfully: 1 meshes / 1 materials
INFO: MODEL: model has 1 material meshes
INFO: VAO: [ID 2] Mesh uploaded successfully to VRAM (GPU)
INFO: VAO: [ID 2] Unloaded vertex data from VRAM (GPU)
INFO: MODEL: Unloaded model from RAM and VRAM

Looking at this, it appears as if the model is being loaded in successfully, but is immediately being unloaded as soon as the model is added to a container. I'm not sure why this is the case, however I assume it might have to do with how the model is copied?

I'm not convinced that this is an issue with the normal raylib library, since these issues don't seem to be persisting with traditional raylib objects. Would you happen to have any insights into this?

[RobLoach]

Is there an example we could port that could demonstrate this?

Curious if the variant is copy assigning instead using the model by reference.

Also, how does C++ respond to the assignment rather than just the constructor?

raylib::Model model2("assets/models/cube/cube.obj");
// Instead of...
raylib::Model model2 = raylib::Model("assets/models/cube/cube.obj");

Would we need to use std::reference_wrapper? Seems like an ugly solution.

[Raelr]

Hey @RobLoach,

Thanks for getting back as quickly as you did!

I'll go ahead and test out the direct assignment! I'll also see if I can write up a short demonstration for you. I'll get back as soon as I have the info you asked for :)

[Raelr]

Hey @RobLoach,

Thanks for the patience, sorry I didn't send this through earlier!

I set up this base project that you should be able to clone here: https://github.com/Raelr/raylib-sandbox

I set up 5 test cases which you can un-comment. These should demonstrate the issue (I hope).

I tried your suggestion, however using an assignment did not make a difference for the model. I also noticed that we CAN add a model to a variant within a map only if we assign the model directly to the map. Here's an example of code that can make it past the setup stage:

raylib::Model variantModel("assets/models/cube/cube.obj");
std::map<std::string, std::variant<raylib::Model, raylib::Texture2D>> variantMap = std::map<std::string, std::variant<raylib::Model, raylib::Texture2D>>();
variantMap.insert({"model", variantModel});

This will make it past the setup stage, however it will crash when the program is closed.

If I try and directly create a variant and pass the Model into it, however, things get a little messy:

raylib::Model model("assets/models/cube/cube.obj");
std::map<std::string, std::variant<raylib::Model, raylib::Texture2D>> map = std::map<std::string, std::variant<raylib::Model, raylib::Texture2D>>();
map.insert({"model", std::variant<raylib::Model, raylib::Texture2D>(model)});

The above code will crash when the model is inserted to the map.

Finally, I also want to note that adding a model to a map seems to unload the model, but doesn't load it back. As such, any attempts to draw models from a map results in a segfault.

I hope this is helpful! I tried to cover all the cases I could find. Please let me know if you have any other questions!

[RobLoach]

Thanks for setting up the sandbox. Tried some of the tests and I see what you mean.

Found you could have the map reference pointers to the objects instead of the objects themselves....

    std::map<std::string, raylib::Model*> map = std::map<std::string, raylib::Model*>();
    raylib::Model model = raylib::Model("assets/models/cube/cube.obj");
    map.insert({"model", &model});

I'm not 100% sure, but what I think is happening is that we were creating the Model object, and then copying it into the map. Then, when the program exits, it would destruct the Model object, and then destruct the copied object within the map, resulting in it trying to free twice.

I've pushed up a Pull Request with a demo of what that would look like over at https://github.com/Raelr/raylib-sandbox/pull/1 .

[Raelr]

Hey @RobLoach,

Thanks for that!

Yes, I'm aware that using pointers does work, however I'd prefer to have the ability to store models as they are. The main reason being is that a lot of objects will be using the resource system and I want to minimise the number of objects exposed to these pointers. If I pass the same pointer around then it runs the risk of someone or something deleting it and invalidating the resource for all other objects. It'd be a nightmare to debug.

That's the main reason why I thought I'd ask about this issue - would this be something that you'd be willing to support?

[RobLoach]

By reference works for the std::map, but not the std::variant.

     raylib::Model model("assets/models/cube/cube.obj");

     std::map<std::string, raylib::Model&> map = std::map<std::string, raylib::Model&>();

     map.insert({"model", model});

Don't have that much experience with std::variant. Do you know why by reference for that wouldn't work?

[Raelr]

Hey @RobLoach,

I'm not sure, but my research seems to indicate that there was some sort of disagreement as to what the = operator should do with variants, so they just chose to disallow it. You can read more about it here: https://stackoverflow.com/questions/54218595/why-are-references-forbidden-in-stdvariant.

This isn't too helpful for our case, I'm afraid :(

[RobLoach]

Hmm. Using std::variant without copy assignment is well beyond my knowledge. Is there another design you could adopt? Using the objects by reference is probably your best bet here. Maybe using your own Asset wrapper class?

[Raelr]

Let me look into it and see if I can find a better solution. I guess the other thing is that we still have the issue where storing raylib::Model in a vector or map will cause a crash when the application is closed even without std::variant.

[RobLoach]

emplace_back will construct the object directly in the vector.

std::vector<raylib::Model> models;
models.emplace_back("assets/models/cube/cube.obj");

I've heard good things of std::move, but can't get it to work properly...

raylib::Model model("assets/models/cube/cube.obj");
std::vector<raylib::Model> models;
models.push_back(std::move(model));

With the map, by reference works...

raylib::Model model("assets/models/cube/cube.obj");
std::map<std::string, raylib::Model&> map = std::map<std::string, raylib::Model&>();
 map.insert({"model", model});

[Raelr]

Hmmmm... I'll see what I can do with this information. I'll let you know if I manage to find a workaround!

[RobLoach]

We may need move constructors for all the objects? Perhaps? 🤔 https://en.cppreference.com/w/cpp/language/move_constructor

[RobLoach]

This feels wrong, but might be the approach....

class Texture {
  Texture(Texture&& old) {
    id = old.id;
    width = old.width;
    height = old.height;
    mipmaps = old.mipmaps;
    format = old.format;
    
    old.id = 0;
    old.width = 0;
    old.height = 0;
    old.mipmaps = 0;
    old.format = 0;
  }

    inline void Unload() {
        if (width > 0) {
           ::UnloadTexture(*this);
        }
    }
}

That would possibly let you construct the object, and move its object ownership to the vector. May also need a assign temporary too.

Texture& operator= (Texture&& other) {
  if (this != &other) {
    id = other.id;
    width = other.width;
    height = other.height;
    mipmaps = other.mipmaps;
    format = other.format;
    
    other.id = 0;
    other.width = 0;
    other.height = 0;
    other.mipmaps = 0;
    other.format = 0;
  }
}

With the above,, you would be able to move ownership of the Model over to the Vector. The draw-back of it, however, is that you would no longer be able to use the local model, since the ownership is now in the vector.

raylib::Model model("assets/models/cube/cube.obj");
std::vector<raylib::Model> models;
models.push_back(std::move(model));

[Raelr]

Hey @RobLoach,

Thanks for looking into that!

Yeah, I think that should be fine for my purposes, since we aren't really re-using the variable after it's added in the same functions. We're trying to keep it somewhat data oriented, so most transformations and changes will happen on an update loop, rather than on the local variable.

As such, I'd likely just write it as:

std::vector<raylib::Model> models;
models.push_back(std::move(raylib::Model("assets/models/cube/cube.obj")));

Or something similar to that. Were you able to test the move constructor out on your end by any chance?

Again, thank you so much for looking further into this!

[RobLoach]

The Font Loading example might be a good place to test out using vector for construction and deconstruction of these data types; https://github.com/RobLoach/raylib-cpp/blob/master/examples/text/text_font_loading.cpp

[furudbat]

We may need move constructors for all the objects? Perhaps? 🤔 https://en.cppreference.com/w/cpp/language/move_constructor

At least every class with an custom dtor to apply the rule-of-five .

I'm personally not a fan of the implicit casting, but IMO the POD raylib types (like ::Model) should also be movable, e.g. when I'm passing the same ::Model to 2 raylib::Models and both uses then same pointers (who is then the owner ?).

Or at least it's need to be copyable like raylib::Image or raylib::Wave.

    explicit constexpr Model(owner<const ::Model&> model) = delete;
    explicit constexpr Model(owner<::Model&&> model) {
        set(model);

        model.meshCount = 0;
        model.materialCount = 0;
        model.meshes = nullptr;
        model.materials = nullptr;
        model.meshMaterial = nullptr;
        model.boneCount = 0;
        model.bones = nullptr;
        model.bindPose = nullptr;
    }
...
    constexpr Model(const Model&) = delete;
    Model(Model&& other) {
        set(other);

        other.meshCount = 0;
        other.materialCount = 0;
        other.meshes = nullptr;
        other.materials = nullptr;
        other.meshMaterial = nullptr;
        other.boneCount = 0;
        other.bones = nullptr;
        other.bindPose = nullptr;
    }

[furudbat]

After "deleting" the copy-ctor (for Texture and Mesh), and make them only-movable, you can use lambdas to "move" the return -value.

@RobLoach

// Texture
    Texture(const ::Texture& other) = delete;
    Texture(::Texture&& other) noexcept { ... }
    Texture(const Texture&) = delete;
    Texture& operator=(Texture&& other) noexcept { ... }

@Raelr

    const auto loadTextureAtlas = [] {
        raylib::Texture atlas_texture;
        atlas_texture.Load("resources/cubicmap_atlas.png");    // Load map texture
        return atlas_texture;
    };


    raylib::Texture texture;
    //texture.Load("resources/cubicmap_atlas.png");    // Load map texture
    texture = loadTextureAtlas();

    const auto loadModel = [&] {
        raylib::Model model;
        raylib::Image imMap;
        raylib::Texture cubicmap;
        imMap.Load("resources/cubicmap.png");      // Load cubicmap image (RAM)
        cubicmap.Load(imMap);                    // Convert image to texture to display (VRAM)
        model.Load(raylib::Mesh::Cubicmap(imMap, Vector3{1.0f, 1.0f, 1.0f}));

        // NOTE: By default each cube is mapped to one part of texture atlas
        model.GetMaterials()[0].maps[MATERIAL_MAP_DIFFUSE].texture = texture;     // Set map diffuse texture

        return model;
    };

        std::variant<raylib::Model, raylib::Texture> variant = loadModel();

        std::unordered_map<std::string, std::variant<raylib::Model, raylib::Texture>> map;
        map["model"] = loadModel();
        map["texture"] = loadTexture();