Resolved How do I consolidate two (almost) identical classes?

I have a class Player that represents a player. I have a class Enemy that represents the enemy. There is literally a single line of code that is different in both classes, and that line of code is a reference to a "controller" script.

The "controller" script for a Player is a class that processes input and outputs data about the state of that player based on the input, (ie, aim direction, movement direction, rotation, etc).

The "controller" script for an Enemy is a class that, rather than processing input from the user, generates that same data based on a series of coroutines and attributes that determine its personality or characteristics (basically a state-machine).

So I have two classes, Player and Enemy that do virtually the same exact thing and are almost entirely decoupled from their respective controllers. My question is this: how do I merge this into a single class? The Player class has the reference: Controller controller; and the Enemy class has the reference: Brain controller; so literally just one word is different in both classes, but how do I make it that its all one class that, depending on whether or not its the player, references a different controller component/script? Also, this reference cannot be a SerializedField due to the fact that the Enemy objects are a pooled asset and lose serialization OnEnable/Disable (iirc) so I think the solution must be in code

 

Make it so Controller and Brain either implement the same interface or extend from a common base class. Then change the reference to reference either that interface or the base class. Presumably if all the code is otherwise the same, Controller and Brain share the necessary public methods/properties/fields and can therefore implement a common interface or extend a common base class.

 

I've watched several vids on interfaces, particularly from jason weimann and infallible code. I almost put in the title of this thread "should I use an interface here?" But then I realized I can't quite connect the dots in this particular situation. Like, what methods would I be adding to this interface? The Controller and Brain methods are different.

As for the classes extending from the same base class... well they (Controller and Brain) already do: they are both derived from monobehaviour as-is. Not sure I follow you on this one either, being that I can't just reference MonoBehaviour (or can I?)

 

You said that only one line is different in the player and enemy scripts:

Presumably, elsewhere in the scripts, you have code that looks something like:

controller.SomeMethod()

or

controller.SomeProperty

or

controller.SomeField

.

Since you said the player and enemy code is otherwise the same, those same properties/fields/methods are on both of the controller scripts right? At least the ones referenced from Player/Enemy. So those shared methods/properties/fields are the things that need to be on the shared base class or interface. Ideally it would only be methods and properties, and then you could easily use an Interface. But that might require some refactoring.

You could but that wouldn't let you access any custom data/methods that you probably care about on your controller scripts.

 

Yes, for some reason though it just hasn't clicked for me yet. I'm a little slow so I'm gonna rewatch some of those vids. In the meantime heres the code (comment at the reference)

Code (CSharp):

1.     public class Player : MonoBehaviour
2.     {
3.         public class Attributes
4.         {
5.             public Attributes(int hp, int dp, int sp)
6.             {
7.                 this.hp = hp;
8.                 this.dp = dp;
9.                 this.sp = sp;
10.             }
11.             public int hp;
12.             public int dp;
13.             public int sp;
14.         }
15.  
16.         [SerializeField] Turret turret = null;
17.         Attributes attributes;
18.         public Controller controller; // <--------- Enemy.cs has Brain controller;
19.  
20.         void Awake()
21.         {
22.             attributes = new Attributes(100, 100, 100);
23.             controller = GetComponent<Controller>();
24.         }
25.  
26.         void OnEnable()
27.         {
28.             Controller.ShootAction += OnShoot;
29.             Controller.ShieldAction += OnShield;
30.             Timing.RunCoroutine(UpdateMEC(), gameObject);
31.         }
32.  
33.         void OnShoot(float charge)
34.         {
35.             turret.Rotate();
36.             turret.Shoot(charge, 1f);
37.         }
38.  
39.         void OnShield(bool isActive)
40.         {
41.             print("Shield activation: " + isActive);
42.         }
43.  
44.         bool isMoving => controller.wasdDirection != Vector3.zero;
45.         void Move()
46.         {
47.             if (!isMoving) return;
48.             transform.position += controller.wasdDirection * Timing.DeltaTime;
49.             //ToDo: other stuff here
50.         }
51.  
52.         IEnumerator<float> UpdateMEC()
53.         {
54.             while (true)
55.             {
56.                 Move();
57.                 yield return Timing.WaitForOneFrame;
58.             }
59.         }
60.     }

 

This part poses a small extra hurdle:

Code (CSharp):

1.         void OnEnable()
2.         {
3.             Controller.ShootAction += OnShoot;
4.             Controller.ShieldAction += OnShield;
5.             Timing.RunCoroutine(UpdateMEC(), gameObject);
6.         }

It seems like you are using some static events. If you were to make this generic, those should really be instance events.

But basically this all seems simple enough. Make an interface called Controller with all the properties and events that you need:

Code (CSharp):

1. public delegate void ShootHandler(float charge);
2. public delegate void ShieldHandler(bool isActive);
3.  
4. public interface IController {
5.   event ShootHandler OnShoot;
6.   event ShieldHandler OnShield;
7.  
8.   Vector3 wasdDirection { get; }
9. }

Then make Controller and Brain implement IController. You may need to make some minor tweaks to get them to conform. Make sure wasdDirection is a property and not a field for example. Make sure the event declarations match with those in the interface.

Then change your Player and enemy script:

public Controller controller;

becomes

public IController controller;

OnEnable changes to:

Code (CSharp):

1.         void OnEnable()
2.         {
3.             controller.OnShoot += OnShoot;
4.             controller.OnShield += OnShield;
5.             Timing.RunCoroutine(UpdateMEC(), gameObject);
6.         }

 

@PraetorBlue

Yeah in between your posts I actually noticed the static events and refactored the entire controller class to be a non-monobehaviour with regular public events and a constructor that takes the gameobject. Only problem with that now is I gotta figure out a different way for the UI to get data from the player, but thats probably for the better.

Thanks a ton for your posts and help. Interfaces have kind of been the elephant in the room for quite some for me, now that I actually have a use case it will help me to use them more in the future.

 

I usually go event-based for this as well. Reason being that the player shouldn't really care what's going on in the UI, so it's generally a "fire-and-forget" situation when player data changes that needs to be updated in the UI.

No problem. Good luck with it and feel free to ask more questions if you get stuck.

 

One reservation I have about events to update UI is in situations where you need a constant loop of updates for a set time. I ran into this when I was implementing my "charge" mechanism where when the player holds down right click, it charges a shot, ie progressively looping through a coroutine to smoothly increase a value which is used as a multiplier for damage. If the player holds it too long and the value reaches 1.0f, it resets to zero, so the player has to time it just right for maximum damage.

Anyways, updating this to say, a sprite bar graphic in the UI with events sends a lot of repeated event calls to update the value. Not sure if this is the best way to do it, but its the only one I've found that works. I've tried just setting a start and end event and having the UI bar fill on a coroutine running in parallel with the one on the actual controller module, but I found that the floating point accuracy is horrendous and the value being represented in the UI was off by a bit too much when compared to the actual value happening under the hood, so I'm pretty much stuck looping events to UI when they hold right click. Kind of silly to worry about optimization at this point though (as I've been told), but still would love a cleaner solution to responsive timed UI updating like that.

 

If it's really something that you need to update every frame, you can always just expose the value in a property and read the property every frame, rather than using events. Events are better suited for things like "health" that usually only update when something happens, like the player getting hit by an attack of some kind. That being said, invoking an event every frame is generally not really a performance problem in and of itself, so it can still work even if the health is updating every frame. It also gets rid of the issue of worrying about which update script runs first, the one that updates the health or the one that draws it on the UI?

 

I have two thoughts about this.. you could
A) get the property value every frame continuously in an update loop as the game runs
B) get the property value every frame continuously in an update loop that only runs in between start/end events

Question: is there any benefit to B over A? Like, if you have a value being read in a continuous coroutine or update loop and that value isn't changing... --it is remaining constant... is that causing any more overhead than say, if the loop weren't running at all?

 

I guess your use case is something like... The value doesn't change all the time, except when... _something happens_. When that thing happens, the value starts changing every frame, until it stops. Then it goes back to not really changing often?

Honestly that sounds overcomplicated to me. I'd probably just read the value every frame, or use events, in which case it would behave exactly as you want - do nothing until it starts doing a lot of things.

Most likely not in any practical sense. You'd have to either read a flag every frame to check if you're in "update every frame" mode, or start a coroutine during that mode. The coroutine strategy could have no overhead during times when you're not updating the value... but again, it seems like overkill. Reading a single property is very cheap, assuming you're not doing anything crazy in the getter for that property.

 

@PraetorBlue

I just wanted to run this by and see if I'm understanding the usage of this correctly now that I've refactored everything and hooked up the interface.

So I've gotten rid of the other class (Enemy.cs), and created a bool isPlayer that is serialized. In the inspector I can check whether or not I want to control that particular prefab as the player. And in the awake method of Player.cs I simply check isPlayer and instantiate the appropriate controller like so:

Code (CSharp):

1.         void Awake()
2.         {
3.             _controller = isPlayer ? new Controller(gameObject) : new Brain(gameObject);
4.             _attributes = new Attributes(100, 100, 100);
5.             _updateMEC = Timing.RunCoroutine(UpdateMEC(), gameObject);
6.         }

I haven't tested as I have yet to implement the Brain class, but I just wanted to make sure this is how you'd use the interface in this situation.

 

The way I was imagining it is you have two prefabs:

PlayerPrefab
EnemyPrefab

On the PlayerPrefab you have these components:
- Player
- Controller

On the EnemyPrefab you have these components:
- Player
- Brain

Then, in Awake() for the Player script, you would just do this:

Code (CSharp):

1. _controller = GetComponent<IController>();

In this imagining, Brain and Controller are MonoBehaviours. It wasn't clear from my earlier reading of your question that this wasn't the case. If they're not MonoBehaviours, the way you've proposed here with the boolean works fine. But I'm curious how you're getting input callbacks on your Brain/Controller objects if they're not MonoBehaviours.

 

My mistake, they were MonoBehaviours but I switched it halfway through the thread. Interestingly enough, the ternary does not work. It throws no errors from omnisharp but I get an error from within Unity console about not being able to do an implicit conversion between the two types... -wont let me run the game. I tried instantiating the two in their own cache variables outside of the ternary but got the same results. Looks like they will have to be Monobehaviours afterall... shame that would have been cool to just have only one same exact script on every character, with an isPlayer checkbox lol.

EDIT: turns out it was just the ternary itself causing the problems. When using regular if, it actually works without being MonoBehaviour components. YES... so giddy right now. Now I've got one script to rule them all lol

As far as the callbacks, I've never had to use them on a monobehaviour before. But there are lots of ways to use the new input system. Here I just instantiate the auto-generated class from my Input Action Asset in the awake of my Controller.cs. I dunno, beats me, but it certainly works without monobehaviour. Heres the script if you want to look at it yourself... and excuse the MEC coroutine pausing and resuming. Great plugin but strange theres no method to generate a coroutine handle without starting one. Makes spawning from an object pool a nightmare but still very worth it imo

Code (CSharp):

1.     public class Controller : IController
2.     {
3.         public event ShootHandler OnShoot;
4.         public event ShieldHandler OnShield;
5.         public event ChargeHandler OnCharge;
6.         public Vector3 moveDirection { get; set; }
7.  
8.         Controls _controls;
9.         GameObject _gameObject;
10.         CoroutineHandle _charge;
11.         float _chargeValue;
12.  
13.         public Controller(GameObject gameObject)
14.         {
15.             _controls = new Controls();
16.             _gameObject = gameObject;
17.             Initialize();
18.         }
19.  
20.         public void Activate()
21.         {
22.             _controls.Enable();
23.             _controls.Game.Move.performed += _ => moveDirection = _.ReadValue<Vector2>();
24.             _controls.Game.Charge.performed += _ => BeginCharge();
25.             _controls.Game.Charge.canceled += _ => EndCharge();
26.             _controls.Game.Defend.performed += _ => OnShield?.Invoke(true);
27.             _controls.Game.Defend.canceled += _ => OnShield?.Invoke(false);
28.             Timing.ResumeCoroutines(_charge);
29.         }
30.  
31.         public void Deactivate()
32.         {
33.             _controls.Game.Move.performed -= _ => moveDirection = _.ReadValue<Vector2>();
34.             _controls.Game.Charge.performed -= _ => BeginCharge();
35.             _controls.Game.Charge.canceled -= _ => EndCharge();
36.             _controls.Game.Defend.performed -= _ => OnShield?.Invoke(true);
37.             _controls.Game.Defend.canceled -= _ => OnShield?.Invoke(false);
38.             _controls.Disable();
39.             Timing.PauseCoroutines(_charge);
40.             _chargeValue = 0f;
41.         }
42.  
43.         void Initialize()
44.         {
45.             _charge = Timing.RunCoroutine(Charge(), _gameObject);
46.             Timing.PauseCoroutines(_charge);
47.             _chargeValue = 0f;
48.         }
49.  
50.         void BeginCharge()
51.         {
52.             _chargeValue = 0f;
53.             OnCharge?.Invoke(_chargeValue);
54.             Timing.ResumeCoroutines(_charge);
55.         }
56.  
57.         void EndCharge()
58.         {
59.             OnShoot?.Invoke(_chargeValue);
60.             Timing.PauseCoroutines(_charge);
61.         }
62.  
63.         IEnumerator<float> Charge()
64.         {
65.             while (true)
66.             {
67.                 _chargeValue += Timing.DeltaTime;
68.                 if (_chargeValue > 1f) _chargeValue = 0f;
69.                 yield return Timing.WaitForOneFrame;
70.             }
71.         }
72.     }

 

Wow I'm so happy now... I just realized what this means: I can make a dictionary or list of different kinds of Brains and switch them at run-time... I could even implement mind control functionality to take control over NPCs/enemies and vice versa. This. Changes. Everything. Okay okay, gotta calm down and focus on finishing the game first though lol

 

Glad you got it working! Enjoy!