Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
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
100 changes: 99 additions & 1 deletion Assets/Tests/InputSystem/Plugins/InputForUITests.cs
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,9 +61,13 @@ public override void Setup()
public override void TearDown()
{
EventProvider.Unsubscribe(InputForUIOnEvent);
EventProvider.ClearMockProvider();
if (!m_ClearedMockProvider)
EventProvider.ClearMockProvider();
m_InputForUIEvents.Clear();

InputSystemProvider.SetOnRegisterActions(null);

// InputSystem.actions setter throws in play mode, so we use the internal manager property here.
InputSystem.manager.actions = storedActions;

#if UNITY_EDITOR
Expand Down Expand Up @@ -92,6 +98,98 @@ public void InputSystemActionAssetIsNotNull()
"Test is invalid since InputSystemProvider actions are not available");
}

// Creates a minimal project-wide asset recognised by SelectInputActionAsset().
// At least one action is required: InputActionMap.enabled is m_EnabledActionsCount > 0,
// so an empty map can never report as enabled.
static InputActionAsset CreateProjectWideAssetWithUIMap(out InputActionMap uiMap)
{
var asset = ScriptableObject.CreateInstance<InputActionAsset>();
uiMap = new InputActionMap("UI");
uiMap.AddAction("Point", InputActionType.PassThrough, "<Mouse>/position");
asset.AddActionMap(uiMap);
return asset;
}

[Test]
[Category(kTestCategory)]
public void Shutdown_DoesNotDisableProjectWideActionsAsset()
{
var asset = CreateProjectWideAssetWithUIMap(out var uiMap);

// A non-UI map the user has enabled — provider must never touch it.
var gameplayMap = new InputActionMap("Gameplay");
gameplayMap.AddAction("Jump", InputActionType.Button, "<Keyboard>/space");
asset.AddActionMap(gameplayMap);
gameplayMap.Enable(); // Enable after all maps are added; modifying the asset while any map is enabled is not allowed.

// InputSystem.actions setter throws in play mode, so we use the internal manager property here.
InputSystem.manager.actions = asset;
try
{
m_InputSystemProvider.Initialize();
Assert.That(uiMap.enabled, Is.True, "UI action map should be enabled by provider initialization.");
Assert.That(gameplayMap.enabled, Is.True, "Provider must not change enabled state of non-UI maps.");

EventProvider.ClearMockProvider();
m_ClearedMockProvider = true;

Assert.That(uiMap.enabled, Is.True, "Provider must not disable the UI map in a project-wide asset on shutdown.");
Assert.That(gameplayMap.enabled, Is.True, "Provider must not disable non-UI maps on shutdown.");
}
finally
{
Object.DestroyImmediate(asset);
}
}

[Test]
[Category(kTestCategory)]
public void Shutdown_DoesNotDisableProjectWideUIMap_WhenAlreadyEnabledBeforeInit()
{
var asset = CreateProjectWideAssetWithUIMap(out var uiMap);
uiMap.Enable(); // User had the UI map enabled before the provider started.

// InputSystem.actions setter throws in play mode, so we use the internal manager property here.
InputSystem.manager.actions = asset;
try
{
m_InputSystemProvider.Initialize();
Assert.That(uiMap.enabled, Is.True, "UI action map should remain enabled after provider initialization.");

EventProvider.ClearMockProvider();
m_ClearedMockProvider = true;

// The provider did not enable the map, so it must not disable it on shutdown.
Assert.That(uiMap.enabled, Is.True, "UI action map must remain enabled after provider shutdown when the user had it enabled before initialization.");
}
finally
{
Object.DestroyImmediate(asset);
}
}

[Test]
[Category(kTestCategory)]
public void Shutdown_DisablesUIActionMap_ForProviderOwnedAsset()
{
InputActionMap capturedUIMap = null;
InputSystemProvider.SetOnRegisterActions(asset =>
capturedUIMap = asset?.FindActionMap("UI", false));

// Remove project-wide actions so the provider falls back to its own internal default asset.
// InputSystem.actions setter throws in play mode, so we use the internal manager property here.
InputSystem.manager.actions = null;
m_InputSystemProvider.Initialize();
InputSystemProvider.SetOnRegisterActions(null);

Assert.That(capturedUIMap, Is.Not.Null, "Provider should have a UI action map in its internal default asset.");
Assert.That(capturedUIMap.enabled, Is.True, "UI action map should be enabled by provider initialization.");

EventProvider.ClearMockProvider();
m_ClearedMockProvider = true;
Assert.That(capturedUIMap.enabled, Is.False, "UI action map should be disabled after provider shutdown for provider-owned assets.");
}

[Test]
[Category(kTestCategory)]
// Checks that mouse events are ignored when a touch is active.
Expand Down
1 change: 1 addition & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

### Fixed

- Fixed `InputSystemProvider` disabling project-wide actions on shutdown when UI Toolkit destroys its objects mid-play. The provider now scopes its lifecycle to the UI action map only and does not disable project-wide actions [UUM-134130](https://issuetracker.unity3d.com/product/unity/issues/guid/UUM-134130)
- Fixed `InputActionRebindingExtensions.GetBindingDisplayString(InputAction, InputBinding, ...)` returning an empty string for composite bindings when the binding mask filters by group [UUM-141423](https://issuetracker.unity3d.com/product/unity/issues/guid/UUM-141423)
- Fixed `InputEventPtr.handled` not preventing actions from triggering when switching devices. The default event handled policy has been changed from `SuppressStateUpdates` (now deprecated) to `SuppressActionEventNotifications`, which keeps device state synchronized while suppressing action callbacks for handled events. [ISXB-1097](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1097)
- Fixed all `InputAction.WasXxxThisFrame()` and `WasXxxThisDynamicUpdate()` polling APIs to use per-action suppression tracking instead of a map-wide flag. Previously, when multiple events arrived in the same frame with mixed handled/unhandled states, the last event's suppression state could incorrectly affect all actions in the map. [ISXB-1097](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1097)
Expand Down
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,18 @@ 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)
// Only touch the UI map so we don't change the enabled state of unrelated maps.
m_UIActionMap = m_InputActionAsset?.FindActionMap("UI", false);
if (m_UIActionMap != null && !m_UIActionMap.enabled)
{
// 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();
m_UIActionMap.Enable();

// For provider-owned assets we are responsible for cleanup on shutdown.
// For project-wide actions the play-mode lifecycle manages the asset, so
// leave it as-is when the provider goes away.
if (m_InputActionAsset != InputSystem.actions)
m_ShouldDisableUIActionMapOnUnregister = true;
}
else
m_InputActionAsset.Enable();
}

void UnregisterAction(ref InputAction action, Action<InputAction.CallbackContext> callback = null)
Expand All @@ -664,8 +670,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

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? 👍/👎

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@AswinRajGopal what is the way forward for this open issue?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@ekcoh I've addressed in this commit dc11f07 ;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@u-pr review the changes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The changes look excellent! You've resolved the issue perfectly by:

  1. Scoping the lifecycle specifically to the "UI" action map instead of disabling the entire asset on unregistration.
  2. Differentiating asset ownership: Correctly ensuring that project-wide assets (InputSystem.actions) are left intact during shutdown, while still cleaning up provider-owned assets.
  3. Comprehensive Test Coverage: The three added tests cover all the edge cases beautifully (project-wide asset initially disabled, project-wide asset initially enabled, and provider-owned asset).

Minor Suggestion for Test Robustness

Since s_OnRegisterActions is a static action callback on InputSystemProvider, it could persist across tests if Initialize() throws an exception before the test is able to call InputSystemProvider.SetOnRegisterActions(null).

To prevent potential test-pollution or memory leaks under failure conditions, consider clearing this callback in the test suite's TearDown:

    public override void TearDown()
    {
        EventProvider.Unsubscribe(InputForUIOnEvent);
        if (!m_ClearedMockProvider)
            EventProvider.ClearMockProvider();
        m_InputForUIEvents.Clear();

        InputSystemProvider.SetOnRegisterActions(null);

        // InputSystem.actions setter throws in play mode, so we use the internal manager property here.
        InputSystem.manager.actions = storedActions;

#if UNITY_EDITOR
📚 Additional Context
  • Packages/com.unity.inputsystem/InputSystem/Runtime/Plugins/InputForUI/InputSystemProvider.cs

🤖 Helpful? 👍/👎


m_UIActionMap = null;
m_ShouldDisableUIActionMapOnUnregister = false;
}

void SelectInputActionAsset()
Expand Down
Loading