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

adrian-koretski-unity3d · 2024-08-13T19:53:50Z

Description

Fixing confusing menu when selecting face buttons due to aliased enum values.

Changes made

Removed aliased enum values.

Testing

Manual testing.

Risk

None.

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.
- JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
- Jira port for the next release set as "Resolved".
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.

After merge:

Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

…fic to gamepad buttons. Also removed aliased values in GamepadButton enum

unity-cla-assistant · 2024-08-13T19:53:55Z

All committers have signed the CLA.

ekcoh · 2024-08-13T21:07:49Z

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

        /// The right trigger button on a gamepad.
        /// </summary>
        RightTrigger = 33,
-


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.

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.

ekcoh · 2024-08-13T21:08:25Z

Packages/com.unity.inputsystem/CHANGELOG.md


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


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

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).

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.

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.

ekcoh · 2024-08-13T21:10:11Z

I would recommend considering a fix based off this one but with inlined/combined options for aliased entries? #1862

ekcoh · 2024-08-13T21:11:47Z

Seems like @Billreyn was also considering that unless opinion has shifted, see #1862 (comment)

…ting projects. Added custom aliased enum drawer base and custom enum drawer for GamepadButton to hide aliased values.

adrian-koretski-unity3d · 2024-08-14T18:31:00Z

Seems like @Billreyn was also considering that unless opinion has shifted, see #1862 (comment)

I believe so. Based on our conversation, we want to display only North/South/East/West

…System into isxb-543

ekcoh · 2024-09-13T08:52:54Z

Assets/Tests/InputSystem/CoreTests_Actions.cs

            ctx => { ++receivedCalls; };
        action.Enable();

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


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

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

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

ekcoh · 2024-09-13T08:53:36Z

Assets/Tests/InputSystem/Plugins/UserTests.cs


        ++InputUser.listenForUnpairedDeviceActivity;

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


This shouldn't need to change either.

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

ekcoh · 2024-09-13T08:55:49Z

Seems like @Billreyn was also considering that unless opinion has shifted, see #1862 (comment)

I believe so. Based on our conversation, we want to display only North/South/East/West

Exactly, with emphasis on "display", hence it should be property drawer only changes as I see it.

ekcoh · 2024-09-13T09:04:17Z

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

        /// </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.


This seems incorrect? Referencing to the same thing

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

ekcoh

Needs a bit more work I believe

ekcoh · 2024-09-13T09:10:22Z

Assets/Tests/InputSystem/CoreTests_Events.cs

        allButtonPresses.Clear();

        Release(gamepad.buttonSouth);
-        InputSystem.QueueStateEvent(gamepad, new GamepadState(GamepadButton.A, GamepadButton.B));


Not expecting any changes needed to this file

ekcoh · 2024-09-13T09:10:36Z

Assets/Tests/InputSystem/CoreTests_State.cs


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

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


Not expecting any changes needed to this file

ekcoh · 2024-09-13T09:14:15Z

Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/AliasedEnumPropertyDrawer.cs

@@ -0,0 +1,72 @@
+#if UNITY_EDITOR


I am afraid this is not doing what its supposed to.....

The following things need to be addressed:

Aliased values should be part of the same entry when the inspector is displayed, e.g. "South / X / A"

Drop down need to allow all values to be picked, e.g. either "South / X / A" or each one individually would be fine from my perspective.

At the moment when selecting e.g. South, the value displayed is "North", so something with mapping is incorrect

I'll go over it and make sure it behaves and displays correctly

ekcoh · 2024-09-13T09:16:20Z

Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/AliasedEnumPropertyDrawer.cs

+
+        private void ProcessDisplayNamesForAliasedEnums()
+        {
+            var enumNamesAndValues = new Dictionary<string, int>();


Since the enum is small and we want consistency, maybe skip this base class and just move into specific GamepadButtonPropertyDrawer class and just use a big switch statement to get it right? That would be quite straightforward. Otherwise there needs to be some smartness to get the order of each aliased combination separated by slashes consistent.

…on specific to gamepad buttons. Also removed aliased values in GamepadButton enum" This reverts commit 110e5c6. # Conflicts: # Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/AliasedEnumPropertyDrawer.cs.meta # Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/GpadButtonPropertyDrawer.cs.meta

…tSystem into isxb-543

Removed general AliasedEnumPropertyDrawer script.

lyndon-unity

Code looks reasonable, one minor recommendation, but I haven't tested it in action.
Suggest adding QA or design for a manual verification

lyndon-unity · 2024-09-17T14:52:48Z

Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/GpadButtonPropertyDrawer.cs

+    /// Property drawer for <see cref = "GamepadButton" />
+    /// </summary >
+    [CustomPropertyDrawer(typeof(GamepadButton))]
+    internal class GpadButtonPropertyDrawer : PropertyDrawer


Minor but can you use a full name here rather than abbreviation
GpadButtonPropertyDrawer ->
GamepadButtonPropertyDrawer

Which also means renaming file and .meta

ekcoh

Approving this to not block this PR in any way while I have limited accessability. I didn't run it on this review round so recommend checking functionality in QA pass.

ekcoh · 2024-09-17T20:45:50Z

Packages/com.unity.inputsystem/CHANGELOG.md


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


This should be reverted as well, suggest to change to something like e.g. "Changed representation of GamepadButton enum values in Inspector to display aliased enum values as a single item to avoid confusion around selection and aliased value display when multiple enum items map to the same numerical value."

ekcoh · 2024-09-17T20:46:01Z

Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/GpadButtonPropertyDrawer.cs

+    /// Property drawer for <see cref = "GamepadButton" />
+    /// </summary >
+    [CustomPropertyDrawer(typeof(GamepadButton))]
+    internal class GpadButtonPropertyDrawer : PropertyDrawer


ekcoh · 2024-09-17T20:46:17Z

Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/GpadButtonPropertyDrawer.cs

+    /// Property drawer for <see cref = "GamepadButton" />
+    /// </summary >
+    [CustomPropertyDrawer(typeof(GamepadButton))]
+    internal class GpadButtonPropertyDrawer : PropertyDrawer


Which also means renaming file and .meta

ekcoh · 2024-09-17T20:47:38Z

Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/GpadButtonPropertyDrawer.cs

+
+            for (var i = 0; i < enumDisplayNames.Length; ++i)
+            {
+                string enumName = enumDisplayNames[i];


This assignment could be on line 40 right since overwritten for the special cases?

ekcoh · 2024-09-17T20:48:09Z

Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/GpadButtonPropertyDrawer.cs

+
+                if (enumName != null)
+                {
+                    enumNamesAndValues.Add(enumName, (int)enumValues.GetValue(i));


Could be moved up to the place where it is currently assigned as null?

I changed it up a little bit. For the cases where we do not want to enter the enum into the list, I continue the for loop, eliminating the need for null/null check. The switch statement sets an string value which is then entered into the list.

Pauliusd01 · 2024-09-18T12:54:36Z

Will check this today or tomorrow morning at the latest

Pauliusd01

Entering play mode with one of these new values selected breaks inspector and throws errors. Also, the enum value does not match the one in the inspector (See debug log in screenshot, whether that's expected I dunno)

-Renamed GpadButtonPropertyDrawer to GamepadButtonPropertyDrawer. -Fixed nullref issue caused by m_EnumDisplayNames being reset when entering/exiting play mode. -Simplified code for setting up enum list. -Updated CHANGELOG.md

adrian-koretski-unity3d · 2024-09-18T14:43:25Z

Entering play mode with one of these new values selected breaks inspector and throws errors. Also, the enum value does not match the one in the inspector (See debug log in screenshot, whether that's expected I dunno)

The nullref issue should be fixed now. The value being displayed will be one of the aliased enums for that value so this is to be expected.

Pauliusd01 · 2024-09-19T06:49:23Z

Packages/com.unity.inputsystem/CHANGELOG.md


 ### Changed
 - Use `ProfilerMarker` instead of `Profiler.BeginSample` and `Profiler.EndSample` when appropriate to enable recording of profiling data.
+- Changed representation of GamepadButton enum values in Inspector to display aliased enum values as a single item to avoid confusion around selection and aliased value display when multiple enum items map to the same numerical value.


Changelog entry in the wrong version and no bug issue link

…k to jira.

Pauliusd01 · 2024-09-19T13:24:21Z

@adrian-koretski-unity3d I approved too hastily, the link has to be to a public issue tracker not our internal jira lol

adrian-koretski-unity3d added 3 commits August 7, 2024 14:38

ISXB-543 Implemented custom drawer base for enums as well as on speci…

110e5c6

…fic to gamepad buttons. Also removed aliased values in GamepadButton enum

Removed custom enum drawer.

50f2a47

Added changes to changelog.

4035fe5

Merge branch 'develop' into isxb-543

60640fb

ekcoh requested changes Aug 13, 2024

View reviewed changes

Re-added aliased enums into GamepadButton for compatibility with exis…

3044d28

…ting projects. Added custom aliased enum drawer base and custom enum drawer for GamepadButton to hide aliased values.

adrian-koretski-unity3d added 3 commits September 9, 2024 13:52

Merge branch 'develop' of https://github.com/Unity-Technologies/Input…

6e50417

…System into isxb-543

Ran the formatter on commit changes.

e09b575

Merge branch 'develop' into isxb-543

a964957

adrian-koretski-unity3d requested a review from ekcoh September 10, 2024 17:41

Merge branch 'develop' into isxb-543

806f943

ekcoh reviewed Sep 13, 2024

View reviewed changes

ekcoh requested changes Sep 13, 2024

View reviewed changes

adrian-koretski-unity3d added 3 commits September 13, 2024 13:32

Merge branch 'isxb-543' of https://github.com/Unity-Technologies/Inpu…

5f1b160

…tSystem into isxb-543

Reworked GpadButtonPropertyDrawer according to feedback.

e7e69d3

Removed general AliasedEnumPropertyDrawer script.

lyndon-unity approved these changes Sep 17, 2024

View reviewed changes

lyndon-unity mentioned this pull request Sep 17, 2024

FIX: ISXB-543 GamepadButton property drawer fix #1862

Closed

4 tasks

lyndon-unity requested a review from Pauliusd01 September 17, 2024 15:05

ekcoh approved these changes Sep 17, 2024

View reviewed changes

Merge branch 'develop' into isxb-543

d85c1ed

Pauliusd01 changed the title ~~CHANGE: Isxb 543 Removed aliased values in Gamepad enum.~~ CHANGE: Removed aliased values in Gamepad enum. (ISXB-543) Sep 18, 2024

Pauliusd01 requested changes Sep 18, 2024

View reviewed changes

adrian-koretski-unity3d requested a review from Pauliusd01 September 18, 2024 17:08

Pauliusd01 reviewed Sep 19, 2024

View reviewed changes

Moved changelog entry for ISXB-543 into correct version and added lin…

d3fa10e

…k to jira.

Pauliusd01 self-requested a review September 19, 2024 13:23

Pauliusd01 approved these changes Sep 19, 2024

View reviewed changes

Changed jira link in changelog for ISXB-543 to public issue tracker.

fc85832

adrian-koretski-unity3d merged commit ddc99ca into develop Sep 19, 2024
77 checks passed

adrian-koretski-unity3d deleted the isxb-543 branch September 19, 2024 15:01


		++InputUser.listenForUnpairedDeviceActivity;

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


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

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

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

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

Uh oh!

Conversation

adrian-koretski-unity3d commented Aug 13, 2024

Description

Changes made

Testing

Risk

Checklist

Uh oh!

unity-cla-assistant commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekcoh commented Aug 13, 2024

Uh oh!

ekcoh commented Aug 13, 2024

Uh oh!

adrian-koretski-unity3d commented Aug 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekcoh commented Sep 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekcoh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyndon-unity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekcoh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

unity-cla-assistant commented Aug 13, 2024 •

edited

Loading

Pauliusd01 left a comment •

edited

Loading