load_texture() memory leak

[sloganking]

loop{
    load_texture("./test.png").await.unwrap();
}

The above rust code will continue to fill ram until it is exhausted. Is this a memory leak or am I supposed to manually deallocate loaded textures? If so then how?

Macroquad version: 0.3.18

[not-fl3]

Texture2D::delete might help

But loading textures in the loop is usually not the best idea, it is almost always better to preload your resources once, before the game loop

[sloganking]

Thanks! Texture2D::delete definitely does help. But it's still strange to me why drop(Texture2D) doesn't implement Texture2D::delete's behavior. And I have to remember to manually dealocate those variables in safe rust. It doesn't seem terribly safe or idiomatic. It also makes dealing with them a little unwieldy, as you can't unload them with various convenience functions like HashMap::retain. Is there a reason I'm not seeing for having it this way instead? What use could we possibly have for data that no longer has any variables pointing to it?

Pre-loading all resources is not an option for me, as I'm working with so many textures that if they were all loaded at once, they'd either take up several GB or exhaust memory entirely. So they have to be streamed.

[sloganking]

Are we interested in removing the copy trait from Texture2D and manually implementing drop() for it?

I see the ReadMe's stated goal

macroquad attempts to avoid any Rust-specific programming concepts like lifetimes/borrowing, making it very friendly for Rust beginners

And I'm not sure how that is guiding macroquad's development decisions. But as it stands, this issue introduces memory leaks, use after free, and double free bugs. The first of which can cause eventual crashing, and the ladder two of which can cause undefined behavior and software vulnerabilities.

I support macroquad's attempt to be easy to program with. But as it stands, by making Texture2D copy and not implement drop(), rather than removing complexity, we've moved complexity from being something that the compiler can detect and warn the programmer about, saving them from themselves. To something that has to be explicitly and correctly managed by the programmer, or they get crashes and software vulnerabilities. This seems to eliminate the value proposition of Rust, to the point that I'm not sure why you'd program in Rust if you wanted to have C style manual memory management.

Summary

Texture2D's current configuration seems to be hiding complexity, rather than eliminating it. And hiding it in places that most safe Rust developers would assume they don't have to look.

Note

• If we do remove the copy trait in order to implement drop(). Around a dozen or so places in macroquad give lifetime errors and would have to be re-thought out.
• ~~Does clone() currently clone the texture data in vram, or just the pointer to it? If the ladder, that would need to be fixed as well.~~
  • It does not clone the actual texture.
• Macroquad may have some other data types with the same issue.

[sloganking]

use macroquad::prelude::*;

#[macroquad::main("test")]
async fn main() {

    let texture1 = load_texture("./test.png").await.unwrap();
    let texture2 = texture1.clone();
    texture1.delete();

    loop{
        clear_background(GRAY);

        let params = DrawTextureParams {
            dest_size: Some(vec2(256., 256.)),
            source: None,
            rotation: 0.,
            flip_x: false,
            flip_y: false,
            pivot: None,
        };

        draw_texture_ex(texture2, 0., 0., WHITE, params);

        next_frame().await
    }
}

gets me this

So currently Texture2D::clone() just clones the pointer to a texture, and not the texture itself. This would need to be fixed as well.

[omarandlorraine]

I've discovered another memory leak

loop {
    load_ttf_font(&"/path/to/font.ttf").await.unwrap();
}

Unlike Texture2D, there is no such method as .delete on a Font object. so I don't have a workaround to this yet.

But loading textures in the loop is usually not the best idea, it is almost always better to preload your resources once, before the game loop

The application I'm working on actually reloads some assets in response to settings changing. The user can change fonts/background images etc. It's a fire alarm so I picked Rust for this project because lives could be depending on this application so I'm very interested in getting this right. Would you accept a pull request to impl Drop for the font and Texture2D (and perhaps other things I find)?

[omarandlorraine]

So currently Texture2D::clone() just clones the pointer to a texture, and not the texture itself. This would need to be fixed as well.

It seems like a different, maybe unrelated bug. What do you think?

[RybekU]

    /// Delete GPU texture, leaving handle unmodified.
    ///
    /// More high-level code on top of miniquad probably is going to call this in Drop implementation of some
    /// more RAII buffer object.
    ///
    /// There is no protection against using deleted textures later. However its not an UB in OpenGl and thats why
    /// this function is not marked as unsafe
    pub fn delete(&self) {
        unsafe {
            glDeleteTextures(1, &self.texture as *const _);
        }
    }

I have decided to dig a bit out of curiosity, and in the miniquad library this was indeed the idea. Comment above is in the source.
Implementation would require introduction of reference counting. Texture2D is just a handle to Texture which is also a handle. The actual textures are stored in the miniquad's GraphicsContext.

[not-fl3]

loop {
    load_ttf_font(&"/path/to/font.ttf").await.unwrap();
}

Unlike Texture2D, there is no such method as .delete on a Font object. so I don't have a workaround to this yet.

But loading textures in the loop is usually not the best idea, it is almost always better to preload your resources once, before the game loop

Sorry, missed this comment! Yes, looks likeFont is missing a .delete :( Feel free to open a separate issue/PR!

[crumblingstatue]

It's very surprising to me that it's not better documented that Texture2d never unloads from memory. This managed to crash my system because it ran out of memory.

[andreymal]

@not-fl3 the Texture2D::delete method has been removed in https://github.com/not-fl3/macroquad/commit/8f2856f09674324b1222d7f344b2d101377430bb. How to deallocate unused textures in the latest version of macroquad?

[omarandlorraine]

@not-fl3 the Texture2D::delete method has been removed in 8f2856f. How to deallocate unused textures in the latest version of macroquad?

I was under the impression that 451bc38 fixes the memory leak and we don't need to deallocate textures in client code, have I got this wrong?

[andreymal]

@omarandlorraine tarsila uses macroquad v0.3, but the delete method is no longer available in macroquad v0.4.2

I'm currently testing the latest versions of macroquad (https://github.com/not-fl3/macroquad/commit/a62130464798d2423f4fc0dcb849fc15e686a6c4) and miniquad (https://github.com/not-fl3/miniquad/commit/0fa2e35d36266485a432176bd04204aab216e58d) and unfortunately the GPU memory is leaking

I guess TextureSlotGuarded::drop is supposed to free memory automatically, but it is not being freed for some reason

[not-fl3]

@not-fl3 the Texture2D::delete method has been removed in 8f2856f. How to deallocate unused textures in the latest version of macroquad?

Thanks for report, should be fixed with https://github.com/not-fl3/macroquad/commit/65451f5bdcf7802c3d62181873d30b9c3c6c49f2