Publisher-subscriber pattern have I done it the right way?

I am new to Unity. I wanted to implement the publisher-subscriber pattern in my game something that was explained in this tutorial: https://learn.unity.com/tutorial/the-observer-pattern#5d77c678edbc2a00204d704d.
But I am not sure if I done it the right way.
I did not wanted to use static fields or static methods. And I also preferred to use unity events instead of C# events.
What I come up with is to create a MessageBrokerService that is a component in my LevelManager, and via that component the MessageBroker instance is available. And also in case of Ciccio (the spawned alien in the game), during instantiation I call Initialize methods to initialize the MessageBroker in the components of the new GameObject. (Actually that was inspired by this article: https://blog.leloctai.com/unity3d-dependency-injection/). See the class diagram in the attachment.

So my question is this approach a right approach? Or this is totally wrong?

And I also created a video about this which you can find here:

 

Your MessageBroker violates open closed principle, I didn't have time to sit through 20 minutes so I quickly scrolled it.

 

Is there code outside of the video - can't watch it

 

Yes. I forgot to mention it. The code is here: https://github.com/devviktoria/bdt0011.

 

Hi,
I really appreciate your feedback, thank you very much. I have a question: this violation can be fixed by creating an IMessageBroker interface, which contains all the properties that the MessageBroker suppose to have. And then using that interface everywhere?

 

Not the one who commented it but I think in this case open-closed principle would be fixed by having the events available in MessageBroker be configurable without changing the code. For example if you wanted to add `HumanDiedEvent`

Right now if you add a new event, you need to edit MessageBroker. You could solve this by having a `Dictionary<string/Event> events` that you can add events to when initializing the broker.

So something like

Code (CSharp):

1. var broker = new MessageBroker();
2. broker.AddEvent("alienDied", new Event());
3. broker.AddEvent("humanDied", new Event());
4.  

Saw this video that did something similar:

He uses ScriptableObject instead of Monobehavior but similar idea except the events publishers aren't centralized

 

Using magic strings as keys is not a good practice. Events should be tied to identifiers, not strings. It is not a good practice because you can't refactor string identifiers, and compiler doesn't catch any errors there.

Additionally, @DevViktoria stuff like this:

Code (csharp):

1. [
2. public class MessageBroker {
3.     private UnityEvent _alienReachedDoor;
4.     private UnityEvent _alienDied;
5.     private GameOverEvent _gameOver;
6.     public UnityEvent AlienReachedDoor {
7.          get => _alienReachedDoor;
8.     }
9.  

Is fairly dubious. Hiding UnityEvent behind a property means that you'll be unable to reassign it, BUT due to the event being a reference type and C# not having const correctness, you can still wreck it, by removing all listeners from it, for example. So there's not a lot of point in doing that.

I'd consider using C# events here, unless you want serialization. Because the main reason to use UnityEvent is to ahve them visible in Editor, or serialization support.

 

No thats abstraction.
I would use a event aggregation pattern. We do this in our game

Example from our game

Code (CSharp):

1.     public struct PlayerKilled
2.     {
3.         public PlayerKilled(ulong killerPlayerId, ulong deadPlayerId, bool affectScore)
4.         {
5.             KillerPlayerId = killerPlayerId;
6.             DeadPlayerId = deadPlayerId;
7.             EventShouldAffectScore = affectScore;
8.         }
9.  
10.         public ulong KillerPlayerId { get; private set; }
11.         public ulong DeadPlayerId { get; private set; }
12.  
13.         public bool EventShouldAffectScore { get; set; }
14.     }

Code (CSharp):

1.     protected override void HandleDeath(ulong killerId, bool affectScore)
2.     {
3.         TimeOfLatestDeath = Time.time;
4.  
5.         EventBus.Publish(new PlayerKilled(killerId, OwnerId, affectScore));
6.  
7.         DisableAvatar();
8.  
9.         if(isServer)
10.         {
11.             InventoryNetworker.RequestDropAllItems(OwningPlayer.NetworkId);
12.         }
13.  
14.         if (IsOwner)
15.         {
16.             PlayerRig.CanInteractWithWorld = false;
17.         }
18.     }

Code (CSharp):

1.     public class ScoreManager : NetworkedMonoBehavior, IEventHandler<PlayerKilled>
2.     {
3.  
4.         protected override void NetworkStart()
5.         {
6.             base.NetworkStart();  
7.             EventBus.Subscribe(this);
8.         }
9.  
10.         protected override void OnDestroy()
11.         {          
12.                 base.OnDestroy();
13.                 EventBus.Unsubscribe(this);
14.             }        
15.  
16.         public void Handle(PlayerKilled evt)
17.         {
18.             if(evt.EventShouldAffectScore)
19.             {
20.                 RegisterKill(evt.KillerPlayerId, evt.DeadPlayerId);
21.             }          
22.         }
23.  
24.    }
25.  

Sorry about formating a bit screwed up wen copy pasting from VS

 

Thank you for your feedback. I haven't thought about this. Thank you for pointing it out.

 

Thank you for your feedback. I guess In your code the EventBus is the event aggregator here. And if my understanding is correct in case of a PlayerKilled event it is only notifying those subscribers who are implementing IEventHandler<PlayerKilled>?
But anyways this is an interesting idea because, if my understanding is correct, it makes the subscription/unsubscription logic easier.

 

Yes, and not only that, it honors open/closed principle. And unlike C# events it decouples subscriber and publisher.

 

Correction Unity Mono doesn't have it. C# have nullable reference types.

 

Just want to throw this out there as a potential concept:

While the interface/implementation approach shown in the tutorial isn't bad, it does mean you will have to create a new interface and implement it, then have your subject manually find/collect all implementations to notify for every event you want to create.

Something like Angular's RxJS library implements the observer pattern by registering callback functions instead, which means the subject doesn't need to manually find/collect all the observers to notify - that's done automatically as soon as an observer subscribes to the subject.

In C#, a similar model could be created using delegates. Rough example:

Code (CSharp):

1. public class Subject {
2.   private readonly List<Action> callbacks = new List<Action>();
3.  
4.   public void Subscribe(Action callback) {
5.     callbacks.Add(callback);
6.   }
7.  
8.   public void Unsubscribe(Action callback) {
9.     callbacks.Remove(callback);
10.   }
11.  
12.   public void UnsubscribeAll() {
13.     callbacks.Clear();
14.   }
15.  
16.   public void Invoke() {
17.     for(int i = 0; i < callbacks.Count; i++) {
18.       callbacks[i].Invoke();
19.     }
20.   }
21. }

Code (CSharp):

1. public class EventBroker {
2.   public readonly Subject onPlayerSpawn = new Subject();
3.   public readonly Subject onPlayerDamaged = new Subject();
4.   public readonly Subject onPlayerDie = new Subject();
5. }

Code (CSharp):

1. public class SomeObserver : MonoBehaviour {
2.   EventBroker broker = new EventBroker();
3.  
4.   void Awake() {
5.     //Lambda expressions can be used, however, they cannot be individually unsubscribed without calling Subject.UnsubscribeAll().
6.     broker.onPlayerDamaged.Subscribe(() => {
7.       Debug.Log("Player was damaged");
8.     });
9.   }
10.  
11.   //An individual callback must be registered as such to be able to unsubscribe without calling Subject.UnsubscribeAll().
12.   void OnEnable() {
13.     broker.onPlayerDie.Subscribe(OnPlayerDie);
14.   }
15.   void OnDisable() {
16.     broker.onPlayerDie.Unsubscribe(OnPlayerDie);
17.   }
18.  
19.   void OnPlayerDie() {
20.     Debug.Log("Game Over");
21.   }
22. }

But to be honest, this does feel like re-creating the function of regular C# events, so you may as well just use them instead.

 

Thank you for the idea. I really appreciate it. And I agree with you. And unfortunately in this solution the events are explicitly defined in the EventBroker, which means it cannot be extended with new events without adding new lines of code to it, just like mine .

 

Open/close principle is about that. Open for addition but closed for modification. Adding a new entry in the EventBroker is just additional, you don't have to change the baseline logic to work it. Works with inheriting from it too.

But SOLID in general is great, although during serious game development, use it as long as it doesn't hinder your actual game development. Your game is more important than any kind of programming technique. Also, if it is working for you, consider it as good enough. Always.

 

You have understood the principle wrong. Its open for extension, without changing its source code. For example through composition, etc.

Adding a new event to its closed source is violating it.

 

True, but I fear as though the only way to create new events without modifying the logic would be to use strings to define the names of new events in a Dictionary.
Something like this:

Code (CSharp):

1. public class EventBroker {
2.   private readonly Dictionary<string, List<Action>>> eventsDict = new Dictionary<string, List<Action>>>();
3.  
4.   public void Subscribe(string eventName, Action callback) {
5.     if(eventsDict.ContainsKey(eventName)) {
6.       eventsDict[eventName].Add(callback);
7.     }
8.     else {
9.       eventsDict.Add(eventName, new List<Action> { callback });
10.     }
11.   }
12.  
13.   public void Unsubscribe(string eventName, Action callback) {
14.     if(eventsDict.TryGetValue(eventName, out List<Action> callbackList)) {
15.       callbackList.Remove(callback);
16.     }
17.   }
18.  
19.   public void UnsubscribeAll(string eventName) {
20.     eventsDict.Remove(eventName);
21.   }
22.  
23.   public void Invoke(string eventName) {
24.     if(eventsDict.TryGetValue(eventName, out List<Action> callbackList)) {
25.       for(int i = 0; i < callbackList.Count; i++) {
26.         callbackList[i].Invoke();
27.       }
28.     }
29.   }
30. }

Code (CSharp):

1. public class SomeClass : MonoBehaviour {
2.   EventBroker broker = new EventBroker();
3.  
4.   void Awake() {
5.     broker.Subscribe("player-spawn", () => {
6.       Debug.Log("Player Spawned");
7.     });
8.  
9.     broker.Subscribe("player-damaged", () => {
10.       Debug.Log("Player was damaged");
11.     });
12.    
13.     broker.Subscribe("player-die", () => {
14.       Debug.Log("Game Over");
15.     });
16.   }
17.  
18.   void SomeOtherMethodElsewhere() {
19.     broker.Invoke("player-spawn");
20.   }
21. }

Personally, I would much rather deal with going into the EventBroker class and adding a new Subject to create a new event than deal with string comparisons like this.

 

Why on earth.

Here is a simple implementation

Code (CSharp):

1.  
2.     public interface IEventHandler<in T> where T : struct
3.     {
4.         void Handle(T evt);
5.     }
6.         public void Subscribe<T>(IEventHandler<T> handler) where T : struct
7.         {
8.             var type = typeof(T);
9.             if (!subscribers.ContainsKey(type))
10.                 subscribers.Add(type, new List<object>());
11.             subscribers[type].Add(handler);
12.         }
13.  
14.         public void Publish<T>(T evt) where T : struct
15.         {
16.             var type = typeof(T);
17.             if (!subscribers.ContainsKey(type)) return;
18.             var handlers = subscribers[type];
19.             foreach (var handler in handlers)
20.                 ((IEventHandler<T>)handler).Handle(evt);
21.         }
22.  
23.  

 

@DevViktoria So, I watched your latest video about your project, some thoughts.

- First of all, great work! Keep it up!
- As far as I know the UI Toolkit only has binding service in the editor, the runtime binding feature will be available in 2021.2 according to the roadmap: https://forum.unity.com/threads/ui-roadmap-q3-2020.980202/ which means, what you have done there won't work in a build only in the editor (and that's why you needed the UnityEditor using statement, because the SerializedObject is a UnityEditor object)
- Your decision to postpone the rewrite of the MessageBroker is on point! Don't fix what's currently working only when you have the time and inclination to do it. In game development tech debt is normal and rarely paid off in full.

 

Why on earth indeed.

That's good, actually. It never occurred to me that you could use the event data object itself as the Dictionary key.

 

Well, this is much better than string keys.

As I said before, you shouldn't use string keys to subscribe to things, as strings cannot be refactored.by IDEs, typos are not detected by the compiler, therefore reliance on string keys for such events increases likelyhood of bugs.

So the key should be an identifier. One candidate is enum. Using type itself also works, though it wouldn't fly in a language where type information is not available at runtime.

One example of overreliance on strings is SqlAlchemy. This thing can drive nuts someone who is used to bullet-proof compile-time checks.

 

You can get creative though. Here is a event bus in javascript that uses the message type as key

Code (CSharp):

1.         function getSubscribers(message) {
2.             var constructor = message.constructor;
3.             if (constructor.__subscribers === undefined) {
4.                 constructor.__subscribers = [];
5.             }
6.  
7.             return constructor.__subscribers;
8.         }

 

Not to throw a spanner in the works or go against what you're trying to build here, but this is the best event listener/subscriber I've used.
http://www.willrmiller.com/?p=87

 

It uses DynamicInvoke though. Thats reflection and really slow. My implementation above uses type casting which is much faster

 

A couple of questions on this generics pattern using

AddEventListener<T>

…

• how would you subscribe to *all* events if it expects a<T>?
  
  If you use the base interface or class eg:
  

  EventManager.AddEventListener<IEvent>

  then you’ll have to cast all triggered events to that eg:
  

  EventManager.TriggerEvent<IEvent>((IEvent) new CustomEvent { Foo : "Bar" })

  but you end up with everything listening to everything
  
  ///
  
  
• secondly, how would you pickup a serialized IEvent reference with a handler of type <T>, where T :
  IEvent
  
  eg you have a scenario where you need to fire an

  IEvent

  eg
  

  [SerializeReference] IEvent _event;
  
  EventManager.TriggerEvent(_event)

  (for instance Odin Inspector would let you pick a specific concrete class) how would you pick it up with a listener of eg type

  <CustomEvent>

  
  
  eg
  
  Code (CSharp):

  1. // ----------
  2. // Class1
  3. // ----------
  4.  
  5. //add event handler to event manager
  6. EventManager.AddEventListener<CustomEvent>(HandleCustomEvent);
  7.  
  8. // handle event
  9. private void HandleCustomEvent(CustomEvent e) {}
  10.  
  11. // ----------
  12. // Class2
  13. // ----------
  14.  
  15. // get event from Inspector
  16. [SerializeReference] IEvent myEvent;
  17.  
  18. // fire event
  19. EventManager.TriggerEvent(myEvent);
  20.  
  21. // whilst the selected Type in the inspector is CustomEvent
  22. // it exists  in the reference an IEvent and won't be picked up by the CustomEvent handler.

thanks for any suggestions

(currently using a hybrid of https://github.com/greghornby/Unity-Type-Based-Event-Manager and https://gist.github.com/DomGries/3786466)