markcwm / openb3d.mod

OpenB3D BlitzMax wrapper, see openb3d.docs for examples
18 stars 7 forks source link

Memory leakage when Freeing TSprites #19

Closed Kippykip closed 4 years ago

Kippykip commented 4 years ago

Example:

While not(AppTerminate()) Local LeakyBoy:TSprite = CreateSprite() FreeEntity(LeakyBoy) GCCollect() Wend

You will see in task manager that the memory usage will continue to rise. Looks like it was just calling the standard TEntity FreeEntity instead of the TMesh FreeEntity code, which TSprites extend from normally.

I made a fix here: https://github.com/Kippykip/openb3dmax.mod/commit/736d79b49a59679d7b6ac250d1a6524a7b95083a

markcwm commented 4 years ago

Thanks for spotting this Kippy! I tried your fix but it's still leaking looking from taskmgr, we should be able to just call Mesh::FreeEntity did you try that? Further investigation needed.

I thought GCMemAlloced was enough to show memory leaks but it doesn't change in NG and oddly only changes in vanilla when you move the mouse.

Kippykip commented 4 years ago

I know that BlitzMax usually tends to keep increasing but eventually should slow down and drop a bunch of usage, I'll have to do another stress test when I'm free to make sure that it's definitely not continuously increasing and is freeing memory. As for the Mesh::FreeEntity I did try that first which is why it's commented out, but it got a MEMORY_ACCESS_VIOLATION, I haven't exactly looked into it 100% though. Maybe the line under it causing the crash?

markcwm commented 4 years ago

Yes, you've to remove the "delete this" line as well as that is done in Mesh::FreeEntity but not in Entity::FreeEntity, so it's a double free which will crash.

Kippykip commented 4 years ago

I changed it to Mesh::FreeEntity();, I left it going for a few hours and it appears to be fixed. It does start increasing initially like before though which made it confusing. But it definitely appears to be fixed.

markcwm commented 4 years ago

Odd, memory just keeps going up after waiting several minutes. I'll have to test more when I get the time.

Kippykip commented 4 years ago

Strange, I'll keep looking into it though!

Kippykip commented 4 years ago

Actually there could be something leaking when unloading stuff in general. I noticed if I load the map with all the game entities, unload and repeat a few times. The ram usages increases a lot until it gets really laggy until it actually crashes. Is there a way of tracking how many OpenB3D TEntities there are at a time?

Kippykip commented 4 years ago

Ok, turns out yes you can:

    Function DebugOpenB3D()

        Local no_ents:Int
        Local no_cams:Int
        Local no_lights:Int
        Local no_pivs:Int
        Local no_meshes:Int
        Local no_sprites:Int
        Local no_bones:Int
        Local no_brushes:Int
        Local no_textures:Int

        For Local ent:TEntity=EachIn TEntity.entity_list

            no_ents:+1

            If TCamera(ent)<>Null Then no_cams:+1
            If TLight(ent)<>Null Then no_lights:+1
            If TPivot(ent)<>Null Then no_pivs:+1
            If TMesh(ent)<>Null Then no_meshes:+1
            If TSprite(ent)<>Null Then no_sprites:+1
            If TBone(ent)<>Null Then no_bones:+1    

        Next

        For Local bruh:TBrush = EachIn TBrush.brush_list
            no_brushes:+1
        Next
    '   
        SetColor(255, 0, 0)
        DrawText("Entities: " + String(no_ents), 0, 120)
        DrawText("Cameras: " + String(no_cams), 0, 140)
        DrawText("Lights: " + String(no_lights), 0, 160)
        DrawText("Pivots: " + String(no_pivs), 0, 180)
        DrawText("Meshes: " + String(no_meshes), 0, 200)
        DrawText("Sprites: " + String(no_sprites), 0, 220)
        DrawText("Bones: " + String(no_bones), 0, 240)
        DrawText("Brushes: " + String(no_brushes), 0, 260)
    End Function

Turns out I had a massive memory leak because I wasn't freeing GetSurfaceBrush as I was unaware I was making a copy each time... Although having an option to get the brush without copying it will be extremely useful. So that's what's been causing my lag problem overtime in my project when I'm playing... Especially on map load as it was cycling through each texture to compare names for animations/pink and black etc. I feel so stupid, although I'll see if I can make an optional parameter for GetSurfaceBrush/GetEntityBrush on whether to make it a copy or not.

markcwm commented 4 years ago

Hi Kippy, been busy so sorry I haven't replied sooner. Yes, GetSurfaceBrush makes a copy like Blitz3d, but if you just want the existing brush use GetSurface(mesh,id).brush or for GetEntityBrush use mesh.brush so there's no need to tweak the parameters, the names are confusing and not clear enough in the docs either, CopySurfaceBrush or CopyBrush would have been better.

markcwm commented 4 years ago

Hi Kippy, I'd like to take this opportunity to apologize for my neglect as I have just moved house, I'd like to say I do appreciate the fixes you've made to Openb3dmax and I hope you are well enough to keep hacking away at this shit. LOL I have added your fixes for FX1 and CopyMesh so please test that, oh and I won't be changing the docs/examples as I like it separate.

Kippykip commented 4 years ago

Oh yeah no problem! And as for the docs/examples, I just wanted them in my fork etc. Also the GetSurface(mesh,id).brush did the trick! Although I already edited a hacky function in my fork, although I'm going to revert everything back now that I know about how to get brushes directly!

markcwm commented 3 years ago

Actually, it turns out you were right KippyKip, there was a memory leak with CreateSprite because FreeEntity doesn't delete the surface (it does delete surfaces of a mesh) so the solution by Angros is to just have a single surface for all sprites, the alternative is to delete the surface in FreeEntity but this would cause slowdown in particles. The fix is in the 1.26 update here 9782c980261d7431084aa1ac30ddbd48a14e9c5a.