-
Notifications
You must be signed in to change notification settings - Fork 331
FIX: IndexOutOfRangeException when enabling InputActionMap. (ISXB-1767) #2296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…at disabling controls doesn't require the same check.
PR Reviewer Guide 🔍(Review updated until commit 3ebbcd6)Here are some key observations to aid the review process:
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent |
PR Code Suggestions ✨No code suggestions found for the PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2296 +/- ##
===========================================
+ Coverage 77.95% 77.97% +0.01%
===========================================
Files 477 477
Lines 97416 97452 +36
===========================================
+ Hits 75943 75985 +42
+ Misses 21473 21467 -6 Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs
Outdated
Show resolved
Hide resolved
jfreire-unity
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one-line fix with test reproducing the issue 🎉
Added some minor comments but no blockers.
| // InputAction.cancel event handler (ISXB-1766). In this case we must skip controls associated with | ||
| // a device that is not connected to the system (Have deviceIndex < 0). We check this here to not | ||
| // cause side effects if aborting later in the call-chain. | ||
| if (!controls[controlIndex].device.added) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could a similar thing happen if a device were disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so which is why I extended the test to disable, enable, disable, enable but this seems to be fine. I have to admit why this is not clear and would also feel better by doing this symmetric. Should we explore making it symmetric or what do you think?
…ub.com:Unity-Technologies/InputSystem into isxb-1767-exceptions-when-disconnecting-device
Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs
Outdated
Show resolved
Hide resolved
|
Persistent review updated to latest commit 3ebbcd6 |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Just updating that I am still looking at this, potentially found issues but need to nail down the repro first |
Pauliusd01
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found an issue with the user repro project, steps are:
Complete steps 2-11 from the ISXB-1767 bug -> instead of disconnecting the gamepad in isolation use WASD keyboard keys to move your character while you do it -> the gamepad disconnects and the console gets spammed with errors for each keyboard key pressed -> this persists even if you leave and reenter play mode
I wasn't able to reproduce this on a new project or without your changes so seems like a legit bug to me. I will paste the full errors to you over slack
Description
This PR is a fix for a bug reported in ISXB-1767 (internal ticket). The reported problem is an
IndexOutOfRangeExceptionbeing thrown fromInputManagerStateMonitors.AddStateChangeMonitorwhich leaves the Input System in an incomplete and inconsistent state preventing automatic recovery.Recreating the exact scenario reported in the ticket was difficult since the reproduction case was a very large project, but I believe I have finally been able to recreate the exact scenario now in test case
Actions_CanHandleDeviceDisconnectWithControlSchemesAndReconnectwhich is also added to this PR to prevent future regression. The scenario involves at least 2 devices being present and being associated with separate control schemes while triggering an action and disconnecting the associated device while disabling and reenabling an action map from the action cancellation callback.The fix involves not allowing adding state monitors to a device with an invalid device index which prevents the problematic exception. In addition to throwing, the exception also has side-effects and leaves the system in an inconsistent state which leads to additional exceptions being thrown afterwards and prevents input bindings from going back into a resolved state. What is interesting is that the presence of control schemes is what creates this issue and it is not completely clear to me why this is essential.
Testing status & QA
Added a more or less exact reproduction of the problematic scenario in
InputManagerStateMonitors.AddStateChangeMonitor. I would also recommend verifying the final version of this PR with the repro project to make sure it's properly solved.Found another issue while testing this bug that I filed separately into ISXB-1776.
Overall Product Risks
Comments to reviewers
Recommend looking for other aspects or side-effects I might have overlooked. Additional insights of the control scheme aspect with relation to this bug is valuable.
To manually trigger the problem do the following reproduction steps:
If using
ProjectWideInputActionsSampleScenethe above script can just be attached to the "Cube" object or any other object.3. Connect a gamepad, e.g. DualSense via USB cable (important since step 6 will be difficult otherwise).
4. Enter play-mode.
5. Press Jump button (E.g. "X" on a DualSense) and keep it pressed to ensure that the action is still in performed state.
6. With your free hand, disconnect the USB cable from the device while still holding "X".
7. Observe failed assert and exceptions in console. It seems the Input System cannot recover from this even if device is reconnected. With this PR the system shall both handle the error and auto-resolve bindings when device is reconnected.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.