Skip to content

FIX: Avoid disabling project-wide actions on shutdown #2346

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
AswinRajGopal wants to merge 6 commits into
base: develop
Choose a base branch
from
+44 −11
Open

FIX: Avoid disabling project-wide actions on shutdown #2346

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
c562118
Added test to cover the fix.
AswinRajGopal Feb 17, 2026
0ed73d8
Fix: Avoid disabling project-wide actions on shutdown
AswinRajGopal Feb 17, 2026
e917c04
Merge branch 'develop' into uum-134130/fix-project-wide-action-onshut…
AswinRajGopal Mar 2, 2026
61807cc
Fix test double shutdown call causing issues.
AswinRajGopal Mar 3, 2026
b087ccd
Fix InputForUI action lifecycle to only manage the UI action map.
AswinRajGopal Mar 23, 2026
9cdf098
Merge branch 'develop' into uum-134130/fix-project-wide-action-onshut…
AswinRajGopal Mar 24, 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
33 changes: 32 additions & 1 deletion Assets/Tests/InputSystem/Plugins/InputForUITests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public class InputForUITests : InputTestFixture
readonly List<Event> m_InputForUIEvents = new List<Event>();
private int m_CurrentInputEventToCheck;
InputSystemProvider m_InputSystemProvider;
private bool m_ClearedMockProvider;

private InputActionAsset storedActions;

Expand All @@ -45,6 +46,7 @@ public override void Setup()
{
base.Setup();
m_CurrentInputEventToCheck = 0;
m_ClearedMockProvider = false;

storedActions = InputSystem.actions;

Expand All @@ -59,7 +61,8 @@ public override void Setup()
public override void TearDown()
{
EventProvider.Unsubscribe(InputForUIOnEvent);
EventProvider.ClearMockProvider();
if (!m_ClearedMockProvider)
EventProvider.ClearMockProvider();
m_InputForUIEvents.Clear();

InputSystem.manager.actions = storedActions;
Expand Down Expand Up @@ -92,6 +95,34 @@ public void InputSystemActionAssetIsNotNull()
"Test is invalid since InputSystemProvider actions are not available");
}

[Test]
[Category(kTestCategory)]
public void Shutdown_DoesNotDisableProjectWideActionsAsset()
{
var asset = ScriptableObject.CreateInstance<InputActionAsset>();
var uiMap = new InputActionMap("UI");
uiMap.AddAction("Point", InputActionType.PassThrough, "<Mouse>/position");
uiMap.AddAction("Navigate", InputActionType.PassThrough, "<Gamepad>/leftStick");
uiMap.AddAction("Submit", InputActionType.Button, "<Keyboard>/enter");
uiMap.AddAction("Cancel", InputActionType.Button, "<Keyboard>/escape");
uiMap.AddAction("Click", InputActionType.PassThrough, "<Mouse>/leftButton");
uiMap.AddAction("MiddleClick", InputActionType.PassThrough, "<Mouse>/middleButton");
uiMap.AddAction("RightClick", InputActionType.PassThrough, "<Mouse>/rightButton");
uiMap.AddAction("ScrollWheel", InputActionType.PassThrough, "<Mouse>/scroll");
asset.AddActionMap(uiMap);

InputSystem.s_Manager.actions = asset;
Copy link
Copy Markdown
Contributor

@u-pr u-pr bot Mar 30, 2026

Choose a reason for hiding this comment

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

low
For better maintainability and consistency with the rest of the test class, consider using the public InputSystem.actions property instead of the internal s_Manager field. This aligns with the access pattern used in the Setup method.

Suggested change
InputSystem.s_Manager.actions = asset;
InputSystem.actions = asset;

🤖 Helpful? 👍/👎


m_InputSystemProvider.Initialize();
Assert.That(asset.enabled, Is.True, "Project-wide actions should be enabled by provider initialization.");

EventProvider.ClearMockProvider();
m_ClearedMockProvider = true;
Assert.That(asset.enabled, Is.True, "Project-wide actions must remain enabled after provider shutdown.");
Copy link
Copy Markdown
Contributor

@u-pr u-pr bot Mar 30, 2026

Choose a reason for hiding this comment

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

medium
Based on the implementation in InputSystemProvider.UnregisterActions, this assertion will likely fail.

If the provider was responsible for enabling the "UI" map (which it will be here since the asset is newly created and its maps are disabled by default), it will disable that map during shutdown. Since no other maps are enabled in this test asset, asset.enabled will return false after the provider is cleared.

🤖 Helpful? 👍/👎


Object.DestroyImmediate(asset);
}

[Test]
[Category(kTestCategory)]
// Checks that mouse events are ignored when a touch is active.
Expand Down
22 changes: 12 additions & 10 deletions Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ internal class InputSystemProvider : IEventProviderImpl

DefaultInputActions m_DefaultInputActions;
InputActionAsset m_InputActionAsset;
InputActionMap m_UIActionMap;
bool m_ShouldDisableUIActionMapOnUnregister;

// Note that these are plain action references instead since InputActionReference do
// not provide any value when this integration doesn't have any UI. If this integration
Expand Down Expand Up @@ -636,14 +638,11 @@ void RegisterActions()
m_RightClickAction = FindActionAndRegisterCallback(Actions.RightClickAction, OnRightClickPerformed);
m_ScrollWheelAction = FindActionAndRegisterCallback(Actions.ScrollWheelAction, OnScrollWheelPerformed);

// When adding new actions, don't forget to add them to UnregisterActions
if (InputSystem.actions == null)
{
// If we've not loaded a user-created set of actions, just enable the UI actions from our defaults.
m_InputActionAsset.FindActionMap("UI", true).Enable();
}
else
m_InputActionAsset.Enable();
// Only touch the UI map so we don't change the enabled state of unrelated maps.
m_UIActionMap = m_InputActionAsset?.FindActionMap("UI", true);
m_ShouldDisableUIActionMapOnUnregister = m_UIActionMap != null && !m_UIActionMap.enabled;
Copy link
Copy Markdown
Contributor

@u-pr u-pr bot Mar 30, 2026

Choose a reason for hiding this comment

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

high
This call will throw an ArgumentException if the m_InputActionAsset does not contain a map named "UI". This is a regression for scenarios where a user provides a custom InputActionAsset (via InputSystem.actions) that uses different map names (e.g., "Default", "GameUI", etc.).

Additionally, if the user's UI actions are located in a map with a different name, only enabling the "UI" map will result in the integration failing to work because the relevant actions will remain disabled. The previous implementation was safer as it enabled the entire asset when a user-provided asset was detected.

Suggested change
m_ShouldDisableUIActionMapOnUnregister = m_UIActionMap != null && !m_UIActionMap.enabled;
m_UIActionMap = m_InputActionAsset?.FindActionMap("UI", false);

🤖 Helpful? 👍/👎

if (m_ShouldDisableUIActionMapOnUnregister)
m_UIActionMap.Enable();
}

void UnregisterAction(ref InputAction action, Action<InputAction.CallbackContext> callback = null)
Expand All @@ -664,8 +663,11 @@ void UnregisterActions()
UnregisterAction(ref m_RightClickAction, OnRightClickPerformed);
UnregisterAction(ref m_ScrollWheelAction, OnScrollWheelPerformed);

if (m_InputActionAsset != null)
m_InputActionAsset.Disable();
if (m_ShouldDisableUIActionMapOnUnregister && m_UIActionMap != null)
m_UIActionMap.Disable();
Copy link
Copy Markdown
Contributor

@u-pr u-pr bot Mar 30, 2026

Choose a reason for hiding this comment

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

medium
There is a logic mismatch between this implementation and the newly added test case Shutdown_DoesNotDisableProjectWideActionsAsset.

In the test, the asset is not enabled initially. RegisterActions will set m_ShouldDisableUIActionMapOnUnregister to true and enable the "UI" map. When UnregisterActions is called, it will then call m_UIActionMap.Disable(). Since "UI" is the only map in the test asset, asset.enabled will become false, causing the assertion at line 121 in InputForUITests.cs to fail.

If the intention is to prevent side-effects on the project-wide asset, you should consider skipping the Disable() call if m_InputActionAsset is the global InputSystem.actions asset.

🤖 Helpful? 👍/👎


m_UIActionMap = null;
m_ShouldDisableUIActionMapOnUnregister = false;
}

void SelectInputActionAsset()
Expand Down
Loading