From 0318ee95c2696346416c1076bb054266f2f5f592 Mon Sep 17 00:00:00 2001 From: Graham Huws Date: Fri, 24 May 2024 17:03:55 +0100 Subject: [PATCH] FIX: Add workaround for cases where button presses can be missed (ISXB-491) --- .../Tests/InputSystem/CorePerformanceTests.cs | 4 +- Assets/Tests/InputSystem/CoreTests_State.cs | 24 +++++++-- Packages/com.unity.inputsystem/CHANGELOG.md | 1 + .../InputSystem/Controls/ButtonControl.cs | 41 ++++++++++++++- .../Controls/InputControlExtensions.cs | 1 + .../InputSystem/Devices/InputDevice.cs | 50 +++++++++++++------ .../InputSystem/Devices/InputDeviceBuilder.cs | 2 + .../InputSystem/InputManager.cs | 9 ++++ 8 files changed, 108 insertions(+), 24 deletions(-) diff --git a/Assets/Tests/InputSystem/CorePerformanceTests.cs b/Assets/Tests/InputSystem/CorePerformanceTests.cs index 80a0c5f6db..12e04801c6 100644 --- a/Assets/Tests/InputSystem/CorePerformanceTests.cs +++ b/Assets/Tests/InputSystem/CorePerformanceTests.cs @@ -782,7 +782,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 +824,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; diff --git a/Assets/Tests/InputSystem/CoreTests_State.cs b/Assets/Tests/InputSystem/CoreTests_State.cs index 7fee57d947..5e349f6e8d 100644 --- a/Assets/Tests/InputSystem/CoreTests_State.cs +++ b/Assets/Tests/InputSystem/CoreTests_State.cs @@ -462,11 +462,17 @@ public void State_CanDetectWhetherButtonStateHasChangedThisFrame() } // 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. + // state of the button, unless the readValueCaching path is enabled. + // However, actions will still see the change. [Test] [Category("State")] - public void State_PressingAndReleasingButtonInSameFrame_DoesNotShowStateChange() + [TestCase(true)] + [TestCase(false)] + public void State_PressingAndReleasingButtonInSameFrame_ShowsStateChange_WithCaching(bool usesReadValueCaching) { + var originalSetting = InputSettings.readValueCachingFeatureEnabled; + InputSystem.settings.SetInternalFeatureFlag(InputFeatureNames.kUseReadValueCaching, usesReadValueCaching); + var gamepad = InputSystem.AddDevice(); var firstState = new GamepadState {buttons = 1 << (int)GamepadButton.B}; @@ -478,8 +484,18 @@ public void State_PressingAndReleasingButtonInSameFrame_DoesNotShowStateChange() InputSystem.Update(); Assert.That(gamepad.buttonEast.isPressed, Is.False); - Assert.That(gamepad.buttonEast.wasPressedThisFrame, Is.False); - Assert.That(gamepad.buttonEast.wasReleasedThisFrame, Is.False); + if (usesReadValueCaching) + { + Assert.That(gamepad.buttonEast.wasPressedThisFrame, Is.True); + Assert.That(gamepad.buttonEast.wasReleasedThisFrame, Is.True); + } + else + { + Assert.That(gamepad.buttonEast.wasPressedThisFrame, Is.False); + Assert.That(gamepad.buttonEast.wasReleasedThisFrame, Is.False); + } + + InputSystem.settings.SetInternalFeatureFlag(InputFeatureNames.kUseReadValueCaching, originalSetting); } [Test] diff --git a/Packages/com.unity.inputsystem/CHANGELOG.md b/Packages/com.unity.inputsystem/CHANGELOG.md index 1a8cd8269e..26c8926a1d 100644 --- a/Packages/com.unity.inputsystem/CHANGELOG.md +++ b/Packages/com.unity.inputsystem/CHANGELOG.md @@ -12,6 +12,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`). As this is an edge case which necessitates checking more data, enabling this fix requires the `"USE_READ_VALUE_CACHING"` feature flag to be enabled. - 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). diff --git a/Packages/com.unity.inputsystem/InputSystem/Controls/ButtonControl.cs b/Packages/com.unity.inputsystem/InputSystem/Controls/ButtonControl.cs index 6036b6d813..ae1035145a 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Controls/ButtonControl.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Controls/ButtonControl.cs @@ -17,6 +17,10 @@ namespace UnityEngine.InputSystem.Controls /// public class ButtonControl : AxisControl { + private uint updateCountLastPressed = uint.MaxValue; + private uint updateCountLastReleased = uint.MaxValue; + private bool lastUpdateWasPress = false; + ////REVIEW: are per-control press points really necessary? can we just drop them? /// /// The minimum value the button has to reach for it to be considered pressed. @@ -118,9 +122,42 @@ public ButtonControl() /// /// /// - public bool wasPressedThisFrame => device.wasUpdatedThisFrame && IsValueConsideredPressed(value) && !IsValueConsideredPressed(ReadValueFromPreviousFrame()); + public bool wasPressedThisFrame + { + get + { + if (InputSettings.readValueCachingFeatureEnabled) + return InputUpdate.s_UpdateStepCount == updateCountLastPressed; + + return device.wasUpdatedThisFrame && IsValueConsideredPressed(value) && !IsValueConsideredPressed(ReadValueFromPreviousFrame()); + } + } + + public bool wasReleasedThisFrame + { + get + { + if (InputSettings.readValueCachingFeatureEnabled) + return InputUpdate.s_UpdateStepCount == updateCountLastReleased; - public bool wasReleasedThisFrame => device.wasUpdatedThisFrame && !IsValueConsideredPressed(value) && IsValueConsideredPressed(ReadValueFromPreviousFrame()); + return device.wasUpdatedThisFrame && !IsValueConsideredPressed(value) && IsValueConsideredPressed(ReadValueFromPreviousFrame()); + } + } + + internal void UpdateWasPressed(uint updateStepCount) + { + var isNowPressed = isPressed; + + if (lastUpdateWasPress != isNowPressed) + { + if (isNowPressed) + updateCountLastPressed = updateStepCount; + else + updateCountLastReleased = updateStepCount; + + lastUpdateWasPress = isNowPressed; + } + } // We make the current global default button press point available as a static so that we don't have to // constantly make the hop from InputSystem.settings -> InputManager.m_Settings -> defaultButtonPressPoint. diff --git a/Packages/com.unity.inputsystem/InputSystem/Controls/InputControlExtensions.cs b/Packages/com.unity.inputsystem/InputSystem/Controls/InputControlExtensions.cs index 0b4288e381..432c55faf6 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Controls/InputControlExtensions.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Controls/InputControlExtensions.cs @@ -1583,6 +1583,7 @@ public static DeviceBuilder Setup(this InputDevice device, int controlCount, int device.m_Device = device; device.m_ChildrenForEachControl = new InputControl[controlCount]; + device.m_UpdatedButtons = new Dictionary(); if (usageCount > 0) { device.m_UsagesForEachControl = new InternedString[usageCount]; diff --git a/Packages/com.unity.inputsystem/InputSystem/Devices/InputDevice.cs b/Packages/com.unity.inputsystem/InputSystem/Devices/InputDevice.cs index 7011dbd9d7..78ad4d5844 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Devices/InputDevice.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Devices/InputDevice.cs @@ -4,6 +4,7 @@ using UnityEngine.InputSystem.LowLevel; using UnityEngine.InputSystem.Utilities; using Unity.Collections.LowLevel.Unsafe; +using UnityEngine.InputSystem.Controls; using UnityEngine.InputSystem.Layouts; ////TODO: runtime remapping of control usages on a per-device basis @@ -678,7 +679,7 @@ internal bool disabledWhileInBackground /// /// Timestamp of last event we received. /// - /// + /// internal double m_LastUpdateTimeInternal; // Update count corresponding to the current front buffers that are active on the device. @@ -704,6 +705,9 @@ internal bool disabledWhileInBackground // NOTE: The device's own children are part of this array as well. internal InputControl[] m_ChildrenForEachControl; + // Used with value caching so we update button press states. + internal Dictionary m_UpdatedButtons; + // An ordered list of ints each containing a bit offset into the state of the device (*without* the added global // offset), a bit count for the size of the state of the control, and an associated index into m_ChildrenForEachControl // for the corresponding control. @@ -972,25 +976,27 @@ internal unsafe void WriteChangedControlStates(byte* deviceStateBuffer, void* st if (m_ControlTreeNodes.Length == 0) return; + // Reset counter for how many controls have updated + m_UpdatedButtons.Clear(); + // if we're dealing with a delta state event or just an individual control update through InputState.ChangeState // the size of the new data will not be the same size as the device state block, so use the 'partial' change state // method to update just those controls that overlap with the changed state. if (m_StateBlock.sizeInBits != stateSizeInBytes * 8) { if (m_ControlTreeNodes[0].leftChildIndex != -1) - WritePartialChangedControlStatesInternal(statePtr, stateSizeInBytes * 8, - stateOffsetInDevice * 8, deviceStateBuffer, m_ControlTreeNodes[0], 0); + WritePartialChangedControlStatesInternal(stateSizeInBytes * 8, + stateOffsetInDevice * 8, m_ControlTreeNodes[0], 0); } else { if (m_ControlTreeNodes[0].leftChildIndex != -1) - WriteChangedControlStatesInternal(statePtr, stateSizeInBytes * 8, - deviceStateBuffer, m_ControlTreeNodes[0], 0); + WriteChangedControlStatesInternal(statePtr, deviceStateBuffer, m_ControlTreeNodes[0], 0); } } - private unsafe void WritePartialChangedControlStatesInternal(void* statePtr, uint stateSizeInBits, - uint stateOffsetInDeviceInBits, byte* deviceStatePtr, ControlBitRangeNode parentNode, uint startOffset) + private void WritePartialChangedControlStatesInternal(uint stateSizeInBits, + uint stateOffsetInDeviceInBits, ControlBitRangeNode parentNode, uint startOffset) { var leftNode = m_ControlTreeNodes[parentNode.leftChildIndex]; // TODO recheck @@ -1001,12 +1007,14 @@ private unsafe void WritePartialChangedControlStatesInternal(void* statePtr, uin for (int i = leftNode.controlStartIndex; i < controlEndIndex; i++) { var controlIndex = m_ControlTreeIndices[i]; - m_ChildrenForEachControl[controlIndex].MarkAsStale(); + var control = m_ChildrenForEachControl[controlIndex]; + control.MarkAsStale(); + if (control is ButtonControl) + m_UpdatedButtons[controlIndex] = (ButtonControl)control; } if (leftNode.leftChildIndex != -1) - WritePartialChangedControlStatesInternal(statePtr, stateSizeInBits, stateOffsetInDeviceInBits, - deviceStatePtr, leftNode, startOffset); + WritePartialChangedControlStatesInternal(stateSizeInBits, stateOffsetInDeviceInBits, leftNode, startOffset); } var rightNode = m_ControlTreeNodes[parentNode.leftChildIndex + 1]; @@ -1018,12 +1026,14 @@ private unsafe void WritePartialChangedControlStatesInternal(void* statePtr, uin for (int i = rightNode.controlStartIndex; i < controlEndIndex; i++) { var controlIndex = m_ControlTreeIndices[i]; - m_ChildrenForEachControl[controlIndex].MarkAsStale(); + var control = m_ChildrenForEachControl[controlIndex]; + control.MarkAsStale(); + if (control is ButtonControl) + m_UpdatedButtons[controlIndex] = (ButtonControl)control; } if (rightNode.leftChildIndex != -1) - WritePartialChangedControlStatesInternal(statePtr, stateSizeInBits, stateOffsetInDeviceInBits, - deviceStatePtr, rightNode, leftNode.endBitOffset); + WritePartialChangedControlStatesInternal(stateSizeInBits, stateOffsetInDeviceInBits, rightNode, leftNode.endBitOffset); } } @@ -1062,7 +1072,7 @@ internal string DumpControlTree() return string.Join("\n", output); } - private unsafe void WriteChangedControlStatesInternal(void* statePtr, uint stateSizeInBits, + private unsafe void WriteChangedControlStatesInternal(void* statePtr, byte* deviceStatePtr, ControlBitRangeNode parentNode, uint startOffset) { var leftNode = m_ControlTreeNodes[parentNode.leftChildIndex]; @@ -1086,12 +1096,16 @@ private unsafe void WriteChangedControlStatesInternal(void* statePtr, uint state // points at a block of memory of the same size as the device state. if (!control.CompareState(deviceStatePtr - m_StateBlock.byteOffset, (byte*)statePtr - m_StateBlock.byteOffset, null)) + { control.MarkAsStale(); + if (control is ButtonControl) + m_UpdatedButtons[controlIndex] = (ButtonControl)control; + } } // process the left child node if it exists if (leftNode.leftChildIndex != -1) - WriteChangedControlStatesInternal(statePtr, stateSizeInBits, deviceStatePtr, + WriteChangedControlStatesInternal(statePtr, deviceStatePtr, leftNode, startOffset); } @@ -1116,11 +1130,15 @@ private unsafe void WriteChangedControlStatesInternal(void* statePtr, uint state if (!control.CompareState(deviceStatePtr - m_StateBlock.byteOffset, (byte*)statePtr - m_StateBlock.byteOffset, null)) + { control.MarkAsStale(); + if (control is ButtonControl) + m_UpdatedButtons[controlIndex] = (ButtonControl)control; + } } if (rightNode.leftChildIndex != -1) - WriteChangedControlStatesInternal(statePtr, stateSizeInBits, deviceStatePtr, + WriteChangedControlStatesInternal(statePtr, deviceStatePtr, rightNode, leftNode.endBitOffset); } diff --git a/Packages/com.unity.inputsystem/InputSystem/Devices/InputDeviceBuilder.cs b/Packages/com.unity.inputsystem/InputSystem/Devices/InputDeviceBuilder.cs index 3b89995e8d..0c8189df94 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Devices/InputDeviceBuilder.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Devices/InputDeviceBuilder.cs @@ -142,6 +142,7 @@ private InputControl InstantiateLayout(InputControlLayout layout, InternedString // we are rebuilding the control hierarchy. m_Device.m_AliasesForEachControl = null; m_Device.m_ChildrenForEachControl = null; + m_Device.m_UpdatedButtons = null; m_Device.m_UsagesForEachControl = null; m_Device.m_UsageToControl = null; @@ -364,6 +365,7 @@ private void AddChildControls(InputControlLayout layout, InternedString variants ref controlLayout); } } + m_Device.m_UpdatedButtons = new Dictionary(); } private InputControl AddChildControl(InputControlLayout layout, InternedString variants, InputControl parent, diff --git a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs index 9b608602e7..b9491ff6de 100644 --- a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs +++ b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs @@ -3650,6 +3650,15 @@ internal unsafe bool UpdateState(InputDevice device, InputUpdateType updateType, stateOffsetInDevice, statePtr, stateSize, flipped); } + if (makeDeviceCurrent) + { + // Update the pressed/not pressed state of all buttons that have changed this update + foreach (var button in device.m_UpdatedButtons.Values) + { + button.UpdateWasPressed(device.m_CurrentUpdateStepCount); + } + } + // Notify listeners. DelegateHelpers.InvokeCallbacksSafe(ref m_DeviceStateChangeListeners, device, eventPtr, "InputSystem.onDeviceStateChange");