Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
0c77ea4
Fixed enabling all action maps for project wide actions overrides pla…
ritamerkl Feb 21, 2025
f6480e7
added changelog
ritamerkl Feb 21, 2025
d9e3b6a
added nullcheck
ritamerkl Feb 24, 2025
6afc84a
changed to duplicate actions for playerinput instead of using project…
ritamerkl Feb 24, 2025
2029be3
Merge branch 'develop' into fix-project-wide-actions-override-playeri…
ritamerkl Feb 24, 2025
98dec35
always duplicate action assets for PlayerInput to avoid manipulating …
ritamerkl Feb 24, 2025
4f64381
undo editing test
ritamerkl Feb 24, 2025
ae27089
reset
ritamerkl Feb 24, 2025
1655b7f
always duplicate PlayerInput action assets and removed redundant pjwd…
ritamerkl Feb 25, 2025
b05b5e8
fix tests and copying of asset
ritamerkl Feb 27, 2025
acf6225
revert deleting comment
ritamerkl Feb 27, 2025
9c6cda3
added comment
ritamerkl Feb 27, 2025
67002de
adjusted copying of asset to work for prefabs too
ritamerkl Feb 28, 2025
0e4b64b
disabling all maps is not necessary anymore with copy
ritamerkl Feb 28, 2025
8fab5a9
added new test to proof the first PlayerInput ActionAsset is copied
ritamerkl Feb 28, 2025
55b7d71
Merge branch 'develop' into fix-project-wide-actions-override-playeri…
ritamerkl Feb 28, 2025
ce0a9a5
renamed copy function
ritamerkl Feb 28, 2025
805f0d9
Merge branch 'develop' into fix-project-wide-actions-override-playeri…
ritamerkl Feb 28, 2025
93a4c69
enable actions on entered playmode
ritamerkl Mar 3, 2025
53a0e98
Merge branch 'develop' into fix-project-wide-actions-override-playeri…
ritamerkl Mar 3, 2025
9654ffd
Merge branch 'develop' into fix-project-wide-actions-override-playeri…
ritamerkl Mar 4, 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
51 changes: 34 additions & 17 deletions Assets/Tests/InputSystem/Plugins/PlayerInputTests.cs
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, Is.SameAs(prefabPlayerInput.actions));
Assert.That(player.actions.actionMaps.Count, Is.EqualTo(prefabPlayerInput.actions.actionMaps.Count));
Assert.That(player.devices, Is.EquivalentTo(new[] { gamepad }));
Assert.That(player.currentControlScheme, Is.EqualTo("Gamepad"));
}
Expand Down Expand Up @@ -108,7 +108,6 @@ public void PlayerInput_CanLinkSpecificDeviceToUI()
var ui = prefab.AddComponent<InputSystemUIInputModule>();
player.uiInputModule = ui;
player.actions = InputActionAsset.FromJson(kActions);
ui.actionsAsset = player.actions;

InputSystem.AddDevice<Gamepad>();
InputSystem.AddDevice<Keyboard>();
Expand All @@ -117,6 +116,7 @@ public void PlayerInput_CanLinkSpecificDeviceToUI()
var gamepad = InputSystem.AddDevice<Gamepad>();

var instance = PlayerInput.Instantiate(prefab, pairWithDevices: gamepad);
ui.actionsAsset = instance.actions;

Assert.That(instance.devices, Is.EquivalentTo(new[] { gamepad }));
Assert.That(ui.actionsAsset.devices, Is.EquivalentTo(new[] { gamepad }));
Expand Down Expand Up @@ -149,11 +149,11 @@ public void PlayerInput_CanUseSameActionsForUIInputModule()
eventSystemGO.SetActive(true);
playerGO.SetActive(true);

Assert.That(actions.FindActionMap("Gameplay").enabled, Is.True);
Assert.That(actions.FindActionMap("UI").enabled, Is.True);
Assert.That(actions["UI/Navigate"].controls, Is.Empty);
Assert.That(actions["UI/Point"].controls, Is.EquivalentTo(new[] { mouse.position }));
Assert.That(actions["UI/Click"].controls, Is.EquivalentTo(new[] { mouse.leftButton }));
Assert.That(player.actions.FindActionMap("Gameplay").enabled, Is.True);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe insert a new line after this one and add a comment why there is a split now?

Assert.That(uiModule.actionsAsset.FindActionMap("UI").enabled, Is.True);
Assert.That(uiModule.actionsAsset["UI/Navigate"].controls, Is.Empty);
Assert.That(uiModule.actionsAsset["UI/Point"].controls, Is.EquivalentTo(new[] { mouse.position }));
Assert.That(uiModule.actionsAsset["UI/Click"].controls, Is.EquivalentTo(new[] { mouse.leftButton }));
}

[Test]
Expand Down Expand Up @@ -391,7 +391,23 @@ public void PlayerInput_CanAssignActionsToPlayer()
var actions = InputActionAsset.FromJson(kActions);
playerInput.actions = actions;

Assert.That(playerInput.actions, Is.SameAs(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));
}

[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]
Expand All @@ -407,13 +423,13 @@ public void PlayerInput_AssigningNewActionsToPlayer_DisablesExistingActions()
playerInput.defaultActionMap = "gameplay";
playerInput.actions = actions1;

Assert.That(actions1.actionMaps[0].enabled, Is.True);
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);

playerInput.actions = actions2;

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

[Test]
Expand Down Expand Up @@ -1714,7 +1730,8 @@ InputDevice[] AddDevices()

// Make sure that no cloning of actions happened on the prefab.
// https://fogbugz.unity3d.com/f/cases/1319756/
Assert.That(playerPrefab.GetComponent<PlayerInput>().actions, Is.SameAs(playerPrefabActions));

Assert.That(playerPrefab.GetComponent<PlayerInput>().actions.actionMaps.Count, Is.EqualTo(playerPrefabActions.actionMaps.Count));
Assert.That(playerPrefab.GetComponent<PlayerInput>().m_ActionsInitialized, Is.False);
}

Expand Down Expand Up @@ -2421,13 +2438,13 @@ public void PlayerInput_DelegatesAreUpdate_WhenActionMapAddedAfterAssignment()
// Disable the asset while adding another action map to it as none
// of the actions in the asset can be enabled during modification
//
actionAsset.Disable();
playerInput.actions.Disable();
var keyboard = InputSystem.AddDevice<Keyboard>();
var newActionMap = actionAsset.AddActionMap("NewMap");
var newActionMap = playerInput.actions.AddActionMap("NewMap");
var newAction = newActionMap.AddAction("NewAction");
newAction.AddBinding("<Keyboard>/k", groups: "Keyboard");
actionAsset.AddControlScheme("Keyboard").WithRequiredDevice<Keyboard>();
actionAsset.Enable();
playerInput.actions.AddControlScheme("Keyboard").WithRequiredDevice<Keyboard>();
playerInput.actions.Enable();

playerInput.currentActionMap = newActionMap;
playerInput.ActivateInput();
Expand Down
3 changes: 3 additions & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ however, it has to be formatted properly to pass verification tests.

## [Unreleased] - yyyy-mm-dd

### Fixed
- Fixed an issue where all action maps were enabled initially for project wide actions, which overrode the PlayerInput action map configuration. [ISXB-920](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-920)

### Changed
- Changed enum value `Key.IMESelected` to obsolete which was not a real key. Please use the ButtonControl `imeSelected`.

Expand Down
10 changes: 2 additions & 8 deletions Packages/com.unity.inputsystem/InputSystem/InputSystem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3609,12 +3609,6 @@ internal static void InitializeInEditor(IInputRuntime runtime = null)
// this would cancel the import of large assets that are dependent on the InputSystem package and import it as a dependency.
EditorApplication.delayCall += ShowRestartWarning;

#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
// Make sure project wide input actions are enabled.
// Note that this will always fail if entering play-mode within editor since not yet in play-mode.
EnableActions();
#endif

RunInitialUpdate();

k_InputInitializeInEditorMarker.End();
Expand Down Expand Up @@ -3657,9 +3651,9 @@ 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
#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
EnableActions();
#endif // UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
#endif // UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
break;

case PlayModeStateChange.ExitingPlayMode:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -342,8 +342,14 @@ public InputActionAsset actions
UninitializeActions();
}

var didChange = m_Actions != null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is related to your question around comparing InputActionAssets? It's not going to be cheap to compare, but a comparator would be useful. Additionally that comparator could compute a hash to make the comparison cheaper. However, this could also be fine since it will very likely be different if it's another instance. Otherwise the project itself is oddly designed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, all this is around avoiding copying the asset multiple times, which works as far as I tested and for the cases I could come up with, but like you said ideally it would be great to be able to compare an copy of asset to it's original to determine that it's a clone of the asset.


m_Actions = value;

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

if (m_Enabled)
{
ClearCaches();
Expand Down Expand Up @@ -1373,14 +1379,7 @@ private void InitializeActions()
for (var i = 0; i < s_AllActivePlayersCount; ++i)
if (s_AllActivePlayers[i].m_Actions == m_Actions && s_AllActivePlayers[i] != this)
{
var oldActions = m_Actions;
m_Actions = Instantiate(m_Actions);
for (var actionMap = 0; actionMap < oldActions.actionMaps.Count; actionMap++)
{
for (var binding = 0; binding < oldActions.actionMaps[actionMap].bindings.Count; binding++)
m_Actions.actionMaps[actionMap].ApplyBindingOverride(binding, oldActions.actionMaps[actionMap].bindings[binding]);
}

CopyActionAssetAndApplyBindingOverrides();
break;
}

Expand Down Expand Up @@ -1430,6 +1429,18 @@ private void InitializeActions()
m_ActionsInitialized = true;
}

private void CopyActionAssetAndApplyBindingOverrides()
{
// duplicate action asset to not operate on the original (as it might be used outside - eg project wide action asset or UIInputModule)
var oldActions = m_Actions;
m_Actions = Instantiate(m_Actions);
for (var actionMap = 0; actionMap < oldActions.actionMaps.Count; actionMap++)
{
for (var binding = 0; binding < oldActions.actionMaps[actionMap].bindings.Count; binding++)
m_Actions.actionMaps[actionMap].ApplyBindingOverride(binding, oldActions.actionMaps[actionMap].bindings[binding]);
}
}

private void UninitializeActions()
{
if (!m_ActionsInitialized)
Expand Down Expand Up @@ -1799,6 +1810,13 @@ 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