ApacheTech-VintageStory-Mods / ApacheTech.VintageMods.CampaignCartographer

Mod for Vintage Story: Adds multiple Cartography related features to the game, such as custom waypoint icons, GPS, auto waypoint markers, and more.
7 stars 4 forks source link

dispose of cairo image surfaces and contexts #10

Closed svanheulen closed 2 years ago

svanheulen commented 2 years ago

Unfortunately c4ca2ac didn't fix issue #9. I wasn't able to get the VS dev environment working in Arch Linux so I couldn't compile and test these changes, but I think this should resolve the issue.

In PlayerMapLayerPatches.cs the LoadTexture method was getting called multiple times from Patch_PlayerMapLayer_OnMapOpenedClient_Prefix, which was causing the _imageSurface and _context class variables to get reassigned before Patch_PlayerMapLayer_Dispose_Prefix had a chance to dispose of them. From the API docs, it seems like ICoreClientAPI.Gui.LoadCairoTexture copies the surface to an OpenGL texture so I think it's safe to dispose of it after that call. But that needs to be tested.

The other 3 files I changed all had the same issue. The ComposeHover method properly disposed of the context and surface but the GenerateEnabledTexture and Compose methods were missing that.

ApacheTech commented 2 years ago

Some of this will not work. I've used using statements, so where you see

    using var imageSurface = new ImageSurface(...);
    using var context = genContext(imageSurface);
    // Do Stuff.

it automatically disposes at the end of the scope. It's the same as doing

using (var imageSurface = new ImageSurface(...))
{
    using (var context = genContext(imageSurface))
    {
        // Do Stuff.
    }
}

or

    var imageSurface = new ImageSurface(...);
    var context = genContext(imageSurface);
    // Do Stuff.
    imageSurface.Dispose();
    context.Dispose();

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/using-statement

I'll have to change Patch_PlayerMapLayer_OnMapOpenedClient_Prefix to handle disposing the surface and context, once they've been handled. The original code was copypasta from the vanilla class, and then just refactored, and extracted into methods for re-use. I just need to follow the code-flow, and work out exactly where things need disposing.

The most annoying thing is that I get no warnings or errors, at all. Even with the environment variables set, and every single debug option in the game enabled. I'm running it through Visual Studio, with all debug and trace output redirected to the Output Window.

svanheulen commented 2 years ago

Dang, I didn't even notice the using and didn't know that was possible. It's been years since I last worked with C#, haha. Anyway I fixed that in my changes.

Almost every warning I get about it is definitely from the LoadTexture method in PlayerMapLayerPatches.cs. But there's also this one and I can't really figure out what code that even corresponds to:

Cairo.Surface is leaking, programmer is missing a call to Dispose
Allocated from:
  at System.Environment.get_StackTrace () [0x00000] in <62b430b945fa49a19a75382ef03e7bed>:0
  at Cairo.CairoDebug.OnAllocated (System.IntPtr obj) [0x00000] in <9157d36272c14477a7c3607dfa4c8e07>:0
  at Cairo.Surface..ctor (System.IntPtr handle, System.Boolean owner) [0x00000] in <9157d36272c14477a7c3607dfa4c8e07>:0
  at Cairo.ImageSurface..ctor (Cairo.Format format, System.Int32 width, System.Int32 height) [0x00000] in <9157d36272c14477a7c3607dfa4c8e07>:0
  at Vintagestory.API.Client.GuiElement.getImageSurfaceFromAsset (Vintagestory.API.Client.ICoreClientAPI capi, Vintagestory.API.Common.AssetLocation textureLoc, System.Int32 mulAlpha) [0x00000] in <1b0fd81d62a540fe9a361f76c1beeb61>:0
  at Vintagestory.API.Client.GuiElementImage.ComposeElements (Cairo.Context context, Cairo.ImageSurface surface) [0x00000] in <1b0fd81d62a540fe9a361f76c1beeb61>:0
  at Vintagestory.API.Client.GuiComposer.Compose (System.Boolean focusFirstElement) [0x00000] in <1b0fd81d62a540fe9a361f76c1beeb61>:0
  at ApacheTech.VintageMods.Core.Abstractions.GUI.GenericDialogue.Compose () [0x00000] in <38aac79831214b348d67f68ad880ba5c>:0
  at ApacheTech.VintageMods.Core.Abstractions.GUI.GenericDialogue.TryOpen () [0x00000] in <38aac79831214b348d67f68ad880ba5c>:0
  at ApacheTech.VintageMods.CampaignCartographer.Features.ManualWaypoints.ManualWaypoints.OnClientDefaultHandler (System.Int32 groupId, Vintagestory.API.Common.CmdArgs args) [0x00000] in <38aac79831214b348d67f68ad880ba5c>:0
  at ApacheTech.VintageMods.FluentChatCommands.Client.FluentClientCommand.CallHandler (System.Int32 groupId, Vintagestory.API.Common.CmdArgs args) [0x00000] in <38aac79831214b348d67f68ad880ba5c>:0
  at Vintagestory.API.Common.ClientChatCommand.CallHandler (Vintagestory.API.Common.IPlayer player, System.Int32 groupId, Vintagestory.API.Common.CmdArgs args) [0x00000] in <1b0fd81d62a540fe9a361f76c1beeb61>:0
  at Vintagestory.Client.NoObf.HudDialogChat.HandleClientCommand (System.Int32 groupid, System.String command, Vintagestory.API.Common.CmdArgs arguments) [0x00000] in <51d71403970348a48fd088cfc0bd201e>:0
  at Vintagestory.Client.NoObf.HudDialogChat.HandleClientCommand (System.String message, System.Int32 groupid) [0x00000] in <51d71403970348a48fd088cfc0bd201e>:0
  at Vintagestory.Client.NoObf.HudDialogChat.HandleClientMessage (System.Int32 groupid, System.String message) [0x00000] in <51d71403970348a48fd088cfc0bd201e>:0
  at Vintagestory.Client.NoObf.HudDialogChat.OnNewClientToServerChatLine (System.Int32 groupId, System.String message, Vintagestory.API.Common.EnumChatType chattype, System.String data) [0x00000] in <51d71403970348a48fd088cfc0bd201e>:0
  at Vintagestory.Client.NoObf.ClientEventManager.TriggerNewClientChatLine (System.Int32 groupid, System.String message, Vintagestory.API.Common.EnumChatType chattype, System.String data) [0x00000] in <51d71403970348a48fd088cfc0bd201e>:0
  at Vintagestory.Client.NoObf.HudDialogChat.OnKeyDown (Vintagestory.API.Client.KeyEvent args) [0x00000] in <51d71403970348a48fd088cfc0bd201e>:0
  at Vintagestory.Client.NoObf.GuiManager.OnKeyDown (Vintagestory.API.Client.KeyEvent args) [0x00000] in <51d71403970348a48fd088cfc0bd201e>:0
  at Vintagestory.Client.NoObf.ClientMain.OnKeyDown (Vintagestory.API.Client.KeyEvent args) [0x00000] in <51d71403970348a48fd088cfc0bd201e>:0
  at _StwAB8SOEkzQf6E25g6RsQJgjIN._00nC6ZcWiu6AtEnnpVFzUDFFOGs (Vintagestory.API.Client.KeyEvent ) [0x00000] in <51d71403970348a48fd088cfc0bd201e>:0
  at _GOrIbcQzY3GhtLg2IA4LWkKoHMd._00nC6ZcWiu6AtEnnpVFzUDFFOGs (Vintagestory.API.Client.KeyEvent ) [0x00000] in <51d71403970348a48fd088cfc0bd201e>:0
  at Vintagestory.Client.NoObf.ClientPlatformWindows.game_KeyDown (System.Object sender, OpenTK.Input.KeyboardKeyEventArgs e) [0x00000] in <51d71403970348a48fd088cfc0bd201e>:0
  at OpenTK.NativeWindow.OnKeyDown (OpenTK.Input.KeyboardKeyEventArgs e) [0x00000] in <a4886e33f9fc416195d24950e7205b94>:0
  at OpenTK.NativeWindow.OnKeyDownInternal (System.Object sender, OpenTK.Input.KeyboardKeyEventArgs e) [0x00000] in <a4886e33f9fc416195d24950e7205b94>:0
  at OpenTK.Platform.NativeWindowBase.OnKeyDown (OpenTK.Input.Key key, System.Boolean repeat) [0x00000] in <a4886e33f9fc416195d24950e7205b94>:0
  at OpenTK.Platform.X11.X11GLNative.ProcessEvents () [0x00000] in <a4886e33f9fc416195d24950e7205b94>:0
  at OpenTK.NativeWindow.ProcessEvents (System.Boolean retainEvents) [0x00000] in <a4886e33f9fc416195d24950e7205b94>:0
  at OpenTK.NativeWindow.ProcessEvents () [0x00000] in <a4886e33f9fc416195d24950e7205b94>:0
  at OpenTK.GameWindow.Run (System.Double updates_per_second, System.Double frames_per_second) [0x00000] in <a4886e33f9fc416195d24950e7205b94>:0
  at OpenTK.GameWindow.Run () [0x00000] in <a4886e33f9fc416195d24950e7205b94>:0
  at _8BjY9BN21hsFddzkQZc9tbMlpVN._HviuT5VYYDU9qGlJh1elYTk082b (_hxNGdplJeipiq0HEVeGbZvJX0CDA , System.String[] ) [0x00000] in <96286b84ba2d4782b6afc0c2655174ca>:0
  at _8BjY9BN21hsFddzkQZc9tbMlpVN+_38offv2pF9cfj1fbOSl2ItB1qKH._zhsGAZwAJLBr2PlaMDu1BzlfJH9 () [0x00000] in <96286b84ba2d4782b6afc0c2655174ca>:0
  at _TktYYz6xXh4DsPktoVWPRMJ9BdG._HviuT5VYYDU9qGlJh1elYTk082b (System.Threading.ThreadStart ) [0x00000] in <51d71403970348a48fd088cfc0bd201e>:0
  at _8BjY9BN21hsFddzkQZc9tbMlpVN..ctor (System.String[] ) [0x00000] in <96286b84ba2d4782b6afc0c2655174ca>:0
  at _8BjY9BN21hsFddzkQZc9tbMlpVN._p3mA0cKFK6BljifVezCewkFIJrsb (System.String[] ) [0x00000] in <96286b84ba2d4782b6afc0c2655174ca>:0
ApacheTech commented 2 years ago

It's the Compose method from the base class for the ManualWaypointsMenuScreen UI. That class doesn't even have any exposed Contexts, or ImageSurfaces, and neither does the base class. It's a problem with GuiElementImage within the vanilla game, which I did tell Tyron to playtest, and optimise before implementing. I'll make a pre-release and upload to GitHub for you. Let me know if it solves stuff, and I'll work from there to fix the image file.

There's likely another one of those stack traces when you See the First Run screen. If you delete the mod's folder in %VINTAGE_STORY_DATA%\ModData\VintageMods... or edit the global and world json settings files, you can re-enable first run to test.

Thank you for taking the time to do this. It's something that would never have been uncovered. I still don't know why I don't get any errors at all, and why the settings keep resetting every time I load the game.

ApacheTech commented 2 years ago

https://github.com/ApacheTechSolutions/ApacheTech.VintageMods.CampaignCartographer/releases/tag/2.0.2-pre.1

svanheulen commented 2 years ago

No warnings/errors at all and I poked around every part of the interface, including the first run dialog. I'll close this PR since it's of no use now. Thanks!

ApacheTech commented 2 years ago

Thanks. I'll try playing around with the image then, and I'll log a bug report with Tyron. Thank you again.