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
Open

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

Show file tree
Hide file tree
Changes from 4 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.s_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
2 changes: 1 addition & 1 deletion 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 @@ -664,7 +664,7 @@ void UnregisterActions()
UnregisterAction(ref m_RightClickAction, OnRightClickPerformed);
UnregisterAction(ref m_ScrollWheelAction, OnScrollWheelPerformed);

if (m_InputActionAsset != null)
if (m_InputActionAsset != null && m_InputActionAsset != InputSystem.actions)
Copy link
Copy Markdown
Collaborator

@jfreire-unity jfreire-unity Mar 6, 2026

Choose a reason for hiding this comment

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

Do we actually have to disable the actions? I'm not a huge fan of disabling this for a particular asset, like project wide actions. I also think the logic we implemented in the past might not be the most correct.

I believe we should always make sure that the UI action map is enabled when registering the actions. Do you know what's the impact of not disabling this asset for a normal asset?

Copy link
Copy Markdown
Collaborator Author

@AswinRajGopal AswinRajGopal Mar 9, 2026

Choose a reason for hiding this comment

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

@jfreire-unity Thanks for the review, I agree that the current change only addresses the reported bug. I’ll improve the fix so that we explicitly ensure the UI action map is enabled when registering actions, and on unregister we only clean up state owned by the provider instead of disabling shared asset state.

m_InputActionAsset.Disable();
}

Expand Down