[help wanted] Input handler review

Hello,

I created a system to handle the player input but i'm not sure if it is designed properly.

1- PlayerInputSystem
Lookup for entities that contains the Player tag and based on the inputs it could append one of the following components to the entity:
- MoveInput
- ActionInput

Code (CSharp):

1.  
2. namespace Rogue.Inputs {
3.     public class PlayerInputSystem : JobComponentSystem {
4.         struct PlayerInputJob : IJobForEachWithEntity<Player> {
5.             public EntityCommandBuffer.Concurrent Commands;
6.  
7.             public float2 axis;
8.             public bool movePressed;
9.             public bool actionPressed;
10.  
11.             public void Execute (Entity entity, int index, [ReadOnly] ref Player player) {
12.                 if (movePressed) {
13.                     Commands.AddComponent (index, entity, new MoveInput { Axis = axis });
14.                 }
15.  
16.                 if (actionPressed) {
17.                     Commands.AddComponent (index, entity, new ActionInput ());
18.                 }
19.             }
20.         }
21.  
22.         private BeginSimulationEntityCommandBufferSystem beginSimEcbSystem;
23.  
24.         protected override void OnCreateManager () {
25.             this.beginSimEcbSystem = World.GetOrCreateSystem<BeginSimulationEntityCommandBufferSystem> ();
26.         }
27.  
28.         protected override JobHandle OnUpdate (JobHandle inputDependencies) {
29.             var job = new PlayerInputJob () {
30.                 Commands = this.beginSimEcbSystem.CreateCommandBuffer ().ToConcurrent (),
31.                 axis = new float2 (Input.GetAxisRaw ("Horizontal"), Input.GetAxisRaw ("Vertical")),
32.                 movePressed = Input.GetButton ("Horizontal") || Input.GetButton ("Vertical"),
33.                 actionPressed = Input.GetButtonDown ("Fire1")
34.             };
35.  
36.             var jobHandle = job.Schedule (this, inputDependencies);
37.  
38.             this.beginSimEcbSystem.AddJobHandleForProducer (jobHandle);
39.  
40.             return jobHandle;
41.         }
42.     }
43. }

2- CleanUpInputSystem
Removes the ActionInput and MoveInput at the end of the simulation

Code (CSharp):

1. namespace Rogue.Inputs {
2.     public class CleanupMoveInputSystem : JobComponentSystem {
3.         struct PlayerInputJob : IJobForEachWithEntity<MoveInput> {
4.             public EntityCommandBuffer.Concurrent Commands;
5.  
6.             public void Execute (Entity entity, int index, [ReadOnly] ref MoveInput player) {
7.                 Commands.RemoveComponent (index, entity, typeof (MoveInput));
8.             }
9.         }
10.  
11.         private EndSimulationEntityCommandBufferSystem endSimEcbSystem;
12.  
13.         protected override void OnCreateManager () {
14.             this.endSimEcbSystem = World.GetOrCreateSystem<EndSimulationEntityCommandBufferSystem> ();
15.         }
16.  
17.         protected override JobHandle OnUpdate (JobHandle inputDependencies) {
18.             var job = new PlayerInputJob () {
19.                 Commands = this.endSimEcbSystem.CreateCommandBuffer ().ToConcurrent ()
20.             };
21.  
22.             var jobHandle = job.Schedule (this, inputDependencies);
23.  
24.             this.endSimEcbSystem.AddJobHandleForProducer (jobHandle);
25.  
26.             return jobHandle;
27.         }
28.     }
29. }
30.  

3- PlayerMovementSystem
Updates player velocity based on the MoveInput component values.

Code (CSharp):

1. namespace Rogue.Players {
2.     public class PlayerMovementSystem : JobComponentSystem {
3.         public struct PlayerInputJob : IJobForEach<MoveInput, MoveSpeed, Velocity, Player> {
4.             public float deltaTime;
5.  
6.             public void Execute ([ReadOnly] ref MoveInput input, [ReadOnly] ref MoveSpeed speed, ref Velocity velocity, [ReadOnly] ref Player player) {
7.                 velocity.Value = input.Axis * speed.Value * deltaTime;
8.             }
9.         }
10.  
11.         protected override JobHandle OnUpdate (JobHandle inputDeps) {
12.             var job = new PlayerInputJob {
13.                 deltaTime = Time.deltaTime
14.             };
15.  
16.             return job.Schedule (this, inputDeps);
17.         }
18.     }
19. }

Is overkill using the job system for this scenario (input handling)? Also there is a side effect on this approach:
1- Player moves horizontally
2- Velocity gets updated based on MoveInput component
3- MoveInput component gets removed
4- Velocity doesn't get updated

Do I need to create a new system to reset the velocity values?

Thanks a lot!

 

Couldn't it just be like this?

Code (CSharp):

1. namespace Rogue.Players
2. {
3.     public class PlayerMovementSystem : JobComponentSystem
4.     {
5.         public struct PlayerInputJob : IJobForEach<Velocity, MoveSpeed, Player>
6.         {
7.             public float2 axis;
8.             public float deltaTime;
9.  
10.             public void Execute(ref Velocity velocity, [ReadOnly] ref MoveSpeed speed, [ReadOnly] ref Player player)
11.             {
12.                 velocity.Value = axis * speed.Value * deltaTime;
13.             }
14.         }
15.  
16.         protected override JobHandle OnUpdate(JobHandle inputDeps)
17.         {
18.             var job = new PlayerInputJob
19.             {
20.                 axis = new float2(Input.GetAxisRaw("Horizontal"), Input.GetAxisRaw("Vertical")),
21.                 deltaTime = Time.deltaTime
22.             };
23.  
24.             return job.Schedule(this, inputDeps);
25.         }
26.     }
27. }

Also, to remove you can just:

Code (CSharp):

1. public class EventManagementSystem : ComponentSystem
2. {
3.     EntityQuery m_Group;
4.  
5.     protected override void OnCreateManager()
6.     {
7.         m_Group = GetEntityQuery(ComponentType.ReadOnly(typeof(MoveInput)));
8.     }
9.  
10.     protected override void OnUpdate()
11.     {
12.         EntityManager.RemoveComponent(m_Group, typeof(MoveInput));
13.     }
14. }

 

Thanks joseph! I found it hard to set boundaries and responsibilities to the systems. (maybe i'm over engineering).

Also I tried this other approach:

Added a Tag component called MoveInput to the Player entity and updated its velocity:

Code (CSharp):

1.     public class PlayerMovementSystem : JobComponentSystem {
2.         public struct ApplyMoveInputJob : IJobForEach<MoveInput, MoveSpeed, Velocity, Player> {
3.             public float deltaTime;
4.  
5.             public void Execute ([ReadOnly] ref MoveInput input, [ReadOnly] ref MoveSpeed speed, ref Velocity velocity, [ReadOnly] ref Player player) {
6.                 velocity.Value = input.Axis * speed.Value * deltaTime;
7.             }
8.         }
9.  
10.         protected override JobHandle OnUpdate (JobHandle inputDeps) {
11.             var job = new ApplyMoveInputJob {
12.                 deltaTime = Time.deltaTime
13.             };
14.  
15.             return job.Schedule (this, inputDeps);
16.         }
17.     }

And then there is another system to set the velocity to zero when the MoveInput is not present:

Code (CSharp):

1.     public class CleanupPlayerMovementSystem : JobComponentSystem {
2.         [ExcludeComponent (typeof (MoveInput))]
3.         public struct CleanMoveInputJob : IJobForEach<Velocity, Player> {
4.             public void Execute (ref Velocity velocity, [ReadOnly] ref Player player) {
5.                 velocity.Value = float2.zero;
6.             }
7.         }
8.  
9.         protected override JobHandle OnUpdate (JobHandle inputDeps) {
10.             var job = new CleanMoveInputJob ();
11.  
12.             return job.Schedule (this, inputDeps);
13.         }
14.     }

Although it is working, I have the feeling that it could be easier! hahaha

 

Reducing the number of systems and simplifying things is a constant battle. This definitely is still overly complicated.

I would try to avoid setting the same component value from multiple systems, especially based on the absence of a component, such as you have in CleanupPlayerMovementSystem. The danger with that is that in two months you'll have forgotten about that system and you'll change the way input works. The MoveInput component will no longer get added or system orders will change and the players velocity will always be set to zero. Then you'll spend an hour trying to figure out why that is happening.

It's better to have the condition in the same job, right where the value is set, or the jobs next to each other in the same system.

 

Thanks for the advice! I implemented the solution that you proposed, it just feel easier to maintain and to understand.