Using inheritance for tidiness, eg. registering with InputAction events...!

Unity Engine
,
1

Hey folks. I’m not a professionally trained programmer and I’m always looking for better practices, so this is a post asking for advice on script architecture and hygiene.

I tend to try to keep scripts fairly small and contained - usually if I go over 300 lines I start thinking about how to split the script up, and if I hit 400 lines I stop everything and break up the script in question.

However, I’m currently writing a large input script / controller for my main character and I’m hitting 300 lines with a lot more relevant, related code to write.

About 30 lines of this were dedicated to variables and registering and unregistering events related to the new InputSystem - as follows…

private PlayerControls controls;

    protected   Vector2             moveInputRaw;       // Hot off the controls
    protected   bool                runInputRaw         = false;
    protected   float               fwdInputRaw;

    public virtual void Awake()
    {
        controls            = new PlayerControls();
    }

    public virtual void OnEnable()
    {
        controls.Enable();
        controls.BipedMovement.MoveWalk.performed   += ctx => moveInputRaw  = ctx.ReadValue<Vector2>();
        controls.BipedMovement.MoveWalk.canceled    += ctx => moveInputRaw  = Vector2.zero;
        controls.BipedMovement.RunModifier.started  += ctx => runInputRaw   = ctx.ReadValueAsButton();
        controls.BipedMovement.RunModifier.canceled += ctx => runInputRaw   = ctx.ReadValueAsButton();
        controls.BipedMovement.fwdInputRaw.performed+= ctx => fwdInputRaw   = ctx.ReadValue<float>();
        controls.BipedMovement.fwdInputRaw.canceled += ctx => fwdInputRaw   = 0.0f;
    }

    public virtual void OnDisable()
    {
        controls.BipedMovement.MoveWalk.performed   -= ctx => moveInputRaw  = ctx.ReadValue<Vector2>();
        controls.BipedMovement.MoveWalk.canceled    -= ctx => moveInputRaw  = Vector2.zero;
        controls.BipedMovement.RunModifier.started  -= ctx => runInputRaw   = ctx.ReadValueAsButton();
        controls.BipedMovement.RunModifier.canceled -= ctx => runInputRaw   = ctx.ReadValueAsButton();
        controls.BipedMovement.fwdInputRaw.performed-= ctx => fwdInputRaw   = ctx.ReadValue<float>();
        controls.BipedMovement.fwdInputRaw.canceled -= ctx => fwdInputRaw   = 0.0f;
        controls.Disable();
    }

Nothing shocking or surprising there, but it’s taking up a lot of space in my input class. My solution has been to offload this to a base class that my Input script can inherit from.

However - I’m used to thinking about Inheritance usually being used for, say, parent classes of NPCs that an NPC Soldier could inherit from, for example. My current use of inheritance seems to stray from that pattern.

Additionally, I’m afraid that I’m basically ‘hiding’ code (commonly used methods etc.) that another developer (or my future self) may need to find in order to debug or extend the code - is it worth enduring larger scripts but have the entire flow of the operations available in one document, or should I be stashing methods with clear names in a parent class, so the overall purpose and action of the script is evident (‘hiding’ some details)?

I’m confused. Any advice would be appreciated. =)

2

Two things:
First, there’s a bug in your code. Your OnDisable is not properly unsubscribing from the events. To do so you would have to do something like this:

void OnEnable() {
  action.performed += RunModifierStarted;
}

void OnDisable() {
  action.performed -= RunModifierStarted;
}

void RunModifierStarted(InputAction.CallbackContext ctx) {
  runInputRaw = ctx.ReadValue<float>();
}

OR this:

Action<InputAction.CallbackContext> runStarted;
void OnEnable() {
  runStarted = ctx => runInputRaw = ctx.ReadValue<float>();
  action.performed += runStarted;
}

void OnDisable() {
  action.performed -= runStarted;
}

Second - inheritance is indeed the wrong approach to this for exactly some of the reasons you said. You should use composition instead. your controls script can be a separate script that you attach to your objects alongside your player script etc. And your player script can then access the data like this:

[RequireComponent(typeof(MyControls))]
public class PlayerScript {
  MyControls controls;
  void Awake() {
    controls = GetComponent<MyControls>();
  }
  void Update() {
     Vector2 moveInput = controls.moveInputRaw;
     // etc...
  }
}
3

Inheritance is discouraged even by proponents of OOP. It leads to tons of duplicate code when done without planning, which is easy to do for games where plans can change on a regular basis. Refactoring tightly wound code is difficult if not impossible depending on the situation. Composition also requires planning, but mitigates the issue inheritance has with tightly wound code, and is far more open ended even when used loosely.

Prioritizing having short scripts over what the scripts do, is basically prioritizing form over function, which never leads to good things. In terms of performance, you are adding an overhead for every script which needs to reference each other. In terms of architecture, you are forcing whoever works with the scripts you write to contend with a web of connections, which is arguably worse than one long script.

4

Very glad I asked about this…! I really appreciate both of your advice.

@PraetorBlue

  1. Goddamnit, I thought it was fishy trying to unsubscribing a lambda expression, or at least the way I was using it. I was trying to avoid memory leaks (as iirc subscribing events is one of the only areas you can produce that in Unity? Or so I was told?). Are lambdas just going to cause trouble if I can’t unregister them?

  2. I love me some composition, but for some reason I think I’ve just developed some inheritance tunnel vision. Thank you, I’ll do what you suggested.

@Laperen
Your first paragraph summarizes clearly the sort of apprehensions I was developing as I’d started refactoring my code.

I also take your point regards form over function, though prior to this point having a rough guideline of ‘400+ lines = sit back and rethink/refactor’ has worked well for me. In this case it’s basically early optimization and we all know about that. =/ I understand your point on the worth of a long script and I’ll take it into account - however, in this case I think I can probably work some composition / state-based separation.

Again, thanks, all very useful information.