FIX: Avoid potential for NullReferenceException in FireStateChangeNotifications (ISXB-724). #1927
Conversation
…ifications (ISXB-724).
Packages/com.unity.inputsystem/InputSystem/InputManagerStateMonitors.cs
Outdated
Show resolved
Hide resolved
|
@stefanunity I realise there's nothing really to test here given that there's no repro steps for the original issue, but just wanted you aware of this regardless in case you had anything to add 😃 |
|
N.B. tvos failure is also happening on develop branch. |
| - Fixed Composite binding isn't triggered after ResetDevice() called during Action handler [ISXB-746](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-746) | ||
|
|
||
| ### Fixed | ||
| - Avoid potential crashes from NullReferenceException in FireStateChangeNotifications. |
There was a problem hiding this comment.
Minor: Use code formatting quotes, e.g. NullReferenceException in FireStateChangeNotifications.
|
|
||
| ### Fixed | ||
| - Avoid potential crashes from NullReferenceException in FireStateChangeNotifications. | ||
| - Fixed Composite binding isn't triggered after ResetDevice() called during Action handler [ISXB-746](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-746). |
There was a problem hiding this comment.
I would suggest rewriting this to explain the actual problem in a more readable sentence than copying the subject of the ticket, e.g.
"Fixed an issue where a composite binding would not be consecutively triggered after ResetDevice() has been called from the associated action handler." + link as currently.
There was a problem hiding this comment.
Maybe not part of your PR but would be good to address. Also noticed fixe lines below lack full stop at the end.
| @@ -86,7 +86,7 @@ public struct GamepadState : IInputStateTypeInfo | |||
| internal const string ButtonSouthShortDisplayName = "Cross"; | |||
| internal const string ButtonNorthShortDisplayName = "Triangle"; | |||
| internal const string ButtonWestShortDisplayName = "Square"; | |||
| internal const string ButtonEastShortDisplayName = "East"; | |||
| internal const string ButtonEastShortDisplayName = "Circle"; | |||
There was a problem hiding this comment.
Good catch, but realize this is solved in a very platform-dependent way in what is supposed to be platform-agnostic code. Will feed this into a doc I am creating on the subject.
| @@ -349,14 +349,28 @@ private unsafe bool ProcessStateChangeMonitors(int deviceIndex, void* newStateFr | |||
|
|
|||
| internal unsafe void FireStateChangeNotifications(int deviceIndex, double internalTime, InputEvent* eventPtr) | |||
| { | |||
| Debug.Assert(m_StateChangeMonitors != null); | |||
| Debug.Assert(m_StateChangeMonitors.Length > deviceIndex); | |||
| if (m_StateChangeMonitors == null) | |||
There was a problem hiding this comment.
I think this is better, good change.
Description
A tentative fix, given no known repro, for ISXB-724.
Changes made
Added early returns in cases we'd otherwise crash.
Checklist
Before review:
Changed,Fixed,Addedsections.([case %number%](https://issuetracker.unity3d.com/issues/...)).Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.