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

graham-huws · 2024-05-24T16:13:09Z

Description

Fix for ISXB-491.
The current design of the input system package relies on the following method for checking if a button has been pressed on the current frame: Check the current value of the button, and compare to the final value of the previous frame.
The issue with this approach is that presses can be missed if they happened very quickly, or there was some form of lag making the update take longer than intended. Whether a button is pressed is only properly evaluated when asked, so if we don't check every input coming in, some can be missed.

Changes made

This PR seeks to solve this in a very simplistic way - check every value that affects ButtonControls and record their pressed state at the time the change comes in. Current belief is that a "clever" solution to this issue would require a fairly extensive rewrite of how the input system works.
Performance comparison data can be viewed on Sheet 2 here.

Checklist

Before review:

Changelog entry added.
- Explains the change in Changed, Fixed, Added sections.
- For API change contains an example snippet and/or migration example.
- FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
- FogBugz is marked as "Resolved" with next release version correctly set.
Tests added/changed, if applicable.
- Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
- Performance tests.
- Integration tests.
Docs for new/changed API's.
- Xmldoc cross references are set correctly.
- Added explanation how the API works.
- Usage code examples added.
- The manual is updated, if needed.

During merge:

Commit message for squash-merge is prefixed with one of the list:
- NEW: ___.
- FIX: ___.
- DOCS: ___.
- CHANGE: ___.
- RELEASE: 1.1.0-preview.3.

Packages/com.unity.inputsystem/InputSystem/Devices/InputDevice.cs

lyndon-unity

Would be good to get a summary of the performance impact. The spreadsheet has lots of data but doesn't make it very clear what the overall impact is.

My main concern is that this fixes lost events but we are hiding it behind a flag. I'd like to know the impact of enabling the flag by default.

Packages/com.unity.inputsystem/InputSystem/InputManager.cs

Assets/Tests/InputSystem/CorePerformanceTests.cs

Packages/com.unity.inputsystem/CHANGELOG.md

Assets/Tests/InputSystem/CoreTests_State.cs

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

Assets/Tests/InputSystem/CoreTests_State.cs

lyndon-unity · 2024-05-30T12:50:26Z

Added Paulius to help testing performance on Android

Pauliusd01 · 2024-06-03T12:27:25Z

The original bug project crashes when built on my only Android testing phone (Xiaomi MI 8 Lite) so I used the QA mobile sensor testing project instead when idling and when interacting with the UI buttons. Not sure what to make of the results so I'm just gonna post the screenshot with analyzer data as well as profiler captures. Capture data

(In the screenshot blue highlighted area is for simply idling the app and the right green area is me pressing UI buttons every 0.5~ sec)

lyndon-unity · 2024-06-03T15:28:23Z

These captures don't show any significant slow down so I think this is good to land (pending reacting to feedback above in PR)

lyndon-unity · 2024-06-03T15:42:32Z

There is a slowdown on some frames for the PreUpdate.NewInputUpdate. 0.20 to 0.34 on one frame Only 2 frames show slow down more and 0.1

InputProcess has some slow down but its minimal 0.03 to 0.04

How many taps were made in this sample ?

Pauliusd01 · 2024-06-04T08:02:51Z

Here's one of me just tapping non stop for around 15 secs

ExtraCaptures2.zip

lyndon-unity · 2024-06-04T14:20:54Z

Here's one of me just tapping non stop for around 15 secs

Sorting by time and looking at the slowest 100 frames (when there is active tap interation) there is a small slowdown in InputUpdate 0.01ms (0.06->0.07ms). But this seems reasonable for the reliability improvement of the edge detection from the fix.

lyndon-unity

I'd like to see this decoupled from the feature flag as I see this as a critical fix that should always be enabled.

Packages/com.unity.inputsystem/CHANGELOG.md

…B-491)

graham-huws · 2024-06-06T14:31:44Z

Have updated to address above comments, plus some extra issues that cropped up. Have added new perf results to the second sheet of the gdoc.

Assets/Tests/InputSystem/CoreTests_State.cs

Packages/com.unity.inputsystem/CHANGELOG.md

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

Packages/com.unity.inputsystem/InputSystem/InputManager.cs

ekcoh · 2024-06-11T07:14:06Z

Assets/Tests/InputSystem/CoreTests_Controls.cs

@@ -488,29 +487,47 @@ public void Controls_ValueCachingWorksAcrossEntireDeviceMemoryRange()
 InputSystem.actions?.Disable();
 #endif

- var keyboard = InputSystem.AddDevice<Keyboard>();
+ // Value caching is supported for all controls, but with changes to button press detection, ButtonControls will


Good catch, thanks for updating this.

However, this makes me wonder, will this evaluation work for detecting "button behavior presses" for controls that are not buttons? E.g. transition of axis under the button behavior (magnitude based) within a sub-frame? Basically this is not unique to buttons in any way, it's all about avoiding aliasing in the sampling process. Not saying these changes not already carry a lot of value, but trying to understand what problems still exist. Hence I wonder if we should setup an additional test case similar to the one done for wasPresseed/wasReleased this frame w.r.t. e.g. axis, or maybe you already can say what we guarantee for that use case? (We could just say, use actions for something like that)

I believe what you mention will be detected just fine, eg. gamepad has a ButtonControl for "left stick up"?

Assets/Tests/InputSystem/CoreTests_State.cs

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

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

Packages/com.unity.inputsystem/InputSystem/Devices/InputDeviceBuilder.cs

ekcoh · 2024-06-11T07:25:02Z

Packages/com.unity.inputsystem/InputSystem/InputManager.cs

@@ -3739,7 +3773,8 @@ internal unsafe bool UpdateState(InputDevice device, InputEvent* eventPtr, Input
 deviceStateSize);
 }

- if (InputSettings.readValueCachingFeatureEnabled)
+ // Keyboard has enough ButtonControls that it's faster to find out which have actually changed here.
+ if (InputSettings.readValueCachingFeatureEnabled || m_Devices[deviceIndex] is Keyboard)


I wonder if there is also a synergy with knowing the button count also here? It sounds like the benefit here is actually more related to the number of buttons on the device rather than it being a Keyboard? Hence maybe this should be a threshold based off number of buttons? Its fine as is but something like that would make it more future proof?

Updated - with some testing with the new changes, it seems that having about 45 buttons that have pressed/releasedThisFrame usage is where it becomes more efficient to take the cache value, so we now swap over at that point.

ekcoh

I am going to approve this one now from my perspective and not taking other reviewer comments into account. I left a few questions and minor comments though for consideration. Great to see this being adressed, thanks @graham-huws!

ekcoh · 2024-06-18T13:59:45Z

@lyndon-unity Could you help summarise what additional updates are needed to transition this from "Request Changes"?

lyndon-unity

I've going to approve this, but ideally we'd follow up to adjust it to limit the performance impact by only applying the extra checks when there is code that needs it. (I..e I think only if wasPressedThisFrame or wasReleasedThisFrame are used by the user). Ideally we'd defer the work to the call site.

I would like to hear the XR team perspective of the performance impact as there is a risk this is unexceptable in their use case.
Ideally they would have the option to opt out of this performance cost if they don't need if for specific actions/bindings.

…lly need it.

…Released are first called.

graham-huws · 2024-06-21T02:02:35Z

Now updated to a new version which only does processing for ButtonControls which have had wasPressedThisFrame/wasReleasedThisFrame called, saving a lot of performance when not many are being utilised.

Added V2 benchmarks to the spreadsheet, and new tests which try varying amounts of buttons to test to see the impact.

… give plenty of info.

lyndon-unity

As the performance is improved for the cases with less calls to wasPressedThisFrame with this update then I'm comfortable with it landing as it fixes the key issue and limits the performance impact to the flows where the checks are needed.

lyndon-unity · 2024-06-24T10:43:30Z

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

- (InputUpdate.s_LatestUpdateType.IsEditorUpdate() ? m_UpdateCountLastReleasedEditor : m_UpdateCountLastReleased);
- #else
- public bool wasPressedThisFrame => InputUpdate.s_UpdateStepCount == m_UpdateCountLastPressed;
+ return device.wasUpdatedThisFrame && currentlyPressed && !pressedLastFrame;


This line is much easier to read than before which is great.

So if I'm understanding correctly, this old flow can still miss some rapid in frame press/release on the first check, since we haven't run though all the transitions.

Its good we have the more expensive flow opted in, so not always paying the cost if never used, but I'd like to see a comment above to state that this can miss a transition on the first ever call.

Agree on the comment. Then if you care for the first call you may at least call this once up front to get it triggered.

lyndon-unity · 2024-06-24T10:46:30Z

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

+ var pressedLastFrame = IsValueConsideredPressed(ReadValueFromPreviousFrame());
+ BeginTestingForFramePresses(currentlyPressed, pressedLastFrame);
+
+ return device.wasUpdatedThisFrame && !currentlyPressed && pressedLastFrame;


Again this is easier to read which is a good change

ekcoh

Looks good, but agree to Lyndons comment.

Did you check consistency with wasPerformedThisFrame? Since that one is only available on actions I think it could have value to have a test that sets up an action bound to button and then coevaluates wasPerformedThisFrame with outcome of wasPressedThisFrame and wasReleasedThisFrame.

ekcoh · 2024-06-28T08:50:23Z

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

- (InputUpdate.s_LatestUpdateType.IsEditorUpdate() ? m_UpdateCountLastReleasedEditor : m_UpdateCountLastReleased);
- #else
- public bool wasPressedThisFrame => InputUpdate.s_UpdateStepCount == m_UpdateCountLastPressed;
+ return device.wasUpdatedThisFrame && currentlyPressed && !pressedLastFrame;


Agree on the comment. Then if you care for the first call you may at least call this once up front to get it triggered.

…ests where it times out for being too slow.

…sues.

…ection timeouts.

graham-huws requested a review from ekcoh May 24, 2024 16:13

lyndon-unity reviewed May 28, 2024

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Devices/InputDevice.cs Show resolved Hide resolved

lyndon-unity reviewed May 28, 2024

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/InputManager.cs Show resolved Hide resolved

ekcoh reviewed May 29, 2024

View reviewed changes

Assets/Tests/InputSystem/CoreTests_State.cs Outdated Show resolved Hide resolved

lyndon-unity requested a review from Pauliusd01 May 30, 2024 12:50

Pauliusd01 removed their request for review June 3, 2024 12:28

This comment was marked as off-topic.

Sign in to view

lyndon-unity requested changes Jun 5, 2024

View reviewed changes

Packages/com.unity.inputsystem/CHANGELOG.md Outdated Show resolved Hide resolved

graham-huws added 4 commits June 6, 2024 12:55

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

37e52c3

…B-491)

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

20072cb

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

3cdc307

Update changelog; expand button press testing

6b544ee

graham-huws force-pushed the ISXB-491-button-was-pressed-this-frame branch from e2b8bed to 6b544ee Compare June 6, 2024 14:24

graham-huws marked this pull request as ready for review June 6, 2024 14:28

Missed format check

6b61bf3

graham-huws requested review from ekcoh, lyndon-unity and Pauliusd01 June 6, 2024 14:30

lyndon-unity reviewed Jun 6, 2024

View reviewed changes

Pauliusd01 removed their request for review June 10, 2024 06:02

ekcoh reviewed Jun 11, 2024

View reviewed changes

ekcoh approved these changes Jun 11, 2024

View reviewed changes

Address PR comments

e8b8c46

lyndon-unity approved these changes Jun 18, 2024

View reviewed changes

graham-huws added 3 commits June 21, 2024 00:47

Add flags so that additional processing only happens when users actua…

b61f848

…lly need it.

Fix up tests, most crucially setting relevant states when wasPressed/…

c1562c9

…Released are first called.

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

e966003

graham-huws requested review from ekcoh and lyndon-unity June 21, 2024 01:58

Reduce test repetition to 250 as it was taking too long. Should still…

907bec8

… give plenty of info.

lyndon-unity approved these changes Jun 24, 2024

View reviewed changes

ekcoh approved these changes Jun 28, 2024

View reviewed changes

graham-huws and others added 4 commits July 1, 2024 22:49

Only loop through buttons that need testing. Exclude tvOS from some t…

c84e2d9

…ests where it times out for being too slow.

Merge branch 'develop' into ISXB-491-button-was-pressed-this-frame

22a7190

Fix up after merge + formatting.

cfc3dc9

Try boosting timeout value on tvOS to avoid test failures.

c7e591d

graham-huws force-pushed the ISXB-491-button-was-pressed-this-frame branch from 262a46f to c7e591d Compare July 2, 2024 14:18

graham-huws and others added 3 commits July 4, 2024 02:51

Undo change, can't be sure it's relevant due to other ongoing tvOS is…

31881e0

…sues.

Merge branch 'develop' into ISXB-491-button-was-pressed-this-frame

43d8d30

Exclude these other tests on tvOS as they seem to really trip up conn…

7a6758c

…ection timeouts.

graham-huws merged commit 511daef into develop Jul 8, 2024
77 of 79 checks passed

graham-huws deleted the ISXB-491-button-was-pressed-this-frame branch July 8, 2024 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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) #1935

graham-huws commented May 24, 2024 •

edited

Loading

lyndon-unity left a comment

lyndon-unity commented May 30, 2024

Pauliusd01 commented Jun 3, 2024 •

edited

Loading

lyndon-unity commented Jun 3, 2024 •

edited

Loading

lyndon-unity commented Jun 3, 2024

This comment was marked as off-topic.

Pauliusd01 commented Jun 4, 2024

lyndon-unity commented Jun 4, 2024

lyndon-unity left a comment

graham-huws commented Jun 6, 2024

ekcoh Jun 11, 2024

ekcoh Jun 11, 2024

graham-huws Jun 21, 2024

ekcoh Jun 11, 2024

graham-huws Jun 21, 2024

ekcoh left a comment

ekcoh commented Jun 18, 2024

lyndon-unity left a comment

graham-huws commented Jun 21, 2024

lyndon-unity left a comment •

edited

Loading

lyndon-unity Jun 24, 2024

lyndon-unity Jun 24, 2024

ekcoh Jun 28, 2024

lyndon-unity Jun 24, 2024

ekcoh left a comment

ekcoh Jun 28, 2024

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) #1935

Conversation

graham-huws commented May 24, 2024 • edited Loading

Description

Changes made

Checklist

lyndon-unity left a comment

Choose a reason for hiding this comment

lyndon-unity commented May 30, 2024

Pauliusd01 commented Jun 3, 2024 • edited Loading

lyndon-unity commented Jun 3, 2024 • edited Loading

lyndon-unity commented Jun 3, 2024

This comment was marked as off-topic.

Pauliusd01 commented Jun 4, 2024

lyndon-unity commented Jun 4, 2024

lyndon-unity left a comment

Choose a reason for hiding this comment

graham-huws commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekcoh left a comment

Choose a reason for hiding this comment

ekcoh commented Jun 18, 2024

lyndon-unity left a comment

Choose a reason for hiding this comment

graham-huws commented Jun 21, 2024

lyndon-unity left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekcoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graham-huws commented May 24, 2024 •

edited

Loading

Pauliusd01 commented Jun 3, 2024 •

edited

Loading

lyndon-unity commented Jun 3, 2024 •

edited

Loading

lyndon-unity left a comment •

edited

Loading