FIX: Swapping InputManager state buffers and update step counts back to latest player update after editor update (#1424)

jimon · web-flow · commit 3d0dcad09112 · 2021-10-15T17:09:23.000+02:00
diff --git a/Assets/Tests/InputSystem/CoreTests_Editor.cs b/Assets/Tests/InputSystem/CoreTests_Editor.cs
@@ -350,7 +350,6 @@ public void Editor_WhenPlaying_EditorUpdatesKeepSeparateStateFromPlayerUpdates()
 
         InputSystem.QueueStateEvent(gamepad, new GamepadState {leftTrigger = 0.25f});
         InputSystem.Update(InputUpdateType.Dynamic);
-
         Assert.That(gamepad.leftTrigger.ReadValue(), Is.EqualTo(0.25).Within(0.000001));
         Assert.That(gamepad.leftTrigger.ReadValueFromPreviousFrame(), Is.Zero.Within(0.000001));
 
@@ -359,13 +358,50 @@ public void Editor_WhenPlaying_EditorUpdatesKeepSeparateStateFromPlayerUpdates()
         // started with.
         InputSystem.QueueStateEvent(gamepad, new GamepadState {leftTrigger = 0.75f});
         InputSystem.Update(InputUpdateType.Editor);
-
         Assert.That(gamepad.leftTrigger.ReadValue(), Is.Zero.Within(0.000001));
         Assert.That(gamepad.leftTrigger.ReadValueFromPreviousFrame(), Is.Zero.Within(0.000001));
 
         // So running a player update now should make the input come through in player state.
         InputSystem.Update(InputUpdateType.Dynamic);
+        Assert.That(gamepad.leftTrigger.ReadValue(), Is.EqualTo(0.75).Within(0.000001));
+        Assert.That(gamepad.leftTrigger.ReadValueFromPreviousFrame(), Is.EqualTo(0.25).Within(0.000001));
+    }
 
+    [Test]
+    [Category("Editor")]
+    // Case 1368559
+    // Case 1367556
+    // Case 1372830
+    public void Editor_WhenPlaying_ItsPossibleToQueryPlayerStateAfterEditorUpdate()
+    {
+        InputSystem.settings.editorInputBehaviorInPlayMode = default;
+
+        var gamepad = InputSystem.AddDevice<Gamepad>();
+
+        // ----------------- Engine frame 1
+
+        InputSystem.QueueStateEvent(gamepad, new GamepadState {leftTrigger = 0.25f});
+        InputSystem.Update(InputUpdateType.Dynamic);
+        Assert.That(gamepad.leftTrigger.ReadValue(), Is.EqualTo(0.25).Within(0.000001));
+        Assert.That(gamepad.leftTrigger.ReadValueFromPreviousFrame(), Is.Zero.Within(0.000001));
+
+        InputSystem.QueueStateEvent(gamepad, new GamepadState {leftTrigger = 0.75f});
+        InputSystem.Update(InputUpdateType.Editor);
+        Assert.That(gamepad.leftTrigger.ReadValue(), Is.Zero.Within(0.000001));
+        Assert.That(gamepad.leftTrigger.ReadValueFromPreviousFrame(), Is.Zero.Within(0.000001));
+
+        // ----------------- Engine frame 2
+
+        // Simulate early player loop callback
+        runtime.onPlayerLoopInitialization();
+
+        // This code might be running in EarlyUpdate or FixedUpdate, _before_ Dynamic update is invoked.
+        // We should read values from last player update, meaning we report values from last frame.
+        Assert.That(gamepad.leftTrigger.ReadValue(), Is.EqualTo(0.25).Within(0.000001));
+        Assert.That(gamepad.leftTrigger.ReadValueFromPreviousFrame(), Is.Zero.Within(0.000001));
+
+        // Running a player update now should make the input come through in player state.
+        InputSystem.Update(InputUpdateType.Dynamic);
         Assert.That(gamepad.leftTrigger.ReadValue(), Is.EqualTo(0.75).Within(0.000001));
         Assert.That(gamepad.leftTrigger.ReadValueFromPreviousFrame(), Is.EqualTo(0.25).Within(0.000001));
     }
diff --git a/Assets/Tests/InputSystem/CoreTests_Events.cs b/Assets/Tests/InputSystem/CoreTests_Events.cs
@@ -490,7 +490,7 @@ public unsafe void Events_InFixedUpdate_AreTimesliced()
         Assert.That(receivedEvents[2].time, Is.EqualTo(2.9).Within(0.00001));
         Assert.That(gamepad.leftTrigger.ReadValue(), Is.EqualTo(0.3456).Within(0.00001));
 
-        Assert.That(InputUpdate.s_LastUpdateRetainedEventCount, Is.Zero);
+        Assert.That(runtime.eventCount, Is.Zero);
 
         receivedEvents.Clear();
 
@@ -509,7 +509,7 @@ public unsafe void Events_InFixedUpdate_AreTimesliced()
         Assert.That(receivedEvents[1].time, Is.EqualTo(3 + 0.002).Within(0.00001));
         Assert.That(gamepad.leftTrigger.ReadValue(), Is.EqualTo(0.2345).Within(0.00001));
 
-        Assert.That(InputUpdate.s_LastUpdateRetainedEventCount, Is.EqualTo(2));
+        Assert.That(runtime.eventCount, Is.EqualTo(2));
 
         receivedEvents.Clear();
 
@@ -521,7 +521,7 @@ public unsafe void Events_InFixedUpdate_AreTimesliced()
         Assert.That(receivedEvents[0].time, Is.EqualTo(3 + 1.0 / 60 + 0.001).Within(0.00001));
         Assert.That(gamepad.leftTrigger.ReadValue(), Is.EqualTo(0.3456).Within(0.00001));
 
-        Assert.That(InputUpdate.s_LastUpdateRetainedEventCount, Is.EqualTo(1));
+        Assert.That(runtime.eventCount, Is.EqualTo(1));
 
         receivedEvents.Clear();
 
@@ -533,7 +533,7 @@ public unsafe void Events_InFixedUpdate_AreTimesliced()
         Assert.That(receivedEvents[0].time, Is.EqualTo(3 + 2 * (1.0 / 60) + 0.001).Within(0.00001));
         Assert.That(gamepad.leftTrigger.ReadValue(), Is.EqualTo(0.4567).Within(0.00001));
 
-        Assert.That(InputUpdate.s_LastUpdateRetainedEventCount, Is.Zero);
+        Assert.That(runtime.eventCount, Is.Zero);
 
         receivedEvents.Clear();
 
@@ -544,7 +544,7 @@ public unsafe void Events_InFixedUpdate_AreTimesliced()
         Assert.That(receivedEvents, Has.Count.Zero);
         Assert.That(gamepad.leftTrigger.ReadValue(), Is.EqualTo(0.4567).Within(0.00001));
 
-        Assert.That(InputUpdate.s_LastUpdateRetainedEventCount, Is.Zero);
+        Assert.That(runtime.eventCount, Is.Zero);
     }
 
     [Test]
diff --git a/Packages/com.unity.inputsystem/CHANGELOG.md b/Packages/com.unity.inputsystem/CHANGELOG.md
@@ -26,6 +26,7 @@ however, it has to be formatted properly to pass verification tests.
 - Generic gamepad short display button names where incorrectly mapped on Switch (`A` instead of `B`, etc).
 - Fixed an issue where resetting an action via `InputAction.Reset()` while being in disabled state would prevent the action from being enabled again. ([case 1370732](https://issuetracker.unity3d.com/product/unity/issues/guid/1370732/)).
 - Fixed "Default constructor not found for type UnityEngine.InputSystem.iOS.LowLevel.iOSStepCounter" any other potential exceptions due to classes, methods, fields and properties being stripped when managed stripping setting set to medium or high ([case 1368761](https://issuetracker.unity3d.com/issues/ios-new-input-system-iosstepcounter-crash-on-launch-with-managed-stripping)).
+- Fixed `action.ReadValue` and others returning invalid data when used from `FixedUpdate` or early update when running in play mode in the editor ([case 1368559](https://issuetracker.unity3d.com/issues/enter-key-is-not-registered-when-using-waspressedthisframe-with-input-system-1-dot-1-1) [case 1367556](https://issuetracker.unity3d.com/issues/input-action-readvalue-always-returns-zero-when-called-from-fixedupdate) [case 1372830](https://issuetracker.unity3d.com/issues/querying-inputs-before-preupdate-dot-newinputupdate-returns-invalid-data-when-running-in-play-mode-in-editor)).
 
 ### Added
 
diff --git a/Packages/com.unity.inputsystem/InputSystem/IInputRuntime.cs b/Packages/com.unity.inputsystem/InputSystem/IInputRuntime.cs
@@ -89,6 +89,13 @@ internal unsafe interface IInputRuntime
 
         Func<InputUpdateType, bool> onShouldRunUpdate { get; set; }
 
+        #if UNITY_EDITOR
+        /// <summary>
+        /// Set delegate to be called during player loop initialization callbacks.
+        /// </summary>
+        Action onPlayerLoopInitialization { get; set; }
+        #endif
+
         /// <summary>
         /// Set delegate to be called when a new device is discovered.
         /// </summary>
diff --git a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs
@@ -1901,13 +1901,19 @@ internal void InstallRuntime(IInputRuntime runtime)
                 m_Runtime.onDeviceDiscovered = null;
                 m_Runtime.onPlayerFocusChanged = null;
                 m_Runtime.onShouldRunUpdate = null;
+                #if UNITY_EDITOR
+                m_Runtime.onPlayerLoopInitialization = null;
+                #endif
             }
 
             m_Runtime = runtime;
             m_Runtime.onUpdate = OnUpdate;
             m_Runtime.onDeviceDiscovered = OnNativeDeviceDiscovered;
             m_Runtime.onPlayerFocusChanged = OnFocusChanged;
             m_Runtime.onShouldRunUpdate = ShouldRunUpdate;
+            #if UNITY_EDITOR
+            m_Runtime.onPlayerLoopInitialization = OnPlayerLoopInitialization;
+            #endif
             m_Runtime.pollingFrequency = pollingFrequency;
             m_HasFocus = m_Runtime.isPlayerFocused;
 
@@ -2011,6 +2017,11 @@ internal struct AvailableDevice
         private InputUpdateType m_CurrentUpdate;
         internal InputStateBuffers m_StateBuffers;
 
+        #if UNITY_EDITOR
+        // remember time offset to correctly restore it after editor mode is done
+        private double latestNonEditorTimeOffsetToRealtimeSinceStartup;
+        #endif
+
         // We don't use UnityEvents and thus don't persist the callbacks during domain reloads.
         // Restoration of UnityActions is unreliable and it's too easy to end up with double
         // registrations what will lead to all kinds of misbehavior.
@@ -2211,7 +2222,7 @@ private unsafe void ReallocateStateBuffers()
 
             // Switch to buffers.
             InputStateBuffers.SwitchTo(m_StateBuffers,
-                InputUpdate.s_LastUpdateType != InputUpdateType.None ? InputUpdate.s_LastUpdateType : defaultUpdateType);
+                InputUpdate.s_LatestUpdateType != InputUpdateType.None ? InputUpdate.s_LatestUpdateType : defaultUpdateType);
 
             ////TODO: need to update state change monitors
         }
@@ -2829,6 +2840,18 @@ internal void LeavePlayMode()
             m_CurrentUpdate = default;
         }
 
+        private void OnPlayerLoopInitialization()
+        {
+            if (!gameIsPlaying || // if game is not playing
+                !InputUpdate.s_LatestUpdateType.IsEditorUpdate() || // or last update was not editor update
+                !InputUpdate.s_LatestNonEditorUpdateType.IsPlayerUpdate()) // or update before that was not player update
+                return; // then no need to restore anything
+
+            InputUpdate.RestoreStateAfterEditorUpdate();
+            InputRuntime.s_CurrentTimeOffsetToRealtimeSinceStartup = latestNonEditorTimeOffsetToRealtimeSinceStartup;
+            InputStateBuffers.SwitchTo(m_StateBuffers, InputUpdate.s_LatestUpdateType);
+        }
+
 #endif
 
         internal bool ShouldRunUpdate(InputUpdateType updateType)
@@ -2908,8 +2931,13 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev
             // Update metrics.
             ++m_Metrics.totalUpdateCount;
 
-            InputUpdate.s_LastUpdateRetainedEventCount = 0;
-            InputUpdate.s_LastUpdateRetainedEventBytes = 0;
+            #if UNITY_EDITOR
+            // If current update is editor update and previous update was non-editor,
+            // store the time offset so we can restore it right after editor update is complete
+            if (((updateType & InputUpdateType.Editor) == InputUpdateType.Editor) && (m_CurrentUpdate & InputUpdateType.Editor) == 0)
+                latestNonEditorTimeOffsetToRealtimeSinceStartup =
+                    InputRuntime.s_CurrentTimeOffsetToRealtimeSinceStartup;
+            #endif
 
             // Store current time offset.
             InputRuntime.s_CurrentTimeOffsetToRealtimeSinceStartup = m_Runtime.currentTimeOffsetToRealtimeSinceStartup;
@@ -3368,11 +3396,6 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev
                     ((double)(Stopwatch.GetTimestamp() - processingStartTime)) / Stopwatch.Frequency;
                 m_Metrics.totalEventLagTime += totalEventLag;
 
-                // Remember how much data we retained so that we don't count it against the next
-                // batch of events that we receive.
-                InputUpdate.s_LastUpdateRetainedEventCount = (uint)m_InputEventStream.numEventsRetainedInBuffer;
-                InputUpdate.s_LastUpdateRetainedEventBytes = m_InputEventStream.numBytesRetainedInBuffer;
-
                 m_InputEventStream.Close(ref eventBuffer);
             }
             catch (Exception)
diff --git a/Packages/com.unity.inputsystem/InputSystem/InputUpdateType.cs b/Packages/com.unity.inputsystem/InputSystem/InputUpdateType.cs
@@ -71,13 +71,12 @@ public enum InputUpdateType
     internal static class InputUpdate
     {
         public static uint s_UpdateStepCount; // read only, but kept as a variable for performance reasons
-        public static InputUpdateType s_LastUpdateType;
+        public static InputUpdateType s_LatestUpdateType;
         public static UpdateStepCount s_PlayerUpdateStepCount;
         #if UNITY_EDITOR
+        public static InputUpdateType s_LatestNonEditorUpdateType;
         public static UpdateStepCount s_EditorUpdateStepCount;
         #endif
-        public static uint s_LastUpdateRetainedEventBytes;
-        public static uint s_LastUpdateRetainedEventCount;
 
         [Serializable]
         public struct UpdateStepCount
@@ -107,22 +106,24 @@ public struct SerializedState
             public InputUpdateType lastUpdateType;
             public UpdateStepCount playerUpdateStepCount;
             #if UNITY_EDITOR
+            public InputUpdateType lastNonEditorUpdateType;
             public UpdateStepCount editorUpdateStepCount;
             #endif
-            public uint lastUpdateRetainedEventBytes;
-            public uint lastUpdateRetainedEventCount;
         }
 
         internal static void OnBeforeUpdate(InputUpdateType type)
         {
-            s_LastUpdateType = type;
+            s_LatestUpdateType = type;
             switch (type)
             {
                 case InputUpdateType.Dynamic:
                 case InputUpdateType.Manual:
                 case InputUpdateType.Fixed:
                     s_PlayerUpdateStepCount.OnBeforeUpdate();
                     s_UpdateStepCount = s_PlayerUpdateStepCount.value;
+                    #if UNITY_EDITOR
+                    s_LatestNonEditorUpdateType = type;
+                    #endif
                     break;
                 #if UNITY_EDITOR
                 case InputUpdateType.Editor:
@@ -135,14 +136,17 @@ internal static void OnBeforeUpdate(InputUpdateType type)
 
         internal static void OnUpdate(InputUpdateType type)
         {
-            s_LastUpdateType = type;
+            s_LatestUpdateType = type;
             switch (type)
             {
                 case InputUpdateType.Dynamic:
                 case InputUpdateType.Manual:
                 case InputUpdateType.Fixed:
                     s_PlayerUpdateStepCount.OnUpdate();
                     s_UpdateStepCount = s_PlayerUpdateStepCount.value;
+                    #if UNITY_EDITOR
+                    s_LatestNonEditorUpdateType = type;
+                    #endif
                     break;
                 #if UNITY_EDITOR
                 case InputUpdateType.Editor:
@@ -153,31 +157,38 @@ internal static void OnUpdate(InputUpdateType type)
             }
         }
 
+        #if UNITY_EDITOR
+        internal static void RestoreStateAfterEditorUpdate()
+        {
+            s_LatestUpdateType = s_LatestNonEditorUpdateType;
+            s_UpdateStepCount = s_PlayerUpdateStepCount.value;
+        }
+
+        #endif
+
         public static SerializedState Save()
         {
             return new SerializedState
             {
-                lastUpdateType = s_LastUpdateType,
+                lastUpdateType = s_LatestUpdateType,
                 playerUpdateStepCount = s_PlayerUpdateStepCount,
                 #if UNITY_EDITOR
-                editorUpdateStepCount = s_EditorUpdateStepCount,
+                lastNonEditorUpdateType = s_LatestNonEditorUpdateType,
+                editorUpdateStepCount = s_EditorUpdateStepCount
                 #endif
-                lastUpdateRetainedEventBytes = s_LastUpdateRetainedEventBytes,
-                lastUpdateRetainedEventCount = s_LastUpdateRetainedEventCount,
             };
         }
 
         public static void Restore(SerializedState state)
         {
-            s_LastUpdateType = state.lastUpdateType;
+            s_LatestUpdateType = state.lastUpdateType;
             s_PlayerUpdateStepCount = state.playerUpdateStepCount;
-            s_LastUpdateRetainedEventBytes = state.lastUpdateRetainedEventBytes;
-            s_LastUpdateRetainedEventCount = state.lastUpdateRetainedEventCount;
             #if UNITY_EDITOR
+            s_LatestNonEditorUpdateType = state.lastNonEditorUpdateType;
             s_EditorUpdateStepCount = state.editorUpdateStepCount;
             #endif
 
-            switch (s_LastUpdateType)
+            switch (s_LatestUpdateType)
             {
                 case InputUpdateType.Dynamic:
                 case InputUpdateType.Manual:
@@ -216,5 +227,13 @@ public static bool IsPlayerUpdate(this InputUpdateType updateType)
                 return false;
             return updateType != default;
         }
+
+        #if UNITY_EDITOR
+        public static bool IsEditorUpdate(this InputUpdateType updateType)
+        {
+            return updateType == InputUpdateType.Editor;
+        }
+
+        #endif
     }
 }
diff --git a/Packages/com.unity.inputsystem/InputSystem/NativeInputRuntime.cs b/Packages/com.unity.inputsystem/InputSystem/NativeInputRuntime.cs
diff --git a/Packages/com.unity.inputsystem/InputSystem/State/InputState.cs b/Packages/com.unity.inputsystem/InputSystem/State/InputState.cs
diff --git a/Packages/com.unity.inputsystem/Tests/TestFixture/InputTestRuntime.cs b/Packages/com.unity.inputsystem/Tests/TestFixture/InputTestRuntime.cs
