CHANGE: Re-enable temporarily disabled tests due to ProjectWide actions (ISX-1455) (#1745)

* re-enable UI testts as they are no longer failing * remove null entries in the projects preloaded assets * Update comment on disabling actions in Test after investigation * Fix assert during binding resolution when binding is active * Re-enable tests failing due to rebinding issue * Add a comment for every case we disable project-wide actions in tests * ProjectWide actions don't require EditorBuildSettings for storage
2023-09-11 09:56:36 +02:00 · 2023-09-11 09:56:36 +02:00 · baec6373dc
--- a/Assets/Tests/InputSystem/CoreTests_Actions.cs
+++ b/Assets/Tests/InputSystem/CoreTests_Actions.cs
@ -529,7 +529,7 @@ partial class CoreTests
    {
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
-        InputSystem.actions?.Disable();
+        InputSystem.actions?.Disable(); // Prevent these actions appearing in the `InputActionTrace`
 #endif

        var gamepad = InputSystem.AddDevice<Gamepad>();
@ -1337,6 +1337,59 @@ partial class CoreTests
        Assert.That(action.controls, Is.EquivalentTo(new InputControl[] {gamepad.leftStick, mouse.delta}));
    }

+    // ReSharper disable once ClassNeverInstantiated.Local
+    private class ConstantFloatTestProcessor : InputProcessor<float>
+    {
+        public const float EXPECTED_VALUE = 0.5f;
+        public override float Process(float value, InputControl control)
+        {
+            return EXPECTED_VALUE;
+        }
+    }
+
+    [Test]
+    [Category("Actions")]
+    public void Actions_ActiveBindingsHaveCorrectBindingIndicesAfterBindingResolution()
+    {
+        // Attaching a processor to a particular binding is how
+        // we test that we are correctly referencing the same binding
+        // from before and after the binding resolution.
+        // This is a tricky issue to tease out. Testing with just ReadValue(),
+        // with the control (i.e. mouse.leftButton) or with action callbacks
+        // could all appear correct because those don't actually use bindingIndex.
+        // This issue originally manifested itself as an assert in another place in the code.
+        InputSystem.RegisterProcessor<ConstantFloatTestProcessor>();
+
+        // This test is sensitive to binding order.
+        // It's important that the active binding is not in the first
+        // position of the action (i.e. not at the default index).
+        var map = new InputActionMap("map");
+        var action = map.AddAction("action1", binding: "<Gamepad>/buttonSouth");
+        action.AddBinding("<Mouse>/leftButton").WithProcessor<ConstantFloatTestProcessor>(); // binding in 2nd position.
+        map.Enable();
+
+        var mouse = InputSystem.AddDevice<Mouse>();
+        Assert.That(action.activeControl, Is.Null);
+        Assert.That(action.ReadValue<float>(), Is.EqualTo(0));
+
+        // Put the action into an active state using the 2nd binding.
+        Press(mouse.leftButton);
+        Assert.That(action.activeControl, Is.SameAs(mouse.leftButton));
+        Assert.That(action.ReadValue<float>(), Is.EqualTo(ConstantFloatTestProcessor.EXPECTED_VALUE));
+
+        // Perform binding resolution while the action is active.
+        var gamepad = InputSystem.AddDevice<Gamepad>();
+        Assert.That(action.activeControl, Is.SameAs(mouse.leftButton));
+
+        // Check the active control is still triggered by the 2nd binding (with the processor attached).
+        Assert.That(action.ReadValue<float>(), Is.EqualTo(ConstantFloatTestProcessor.EXPECTED_VALUE));
+
+        // Perform binding resolution again.
+        // This would cause an assert if the binding indices were incorrect
+        // as the binding resolver would choke on the bad data at this point.
+        InputSystem.RemoveDevice(gamepad);
+    }
+
    [Test]
    [Category("Actions")]
    public void Actions_CanDisableAndEnableOtherAction_FromCallback()
@ -2049,7 +2102,7 @@ partial class CoreTests
    {
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
-        InputSystem.actions?.Disable();
+        InputSystem.actions?.Disable(); // Prevent these actions appearing in the `InputActionTrace`
 #endif

        var asset = ScriptableObject.CreateInstance<InputActionAsset>();
@ -2303,7 +2356,7 @@ partial class CoreTests
    {
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
-        InputSystem.actions?.Disable();
+        InputSystem.actions?.Disable(); // Prevent these actions appearing in the `InputActionTrace`
 #endif

        var gamepad = InputSystem.AddDevice<Gamepad>();
@ -2595,7 +2648,7 @@ partial class CoreTests
    {
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
-        InputSystem.actions?.Disable();
+        InputSystem.actions?.Disable(); // Prevent these actions appearing in the `InputActionTrace`
 #endif

        var keyboard = InputSystem.AddDevice<Keyboard>();
@ -3120,7 +3173,7 @@ partial class CoreTests
    {
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
-        InputSystem.actions?.Disable();
+        InputSystem.actions?.Disable(); // Prevent these actions appearing in the `InputActionTrace`
 #endif

        var gamepad = InputSystem.AddDevice<Gamepad>();
@ -3568,7 +3621,7 @@ partial class CoreTests
    {
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
-        InputSystem.actions?.Disable();
+        InputSystem.actions?.Disable(); // Remove from `ListEnabledActions`
 #endif

        var action = new InputAction(binding: "<Gamepad>/leftStick");
@ -3816,7 +3869,7 @@ partial class CoreTests
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
        InputSystem.actions?.Disable();
-        InputActionState.DestroyAllActionMapStates(); // Also remove controls so the changed control count is accurate
+        InputActionState.DestroyAllActionMapStates(); // Required for `onActionChange` to report correct number of changes
 #endif

        var gamepad = InputSystem.AddDevice<Gamepad>();
@ -4708,7 +4761,7 @@ partial class CoreTests
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
        InputSystem.actions?.Disable();
-        InputActionState.DestroyAllActionMapStates(); // Also remove controls so the changed control count is accurate
+        InputActionState.DestroyAllActionMapStates(); // Required for `onActionChange` to report correct number of changes
 #endif

        var gamepad = InputSystem.AddDevice<Gamepad>();
@ -4856,8 +4909,8 @@ partial class CoreTests
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
        InputSystem.actions?.Disable();
-        InputActionState.DestroyAllActionMapStates(); // Also remove controls so the changed control count is accurate
-#endif
+        InputActionState.DestroyAllActionMapStates(); // Required for `onActionChange` to report correct number of changes
+ #endif

        var enabledAction = new InputAction("enabledAction", binding: "<Gamepad>/leftTrigger");

@ -4918,7 +4971,7 @@ partial class CoreTests
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
        InputSystem.actions?.Disable();
-        InputActionState.DestroyAllActionMapStates(); // Also remove controls so the changed control count is accurate
+        InputActionState.DestroyAllActionMapStates(); // Required for `onActionChange` to report correct number of changes
 #endif

        var actionMap = new InputActionMap("map");
@ -4949,8 +5002,8 @@ partial class CoreTests
    {
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
-        InputSystem.actions?.Disable();  // Worked in Standalone
-        InputActionState.DestroyAllActionMapStates(); // Also remove controls so the changed control count is accurate
+        InputSystem.actions?.Disable();
+        InputActionState.DestroyAllActionMapStates(); // Required for `onActionChange` to report correct number of changes
 #endif

        var asset = ScriptableObject.CreateInstance<InputActionAsset>();
@ -5020,12 +5073,6 @@ partial class CoreTests
    [Category("Actions")]
    public void Actions_WhenControlsUpdate_InProgressActionsKeepGoing()
    {
-#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
-        // @TODO: This should not need disabled for this test (https://jira.unity3d.com/browse/ISX-1455)
-        // Causes: "[Assert] Could not find active control after binding resolution"
-        // when RemoveDevice() called
-        InputSystem.actions.Disable();
-#endif
        currentTime = 0;
        var gamepad = InputSystem.AddDevice<Gamepad>();

@ -5145,7 +5192,7 @@ partial class CoreTests
    {
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
-        InputSystem.actions?.Disable();
+        InputSystem.actions?.Disable(); // Remove from `ListEnabledActions`
 #endif

        var action1 = new InputAction(name: "a");
@ -9789,7 +9836,7 @@ partial class CoreTests
    {
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
-        InputSystem.actions?.Disable();
+        InputSystem.actions?.Disable(); // Prevent these actions appearing in the `InputActionTrace`
 #endif

        var touchscreen = InputSystem.AddDevice<Touchscreen>();
@ -9864,7 +9911,7 @@ partial class CoreTests
    {
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
-        InputSystem.actions?.Disable();
+        InputSystem.actions?.Disable(); // Prevent these actions appearing in the `InputActionTrace`
 #endif

        // Give us known parameters for tap detection.
--- a/Assets/Tests/InputSystem/CoreTests_Actions_Interactions.cs
+++ b/Assets/Tests/InputSystem/CoreTests_Actions_Interactions.cs
@ -753,7 +753,7 @@ internal partial class CoreTests
    {
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
-        InputSystem.actions.Disable();
+        InputSystem.actions?.Disable(); // Prevent these actions appearing in the `InputActionTrace`
 #endif

        var gamepad = InputSystem.AddDevice<Gamepad>();
--- a/Assets/Tests/InputSystem/CoreTests_Controls.cs
+++ b/Assets/Tests/InputSystem/CoreTests_Controls.cs
@ -478,10 +478,14 @@ partial class CoreTests
    [Category("Controls")]
    public void Controls_ValueCachingWorksAcrossEntireDeviceMemoryRange()
    {
-        // @TODO: This should not need disabled for this test (https://jira.unity3d.com/browse/ISX-1455)
-        // Somehow the presence of action controls prevents those controls being marked as stale
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
-        InputSystem.actions.Disable();
+        // Exclude project-wide actions from this test
+        // The presence of any enabled actions means we have installed StateChangeMonitors
+        // which interferes with this test. Essentially when we update the device state
+        // and invalidate the cache (make it stale), immediately afterwards we
+        // call NotifyControlStateChanged for each of the actions which _may_ cause a Read()
+        // on the control and make it immediately cached (non-stale) again.
+        InputSystem.actions?.Disable();
 #endif

        var keyboard = InputSystem.AddDevice<Keyboard>();
--- a/Assets/Tests/InputSystem/CoreTests_Devices.cs
+++ b/Assets/Tests/InputSystem/CoreTests_Devices.cs
@ -4100,7 +4100,7 @@ partial class CoreTests
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
        // Exclude project-wide actions from this test
        // Prevent GC Allocations happening later in test
-        InputSystem.actions.Disable();
+        InputSystem.actions?.Disable();
        InputActionState.DestroyAllActionMapStates();
 #endif

@ -4390,12 +4390,6 @@ partial class CoreTests
    [TestCase(false, InputSettings.BackgroundBehavior.ResetAndDisableNonBackgroundDevices, InputSettings.EditorInputBehaviorInPlayMode.AllDeviceInputAlwaysGoesToGameView)]
    public unsafe void Devices_CanHandleFocusChanges(bool appRunInBackground, InputSettings.BackgroundBehavior backgroundBehavior, InputSettings.EditorInputBehaviorInPlayMode editorInputBehaviorInPlayMode)
    {
-#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
-        // @TODO: This should not need disabled for this test (https://jira.unity3d.com/browse/ISX-1455)
-        // Causes: "[Assert] Could not find active control after binding resolution"
-        // due to: mouse3 = InputSystem.AddDevice<Mouse>();
-        InputSystem.actions.Disable();
-#endif
        // The constant leads to "Unreachable code detected" warnings.
        #pragma warning disable CS0162

@ -5363,16 +5357,6 @@ partial class CoreTests
    [Category("Devices")]
    public void Devices_RemovingDevice_MakesNextDeviceOfTypeCurrent()
    {
-#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
-        // @TODO: This should not need disabled for this test (https://jira.unity3d.com/browse/ISX-1455)
-        // Causes: "[Assert] Could not find active control after binding resolution"
-        // during  point where Pointer is removed
-        // - InputActionState.OnDeviceChange(device, InputDeviceChange.Removed);
-        // - LazyResolveBindings(bool fullResolve)
-        // - ResolveBindings()
-        // - RestoreActionStatesAfterReResolvingBindings(UnmanagedMemory oldState, InputControlList<InputControl> activeControls, bool isFullResolve)
-        InputSystem.actions.Disable();
-#endif
        var mouse = InputSystem.AddDevice<Mouse>();
        Press(mouse.leftButton);
        Assert.That(Pointer.current, Is.EqualTo(mouse));
--- a/Assets/Tests/InputSystem/CoreTests_Editor.cs
+++ b/Assets/Tests/InputSystem/CoreTests_Editor.cs
@ -2838,10 +2838,11 @@ partial class CoreTests
    public void Editor_LeavingPlayMode_DestroysAllActionStates()
    {
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
-        // With Project-wide Actions `InputSystem.actions`, we begin with some initial ActionState
        // Exclude project-wide actions from this test
+        // With Project-wide Actions `InputSystem.actions`, we begin with some initial ActionState
+        // Disabling Project-wide actions so that we begin from zero.
        Assert.That(InputActionState.s_GlobalState.globalList.length, Is.EqualTo(1));
-        InputSystem.actions.Disable();
+        InputSystem.actions?.Disable();
        InputActionState.DestroyAllActionMapStates();
 #endif

--- a/Assets/Tests/InputSystem/CoreTests_ProjectWideActions.cs
+++ b/Assets/Tests/InputSystem/CoreTests_ProjectWideActions.cs
@ -163,7 +163,7 @@ internal partial class CoreTests
        Assert.That(enabledActions, Has.Exactly(1).SameAs(action));

        // Disabling works
-        InputSystem.actions.Disable();
+        InputSystem.actions?.Disable();
        enabledActions = InputSystem.ListEnabledActions();
        Assert.That(enabledActions, Has.Count.EqualTo(1));
        Assert.That(enabledActions, Has.Exactly(1).SameAs(action));
--- a/Assets/Tests/InputSystem/Plugins/UITests.cs
+++ b/Assets/Tests/InputSystem/Plugins/UITests.cs
@ -3231,8 +3231,6 @@ internal class UITests : CoreTestsFixture
        Assert.That(scene.eventSystem.currentSelectedGameObject, Is.SameAs(scene.leftGameObject));
    }

-// @TODO: This should not need disabled for this test (https://jira.unity3d.com/browse/ISX-1455)
-#if !UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS || UNITY_EDITOR
    [UnityTest]
    [Category("UI")]
    public IEnumerator UI_WhenBindingsAreReResolved_PointerStatesAreKeptInSync()
@ -3283,8 +3281,6 @@ internal class UITests : CoreTestsFixture
        Assert.That(EventSystem.current.IsPointerOverGameObject(), Is.True);
    }

-#endif
-
    ////REVIEW: While `deselectOnBackgroundClick` does solve the problem of breaking keyboard and gamepad navigation, the question
    ////        IMO is whether navigation should even be affected that way by not having a current selection. Seems to me that the
    ////        the system should remember the last selected object and start up navigation from there when nothing is selected.
@ -3800,8 +3796,6 @@ internal class UITests : CoreTestsFixture
        Assert.That(clicked, Is.True);
    }

-// @TODO: This should not need disabled for this test (https://jira.unity3d.com/browse/ISX-1455)
-#if !UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS || UNITY_EDITOR
    [UnityTest]
    [Category("UI")]
    public IEnumerator UI_WhenCursorIsLockedToScreenCenter_PointerEnterAndExitEventsFire()
@ -3834,8 +3828,6 @@ internal class UITests : CoreTestsFixture
        Assert.That(callbackReceiver.events.Any(e => e.type == EventType.PointerExit), Is.True);
    }

-#endif
-
    #region Multi Display Tests
 #if UNITY_2022_3_OR_NEWER // displayIndex is only available from 2022.3 onwards

--- a/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs
+++ b/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs
@ -551,6 +551,7 @@ namespace UnityEngine.InputSystem
                newActionState.pressedInUpdate = oldActionState.pressedInUpdate;
                newActionState.releasedInUpdate = oldActionState.releasedInUpdate;
                newActionState.startTime = oldActionState.startTime;
+                newActionState.bindingIndex = oldActionState.bindingIndex;

                if (oldActionState.phase != InputActionPhase.Disabled)
                {
@ -628,6 +629,7 @@ namespace UnityEngine.InputSystem
                    // so we can simply look on the binding for where the control is now.
                    var newControlIndex = FindControlIndexOnBinding(bindingIndex, control);

+                    // This assert is used by test: Actions_ActiveBindingsHaveCorrectBindingIndicesAfterBindingResolution
                    Debug.Assert(newControlIndex != kInvalidIndex, "Could not find active control after binding resolution");
                    if (newControlIndex != kInvalidIndex)
                    {
--- a/Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsBuildProvider.cs
+++ b/Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsBuildProvider.cs
@ -19,14 +19,12 @@ namespace UnityEngine.InputSystem.Editor
            var preloadedAssets = PlayerSettings.GetPreloadedAssets();

 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
-            EditorBuildSettings.TryGetConfigObject(InputSettingsProvider.kEditorBuildSettingsActionsConfigKey,
-                out m_ProjectWideActions);
-
+            m_ProjectWideActions = Editor.ProjectWideActionsAsset.GetOrCreate();
            if (m_ProjectWideActions != null)
            {
-                if (!preloadedAssets.Contains(InputSystem.actions))
+                if (!preloadedAssets.Contains(m_ProjectWideActions))
                {
-                    ArrayHelpers.Append(ref preloadedAssets, InputSystem.actions);
+                    ArrayHelpers.Append(ref preloadedAssets, m_ProjectWideActions);
                    PlayerSettings.SetPreloadedAssets(preloadedAssets);
                }
            }
--- a/Packages/com.unity.inputsystem/InputSystem/InputSystem.cs
+++ b/Packages/com.unity.inputsystem/InputSystem/InputSystem.cs
@ -3032,13 +3032,7 @@ namespace UnityEngine.InputSystem
                    return s_projectWideActions;

                #if UNITY_EDITOR
-                // Load the InputActionsAsset and store it in EditorBuildSettings so it can be packed in Player builds
                s_projectWideActions = Editor.ProjectWideActionsAsset.GetOrCreate();
-                if (!string.IsNullOrEmpty(AssetDatabase.GetAssetPath(s_projectWideActions)))
-                {
-                    EditorBuildSettings.AddConfigObject(InputSettingsProvider.kEditorBuildSettingsActionsConfigKey,
-                        s_projectWideActions, true);
-                }
                #else
                s_projectWideActions = Resources.FindObjectsOfTypeAll<InputActionAsset>().FirstOrDefault(o => o != null && o.name == kProjectWideActionsAssetName);
                #endif
@ -3056,14 +3050,6 @@ namespace UnityEngine.InputSystem
                if (s_projectWideActions == value)
                    return;

-                #if UNITY_EDITOR
-                if (!string.IsNullOrEmpty(AssetDatabase.GetAssetPath(value)))
-                {
-                    EditorBuildSettings.AddConfigObject(InputSettingsProvider.kEditorBuildSettingsActionsConfigKey,
-                        value, true);
-                }
-                #endif
-
                s_projectWideActions?.Disable();
                s_projectWideActions = value;
                s_projectWideActions.Enable();
--- a/ProjectSettings/ProjectSettings.asset
+++ b/ProjectSettings/ProjectSettings.asset
@ -120,8 +120,6 @@ PlayerSettings:
    Others: 1
  bundleVersion: 1.0
  preloadedAssets:
-  - {fileID: 0}
-  - {fileID: 0}
  metroInputSource: 0
  wsaTransparentSwapchain: 0
  m_HolographicPauseOnTrackingLoss: 1