Feedback Event system refactoring

Been a while since I looked at Unity and have started to play with it again (I am not a game programmer and normally code in Java), noticed this project and I am so impressed with the code quality and patterns used. Hopefully I can contribute in some way however small. The FSM you implemented is outstanding, love it, however I do have a slight concern with the event system. A channel should abstract the mechanism to move events from a source to destination but here it seems to be coupled to types of message and this makes it hard to fathom who raises and who handles an event, it took me a while to understand where the CameraManager::
OnFrameObjectEvent was generated.

I also feel that because the channel has a specific type that you loose the benefit of encapsulating a schema into an event and being able to refactor without changing the transport, i.e. an event on the
TransformEventChannelSO can't add other info without creating a new channel for it. And finally the input events impl looks different to other channels but should be the same.

In my opinion an event aggregator would be a more suitable option here so I remembered a nice one in the caliburn.micro project, took out all the thread stuff, simplified it a little and tried it on a project. I followed the SO route and made an EventAggregatorSO which also acted as a proxy.

Code (CSharp):

1. public interface IEventAggregator {
2.    void Subscribe(object subscriber);
3.  
4.     void Unsubscribe(object subscriber);
5.  
6.     void Publish(object message);
7. }
8.  
9. public class EventAggregatorSO : ScriptableObject, IEventAggregator
10.     {
11.         private readonly EventAggregator _eventAggregator = new EventAggregator();
12.  
13. ...
14. }

The consumers of events can now explicitly show what they handle and register their intent in the enable and disable events. A subscription would understand from reflection the handled types and this simplifies registration, no need to sub unsub new events.

Code (CSharp):

1. public class Protagonist : MonoBehaviour,
2.     IHandle<JumpEvent>,
3.     IHandle<JumpCancelledEvent>,
4.     IHandle<MoveEvent>,
5.     IHandle<StartedRunningEvent>,
6.     IHandle<StoppedRunningEvent>

The consumers and producers just need an instance to the SO but to me the benefit is now you have event types which can be changed at will without affecting event handling and also using the IDE quick to find out who works with specific events. Also input events and game events use the same event bus, which has other benefits such as testing and simulation.

If your interested I can do a PR to demonstrate the changes.

 

Thanks, that's actually a contribution from a community person, @deivsky (originally, but with plenty of additional contributions from other people).

I'm not sure I agree. I do agree that in general it can be hard to know who's firing an event, this is kind of inevitable since we use public SO references in the Inspector, that can't be tracked in the code.

However, in general you can get a guess by right-clicking the event class in question and choosing "Find All References...". Here for instance, there's only one other class using it (SpawnSystem) which in fact is the correct answer. We think this tradeoff is alright given the relative small size of the project.

For the flexibility and for testing, we can generally already swap channels (not type, ofc) by referencing a different SO instance to "reroute" an event to different objects.

So given the above, it's not super clear to me what the refactor would add in terms of benefits, given that we'd have to rework half of the game's scripts. Maybe I'm missing the point!

 

Appreciate the response, the point I was trying to make was for loosely coupled events a proven pattern is to use an event aggregator see https://martinfowler.com/eaaDev/EventAggregator.html and given quite a few people will look at this project for inspiration one misses an opportunity to demonstrate best practice. Apologies if this is too blunt but the current event implementation is a poor design choice and would benefit from a refactoring which in itself is the life blood of good coding. A key component is that there should only be a single source for events to keep code simple and easy to maintain.
Take a look at /scripts/events/ScriptableObjects/UI because the events are not simple primitives the code now needs a channel and event pair. Also the Fade channel, is it an event or a channel?

Quests would be a perfect example for leveraging a simple event system. The quest instance would subscribe for a specific set, imagine the chef needed to cook a recipe. The quest would subscribe to inventory events constituting the ingredients and utensils and timing events maybe the cooking. Using the existing design we would need to load the quest with multiple channels. Then later say we add a special recipe that needs the chef to be trained in puffer fish prep. Do we add yet another channel, event pair. Within no time there will be a SO maintenance nightmare.

I understand your reluctance to change and can leave it as a difference in opinion but would it be okay for me to do a simple PR with an implementation for the input events to demonstrate how it would cleanup the code and be extensible for any future needs?

 

Does this refactoring still give us the ability to wire events in the Unity editor? My understanding is that we use typed channels because each one uses a typed UnityEvent, which automatically provides a standard way of specifying handlers (and their parameters) via the Inspector.

 

I see your point, @zaradai. And by the way, some coders on the project might have taken the event channel stuff a bit too seriously, creating events where another pattern was better. This has led us to have many, maybe too many event channels. I will have to do a review soon.

In any case, do you want to create a small PR? Maybe I can get better what do you have in mind. Maybe isolated from the rest, you don't need to create one that already substitutes all of the channels/events we have.
Is it possible to avoid reflection though? (thinking)

 

Thanks for the response. I have raised a PR with a limited demonstration of the changes as suggested. I put in 2 versions.
1. EventAggregator - My preferred approach using reflection to look for handled events and implemented for Protagonist input events.
2. UnityEventBus - A different approach without reflection as requested where you subscribe a UnityAction for the event. The UnityAction may be a public instance for use via the inspector or can be assigned a method to be invoked. I used the CameraManager for this demonstration.

The input events were a useful choice because i was able to test easier moving the camera and character.

 

I've been looking at your solutions, @zaradai. It's good work, however I don't fully understand all of the advantages.
Disclaimer: for the purpose of this discussion please ignore the button handling in InputReader, I do know that it's a tightly coupled system so I'll focus on the EventChannel implementation we have which we are using for other things, like the comment you made about the Camera framing event:

If we were to go all the way, we'd have to generate a Void event channel for each button press, and use those as a vector for input events.

So:

In your first post, you mention that your changes bring decoupling and ease to understand where an event is fired.
If I understand it correctly (I'm going to comment on the UnityEventBus implementation), yours goes this way:
A script has a reference to the generic UnityEventBusSO > Subscribes "on it" to a specific event (say, JumpEvent) > InputReader SO has a reference to a generic UnityEventBusSO > raises the JumpEvent event on that SO > the SO speaks with a class instance UnityEventBus, and checks whether anyone is subscribed > that object is notified

In our implementation (again, if we were to use EventChannels fully in the InputReader) we'd have something like this:
A script has a reference to a VoidEventChannelSO > Subscribes "on it" to its only event > InputReader SO has a reference to the VoidEventChannelSO (aptly named JumpEventChannel) > raises an event on it > the object is notified

So, what are the differences? For me, one is that there is less code bouncing around. While you might not appreciate this, the reality is that this is a project people will need to explore and understand, so the less complicated solutions, the better.

Second, the reason why you couldn't find the camera event is because it's a "generic" TransformEventChannelSO, meaning it's an event channel that carries information about a Transform, and not specific to camera events. If we wanted to make it specific, we could just make an event channel of that type, then one would be able to use the code IDE tools to track down usage. So in this sense, tracking down references is covered by the current implementation.
But I am inclined not to create a million specific event types, given that in this project for instance, only a handful of scripts will use TransformEventChannelSO so by looking for references to that, you would have quickly found who was generating the event. Makes sense?

Regarding decoupling.
In your implementation, sure everything is decoupled, but because you are using one "bus" SO for all input events (and potentially ALL events) to reroute just one event one would have to: create another bus SO, add a reference to the script that needs it, and then reroute only some events on one of them and all the others on the second SO... this can also create confusion as to which event is travelling on which bus, since the bus SO wouldn't have a name that reflects that nor you could see it from its Inspector.

Rerouting events is totally possible today just by creating another SO for a specific event (in the Unity editor) and attaching it via the Inspector, in place of the first one, because each event channel only transports one event, so it's kind of easier to modularise them this way.

OK I'll stop here, I've written a wall of text
Please let me know what you think. I might be wrong here and there, or have misunderstood your implementation.