Not sure how this code will eventually develop (so it is possible that it will not work in future) but in general, when events are used, there is often no need to use the singleton pattern.
There are two reasons it was needed in the video:
- to access the event “queue” so “selected unit visuals” can subscribe
- to get the currently selected unit by calling a getter on the UnitActionSystem
While the latter one is nice, it couples the SelectedUnitVisual class with UnitActionSystem a bit more (since it now expects it to own/have the information about currently selected character). It could be avoided if the event itself carried the new Unit value (which is easily doable by replacing EventHandler with Action) making the decoupling stronger.
So we are left with the problem of how to allow all those SelectedUnitVisuals to subscribe. This can be solved by making the event handler static.
Altogether it could look like this:
public static event Action<Unit> OnSelectedUnitChanged;
on UnitActionSystem side and:
void Awake() {
//...
UnitActionSystem.OnSelectedUnitChanged += UnitActionSystem_OnSelectedUnitChanged;
}
void UnitActionSystem_OnSelectedUnitChanged(Unit newUnit) {
_meshRenderer.enabled = (newUnit == _associatedUnit);
}
in SelectedUnitVisual.
One disadvantage I can see is that if multiple UnitActionSystems got created, the events will probably be fired twice each time and I don’t see a nice way of alerting the programmer to that (other than doing the whole Instance check in Awake method, at which point it is basically a full singleton).
But in general, I find this solution nicer, as it forces stronger decoupling by default.
It also requires less boilerplate code than the singleton solution, which I’d say makes it more readable.