Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions Assets/Tests/InputSystem/CoreTests_Actions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12581,4 +12581,67 @@ public void Actions_ActionMapDisabledDuringOnAfterSerialization()
Assert.That(map.enabled, Is.True);
Assert.That(map.FindAction("MyAction", true).enabled, Is.True);
}

// Verifies that a multi-control-scheme project with a disconnected device that attempts to enable the associated
// actions via an action cancellation callback doesn't result in IndexOutOfBoundsException, and that the binding
// will be restored upon device reconnect. We use the same bindings as the associated ISXB issue to make sure
// we test the exact same scenario.
[Test, Description("https://jira.unity3d.com/browse/ISXB-1767")]
public void Actions_CanHandleDeviceDisconnectWithControlSchemesAndReconnect()
{
int started = 0;
int performed = 0;
int canceled = 0;

// Create an input action asset object.
var actions = ScriptableObject.CreateInstance<InputActionAsset>();

// These control schemes are critical to this test. Without them the exception won't happen.
var keyboardScheme = actions.AddControlScheme("Keyboard").WithRequiredDevice<Keyboard>();
var gamepadScheme = actions.AddControlScheme("Gamepad").WithRequiredDevice<Gamepad>();

// Create a single action map since its sufficient for the scenario.
var map = actions.AddActionMap("map");

var action = map.AddAction(name: "Toggle", InputActionType.Button);
action.AddBinding("<Gamepad>/leftTrigger");
action.started += context => ++ started;
action.performed += context => ++ performed;
action.canceled += (context) =>
{
// In reported issue, map state is changed from cancellation callback.
map.Disable();
map.Enable();

// This is not part of the bug reported in ISXB-1767 but extends the test coverage since
// it makes sure Disable() is safe after logically skipped Enable().
map.Disable();
map.Enable();

++canceled;
};

// Add a keyboard and a gamepad.
var keyboard = InputSystem.AddDevice<Keyboard>();
var gamepad = InputSystem.AddDevice<Gamepad>();

// Enable the map, press (and hold) the left trigger and assert action is firing.
map.Enable();
Press(gamepad.leftTrigger, queueEventOnly: true);
InputSystem.Update();
Assert.That(started, Is.EqualTo(1));

// Remove the gamepad device. This is consistent with event queue based removal (not kept on list).
InputSystem.RemoveDevice(gamepad);
InputSystem.Update();
Assert.That(canceled, Is.EqualTo(1));

// Reconnect the disconnected gamepad
InputSystem.AddDevice(gamepad);

// Interact again
Press(gamepad.leftTrigger, queueEventOnly: true);
InputSystem.Update();
Assert.That(started, Is.EqualTo(2));
}
}
1 change: 1 addition & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ however, it has to be formatted properly to pass verification tests.
- Align title font size with toolbar style in `Input Action` window.
- Updated Action Properties headers to use colors consistent with GameObject component headers.
- Fixed misaligned Virtual Cursor when changing resolution [ISXB-1119](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1119)
- Fixed an issue where `IndexOutOfRangeException` was thrown from `InputManagerStateMonitors.AddStateChangeMonitor` when attempting to enable an action map from within an `InputAction.cancel` callback when using control schemes. (ISXB-1767).

### Added

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1153,6 +1153,13 @@
if (IsControlEnabled(controlIndex))
continue;

// We might end up here if an action map is enabled from e.g. an event processing callback such as
// InputAction.cancel event handler (ISXB-1767). 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)
Copy link
Collaborator

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?

Copy link
Collaborator Author

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?

continue;

var bindingIndex = controlIndexToBindingIndex[controlIndex];
var mapControlAndBindingIndex = ToCombinedMapAndControlAndBindingIndex(mapIndex, controlIndex, bindingIndex);

Expand Down Expand Up @@ -1474,8 +1481,18 @@
if (m_OnBeforeUpdateHooked)
bindingStatePtr->initialStateCheckPending = false;

// Store magnitude. We do this once and then only read it from here.
var control = controls[controlIndex];

// We might end up here if an action map is enabled from e.g. an event processing callback such as
// InputAction.cancel event handler (ISXB-1767). 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 (control == null || !controls[controlIndex].device.added)
{
return;

Check warning on line 1492 in Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs#L1491-L1492

Added lines #L1491 - L1492 were not covered by tests
}

// Store magnitude. We do this once and then only read it from here.
trigger.magnitude = control.CheckStateIsAtDefault() ? 0f : control.magnitude;
controlMagnitudes[controlIndex] = trigger.magnitude;

Expand Down