KSPModdingLibs / KSPCommunityFixes

Community patches for bugs in the KSP codebase
49 stars 17 forks source link

The GameEvents memoryleak fix should have an option for removing modded event handlers #204

Open JonnyOThan opened 6 months ago

JonnyOThan commented 6 months ago

Currently, the memoryleak fix that cleans up destroyed objects from GameEvents handlers does not remove callbacks from mods. Ideally, these would be fixed by the modders.

In practice, most of these are caused by mods that are no longer maintained. It would be nice to have a switch in KSPCF to clean them up as well, and possible a way to keep some by name in case it actually breaks something.

gotmachine commented 6 months ago

The main reason is that some plugins are actually relying on a pattern where a KSPAddon is instantiated once, register some GameEvents handlers from that instance, with those handlers being expected to be kept around for the whole application lifetime, even after a scene switch and the destruction of the KSPAddon. Said otherwise, they rely on the fact that the instance will be kept alive by the GameEvent handler, and KSPCF has no way to detect if such a pattern is actually in use or not. I can't remember which plugins I identified doing that, but there was a couple of them.

IMO, the value of the KSPCF patch is to fix the stock leaks, and to notify about this specific source of leaks in plugins, but even as an option, trying to proactively alter other plugins behavior is likely to cause issues and won't result in a memory leaks free KSP ecosystem anyway, as there are a bunch of other ways plugins can be (are...) involved in memory leaks : non-GameEvent callbacks, static references...

JonnyOThan commented 6 months ago

Yeah that makes sense, and I don't think this should be the default behavior. But perhaps instead of a global switch with opt-out, there should be an opt in list? So that people with memory leaks in unmaintained mods have a fighting chance to improve it without totally adopting maintenance of the mod.

gotmachine commented 6 months ago

I guess an opt-in config list of assembly names can't hurt.