FIX: Add workaround for cases where button presses can be missed (ISX…

…B-491)

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;
 

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<Gamepad>();
 
  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]

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).

Packages/com.unity.inputsystem/InputSystem/Controls/ButtonControl.cs

@@ -17,6 +17,10 @@ namespace UnityEngine.InputSystem.Controls
  /// </remarks>
  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?
  /// <summary>
  /// The minimum value the button has to reach for it to be considered pressed.
@@ -118,9 +122,42 @@ public ButtonControl()
  /// </code>
  /// </example>
  /// </remarks>
- 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.

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<int, ButtonControl>();
  if (usageCount > 0)
  {
  device.m_UsagesForEachControl = new InternedString[usageCount];

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
  /// <summary>
  /// Timestamp of last event we received.
  /// </summary>
- /// <seealso cref="InputEvent.time"/>
+ /// <seealso cref="LowLevel.InputEvent.time"/>
  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<int, ButtonControl> 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);
  }
 

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<int, ButtonControl>();
  }
 
  private InputControl AddChildControl(InputControlLayout layout, InternedString variants, InputControl parent,

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");