getnamo / GlobalEventSystem-Unreal

Loosely coupled internal event system plugin for the Unreal Engine.
MIT License
276 stars 44 forks source link

Event listeners persisting between PIE sessions #22

Open reisandbeans opened 3 years ago

reisandbeans commented 3 years ago

I noticed that due to the static nature of the default event handler, it's keeping objects in memory between PIE sessions. While I do see some mechanisms in the code to clean up stale listeners, my editor still crashed on some occasions when the game tried to emit events and there were some stale listeners from previous sessions.

I don't know if that's intended behavior or not, but it was happening quite often for me, so I ended up adding another static method to the EventHandler class that will re-instantiate the default event handler (so the old one can be garbage collected), and I call that on the OnStart method of the GameInstance class, which solved the problem for me. That said, would be nice to see an "official" support for hooking this plugin into the game lifecycle to do some clean up

getnamo commented 3 years ago

Ideally your 'bind' action should be balanced with an unbind to control for life cycle of the caller. It's usually enough to add GESUnbindAllEventsForContext to any endplay https://github.com/getnamo/global-event-system-ue4#unbinding.

Simply wrapping it up to end world can still cause you to crash if you e.g. delete the actor before the world ends and so unbinding on e.g. the listener actor/object endplay/destruction like the above is the recommended method.

getnamo commented 3 years ago

It might be possible to enhance this to do auto-unbind automatically e.g. by auto-adding yourself somehow to the destructor of the actor, but I haven't found a clean way to do that yet. Thoughts and code contributions welcome :)

reisandbeans commented 3 years ago

I have an idea that I'd like to try. I will let you know if it works

reisandbeans commented 3 years ago

@getnamo I created an example PR with the idea I mentioned a few days ago, check it out: https://github.com/getnamo/global-event-system-ue4/pull/23

Feel free to change it, or discard it completely. To be honest, even though I'm experienced an developer, I consider myself too newbie when it comes to unreal and I feel like I don't know what doing most of the time, so I could be in the wrong track here 😅

getnamo commented 3 years ago

It's an interesting approach could be useful for PIE start/re-start. I only wonder if the clear function is guaranteed to call before other begin play functions otherwise bugs could appear from clearing after a few setup calls have already been done. Let me know if your testing finds it always called before begin play.

reisandbeans commented 3 years ago

so far it seems to work, but since I was focused on something else, I've done some very superficial testing. I will run some more thorough tests and will let you know