Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
106 changes: 106 additions & 0 deletions Assets/Tests/InputSystem.Editor/InputTestFixtureTeardownTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
using NUnit.Framework;
using UnityEngine;
using UnityEngine.InputSystem;

/// <summary>
/// Regression tests for IN-107889: InputTestFixture.TearDown() fails to reset Input System state
/// between consecutive scene-based tests.
/// </summary>
/// <remarks>
/// Bug: When an <see cref="InputActionMap"/> belonging to the project-wide <see cref="InputSystem.actions"/>
/// asset is enabled before a test's <see cref="InputTestFixture.Setup"/> runs, the TearDown/Restore
/// cycle leaves the action map disconnected from its saved <see cref="InputActionState"/>.
///
/// Root cause:
/// 1. <c>TestHook_DisableActions()</c> calls <c>Disable()</c> + <c>OnSetupChanged()</c> on the
/// project-wide asset's maps. <c>Disable()</c> modifies the action state memory (phases → Disabled)
/// on the <em>same</em> InputActionState objects that were just saved in the snapshot, because
/// <c>Disable()</c> is called after <c>SaveAndResetState()</c> but references the same managed objects.
/// 2. <c>OnSetupChanged()</c> sets <c>map.m_State = null</c>, disconnecting the map from its state.
/// 3. <c>Restore()</c> restores <c>s_GlobalState</c> (the registry) but does NOT restore the per-map
/// back-references (<c>InputActionMap.m_State</c>, <c>m_MapIndexInState</c>) or the action phase
/// memory.
///
/// The fix:
/// 1. <c>TestHook_DisableActions()</c> should disconnect maps without modifying the saved state's memory,
/// so the saved snapshot retains the correct enabled phases.
/// 2. <c>Restore()</c> should re-link the back-references after <c>RestoreSavedState()</c> and
/// recompute <c>m_EnabledActionsCount</c> from the restored action phases.
/// </remarks>
internal class InputTestFixtureTeardownTests : InputTestFixture
{
// Simulates a scene with a PlayerInput component using project-wide actions.
// Created once before any [SetUp] runs, like a scene's OnEnable().
private InputActionAsset m_PreTestAsset;
private InputActionMap m_PreTestMap;
private InputAction m_PreTestAction;

[OneTimeSetUp]
public void SimulateSceneLoad()
{
// Create an action asset simulating project-wide actions that a scene's
// PlayerInput component would use.
m_PreTestAsset = ScriptableObject.CreateInstance<InputActionAsset>();
m_PreTestAsset.hideFlags = HideFlags.HideAndDontSave;
m_PreTestMap = m_PreTestAsset.AddActionMap("Player");
m_PreTestAction = m_PreTestMap.AddAction("Fire", InputActionType.Button);

// Enable the map - creates an InputActionState and registers it in s_GlobalState.
// This simulates PlayerInput.ActivateInput() running before the test starts.
m_PreTestMap.Enable();

// Register as project-wide actions on the real manager.
// This causes TestHook_DisableActions() to treat our asset like project-wide actions
// (i.e. call Disable() + OnSetupChanged() on it during each Setup()).
InputSystem.s_Manager.actions = m_PreTestAsset;
}

[OneTimeTearDown]
public void VerifyPreTestStatePreserved()
{
// After all test TearDowns, the action map should still be linked to its saved
// InputActionState and the enabled count should reflect the pre-test enabled state.
//
// With the bug: map.m_State is null and map.enabled is false because:
// - TestHook_DisableActions() called Disable() on the map (clearing enabled state
// in the shared state memory) then OnSetupChanged() (setting m_State = null)
// - Restore() restores s_GlobalState but not map.m_State or m_EnabledActionsCount
//
// With the fix: map.m_State is properly re-linked and map.enabled is true because:
// - TestHook_DisableActions() no longer modifies the saved state's memory
// - Restore() calls RelinkRestoredStates() to restore back-references and
// recompute m_EnabledActionsCount from the action phase memory

Assert.That(m_PreTestMap.m_State, Is.Not.Null,
"Action map should be linked to its saved InputActionState after Restore(). " +
"m_State was cleared by OnSetupChanged() during TestHook_DisableActions() " +
"and was not re-linked by Restore().");

Assert.That(m_PreTestMap.enabled, Is.True,
"Action map should be enabled after Restore(): it was enabled before any test ran " +
"and should be restored to that enabled state after TearDown().");

// Clean up
InputSystem.s_Manager.actions = null;
Object.DestroyImmediate(m_PreTestAsset);
}

[Test]
[Order(1)]
public void TearDown_FirstTest_ProjectWideActionsAreReenabledForTest()
{
// During this test, project-wide actions may have been re-enabled by TestHook_EnableActions
// (or left disabled if TestHook_EnableActions is a no-op for the test manager).
// We're just running to trigger a Setup/TearDown cycle.
Assert.Pass("First test ran successfully (triggering Setup/TearDown cycle)");
}

Check warning on line 96 in Assets/Tests/InputSystem.Editor/InputTestFixtureTeardownTests.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Assets/Tests/InputSystem.Editor/InputTestFixtureTeardownTests.cs#L96

Added line #L96 was not covered by tests

[Test]
[Order(2)]
public void TearDown_SecondTest_StateRemainsCorrectAfterSecondCycle()
{
// After the first test's TearDown() + this Setup(), verify setup completes without errors.
// The [OneTimeTearDown] contains the actual assertion for the post-Restore() state.
Assert.Pass("Second test ran successfully (triggering second Setup/TearDown cycle)");
}

Check warning on line 105 in Assets/Tests/InputSystem.Editor/InputTestFixtureTeardownTests.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Assets/Tests/InputSystem.Editor/InputTestFixtureTeardownTests.cs#L105

Added line #L105 was not covered by tests
}
84 changes: 84 additions & 0 deletions Assets/Tests/InputSystem/TouchscreenMultiDisplayTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
using NUnit.Framework;
using UnityEngine;
using UnityEngine.InputSystem;
using UnityEngine.InputSystem.LowLevel;
using TouchPhase = UnityEngine.InputSystem.TouchPhase;

// Tests covering touch tracking across multiple physical touchscreen monitors.
// Regression coverage for IN-108611: touches from one screen incorrectly matching
// ongoing touches from another screen when both screens report the same touchId.
[TestFixture]
internal class TouchscreenMultiDisplayTests : CoreTestsFixture
{
// When two physical touchscreens both have an active touch with the same touchId,
// a Move event from screen 2 must update screen 2's touch slot, not screen 1's.
//
// Failure mode (pre-fix): OnStateEvent matches ongoing touches by touchId alone,
// so screen 2's Move event finds screen 1's slot first (both have touchId=1) and
// incorrectly updates it, leaving screen 1's position changed and screen 2 stale.
[Test]
[Category("Devices")]
public void Devices_TouchMoveOnSecondDisplay_DoesNotUpdateTouchOnFirstDisplay()
{
var device = InputSystem.AddDevice<Touchscreen>();

// Finger down on display 0 (touchId=1).
InputSystem.QueueStateEvent(device, new TouchState
{
phase = TouchPhase.Began,
touchId = 1,
position = new Vector2(100, 100),
displayIndex = 0,
});

// Finger down on display 1 — same touchId, different screen.
InputSystem.QueueStateEvent(device, new TouchState
{
phase = TouchPhase.Began,
touchId = 1,
position = new Vector2(200, 200),
displayIndex = 1,
});

InputSystem.Update();

// Both touches should be allocated to separate slots.
Assert.That(device.touches[0].phase.ReadValue(), Is.EqualTo(TouchPhase.Began));
Assert.That(device.touches[1].phase.ReadValue(), Is.EqualTo(TouchPhase.Began));

var display0SlotIndex = -1;
var display1SlotIndex = -1;
for (var i = 0; i < 2; i++)
{
var displayIdx = device.touches[i].displayIndex.ReadValue();
if (displayIdx == 0) display0SlotIndex = i;
else if (displayIdx == 1) display1SlotIndex = i;
}

Assert.That(display0SlotIndex, Is.Not.EqualTo(-1), "No touch slot found for display 0");
Assert.That(display1SlotIndex, Is.Not.EqualTo(-1), "No touch slot found for display 1");

var display0PositionBefore = device.touches[display0SlotIndex].position.ReadValue();

// Swipe on display 1 (same touchId=1 as display 0's held touch).
InputSystem.QueueStateEvent(device, new TouchState
{
phase = TouchPhase.Moved,
touchId = 1,
position = new Vector2(300, 300),
displayIndex = 1,
});

InputSystem.Update();

// Display 0's touch must be unchanged.
Assert.That(device.touches[display0SlotIndex].position.ReadValue(), Is.EqualTo(display0PositionBefore),
"Touch on display 0 was incorrectly updated by a Move event from display 1");

// Display 1's touch must reflect the new position.
Assert.That(device.touches[display1SlotIndex].position.ReadValue(), Is.EqualTo(new Vector2(300, 300)),
"Touch on display 1 was not updated by its own Move event");

Assert.That(device.touches[display1SlotIndex].phase.ReadValue(), Is.EqualTo(TouchPhase.Moved));
}
}
2 changes: 2 additions & 0 deletions Assets/Tests/InputSystem/TouchscreenMultiDisplayTests.cs.meta

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -4532,6 +4532,64 @@
return savedState;
}

/// <summary>
/// After <see cref="InputTestStateManager.Restore"/> restores the global registry via
/// <c>RestoreSavedState()</c>, the per-map back-references on each <see cref="InputActionMap"/>
/// (<c>m_State</c>, <c>m_MapIndexInState</c>) and the per-action index
/// (<c>InputAction.m_ActionIndexInState</c>) may be stale because they were cleared by
/// <c>Destroy()</c> during <c>StaticDisposeCurrentState()</c>. This method re-links those
/// references from the restored <see cref="s_GlobalState"/> and recomputes
/// <c>m_EnabledActionsCount</c> from the action phase memory so that maps and actions
/// correctly reflect their pre-test enabled state. See IN-107889.
/// </summary>
internal static void RelinkRestoredStates()
{
var count = s_GlobalState.globalList.length;
for (var i = 0; i < count; ++i)
{
var handle = s_GlobalState.globalList[i];
if (!handle.IsAllocated)
continue;

Check warning on line 4552 in Packages/com.unity.inputsystem/InputSystem/Runtime/Actions/InputActionState.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Runtime/Actions/InputActionState.cs#L4552

Added line #L4552 was not covered by tests
if (handle.Target is InputActionState state)
state.RelinkMapsAndRecomputeEnabledCount();
}
}

private unsafe void RelinkMapsAndRecomputeEnabledCount()
{
for (var mapIndex = 0; mapIndex < totalMapCount; ++mapIndex)
{
var map = maps[mapIndex];
if (map == null)
continue;

Check warning on line 4564 in Packages/com.unity.inputsystem/InputSystem/Runtime/Actions/InputActionState.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Runtime/Actions/InputActionState.cs#L4564

Added line #L4564 was not covered by tests

map.m_State = this;
map.m_MapIndexInState = mapIndex;

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.

high

Since TestHook_DisableActions() now explicitly nulls out m_SharedStateForAllMaps on the project-wide action asset, RelinkRestoredStates() needs to restore it here.

Without this, the restored action maps will successfully reconnect their m_State, but their parent InputActionAsset will be left with a null m_SharedStateForAllMaps. This breaks asset-level reactivity: methods like InputActionAsset.bindingMask and InputActionAsset.devices setters rely on ReResolveIfNecessary(), which returns early and does nothing if m_SharedStateForAllMaps == null. As a result, subsequent tests that try to apply overrides to the project-wide asset would see those changes silently ignored.

You can safely restore it using the map's asset reference:

Suggested replacement omitted because it is too large for an inline review comment.

🤖 Helpful? 👍/👎 by bug_hunter_agent


if (map.m_Asset != null && map.m_Asset.m_SharedStateForAllMaps == null)
map.m_Asset.m_SharedStateForAllMaps = this;

var indices = mapIndices[mapIndex];
var mapActions = map.m_Actions;
if (mapActions != null)
{
for (var k = 0; k < indices.actionCount; ++k)
mapActions[k].m_ActionIndexInState = indices.actionStartIndex + k;
}

// Recompute m_EnabledActionsCount from the restored action phase memory.
// This correctly reflects enabled/disabled state without any explicit Disable()
// call having been made (see TestHook_DisableActions changes for IN-107889).
var enabledCount = 0;
for (var k = 0; k < indices.actionCount; ++k)
{
if (!actionStates[indices.actionStartIndex + k].isDisabled)
++enabledCount;
}
map.m_EnabledActionsCount = enabledCount;
}
}

private void AddToGlobalList()
{
CompactGlobalList();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ protected override void FinishSetup()
var touchId = newTouchState.touchId;
for (var i = 0; i < touchControlCount; ++i)
{
if (currentTouchState[i].touchId == touchId)
if (currentTouchState[i].touchId == touchId && currentTouchState[i].displayIndex == newTouchState.displayIndex)
{
// Preserve primary touch state.
var isPrimaryTouch = currentTouchState[i].isPrimaryTouch;
Expand Down Expand Up @@ -915,7 +915,7 @@ unsafe bool IInputStateCallbackReceiver.GetStateOffsetForEvent(InputControl cont
for (var i = 0; i < touchControlCount; ++i)
{
var touch = &currentTouchState[i];
if (touch->touchId == eventTouchId || (!touch->isInProgress && eventTouchPhase.IsActive()))
if ((touch->touchId == eventTouchId && touch->displayIndex == eventTouchState->displayIndex) || (!touch->isInProgress && eventTouchPhase.IsActive()))
{
offset = primaryTouch.m_StateBlock.byteOffset + primaryTouch.m_StateBlock.alignedSizeInBytes - m_StateBlock.byteOffset +
(uint)(i * UnsafeUtility.SizeOf<TouchState>());
Expand Down Expand Up @@ -1002,7 +1002,7 @@ internal static unsafe bool MergeForward(InputEventPtr currentEventPtr, InputEve
var currentState = (TouchState*)currentEvent->state;
var nextState = (TouchState*)nextEvent->state;

if (currentState->touchId != nextState->touchId || currentState->phaseId != nextState->phaseId || currentState->flags != nextState->flags)
if (currentState->touchId != nextState->touchId || currentState->phaseId != nextState->phaseId || currentState->flags != nextState->flags || currentState->displayIndex != nextState->displayIndex)
return false;

nextState->delta += currentState->delta;
Expand Down
43 changes: 41 additions & 2 deletions Packages/com.unity.inputsystem/InputSystem/Runtime/InputSystem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3518,9 +3518,48 @@ internal static void InitializeInPlayer(IInputRuntime runtime = null, bool loadS
#if UNITY_INCLUDE_TESTS
internal static void TestHook_DisableActions()
{
DisableActions(triggerSetupChanged: true);
if (s_Manager != null)
// Disconnect the project-wide action maps from their InputActionState without
// calling Disable(), which would corrupt the saved state snapshot.
//
// The normal DisableActions() path calls Disable() then OnSetupChanged() on the
// project-wide asset. Disable() modifies action phase memory on the *same* managed
// InputActionState objects that were already captured in the SaveAndResetState()
// snapshot (GCHandles point to the live objects; no deep copy is made). This means
// the restored snapshot has disabled phases even though the maps were enabled when
// the snapshot was taken, causing Restore() to leave maps incorrectly disabled.
//
// Instead, we just null out the map back-references and let Restore() re-link them.
// Control monitors registered through the old runtime don't need unsubscribing here
// because the test manager installs a new runtime; the old runtime is never polled
// during the test. See IN-107889.
var projectWideActions = s_Manager?.actions;
if (projectWideActions != null)
{
DisconnectActionMaps(projectWideActions);
s_Manager.actions = null;
}

// Also disconnect the configured project-wide asset even if manager.actions is null.
// RelinkRestoredStates() restores m_EnabledActionsCount on maps after Restore(). If a
// subsequent test's manager.actions is null (e.g. a previous OneTimeTearDown cleared it),
// TestHook_DisableActions would be a no-op and those maps would keep m_State set and
// m_EnabledActionsCount == m_Actions.Length. InputActionMap.Enable() would then
// early-return thinking all actions are already enabled, but the state is not in
// s_GlobalState (which was cleared by SaveAndResetState()). See IN-107889.
var configuredActions = InputManager.s_GetProjectWideActions?.Invoke();
if (configuredActions != null && configuredActions != projectWideActions)
DisconnectActionMaps(configuredActions);
}

private static void DisconnectActionMaps(InputActionAsset asset)
{
foreach (var map in asset.actionMaps)
{
map.m_State = null;
map.m_MapIndexInState = InputActionState.kInvalidIndex;
map.m_EnabledActionsCount = 0;
}
asset.m_SharedStateForAllMaps = null;
}

internal static void TestHook_EnableActions()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,9 @@ protected void OnEnable()
if (m_TouchIds == null)
m_TouchIds = new int[simulatedTouchscreen.touches.Count];

if (m_TouchDisplayIndices == null)
m_TouchDisplayIndices = new byte[simulatedTouchscreen.touches.Count];

foreach (var device in InputSystem.devices)
OnDeviceChange(device, InputDeviceChange.Added);

Expand Down Expand Up @@ -306,10 +309,12 @@ private unsafe void UpdateTouch(int touchIndex, int pointerIndex, TouchPhase pha
touch.startPosition = position;
touch.touchId = ++m_LastTouchId;
m_TouchIds[touchIndex] = m_LastTouchId;
m_TouchDisplayIndices[touchIndex] = displayIndex;
}
else
{
touch.touchId = m_TouchIds[touchIndex];
touch.displayIndex = m_TouchDisplayIndices[touchIndex];
}

//NOTE: Processing these events still happen in the current frame.
Expand All @@ -327,6 +332,7 @@ private unsafe void UpdateTouch(int touchIndex, int pointerIndex, TouchPhase pha
[NonSerialized] private int[] m_CurrentDisplayIndices;
[NonSerialized] private ButtonControl[] m_Touches;
[NonSerialized] private int[] m_TouchIds;
[NonSerialized] private byte[] m_TouchDisplayIndices;

[NonSerialized] private int m_LastTouchId;
[NonSerialized] private Action<InputDevice, InputDeviceChange> m_OnDeviceChange;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ public void Restore()
state.inputUserState.RestoreSavedState();
state.touchState.RestoreSavedState();
state.inputActionState.RestoreSavedState();
// Re-link per-map/per-action back-references that were cleared during
// StaticDisposeCurrentState(). Also recomputes m_EnabledActionsCount
// from the restored action phase memory. See IN-107889.
InputActionState.RelinkRestoredStates();

InputSystemTestHooks.TestHook_RestoreFromSavedState(state.manager, state.remote, state.remoteConnection);
InputUpdate.Restore(state.managerState.updateState);
Expand Down
Loading