Skip to content

FIX: PlayerInput does not work with C# wrappers (ISXB-1535) #2188

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
wants to merge 7 commits into
base: develop
Choose a base branch
from
+90 −40
Open

FIX: PlayerInput does not work with C# wrappers (ISXB-1535) #2188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a6f71c0
Revert PlayerInput asset copy for single player
jfreire-unity May 30, 2025
21ad066
Only enable PWA actions if they are all disabled
jfreire-unity May 30, 2025
2eb009a
Change to disable PWA actions when using it in PlayerInput
jfreire-unity Jun 2, 2025
81d708a
Fix comment
jfreire-unity Jun 3, 2025
a37ee76
Merge branch 'develop' into isxb-1535-playerinput-does-not-work-w-csh…
jfreire-unity Jun 4, 2025
f7b6585
Use Description for TestCases
jfreire-unity Jun 5, 2025
adb79c6
Only EnableActions() on InitializeInEditor()
jfreire-unity Jun 6, 2025
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
62 changes: 59 additions & 3 deletions Assets/Tests/InputSystem/CoreTests_ProjectWideActions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
using NUnit.Framework;
using UnityEngine.InputSystem;
using UnityEngine.TestTools;
using UnityEngine;
using System.Collections;

#if UNITY_EDITOR
using System.IO;
using UnityEditor;
using UnityEngine;
using UnityEngine.InputSystem.Editor;
#endif // UNITY_EDITOR

Expand Down Expand Up @@ -101,10 +102,10 @@ public void ProjectWideActions_IsAutomaticallyAssignedFromPersistedAsset_WhenRun
Assert.That(InputSystem.actions, Is.Not.Null);

// In editor play-mode we may as well verify that the asset has the expected name
#if UNITY_EDITOR
#if UNITY_EDITOR
var expectedName = InputActionImporter.NameFromAssetPath(AssetDatabase.GetAssetPath(InputSystem.actions));
Assert.That(InputSystem.actions.name, Is.EqualTo(expectedName));
#endif
#endif
}

[Test]
Expand Down Expand Up @@ -140,6 +141,61 @@ public void ProjectWideActions_AppearInEnabledActions()

// TODO Modifying the actions object after being assigned should also enable newly added actions?
}

[Category(TestCategory)]
[Test]
[TestCase("Player", true, Description = "PlayerInput using project-wide actions can have default action map set " +
"enabled, and all others disabled.")]
[TestCase(null, false, Description = "PlayerInput using project wide actions has all action maps of project-wide " +
"actions disabled, if there is no default action map assigned.")]
public void ProjectWideActions_CanEnableCurrentActionMapOfPlayerInput(string actionMapName, bool expectedResult)
{
var keyboard = InputSystem.AddDevice<Keyboard>();
var go = new GameObject("PlayerInput");

go.SetActive(false);
var playerInput = go.AddComponent<PlayerInput>();
playerInput.actions = InputSystem.actions;
// Default action map to be enabled. All others are expected to be disabled since InputSystem.actions.Disable()
// will be called.
playerInput.defaultActionMap = actionMapName;

// PWA actions assigned to playerInput start enabled
Assert.That(playerInput.actions.enabled, Is.True);
Assert.That(ReferenceEquals(playerInput.actions, InputSystem.actions));

#if UNITY_EDITOR
InputSystem.OnPlayModeChange(PlayModeStateChange.ExitingEditMode);
#endif

// This makes sure to call PlayerInput.OnEnable()
go.SetActive(true);

// This mimics usings to disabling actions on Start/Awake.
InputSystem.actions.Disable();

if (!string.IsNullOrEmpty(actionMapName))
playerInput.SwitchCurrentActionMap("Player");

Assert.That(playerInput.actions.enabled, Is.EqualTo(expectedResult));

// We do this on the editor to make sure project-wide actions maintain the enabled state
// after entering PlayMode.
#if UNITY_EDITOR
InputSystem.OnPlayModeChange(PlayModeStateChange.EnteredPlayMode);
Assert.That(playerInput.actions.enabled, Is.EqualTo(expectedResult));
#endif

if (!string.IsNullOrEmpty(actionMapName))
Assert.That(playerInput.currentActionMap.enabled, Is.EqualTo(expectedResult));

// Check state of all action maps in the asset
Assert.That(InputSystem.actions.FindActionMap("Player").enabled, Is.EqualTo(expectedResult));
Assert.That(InputSystem.actions.FindActionMap("UI").enabled, Is.False);

//NOTE: Asset actions are considered enabled even if a single action map is enabled
Assert.That(playerInput.actions.enabled, Is.EqualTo(expectedResult));
}
}

#endif
37 changes: 19 additions & 18 deletions Assets/Tests/InputSystem/Plugins/PlayerInputTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public void PlayerInput_CanInstantiatePlayer()

Assert.That(player, Is.Not.Null);
Assert.That(player.playerIndex, Is.EqualTo(0));
Assert.That(player.actions.actionMaps.Count, Is.EqualTo(prefabPlayerInput.actions.actionMaps.Count));
Assert.That(player.actions, Is.SameAs(prefabPlayerInput.actions));
Assert.That(player.devices, Is.EquivalentTo(new[] { gamepad }));
Assert.That(player.currentControlScheme, Is.EqualTo("Gamepad"));
}
Expand Down Expand Up @@ -395,21 +395,6 @@ public void PlayerInput_CanAssignActionsToPlayer()
Assert.That(playerInput.actions.actionMaps[0].name, Is.EqualTo(actions.actionMaps[0].name));
}

[Test]
[Category("PlayerInput")]
public void PlayerInput_CopiesActionAssetForFirstPlayer()
{
var go = new GameObject();
var playerInput = go.AddComponent<PlayerInput>();

var actions = InputActionAsset.FromJson(kActions);
playerInput.actions = actions;

Assert.That(playerInput.actions.actionMaps.Count, Is.EqualTo(actions.actionMaps.Count));
Assert.That(playerInput.actions.actionMaps[0].name, Is.EqualTo(actions.actionMaps[0].name));
Assert.That(playerInput.actions.GetInstanceID(), !Is.EqualTo(actions.GetInstanceID()));
}

[Test]
[Category("PlayerInput")]
public void PlayerInput_AssigningNewActionsToPlayer_DisablesExistingActions()
Expand All @@ -424,12 +409,12 @@ public void PlayerInput_AssigningNewActionsToPlayer_DisablesExistingActions()
playerInput.actions = actions1;

Assert.That(playerInput.actions.actionMaps[0].enabled, Is.True);
Assert.That(actions1.actionMaps[0].enabled, Is.False);
Assert.That(actions2.actionMaps[0].enabled, Is.False);

playerInput.actions = actions2;

Assert.That(actions2.actionMaps[0].enabled, Is.False);
Assert.That(playerInput.actions.actionMaps[0].enabled, Is.True);
Assert.That(actions1.actionMaps[0].enabled, Is.False);
}

[Test]
Expand Down Expand Up @@ -497,6 +482,22 @@ public void PlayerInput_DuplicatingActions_AssignsNewInstanceToUI()
Assert.That(playerInput2.actions, Is.SameAs(ui2.actionsAsset));
}

[Test]
[Category("PlayerInput")]
public void PlayerInput_ActionFromCodeGeneratedActionIsTheSameBeingReferenced()
{
var go = new GameObject();

var playerInput = go.AddComponent<PlayerInput>();
var ui = go.AddComponent<InputSystemUIInputModule>();
var defaultActions = new DefaultInputActions();

playerInput.uiInputModule = ui;
playerInput.actions = defaultActions.asset;

Assert.That(defaultActions.UI.Submit == playerInput.actions.FindAction("Submit"), Is.True);
}

[Test]
[Category("PlayerInput")]
public void PlayerInput_CanPassivateAndReactivateInputBySendingMessages()
Expand Down
9 changes: 3 additions & 6 deletions Packages/com.unity.inputsystem/InputSystem/InputSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3408,7 +3408,6 @@ public static int ListEnabledActions(List<InputAction> actions)

#endregion


/// <summary>
/// The current version of the input system package.
/// </summary>
Expand Down Expand Up @@ -3651,9 +3650,7 @@ internal static void OnPlayModeChange(PlayModeStateChange change)
case PlayModeStateChange.EnteredPlayMode:
s_SystemObject.enterPlayModeTime = InputRuntime.s_Instance.currentTime;
s_Manager.SyncAllDevicesAfterEnteringPlayMode();
#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
EnableActions();
#endif // UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS

break;

case PlayModeStateChange.ExitingPlayMode:
Expand Down Expand Up @@ -3911,8 +3908,8 @@ private static void Reset(bool enableRemoting = false, IInputRuntime runtime = n
InputUser.ResetGlobals();
EnhancedTouchSupport.Reset();

// This is the point where we initialise project-wide actions for the Editor, Editor Tests and Player Tests.
// Note this is too early for editor ! actions is not setup yet.
// // This is the point where we initialise project-wide actions for the Editor, Editor Tests and Player Tests.
// // Note this is too early for editor ! actions is not setup yet.
#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
EnableActions();
#endif
Expand Down
22 changes: 9 additions & 13 deletions Packages/com.unity.inputsystem/InputSystem/Plugins/PlayerInput/PlayerInput.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,14 +342,8 @@ public InputActionAsset actions
UninitializeActions();
}

var didChange = m_Actions != null;

m_Actions = value;

if (didChange || m_Enabled)
// copy action asset for the first player so that the original asset stays untouched
CopyActionAssetAndApplyBindingOverrides();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am still concerned with removing the copy for the first player.
In the long run this potentially leads to conflicts again as soon as action assets are accessed and operated from somewhere out of the player input.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

leads to conflicts again

Can you specify which conflicts have occurred before? I'm not aware of any other bugs/conlfits that would require a copy for the first player.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I am referring to is that the project wide actions was the first occurrence, since the asset can be operated freely from anywhere else without further restrictions I think it's not unlikely to encounter similar problems in the future (even if we can not directly think of one now).

Copy link
Collaborator Author

@jfreire-unity jfreire-unity Jun 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe project-wide actions didn't consider this use case during the design phase. In the end, PWA it's just another asset with a "small" difference that has all actions enabled. The normal behavior of an asset is to come with actions disabled. But this is not really relevant to the discussion of doing copies, which I want to explore further.

Making a copy during assignment (playerInput.asset = myAsset) is problematic for two reasons:

  • It breaks expected behavior in C# - as a developer, I assume reference types maintain references rather than creating copies on assignment.
  • It introduces inconsistency with our existing code, like InputSystemUIInputModule.actionAsset which maintains a reference, and doesn't do a copy.

I feel we get into a slippery slope of inconsistencies if we go down the path of doing copies for every asset assignment.

I'm fine with going with a different direction, but I would like to remove the copy behavior on assignment regardless of what we agree on doing.

Copy link
Collaborator

@ritamerkl ritamerkl Jun 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I agree on that, assigning with a copy under the hood and the reference is indeed confusing.
Since this happens for all other players (except for the first), the most consistent way to do it would probably to not do it at all, or changing the behaviour to make the copy more visible.
To get a consistent behaviour I think wee need to decide for one way. What I am thinking now is:

  • defining a "rootAsset" which can be assigned and then just make the player copy available. (breaking change)
  • not copying the asset but outsourcing the runtime data to a metafile for instance (bigger change, but maybe not necessarily breaking change?)

I think the third option is the solution you proposed, which rolls back to the previous inconsistent state. I think that's valid with the argument to not break anything, though I believe it's not ideal in terms of consistent behaviour.


if (m_Enabled)
{
ClearCaches();
Expand Down Expand Up @@ -960,6 +954,15 @@ public void ActivateInput()

m_InputActive = true;

// Disable project-wide actions if they are being used by this PlayerInput.
#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
if (m_Actions != null && m_Actions == InputSystem.actions)
{
InputSystem.AllowEnableActionsAfterEnterPlayMode(false);
InputSystem.actions.Disable();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will have an effect on the PlayerInput asset too, is disabling of all actions intended at this spot?

}
#endif

// If we have no current action map but there's a default
// action map, make it current.
if (m_CurrentActionMap == null && m_Actions != null && !string.IsNullOrEmpty(m_DefaultActionMap))
Expand Down Expand Up @@ -1810,13 +1813,6 @@ void Reset()

#endif

private void Awake()
{
// If an action asset is assigned copy it to avoid modifying the original asset.
if (m_Actions != null)
CopyActionAssetAndApplyBindingOverrides();
}

private void OnEnable()
{
m_Enabled = true;
Expand Down