Improving Game EventSystem

Question

There is a Unity EventManager class from Unity tutorial that allows events to be registered, unregistered and invoked. The bad side of that EventManager code is that it uses UnityEvent which is slow and it does not take parameter.

I decided to rewrite it with Action from C# instead of Unity's UnityEvent and also added support for parameter. Below is the new EventManager I modified that uses Action and it also allows any amount of parameter to be passed to the event.

using System;
using System.Collections;
using System.Collections.Generic;
using UnityEngine;
public class EventManager : MonoBehaviour
{
 private Dictionary<string, Action<EventParam>> eventDictionary;
 private static EventManager eventManager;
 public static EventManager instance
 {
 get
 {
 if (!eventManager)
 {
 eventManager = FindObjectOfType(typeof(EventManager)) as EventManager;
 if (!eventManager)
 {
 Debug.LogError("There needs to be one active EventManger script on a GameObject in your scene.");
 }
 else
 {
 eventManager.Init();
 }
 }
 return eventManager;
 }
 }
 void Init()
 {
 if (eventDictionary == null)
 {
 eventDictionary = new Dictionary<string, Action<EventParam>>();
 }
 }
 public static void StartListening(string eventName, Action<EventParam> listener)
 {
 Action<EventParam> thisEvent;
 if (instance.eventDictionary.TryGetValue(eventName, out thisEvent))
 {
 //Add more event to the existing one
 thisEvent += listener;
 //Update the Dictionary
 instance.eventDictionary[eventName] = thisEvent;
 }
 else
 {
 //Add event to the Dictionary for the first time
 thisEvent += listener;
 instance.eventDictionary.Add(eventName, thisEvent);
 }
 }
 public static void StopListening(string eventName, Action<EventParam> listener)
 {
 if (eventManager == null) return;
 Action<EventParam> thisEvent;
 if (instance.eventDictionary.TryGetValue(eventName, out thisEvent))
 {
 //Remove event from the existing one
 thisEvent -= listener;
 //Update the Dictionary
 instance.eventDictionary[eventName] = thisEvent;
 }
 }
 public static void TriggerEvent(string eventName, EventParam eventParam)
 {
 Action<EventParam> thisEvent = null;
 if (instance.eventDictionary.TryGetValue(eventName, out thisEvent))
 {
 thisEvent.Invoke(eventParam);
 // OR USE instance.eventDictionary[eventName](eventParam);
 }
 }
}
//Re-usable structure/ Can be a class to. Add all parameters you need inside it
public struct EventParam
{
 public string param1;
 public int param2;
 public float param3;
 public bool param4;
}

USAGE:

public class Test : MonoBehaviour
{
 private Action<EventParam> someListener1;
 private Action<EventParam> someListener2;
 private Action<EventParam> someListener3;
 void Awake()
 {
 someListener1 = new Action<EventParam>(SomeFunction);
 someListener2 = new Action<EventParam>(SomeOtherFunction);
 someListener3 = new Action<EventParam>(SomeThirdFunction);
 StartCoroutine(invokeTest());
 }
 IEnumerator invokeTest()
 {
 WaitForSeconds waitTime = new WaitForSeconds(0.5f);
 //Create parameter to pass to the event
 EventParam eventParam = new EventParam();
 eventParam.param1 = "Hello";
 eventParam.param2 = 99;
 eventParam.param3 = 43.4f;
 eventParam.param4 = true;
 while (true)
 {
 yield return waitTime;
 EventManager.TriggerEvent("test", eventParam);
 yield return waitTime;
 EventManager.TriggerEvent("Spawn", eventParam);
 yield return waitTime;
 EventManager.TriggerEvent("Destroy", eventParam);
 }
 }
 void OnEnable()
 {
 //Register With Action variable
 EventManager.StartListening("test", someListener1);
 EventManager.StartListening("Spawn", someListener2);
 EventManager.StartListening("Destroy", someListener3);
 //OR Register Directly to function
 EventManager.StartListening("test", SomeFunction);
 EventManager.StartListening("Spawn", SomeOtherFunction);
 EventManager.StartListening("Destroy", SomeThirdFunction);
 }
 void OnDisable()
 {
 //Un-Register With Action variable
 EventManager.StopListening("test", someListener1);
 EventManager.StopListening("Spawn", someListener2);
 EventManager.StopListening("Destroy", someListener3);
 //OR Un-Register Directly to function
 EventManager.StopListening("test", SomeFunction);
 EventManager.StopListening("Spawn", SomeOtherFunction);
 EventManager.StopListening("Destroy", SomeThirdFunction);
 }
 void SomeFunction(EventParam eventParam)
 {
 Debug.Log("Some Function was called!");
 }
 void SomeOtherFunction(EventParam eventParam)
 {
 Debug.Log("Some Other Function was called!");
 }
 void SomeThirdFunction(EventParam eventParam)
 {
 Debug.Log("Some Third Function was called!");
 }
}

---

It's working fine.I know I can use the params keyword to receive multiple arguments but I am trying to avoid using the params keyword as it also creates garbage during run-time so I used a struct called "EventParam" to hold and pass all my parameters.

I want to know if there is anything to improve in this script and if there is any other better way to handle parameter other than the current way I am currently doing it.

• Ed Marty: Note that UnityEvent can be parameterized to take parameters via UnityEvent<ParameterTypeA, ParameterTypeB> etc.
• Programmer: @EdMarty Yes I know but that doesn't matter since part of the improvement was getting ride of UnityEvent and using Action. I don't use UnityEvent
• Ed Marty: Yeah,I'm not suggesting you switch back to use it. I'm only mentioning it because at the beginning of your question you say that it's not parameterizable.
• Programmer: @EdMarty I meant that the EventManager from that tutorial does not take parameter not the UnityEvent. Bad phrasing I guess.

Answer 1

1. Why do you have both: static methods and static instance? You should either remove the instance property (as if you had a "static" class, just make eventDictionary static), or keep it, but make methods non-static (singleton (anti-)pattern).

2. I'm pretty sure this line should throw NullReferenceException.

   //Add event to the Dictionary for the first time
   thisEvent += listener;
   

   If thisEvent was not found in a dictionary, it should be null at this point.

3. This does not look reusable to me:

   //Re-usable structure/ Can be a class to. Add all parameters you need inside it
   public struct EventParam
   {
    public string param1;
    public int param2;
    public float param3;
    public bool param4;
   }
   

   The meaning of those fields is extremely unclear, and the suggestion to "add all parameters you need" sounds like a bad idea. What if two events need different sets of parameters? What if one of them needs two floats? How will this architecture evolve then? I see two ways to make this class reusable.

   • One is to use weakly typed parameters, so instead of passing EventParam you pass object and trust that event handlers will do the cast correctly. Very usafe, but easy to implement.
   • A much better approach is to use generic methods and instead of EventParam use generic argument as parameter. It is harder to implement properly though.

• Programmer: 1. I wanted to have only one instance of EventManager. It is a component that is supposed to be attached to a GameObject so I can't make that class static because then inheriting from MonoBehaviour would be useless since it can't be attached to a GameObject. With your comment, I now think that instance variable should changed to a private instead of public since it's not being used outside of that class. - 2 I don't understand what you mean. Could you please rephrase that or add more info?
• Programmer: 3. The EventParam is just a wrapper for parameters. The user is supposed to add/remove variables they want to pass inside the struct. The four variables inside it are just used for test. I can't use object for that since it creates garbage due to boxing/unboxing of types. I don't think you can use generics for this without using reflection. If you think otherwise, it would be awesome to provide an example. My biggest goal here is performance not readability and I have avoided stuff like using object for params or using reflection.
• Nikita B: 1. That's why I've put "static" in quotes. I see that you need inheritance, but it does not change the fact that you can still make all the methods static an remove the instance property. 2. I mean that this line will throw exception. I do not know how to say it differently. :) 3. You do not need reflection, but you do need a strong knowledge of C#. Here is a standalnoe implementation form Prism, for example: github.com/tngraf/Prism.EventAggregator .
• Nikita B: P.S. You should not sacrifice readability in favor of some abstract performance gains, unless you have a specific goal in mind. Write a solid readable code, then test its performance against your concrete goals. Only then, if it is "too slow", you should start optimizing it. Don't jump to conclusions, assuming that it will be to slow. In most cases - it simply won't be the case.
• Binary Impact BIG: You can ensure to have only one instance by using the tipp to make it a singleton and let unity create it when needed. Like this: private static EventManager eventManager = null; public static EventManager instance{ get { if (eventManager == null) { GameObject EventManagerGO = new GameObject ("EventManager"); eventManager = EventManagerGO.AddComponent<EventManager> (); eventManager.Init (); } return eventManager; } protected set { } }

Answer 2

Just an Information, which helps with either the unity tutorial EventManager or the one posted above:

Use the StartListening call in Awake() and the StopListening in ~YourClass() to have disabled GameObjects listen to events.

(This is an answer not a comment, because I cannot post comments yet.)

• Programmer: Yes, I know about this. Forget about the Test script. What I really want to improve is the EventManager class itself.

Answer 3

I am doing a similar thing, but I created an IParamEvent. I then create a class for each event and inherit from IParamEvent.

public interface IEventParam
{ 
 string Name { get; set; }
}
class CarSpeedParam : IEventParam
{
 public string Name { get; set; }
 public int Speed { get; set; }
}

• dfhwze: Could you show how to implement this pattern in the OP's code?
• Vogel612: Forgive my ignorance, but in what way is this an improvement on the code that OP presented? You're not gaining very much by inheriting a Name property for every custom event...
• Toby Speight: Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please edit to show what aspects of the question code prompted you to write this version, and in what ways it's an improvement over the original. It may be worth (re-)reading How to Answer.