Skip to content

CHANGE: Removed aliased values in Gamepad enum. (ISXB-543) #1983

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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Sep 19, 2024
Merged

CHANGE: Removed aliased values in Gamepad enum. (ISXB-543) #1983

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
110e5c6
ISXB-543 Implemented custom drawer base for enums as well as on spe…
adrian-koretski-unity3d Aug 7, 2024
50f2a47
Removed custom enum drawer.
adrian-koretski-unity3d Aug 13, 2024
4035fe5
Added changes to changelog.
adrian-koretski-unity3d Aug 13, 2024
60640fb
Merge branch 'develop' into isxb-543
adrian-koretski-unity3d Aug 13, 2024
3044d28
Re-added aliased enums into GamepadButton for compatibility with exis…
adrian-koretski-unity3d Aug 14, 2024
6e50417
Merge branch 'develop' of https://github.com/Unity-Technologies/Input…
adrian-koretski-unity3d Sep 9, 2024
e09b575
Ran the formatter on commit changes.
adrian-koretski-unity3d Sep 9, 2024
a964957
Merge branch 'develop' into isxb-543
adrian-koretski-unity3d Sep 10, 2024
806f943
Merge branch 'develop' into isxb-543
adrian-koretski-unity3d Sep 12, 2024
1318c70
Revert "ISXB-543 Implemented custom drawer base for enums as well a…
adrian-koretski-unity3d Sep 13, 2024
5f1b160
Merge branch 'isxb-543' of https://github.com/Unity-Technologies/Inpu…
adrian-koretski-unity3d Sep 13, 2024
e7e69d3
Reworked GpadButtonPropertyDrawer according to feedback.
adrian-koretski-unity3d Sep 16, 2024
d85c1ed
Merge branch 'develop' into isxb-543
adrian-koretski-unity3d Sep 18, 2024
dd2de37
Fixed GamepadButtonPropertyDrawer issues based on PR feedback.
adrian-koretski-unity3d Sep 18, 2024
d3fa10e
Moved changelog entry for ISXB-543 into correct version and added lin…
adrian-koretski-unity3d Sep 19, 2024
fc85832
Changed jira link in changelog for ISXB-543 to public issue tracker.
adrian-koretski-unity3d Sep 19, 2024
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
6 changes: 3 additions & 3 deletions Assets/Tests/InputSystem/CoreTests_Actions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4661,7 +4661,7 @@ public void Actions_PressingAndReleasingButtonInSameUpdate_StillTriggersAction()
ctx => { ++receivedCalls; };
action.Enable();

var firstState = new GamepadState {buttons = 1 << (int)GamepadButton.B};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix shouldn't have any impact on existing tests?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hence I wouldn't expect any changes to any tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is due to my first pass where I removed the aliased values. Will revert

var firstState = new GamepadState {buttons = 1 << (int)GamepadButton.East};
var secondState = new GamepadState {buttons = 0};

InputSystem.QueueStateEvent(gamepad, firstState);
Expand Down Expand Up @@ -6925,7 +6925,7 @@ public void Actions_CanDistinguishTapAndSlowTapOnSameAction()
trace.SubscribeTo(action);

// Perform tap.
InputSystem.QueueStateEvent(gamepad, new GamepadState().WithButton(GamepadButton.A), 0.0);
InputSystem.QueueStateEvent(gamepad, new GamepadState().WithButton(GamepadButton.South), 0.0);
InputSystem.QueueStateEvent(gamepad, new GamepadState(), 0.05);
InputSystem.Update();

Expand All @@ -6942,7 +6942,7 @@ public void Actions_CanDistinguishTapAndSlowTapOnSameAction()
trace.Clear();

// Perform slow tap.
InputSystem.QueueStateEvent(gamepad, new GamepadState().WithButton(GamepadButton.A), 2.0);
InputSystem.QueueStateEvent(gamepad, new GamepadState().WithButton(GamepadButton.South), 2.0);
InputSystem.QueueStateEvent(gamepad, new GamepadState(),
2.0 + InputSystem.settings.defaultSlowTapTime + 0.0001);
InputSystem.Update();
Expand Down
8 changes: 4 additions & 4 deletions Assets/Tests/InputSystem/CoreTests_Devices.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4160,7 +4160,7 @@ public void Devices_AreMadeCurrentWhenReceivingStateEvent()

Assert.That(Gamepad.current, Is.Not.SameAs(gamepad1));

InputSystem.QueueStateEvent(gamepad1, new GamepadState().WithButton(GamepadButton.A));
InputSystem.QueueStateEvent(gamepad1, new GamepadState().WithButton(GamepadButton.South));
InputSystem.Update();

Assert.That(Gamepad.current, Is.SameAs(gamepad1));
Expand All @@ -4179,15 +4179,15 @@ public void Devices_AreNotMadeCurrentWhenReceivingStateEventWithNoControlsChange
var gamepad1 = InputSystem.AddDevice<Gamepad>();
var gamepad2 = InputSystem.AddDevice<Gamepad>();

InputSystem.QueueStateEvent(gamepad1, new GamepadState().WithButton(GamepadButton.A));
InputSystem.QueueStateEvent(gamepad1, new GamepadState().WithButton(GamepadButton.South));
InputSystem.Update();
Assert.That(Gamepad.current, Is.SameAs(gamepad1));

InputSystem.QueueStateEvent(gamepad2, new GamepadState().WithButton(GamepadButton.B));
InputSystem.QueueStateEvent(gamepad2, new GamepadState().WithButton(GamepadButton.East));
InputSystem.Update();
Assert.That(Gamepad.current, Is.SameAs(gamepad2));

InputSystem.QueueStateEvent(gamepad1, new GamepadState().WithButton(GamepadButton.A));
InputSystem.QueueStateEvent(gamepad1, new GamepadState().WithButton(GamepadButton.South));
InputSystem.Update();

// If none of the controls changed, a state event shouldn't switch current gamepad.
Expand Down
18 changes: 9 additions & 9 deletions Assets/Tests/InputSystem/CoreTests_Events.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public void Events_CanListenForEvents()
allButtonPresses.Clear();

Release(gamepad.buttonSouth);
InputSystem.QueueStateEvent(gamepad, new GamepadState(GamepadButton.A, GamepadButton.B));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not expecting any changes needed to this file

InputSystem.QueueStateEvent(gamepad, new GamepadState(GamepadButton.South, GamepadButton.East));
InputSystem.Update();

Assert.That(callOnceOnButtonPressCount, Is.EqualTo(1));
Expand Down Expand Up @@ -135,7 +135,7 @@ public void Events_CanListenForButtonPresses()

Assert.That(callCount, Is.Zero);

InputSystem.QueueStateEvent(gamepad, new GamepadState(GamepadButton.A));
InputSystem.QueueStateEvent(gamepad, new GamepadState(GamepadButton.South));
InputSystem.Update();

Assert.That(callCount, Is.EqualTo(1));
Expand All @@ -145,7 +145,7 @@ public void Events_CanListenForButtonPresses()

Assert.That(callCount, Is.EqualTo(1));

InputSystem.QueueStateEvent(gamepad, new GamepadState(GamepadButton.A));
InputSystem.QueueStateEvent(gamepad, new GamepadState(GamepadButton.South));
InputSystem.Update();

Assert.That(callCount, Is.EqualTo(1));
Expand Down Expand Up @@ -222,7 +222,7 @@ public void Events_CanGetAllButtonPressesInEvent()
controls = eventPtr.GetAllButtonPresses().ToList();
};

InputSystem.QueueStateEvent(gamepad, new GamepadState(GamepadButton.A, GamepadButton.B));
InputSystem.QueueStateEvent(gamepad, new GamepadState(GamepadButton.South, GamepadButton.East));
InputSystem.Update();

Assert.That(controls, Is.EquivalentTo(new[] { gamepad.aButton, gamepad.bButton }));
Expand Down Expand Up @@ -1319,7 +1319,7 @@ public unsafe void Events_CanTraceEventsOfDevice_AndFilterEventsThroughCallback(

trace.Enable();

InputSystem.QueueStateEvent(gamepad, new GamepadState(GamepadButton.A));
InputSystem.QueueStateEvent(gamepad, new GamepadState(GamepadButton.South));
InputSystem.QueueStateEvent(gamepad, default(GamepadState));
InputSystem.Update();

Expand All @@ -1338,8 +1338,8 @@ public void Events_CanTraceEventsOfDevice_AndGrowBufferAsNeeded()
{
trace.Enable();

InputSystem.QueueStateEvent(device, new GamepadState().WithButton(GamepadButton.A));
InputSystem.QueueStateEvent(device, new GamepadState().WithButton(GamepadButton.B));
InputSystem.QueueStateEvent(device, new GamepadState().WithButton(GamepadButton.South));
InputSystem.QueueStateEvent(device, new GamepadState().WithButton(GamepadButton.East));

InputSystem.Update();

Expand All @@ -1358,7 +1358,7 @@ public void Events_CanTraceEventsOfDevice_AndRecordFrameBoundaries()
{
trace.Enable();

InputSystem.QueueStateEvent(gamepad, new GamepadState(GamepadButton.A));
InputSystem.QueueStateEvent(gamepad, new GamepadState(GamepadButton.South));
InputSystem.Update();

Assert.That(trace.eventCount, Is.EqualTo(2));
Expand Down Expand Up @@ -1539,7 +1539,7 @@ public void Events_CanPersistEventTracesInStream()
originalTrace.Enable();

InputSystem.QueueStateEvent(pen, new PenState { position = new Vector2(123, 234) });
InputSystem.QueueStateEvent(gamepad, new GamepadState(GamepadButton.A));
InputSystem.QueueStateEvent(gamepad, new GamepadState(GamepadButton.South));
InputSystem.QueueStateEvent(pen, new PenState { position = new Vector2(234, 345) });

InputSystem.Update();
Expand Down
6 changes: 3 additions & 3 deletions Assets/Tests/InputSystem/CoreTests_State.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ public void State_CanUpdateButtonStateUsingEvent()

Assert.That(gamepad.buttonEast.isPressed, Is.False);

var newState = new GamepadState {buttons = 1 << (int)GamepadButton.B};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not expecting any changes needed to this file

var newState = new GamepadState {buttons = 1 << (int)GamepadButton.East};
InputSystem.QueueStateEvent(gamepad, newState);
InputSystem.Update();

Expand All @@ -440,7 +440,7 @@ public void State_CanDetectWhetherButtonStateHasChangedThisFrame()
Assert.That(gamepad.buttonEast.wasPressedThisFrame, Is.False);
Assert.That(gamepad.buttonEast.wasReleasedThisFrame, Is.False);

var firstState = new GamepadState {buttons = 1 << (int)GamepadButton.B};
var firstState = new GamepadState {buttons = 1 << (int)GamepadButton.East};
InputSystem.QueueStateEvent(gamepad, firstState);
InputSystem.Update();

Expand Down Expand Up @@ -472,7 +472,7 @@ public void State_PressingAndReleasingButtonInSameFrame_ShowsStateChange(bool us

var gamepad = InputSystem.AddDevice<Gamepad>();

var firstState = new GamepadState {buttons = 1 << (int)GamepadButton.B};
var firstState = new GamepadState {buttons = 1 << (int)GamepadButton.East};
var secondState = new GamepadState {buttons = 0};

InputSystem.QueueStateEvent(gamepad, firstState);
Expand Down
2 changes: 1 addition & 1 deletion Assets/Tests/InputSystem/Plugins/UserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -942,7 +942,7 @@ public void Users_CanDetectUseOfUnpairedDevice()

++InputUser.listenForUnpairedDeviceActivity;

InputSystem.QueueStateEvent(gamepad, new GamepadState { leftStick = new Vector2(1, 0)}.WithButton(GamepadButton.A));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't need to change either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is due to my first pass where I removed the aliased values. Will revert

InputSystem.QueueStateEvent(gamepad, new GamepadState { leftStick = new Vector2(1, 0)}.WithButton(GamepadButton.South));
InputSystem.Update();

Assert.That(receivedControls, Has.Count.EqualTo(2));
Expand Down
1 change: 1 addition & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ however, it has to be formatted properly to pass verification tests.

### Changed
- Use `ProfilerMarker` instead of `Profiler.BeginSample` and `Profiler.EndSample` when appropriate to enable recording of profiling data.
- Removed aliased values in GamepadButton enum.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not want to release a major revision due to this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we plan to do so down the line? Perhaps mark the aliased values as depricated (I'm not sure how we go about doing this).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We basically shouldn't modify the enum at all, we should aim for a completely editor-based "fix" here. There is no functional issue with the current solution, its only confusing users from UX perspective.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense though to file an internal ticket to consider removing aliases for control enums down the line (in a major future release). I have now filed it.


### Added
- Added tests for Input Action Editor UI for managing action maps (List, create, rename, delete) (ISX-2087)
Expand Down
66 changes: 4 additions & 62 deletions Packages/com.unity.inputsystem/InputSystem/Devices/Gamepad.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,31 +255,31 @@ public enum GamepadButton
/// The upper action button on a gamepad.
/// </summary>
/// <remarks>
/// Identical to <see cref="Y"/> and <see cref="Triangle"/> which are the Xbox and PlayStation controller names for this button.
/// Identical to <see cref="North"/> and <see cref="North"/> which are the Xbox and PlayStation controller names for this button.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems incorrect? Referencing to the same thing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is due to my first pass where I removed the aliased values. Will revert

/// </remarks>
North = 4,

/// <summary>
/// The right action button on a gamepad.
/// </summary>
/// <remarks>
/// Identical to <see cref="B"/> and <see cref="Circle"/> which are the Xbox and PlayStation controller names for this button.
/// Identical to <see cref="East"/> and <see cref="East"/> which are the Xbox and PlayStation controller names for this button.
/// </remarks>
East = 5,

/// <summary>
/// The lower action button on a gamepad.
/// </summary>
/// <remarks>
/// Identical to <see cref="A"/> and <see cref="Cross"/> which are the Xbox and PlayStation controller names for this button.
/// Identical to <see cref="South"/> and <see cref="South"/> which are the Xbox and PlayStation controller names for this button.
/// </remarks>
South = 6,

/// <summary>
/// The left action button on a gamepad.
/// </summary>
/// <remarks>
/// Identical to <see cref="X"/> and <see cref="Square"/> which are the Xbox and PlayStation controller names for this button.
/// Identical to <see cref="West"/> and <see cref="West"/> which are the Xbox and PlayStation controller names for this button.
/// </remarks>
West = 7,

Expand Down Expand Up @@ -326,64 +326,6 @@ public enum GamepadButton
/// The right trigger button on a gamepad.
/// </summary>
RightTrigger = 33,

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot remove these from this enum without requiring a major version revision since it would break public API for existing customers (and according to semver), hence this is not a viable way forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't thought of this. I re-added the aliased enum values (although I did not reset them in the other files that used them to move towards north/south/east/west). I also added a slightly modified version of the aliased enum drawer from #1862 where we can remove values from the drawer.

/// <summary>
/// The X button on an Xbox controller.
/// </summary>
/// <remarks>
/// Identical to <see cref="West"/>, which is the generic name of this button.
/// </remarks>
X = West,
/// <summary>
/// The Y button on an Xbox controller.
/// </summary>
/// <remarks>
/// Identical to <see cref="North"/>, which is the generic name of this button.
/// </remarks>
Y = North,
/// <summary>
/// The A button on an Xbox controller.
/// </summary>
/// <remarks>
/// Identical to <see cref="South"/>, which is the generic name of this button.
/// </remarks>
A = South,
/// <summary>
/// The B button on an Xbox controller.
/// </summary>
/// <remarks>
/// Identical to <see cref="East"/>, which is the generic name of this button.
/// </remarks>
B = East,

/// <summary>
/// The cross button on a PlayStation controller.
/// </summary>
/// <remarks>
/// Identical to <see cref="South"/>, which is the generic name of this button.
/// </remarks>
Cross = South,
/// <summary>
/// The square button on a PlayStation controller.
/// </summary>
/// <remarks>
/// Identical to <see cref="West"/>, which is the generic name of this button.
/// </remarks>
Square = West,
/// <summary>
/// The triangle button on a PlayStation controller.
/// </summary>
/// <remarks>
/// Identical to <see cref="North"/>, which is the generic name of this button.
/// </remarks>
Triangle = North,
/// <summary>
/// The circle button on a PlayStation controller.
/// </summary>
/// <remarks>
/// Identical to <see cref="East"/>, which is the generic name of this button.
/// </remarks>
Circle = East,
}
}

Expand Down