FIX: Add workaround for cases where button presses can be missed (ISXB-491) (#1935)

* FIX: Add workaround for cases where button presses can be missed (ISXB-491)

* Update button press change to work without cached value feature flag.

* Further improvements new tests, account for separate Editor input state.

* Update changelog; expand button press testing

* Missed format check

* Address PR comments

* Add flags so that additional processing only happens when users actually need it.

* Fix up tests, most crucially setting relevant states when wasPressed/Released are first called.

* Fix for 2019.4 being very, very old (HashSet capacity support).

* Reduce test repetition to 250 as it was taking too long. Should still give plenty of info.

* Only loop through buttons that need testing. Exclude tvOS from some tests where it times out for being too slow.

* Fix up after merge + formatting.

* Try boosting timeout value on tvOS to avoid test failures.

* Undo change, can't be sure it's relevant due to other ongoing tvOS issues.

* Exclude these other tests on tvOS as they seem to really trip up connection timeouts.

Author: graham-huws

.github/pull_request_template.md

@@ -21,7 +21,7 @@ Before review:
 - [ ] Changelog entry added.
     - Explains the change in `Changed`, `Fixed`, `Added` sections.
     - For API change contains an example snippet and/or migration example.
-    - JIRA ticket linked, example ([case %<ID>%](https://issuetracker.unity3d.com/product/unity/issues/guid/<ID>)). If it is a private issue, just add the case ID without a link.	
+    - JIRA ticket linked, example ([case %<ID>%](https://issuetracker.unity3d.com/product/unity/issues/guid/<ID>)). If it is a private issue, just add the case ID without a link.
     - Jira port for the next release set as "Resolved".
 - [ ] Tests added/changed, if applicable.
     - Functional tests `Area_CanDoX`, `Area_CanDoX_EvenIfYIsTheCase`, `Area_WhenIDoX_AndYHappens_ThisIsTheResult`.
@@ -44,4 +44,4 @@ During merge:
 
 After merge:
 
-- [ ] Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444. 
+- [ ] Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

Assets/Tests/InputSystem/CorePerformanceTests.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Collections.Generic;
 using UnityEngine.InputSystem;
 using UnityEngine.InputSystem.LowLevel;
 using NUnit.Framework;
@@ -10,6 +11,7 @@
 using UnityEngine.InputSystem.Layouts;
 using UnityEngine.InputSystem.Users;
 using UnityEngine.InputSystem.Utilities;
+using UnityEngine.TestTools;
 
 ////TODO: add test for domain reload logic
 
@@ -719,6 +721,94 @@ public void Performance_OptimizedControls_ReadAndUpdateGamepadNewValuesEveryFram
             .Run();
     }
 
+    // tvOS builders are way too slow for this and regularly time out, so skip there.
+    [Test, Performance, UnityPlatform(exclude = new[] { RuntimePlatform.tvOS })]
+    [Category("Performance")]
+    [TestCase(OptimizationTestType.NoOptimization)]
+    [TestCase(OptimizationTestType.ReadValueCaching)]
+    // These tests shows a use case where ReadValueCaching optimization will perform better than without any
+    // optimization.
+    // It shows that there's a performance improvement when the control values being read are not changing every frame.
+    //
+    // NOTE: Performance is expected to be near-identical between the two optimisation settings, since Keyboard takes
+    // the ReadValueCaching paths in UpdateState.
+    public void Performance_OptimizedControls_ReadAndUpdateKeyboard1kTimes(OptimizationTestType testType)
+    {
+        SetInternalFeatureFlagsFromTestType(testType);
+
+        var keyboard = InputSystem.AddDevice<Keyboard>();
+
+        InputSystem.Update();
+
+        Measure.Method(() =>
+        {
+            InputSystem.QueueStateEvent(keyboard, new KeyboardState(Key.F));
+            InputSystem.Update();
+
+            for (var i = 0; i < 1000; ++i)
+            {
+                InputSystem.Update();
+
+                if (i % 200 == 0)
+                {
+                    // Make sure there's a new different value every 100 frames to mark the cached value as stale.
+                    InputSystem.QueueStateEvent(keyboard, new KeyboardState(Key.F));
+                    InputSystem.Update();
+                }
+                else if ((i + 100) % 200 == 0)
+                {
+                    InputSystem.QueueStateEvent(keyboard, new KeyboardState());
+                    InputSystem.Update();
+                }
+            }
+        })
+            .MeasurementCount(100)
+            .WarmupCount(10)
+            .Run();
+    }
+
+    // tvOS builders are way too slow for this and regularly time out, so skip there.
+    [Test, Performance, UnityPlatform(exclude = new[] { RuntimePlatform.tvOS })]
+    [Category("Performance")]
+    [TestCase(OptimizationTestType.NoOptimization)]
+    [TestCase(OptimizationTestType.ReadValueCaching)]
+    // This shows a use case where ReadValueCaching optimization will perform worse when controls have stale cached
+    // values every frame. Meaning, when control values change in every frame.
+    //
+    // NOTE: Performance is expected to be near-identical between the two optimisation settings, since Keyboard takes
+    // the ReadValueCaching paths in UpdateState.
+    public void Performance_OptimizedControls_ReadAndUpdateKeyboardNewValuesEveryFrame1kTimes(OptimizationTestType testType)
+    {
+        SetInternalFeatureFlagsFromTestType(testType);
+
+        var keyboard = InputSystem.AddDevice<Keyboard>();
+
+        InputSystem.Update();
+
+        Measure.Method(() =>
+        {
+            float val = keyboard.fKey.value;
+            InputSystem.QueueStateEvent(keyboard, new KeyboardState(Key.F));
+            InputSystem.Update();
+
+            for (var i = 0; i < 1000; ++i)
+            {
+                InputSystem.Update();
+                val = keyboard.fKey.value;
+                // Make sure there's a new different value every frames to mark the cached value as stale.
+                InputSystem.QueueStateEvent(keyboard, new KeyboardState());
+
+                InputSystem.Update();
+                val = keyboard.fKey.value;
+                // Make sure there's a new different value every frames to mark the cached value as stale.
+                InputSystem.QueueStateEvent(keyboard, new KeyboardState(Key.F));
+            }
+        })
+            .MeasurementCount(100)
+            .WarmupCount(10)
+            .Run();
+    }
+
     [Test, Performance]
     [Category("Performance")]
     [TestCase(OptimizationTestType.NoOptimization)]
@@ -766,6 +856,132 @@ void CallUpdate()
         }
     }
 
+    // tvOS builders are way too slow for this and regularly time out, so skip there.
+    [Test, Performance, UnityPlatform(exclude = new[] { RuntimePlatform.tvOS })]
+    [Category("Performance")]
+    [TestCase(OptimizationTestType.NoOptimization, 1)]
+    [TestCase(OptimizationTestType.OptimizedControls, 1)]
+    [TestCase(OptimizationTestType.ReadValueCaching, 1)]
+    [TestCase(OptimizationTestType.OptimizedControlsAndReadValueCaching, 1)]
+
+    [TestCase(OptimizationTestType.NoOptimization, 33)]
+    [TestCase(OptimizationTestType.OptimizedControls, 33)]
+    [TestCase(OptimizationTestType.ReadValueCaching, 33)]
+    [TestCase(OptimizationTestType.OptimizedControlsAndReadValueCaching, 33)]
+
+    [TestCase(OptimizationTestType.NoOptimization, 66)]
+    [TestCase(OptimizationTestType.OptimizedControls, 66)]
+    [TestCase(OptimizationTestType.ReadValueCaching, 66)]
+    [TestCase(OptimizationTestType.OptimizedControlsAndReadValueCaching, 66)]
+
+    [TestCase(OptimizationTestType.NoOptimization, 100)]
+    [TestCase(OptimizationTestType.OptimizedControls, 100)]
+    [TestCase(OptimizationTestType.ReadValueCaching, 100)]
+    [TestCase(OptimizationTestType.OptimizedControlsAndReadValueCaching, 100)]
+    // Test the effect on performance that occurs when we check more and more buttons of the controller for their press/release within the frame.
+
+    public void Performance_OptimizedControls_Gamepad_250PressAndUpdate_WasPressedThisFrame_PercentButtonsTested(OptimizationTestType testType, float percentageOfButtonsTested)
+    {
+        SetInternalFeatureFlagsFromTestType(testType);
+
+        var gamepad = InputSystem.AddDevice<Gamepad>();
+        InputSystem.Update();
+
+        var childrenThatAreButtons = new List<ButtonControl>();
+        foreach (var child in gamepad.m_ChildrenForEachControl)
+        {
+            if (child.isButton)
+                childrenThatAreButtons.Add((ButtonControl)child);
+        }
+
+        var buttonsToTest = Math.Max(1, childrenThatAreButtons.Count * percentageOfButtonsTested / 100);
+        for (int i = 0; i < buttonsToTest; ++i)
+        {
+            // Calling wasPressedThisFrame/wasReleasedThisFrame marks the button to care about the value,
+            // which affects processing of future events to care about state changes so that this call is accurate.
+            var press = childrenThatAreButtons[i].wasPressedThisFrame;
+        }
+
+        Measure.Method(() =>
+        {
+            CallUpdate();
+        })
+            .MeasurementCount(100)
+            .SampleGroup("Gamepad Only")
+            .WarmupCount(10)
+            .Run();
+
+        return;
+
+        void CallUpdate()
+        {
+            for (var i = 0; i < 250; ++i) PressAndRelease(gamepad.buttonSouth);
+        }
+    }
+
+    // tvOS builders are way too slow for this and regularly time out, so skip there.
+    [Test, Performance, UnityPlatform(exclude = new[] { RuntimePlatform.tvOS })]
+    [Category("Performance")]
+    [TestCase(OptimizationTestType.NoOptimization, 1)]
+    [TestCase(OptimizationTestType.OptimizedControls, 1)]
+    [TestCase(OptimizationTestType.ReadValueCaching, 1)]
+    [TestCase(OptimizationTestType.OptimizedControlsAndReadValueCaching, 1)]
+
+    [TestCase(OptimizationTestType.NoOptimization, 33)]
+    [TestCase(OptimizationTestType.OptimizedControls, 33)]
+    [TestCase(OptimizationTestType.ReadValueCaching, 33)]
+    [TestCase(OptimizationTestType.OptimizedControlsAndReadValueCaching, 33)]
+
+    [TestCase(OptimizationTestType.NoOptimization, 66)]
+    [TestCase(OptimizationTestType.OptimizedControls, 66)]
+    [TestCase(OptimizationTestType.ReadValueCaching, 66)]
+    [TestCase(OptimizationTestType.OptimizedControlsAndReadValueCaching, 66)]
+
+    [TestCase(OptimizationTestType.NoOptimization, 100)]
+    [TestCase(OptimizationTestType.OptimizedControls, 100)]
+    [TestCase(OptimizationTestType.ReadValueCaching, 100)]
+    [TestCase(OptimizationTestType.OptimizedControlsAndReadValueCaching, 100)]
+    // Test the effect on performance that occurs when we check more and more buttons of the controller for their press/release within the frame.
+
+    public void Performance_OptimizedControls_Keyboard_250PressAndUpdate_WasPressedThisFrame_PercentButtonsTested(OptimizationTestType testType, float percentageOfButtonsTested)
+    {
+        SetInternalFeatureFlagsFromTestType(testType);
+
+        var keyboard = InputSystem.AddDevice<Keyboard>();
+        InputSystem.Update();
+
+        var childrenThatAreButtons = new List<ButtonControl>();
+        foreach (var child in keyboard.m_ChildrenForEachControl)
+        {
+            if (child.isButton)
+                childrenThatAreButtons.Add((ButtonControl)child);
+        }
+
+        var buttonsToTest = Math.Max(1, childrenThatAreButtons.Count * percentageOfButtonsTested / 100);
+        for (int i = 0; i < buttonsToTest; ++i)
+        {
+            // Calling wasPressedThisFrame/wasReleasedThisFrame marks the button to care about the value,
+            // which affects processing of future events to care about state changes so that this call is accurate.
+            var press = childrenThatAreButtons[i].wasPressedThisFrame;
+        }
+
+        Measure.Method(() =>
+        {
+            CallUpdate();
+        })
+            .MeasurementCount(100)
+            .SampleGroup("Keyboard Only")
+            .WarmupCount(10)
+            .Run();
+
+        return;
+
+        void CallUpdate()
+        {
+            for (var i = 0; i < 250; ++i) PressAndRelease(keyboard.fKey);
+        }
+    }
+
     [Test, Performance]
     [Category("Performance")]
     [TestCase(OptimizationTestType.NoOptimization)]
@@ -782,7 +998,7 @@ public void Performance_OptimizedControls_EvaluateStaleControlReadsWhenGamepadSt
 
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
         // Disable the project wide actions actions to avoid performance impact.
-        InputSystem.actions.Disable();
+        InputSystem.actions?.Disable();
 #endif
 
         Measure.Method(() =>
@@ -824,7 +1040,7 @@ public void Performance_OptimizedControls_EvaluateStaleControlReadsWhenGamepadSt
 
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
         // Re-enable the project wide actions actions.
-        InputSystem.actions.Enable();
+        InputSystem.actions?.Enable();
 #endif
         return;
 

Assets/Tests/InputSystem/CoreTests_Controls.cs

@@ -15,7 +15,6 @@
 using UnityEngine.InputSystem.Processors;
 using UnityEngine.InputSystem.Utilities;
 using UnityEngine.Profiling;
-using UnityEngine.Scripting;
 using UnityEngine.TestTools.Constraints;
 using Is = UnityEngine.TestTools.Constraints.Is;
 
@@ -476,6 +475,9 @@ public unsafe void Controls_ValueIsReadFromStateMemoryOnlyWhenControlHasBeenMark
 
     [Test]
     [Category("Controls")]
+    // NOTE! Value caching will never be true for ButtonControls where users call wasPressedThisFrame/wasReleasedThisFrame
+    // following the first frame where either of those are called.
+    // This doesn't apply to this test, but just in case it gets edited/duplicated in future...
     public void Controls_ValueCachingWorksAcrossEntireDeviceMemoryRange()
     {
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS

Assets/Tests/InputSystem/CoreTests_State.cs

@@ -461,12 +461,15 @@ public void State_CanDetectWhetherButtonStateHasChangedThisFrame()
         Assert.That(gamepad.buttonEast.wasReleasedThisFrame, Is.True);
     }
 
-    // The way we keep state does not allow observing the state change on the final
-    // state of the button. However, actions will still see the change.
     [Test]
     [Category("State")]
-    public void State_PressingAndReleasingButtonInSameFrame_DoesNotShowStateChange()
+    [TestCase(true)]
+    [TestCase(false)]
+    public void State_PressingAndReleasingButtonInSameFrame_ShowsStateChange(bool usesReadValueCaching)
     {
+        var originalSetting = InputSystem.settings.IsFeatureEnabled(InputFeatureNames.kUseReadValueCaching);
+        InputSystem.settings.SetInternalFeatureFlag(InputFeatureNames.kUseReadValueCaching, usesReadValueCaching);
+
         var gamepad = InputSystem.AddDevice<Gamepad>();
 
         var firstState = new GamepadState {buttons = 1 << (int)GamepadButton.B};
@@ -477,9 +480,45 @@ public void State_PressingAndReleasingButtonInSameFrame_DoesNotShowStateChange()
 
         InputSystem.Update();
 
+        // We don't listen for inter-frame press/releases until we see them being requested, so the first time we try
+        // to detect it for a given device+ButtonControl, we'll miss the event.
         Assert.That(gamepad.buttonEast.isPressed, Is.False);
         Assert.That(gamepad.buttonEast.wasPressedThisFrame, Is.False);
         Assert.That(gamepad.buttonEast.wasReleasedThisFrame, Is.False);
+
+        InputSystem.QueueStateEvent(gamepad, firstState);
+        InputSystem.QueueStateEvent(gamepad, secondState);
+
+        InputSystem.Update();
+
+        Assert.That(gamepad.buttonEast.isPressed, Is.False);
+        Assert.That(gamepad.buttonEast.wasPressedThisFrame, Is.True);
+        Assert.That(gamepad.buttonEast.wasReleasedThisFrame, Is.True);
+
+        InputSystem.QueueStateEvent(gamepad, firstState);
+        InputSystem.QueueStateEvent(gamepad, secondState);
+        InputSystem.QueueStateEvent(gamepad, firstState);
+
+        InputSystem.Update();
+
+        Assert.That(gamepad.buttonEast.isPressed, Is.True);
+        Assert.That(gamepad.buttonEast.wasPressedThisFrame, Is.True);
+        Assert.That(gamepad.buttonEast.wasReleasedThisFrame, Is.True);
+
+        InputSystem.QueueStateEvent(gamepad, firstState);
+        InputSystem.QueueStateEvent(gamepad, secondState);
+        InputSystem.QueueStateEvent(gamepad, firstState);
+        InputSystem.QueueStateEvent(gamepad, secondState);
+        InputSystem.QueueStateEvent(gamepad, firstState);
+        InputSystem.QueueStateEvent(gamepad, secondState);
+
+        InputSystem.Update();
+
+        Assert.That(gamepad.buttonEast.isPressed, Is.False);
+        Assert.That(gamepad.buttonEast.wasPressedThisFrame, Is.True);
+        Assert.That(gamepad.buttonEast.wasReleasedThisFrame, Is.True);
+
+        InputSystem.settings.SetInternalFeatureFlag(InputFeatureNames.kUseReadValueCaching, originalSetting);
     }
 
     [Test]

Packages/com.unity.inputsystem/CHANGELOG.md

@@ -17,6 +17,7 @@ however, it has to be formatted properly to pass verification tests.
 
 ### Fixed
 - Avoid potential crashes from `NullReferenceException` in `FireStateChangeNotifications`.
+- Fixed cases where `wasPressedThisFrame` would not return true if a press and release happened within the same frame before being queried (and vice versa for `wasReleasedThisFrame`).
 - Fixed an issue where a composite binding would not be consecutively triggered after ResetDevice() has been called from the associated action handler [ISXB-746](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-746).
 - Fixed resource designation for "d_InputControl" icon to address CI failure.
 - Fixed an issue where a composite binding would not be consecutively triggered after disabling actions while there are action modifiers in progress [ISXB-505](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-505).