Skip to content

two players can play on one keyboard #52

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pro2p wants to merge 6 commits into
base: main
Choose a base branch
from
Open

two players can play on one keyboard #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ffc73f0
two players can play on one keyboard
pro2p May 6, 2026
2769b3d
fix
pro2p May 6, 2026
931f39b
fix
pro2p May 6, 2026
9fe62a6
fix
pro2p May 6, 2026
80c3a5c
fiiiix
pro2p May 6, 2026
1bcd70b
outsource view-related calculations and replace if-else by dictionary…
pro2p May 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions Assets/Game/Features/Input/PlayerInputProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,31 @@
using Automathon.Utility;
using System.Collections.Generic;
using UnityEngine;
using UnityEngine.InputSystem;

namespace Automathon.Game.Input
{
public class PlayerInputProvider : IInputProvider
{
private bool isGamepad;
public enum PlayerControlsType { LeftKeyboard, RightKeyboard, Gamepad }
public PlayerControlsType PlayerControls { get; private set; }
private InputAction dashAction;
private InputAction grenadeAction;
private InputAction shieldAction;
private InputAction shootAction;
private InputAction moveAction;
private InputAction aimAction;

private Dictionary<string, PlayerControlsType> schemeToControlsType = new Dictionary<string, PlayerControlsType>
{
{ "Gamepad", PlayerControlsType.Gamepad },
{ "Keyboard_left", PlayerControlsType.LeftKeyboard },
{ "Keyboard_right", PlayerControlsType.RightKeyboard }
};

public PlayerInputProvider(PlayerInput playerInput)
{
isGamepad = playerInput.currentControlScheme == "Gamepad";
PlayerControls = schemeToControlsType[playerInput.currentControlScheme];
dashAction = playerInput.actions["Dash"];
grenadeAction = playerInput.actions["Grenade"];
shieldAction = playerInput.actions["Shield"];
Expand All @@ -39,13 +50,13 @@ public Vector2Int GetMilliMovementDir()

public Vector2Int GetMilliAimingDir()
{
if (isGamepad)
Vector2 aimingDir = aimAction.ReadValue<Vector2>();
if (PlayerControls == PlayerControlsType.RightKeyboard)
{
Vector2 aimingDir = aimAction.ReadValue<Vector2>();
return new Vector2Int((int)(aimingDir.x * 1000), (int)(aimingDir.y * 1000));
Vector3 worldPos = aimingDir.ScreenToWorldSpace();
return ((Vector2)worldPos).ToVector2IntScaled();
}
Comment on lines +53 to 58
Copy link
Copy Markdown
Collaborator

@thomasbringer-madbox thomasbringer-madbox May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is this issue - i believe it's out of scope for this PR, i suggest we fix in upcoming PR.


return GetMilliMovementDir();
return new Vector2Int((int)(aimingDir.x * 1000), (int)(aimingDir.y * 1000));
Copy link
Copy Markdown
Collaborator

@thomasbringer-madbox thomasbringer-madbox May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also some calculation here that could potentially be outsourced

}
}
}
45 changes: 29 additions & 16 deletions Assets/Game/Features/Lobby/TankSpawnerView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,31 +13,38 @@ public class TankSpawnerView : MonoBehaviour
[SerializeField] private PlayerInputManager playerInputManager;
private Dictionary<PlayerInput, IInputProvider> inputProviders = new();

private void Awake()
{
playerInputManager.onPlayerLeft += OnPlayerLeft;
}
Comment on lines +16 to +19
Copy link
Copy Markdown
Collaborator

@thomasbringer-madbox thomasbringer-madbox May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I missed it, i see no unsubscription: possible memory leak.

Copy link
Copy Markdown
Collaborator Author

@pro2p pro2p May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I'm not sure how this code works, I did not write it and didn't quite look into it, I just assumed that it was preferable to subscribe to that event in Awake rather than doing it every frame in Update.


Copy link
Copy Markdown
Collaborator

@thomasbringer-madbox thomasbringer-madbox May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, correct I believe

private void Update()
{
if (GameplayManager.State != GameplayManager.GameState.Lobby)
return;

playerInputManager.onPlayerLeft += OnPlayerLeft;

HandleGamepadJoinInput();
HandleKeyboardJoinInput();

if (Keyboard.current != null && Keyboard.current.escapeKey.wasPressedThisFrame)
if (Keyboard.current != null)
{
PlayerInput playerInputToRemove = null;

foreach (PlayerInput playerInput in inputProviders.Keys)
if (Keyboard.current.escapeKey.wasPressedThisFrame || Keyboard.current.deleteKey.wasPressedThisFrame)
{
if (playerInput.currentControlScheme == "Keyboard&Mouse")
bool isLeftPlayer = Keyboard.current.escapeKey.wasPressedThisFrame;
PlayerInput playerInputToRemove = null;

foreach (PlayerInput playerInput in inputProviders.Keys)
{
playerInputToRemove = playerInput;
break;
if ((isLeftPlayer && playerInput.currentControlScheme == "Keyboard_left") || (!isLeftPlayer && playerInput.currentControlScheme == "Keyboard_right"))
{
playerInputToRemove = playerInput;
break;
}
}
Comment on lines +33 to 43
Copy link
Copy Markdown
Collaborator

@thomasbringer-madbox thomasbringer-madbox May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still very painful to read, especially that if statement. I'm sure there are more elegant ways to write it. Consider using a Left/Right enum.

This is a slight improvement ChatGPT suggested (there could be better ways though, for instance with a left/right enum)

bool isLeftPlayer = Keyboard.current.escapeKey.wasPressedThisFrame;

string targetScheme = isLeftPlayer ? "Keyboard_left" : "Keyboard_right";

PlayerInput playerInputToRemove = null;

foreach (PlayerInput playerInput in inputProviders.Keys)
{
    if (playerInput.currentControlScheme != targetScheme)
        continue;

    playerInputToRemove = playerInput;
    break;
}

}

if (playerInputToRemove != null)
OnPlayerLeft(playerInputToRemove);
if (playerInputToRemove != null)
OnPlayerLeft(playerInputToRemove);
Copy link
Copy Markdown
Collaborator

@thomasbringer-madbox thomasbringer-madbox May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's usually not a good practice to have a same method be both triggered by an event AND be triggered manually. Now here we're doing some weird stuff so not sure the core issue, but still a possibly relevant lesson:

You would prefer using the following pattern:
OnDidStuff subscribes to event DidStuff
OnDidStuff simply calls method DoThings
And then you can also call manually DoThings whenever required.
This way, all method names correspond exactly to their functionality. It's also super readable code. A more real example could be:
OnScored subscribes to event Scored
OnScored simply calls method PlayScoreVFX. This way it's super readable that we trigger a VFX every time we score a point.

Copy link
Copy Markdown
Collaborator

@thomasbringer-madbox thomasbringer-madbox May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A potential issue yes, kinda linked to this comment

}
}
}

Expand All @@ -49,7 +56,12 @@ private void HandleKeyboardJoinInput()
if (keyboard.eKey.wasPressedThisFrame || keyboard.wKey.wasPressedThisFrame || keyboard.aKey.wasPressedThisFrame ||
keyboard.sKey.wasPressedThisFrame || keyboard.dKey.wasPressedThisFrame || keyboard.qKey.wasPressedThisFrame)
{
JoinKeyboardPlayer();
JoinKeyboardPlayer(true);
}
else if (keyboard.upArrowKey.wasPressedThisFrame || keyboard.downArrowKey.wasPressedThisFrame || keyboard.leftArrowKey.wasPressedThisFrame ||
keyboard.rightArrowKey.wasPressedThisFrame || keyboard.rightShiftKey.wasPressedThisFrame || keyboard.slashKey.wasPressedThisFrame)
{
JoinKeyboardPlayer(false);
}
}

Expand Down Expand Up @@ -103,23 +115,24 @@ private void JoinGamepadPlayer(Gamepad gamepad)
SpawnTank(playerInput);
}

private void JoinKeyboardPlayer()
private void JoinKeyboardPlayer(bool isLeft)
Copy link
Copy Markdown
Collaborator

@thomasbringer-madbox thomasbringer-madbox May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using booleans for keyboard players (left/right) is hard to read and maintain. Consider using a Left/Right enum.

{
if (PlayerInput.all.Count >= playerInputManager.maxPlayerCount)
return;

foreach (PlayerInput playerInputTemp in inputProviders.Keys)
{
if (playerInputTemp.currentControlScheme == "Keyboard&Mouse")
if ((isLeft && playerInputTemp.currentControlScheme == "Keyboard_left") || (!isLeft && playerInputTemp.currentControlScheme == "Keyboard_right"))
Copy link
Copy Markdown
Collaborator

@thomasbringer-madbox thomasbringer-madbox May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still looks bad and looks almost duplicate to the logic from before, consider refactoring it into a single method

return;
}

// 2. Manually trigger the join
// We pass -1 for playerIndex to let Unity assign the next available index (0, 1, 2, etc.)
string controlScheme = isLeft ? "Keyboard_left" : "Keyboard_right";
PlayerInput playerInput = playerInputManager.JoinPlayer(
playerIndex: -1,
splitScreenIndex: -1,
controlScheme: "Keyboard&Mouse",
controlScheme: controlScheme,
pairWithDevice: Keyboard.current
);

Expand Down
2 changes: 1 addition & 1 deletion Assets/Game/Features/Shield/ShieldAbility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public ShieldAbility(Func<bool> shouldActivate) : base(cooldown: COOLDOWN_MILLIS
protected override void Activate()
{
Vector2Int position = Tank.Position + Tank.LastMilliDirection * SPAWN_DISTANCE_FROM_TANK / 1000;
int rotationMilliRad = Tank.RotationMilli + IntMath.PI_MILLI / 2;
int rotationMilliRad = Tank.LastMilliDirection.CalculateAngleMilliRad() + IntMath.PI_MILLI / 2;

GameplayManager.Instantiate(new Shield(position, rotationMilliRad));
}
Expand Down
12 changes: 9 additions & 3 deletions Assets/Game/Features/Tank/Tank.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,22 @@ public override void Update()
rigidbody.Velocity = movementInput * SPEED / 1000;

Vector2Int directionInput = InputProvider.GetMilliAimingDir();
if (InputProvider is PlayerInputProvider playerInputProvider && playerInputProvider.PlayerControls == PlayerInputProvider.playerControlsType.Right)
Copy link
Copy Markdown
Collaborator

@thomasbringer-madbox thomasbringer-madbox May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AH. Your code doesn't compile but you made the PR anyway? 😭

{
directionInput = new Vector2Int(directionInput.X - Position.X, directionInput.Y - Position.Y);
Copy link
Copy Markdown
Collaborator

@thomasbringer-madbox thomasbringer-madbox May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a design point of view, I sure wouldn't want to play a top-down shooter... where you can't aim.

}
directionInput.NormalizeAtScale(1000);

if (directionInput != Vector2Int.Zero)
{
RotationMilli = directionInput.CalculateAngleMilliRad();
rigidbody.AngularVelocityMilli = 0;

LastMilliDirection = directionInput;
LastMilliDirection.NormalizeAtScale(1000);
}
if (movementInput != Vector2Int.Zero)
{
RotationMilli = movementInput.CalculateAngleMilliRad();
rigidbody.AngularVelocityMilli = 0;
}
}

private void Death()
Expand Down
4 changes: 4 additions & 0 deletions Assets/Game/Features/Utility/ViewMath.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using Automathon.Game.World;
using UnityEngine;

namespace Automathon.Utility
Expand All @@ -8,5 +9,8 @@ public static Quaternion MilliRadRotationToQuaternion(float rotationMillirad)
{
return Quaternion.Euler(0, 0, rotationMillirad * Mathf.Rad2Deg / 1000f);
}

public static Vector2Int ToVector2IntScaled(this Vector2 vector)
=> new Vector2Int((int)(vector.x * WorldConstants.SPACE_SCALE), (int)(vector.y * WorldConstants.SPACE_SCALE));
}
}
1 change: 1 addition & 0 deletions Assets/Game/Features/World/WorldConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@
public static class WorldConstants
{
public const int SPACE_SCALE = 1000;
public const int CAMERA_DISTANCE = 10000;
Copy link
Copy Markdown
Collaborator

@thomasbringer-madbox thomasbringer-madbox May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be removed, see this comment

}
}
Loading