Fix issues with RenderTarget leaking memory. #719
Conversation
src/texture.rs
Outdated
| @@ -340,6 +346,12 @@ pub struct RenderTarget { | |||
| pub render_pass: miniquad::RenderPass, | |||
| } | |||
|
|
|||
| impl Drop for RenderTarget { | |||
There was a problem hiding this comment.
will this lead to a use after free kind of error?
{
let a = render_target.clone();
}
draw_texture(render_target);
There was a problem hiding this comment.
right, yeah assuming clone only copies the underlying resources in a shallow way, i guess it would yeah
it seems like both the Texture2D and RenderPass held is basically unmanaged, so you'd have to explicitly do the copy presumably in the Clone impl for RenderTarget
i'll have a look tonight and see if i can make this more sane/along with making an explicit test for the use-after-free case
There was a problem hiding this comment.
i had another pass at it (creating a macroquad wrapper type for RenderPass), but it changes the public API which isn't very nice 🤔 plus the name overlaps with the miniquad name, seems like there's a bunch of little bits in the macroquad API that are still sort of partially unmanaged, seems hard to untangle without breaking backwards compatability
|
Let me split this PR into two parts so that the Atlas leak issue can be fixed separately of this bigger RenderTarget/RenderPass issue quickly ... There we go, split the PR so discussion for the render target bit can continue here if needed |
* Fix issue with RenderTarget leaking memory by adding a drop impl, reported in issue not-fl3#635
profan
changed the title
|
Thanks for PR! |
The memory leak issues just floating around were bothering me, and seemed simple enough to fix so figured why not :)
I also looked at #628 but couldn't actually reproduce the issue, so left that one alone for now.