Skip to content
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

FIX: ISXB-543 GamepadButton property drawer fix #1862

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

ekcoh
Copy link
Collaborator

@ekcoh ekcoh commented Feb 29, 2024

Description

FIX: ISXB-543 GamepadButton property drawer now has consistent behaviour for which aliased enum value is selected and drop-down shows alias mapping. https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-543

Original contribution by @ppandi-rythmos in #1860 routed via this PR.

Changes made

Introducing a custom aliased enum property drawer and a specialised version for GamepadButton enum type.

Notes

Easiest to test by creating a script with a GamepadButton public property and check in Inspector.

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.
    • FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
    • FogBugz is marked as "Resolved" with next release version correctly set.
  • 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.

…our for which aliased enum value is selected and drop-down shows alias mapping.
@Pauliusd01
Copy link
Collaborator

Pauliusd01 commented Feb 29, 2024

I am still reproducing the issue:

Unity_2024-02-29_11-56-25.mp4

Is it because the circle is technically the east button so It's kind of giving me the right option? But why have them split to more platform specific options then

@ekcoh
Copy link
Collaborator Author

ekcoh commented Feb 29, 2024

I am still reproducing the issue:

Unity_2024-02-29_11-56-25.mp4

Is it because the circle is technically the east button so It's kind of giving me the right option? But why have them split to more platform specific options then

This is a pure usability problem, it could be done differently. What would you suggest? That we combine the items on a single line? E.g. "North / Y / Triangle" instead? Its the same value.

Yes technically they are the same value.

@ekcoh
Copy link
Collaborator Author

ekcoh commented Feb 29, 2024

Before this PR it was completely random which one of the aliased values was being converted to string making it even more confusing.

@Pauliusd01
Copy link
Collaborator

Pauliusd01 commented Feb 29, 2024

I am still reproducing the issue:
Unity_2024-02-29_11-56-25.mp4
Is it because the circle is technically the east button so It's kind of giving me the right option? But why have them split to more platform specific options then

This is a pure usability problem, it could be done differently. What would you suggest? That we combine the items on a single line? E.g. "North / Y / Triangle" instead? Its the same value.

Yes technically they are the same value.

Thanks for explaining, yes combining them makes sense to me but it is not necessary to do in this PR. I've found that It also breaks the inspector and throws these errors when actually used in PlayMode:

NullReferenceException: Object reference not set to an instance of an object
UnityEditor.EditorGUIUtility.TempContent (System.String[] texts) (at D:/Gitrepo/unity/Editor/Mono/EditorGUIUtility.cs:1064)
UnityEditor.EditorGUI.Popup (UnityEngine.Rect position, System.String label, System.Int32 selectedIndex, System.String[] displayedOptions, UnityEngine.GUIStyle style) (at D:/Gitrepo/unity/Editor/Mono/EditorGUI.cs:8664)
UnityEditor.EditorGUI.Popup (UnityEngine.Rect position, System.String label, System.Int32 selectedIndex, System.String[] displayedOptions) (at D:/Gitrepo/unity/Editor/Mono/EditorGUI.cs:8659)
UnityEngine.InputSystem.Editor.AliasedEnumPropertyDrawer`1[T].OnGUI (UnityEngine.Rect position, UnityEditor.SerializedProperty property, UnityEngine.GUIContent label) (at ./Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/EnumPropertyDrawer.cs:56)
UnityEditor.PropertyDrawer.OnGUISafe (UnityEngine.Rect position, UnityEditor.SerializedProperty property, UnityEngine.GUIContent label) (at D:/Gitrepo/unity/Editor/Mono/Inspector/Core/ScriptAttributeGUI/PropertyDrawer.cs:28)
UnityEditor.PropertyHandler.OnGUI (UnityEngine.Rect position, UnityEditor.SerializedProperty property, UnityEngine.GUIContent label, System.Boolean includeChildren, UnityEngine.Rect visibleArea) (at D:/Gitrepo/unity/Editor/Mono/Inspector/Core/ScriptAttributeGUI/PropertyHandler.cs:195)
UnityEditor.PropertyHandler.OnGUI (UnityEngine.Rect position, UnityEditor.SerializedProperty property, UnityEngine.GUIContent label, System.Boolean includeChildren) (at D:/Gitrepo/unity/Editor/Mono/Inspector/Core/ScriptAttributeGUI/PropertyHandler.cs:155)
UnityEditor.PropertyHandler.OnGUILayout (UnityEditor.SerializedProperty property, UnityEngine.GUIContent label, System.Boolean includeChildren, UnityEngine.GUILayoutOption[] options) (at D:/Gitrepo/unity/Editor/Mono/Inspector/Core/ScriptAttributeGUI/PropertyHandler.cs:314)
UnityEditor.EditorGUILayout.PropertyField (UnityEditor.SerializedProperty property, UnityEngine.GUIContent label, System.Boolean includeChildren, UnityEngine.GUILayoutOption[] options) (at D:/Gitrepo/unity/Editor/Mono/EditorGUILayout.cs:2079)
UnityEditor.EditorGUILayout.PropertyField (UnityEditor.SerializedProperty property, System.Boolean includeChildren, UnityEngine.GUILayoutOption[] options) (at D:/Gitrepo/unity/Editor/Mono/EditorGUILayout.cs:2073)
UnityEditor.UIElements.PropertyField.<CreatePropertyIMGUIContainer>b__49_0 () (at D:/Gitrepo/unity/Editor/Mono/UIElements/Controls/PropertyField.cs:439)
UnityEngine.UIElements.IMGUIContainer.DoOnGUI (UnityEngine.Event evt, UnityEngine.Matrix4x4 parentTransform, UnityEngine.Rect clippingRect, System.Boolean isComputingLayout, UnityEngine.Rect layoutSize, System.Action onGUIHandler, System.Boolean canAffectFocus) (at D:/Gitrepo/unity/Modules/UIElements/Core/IMGUIContainer.cs:396)
UnityEngine.GUIUtility:ProcessEvent(Int32, IntPtr, Boolean&) (at D:/Gitrepo/unity/Modules/IMGUI/GUIUtility.cs:219)

This is all my script is doing:

using UnityEngine;
using UnityEngine.InputSystem.LowLevel;

public class NewMonoBehaviourScript : MonoBehaviour
{
    public GamepadButton myButton;
 
    void Update()
    {
        Debug.Log(myButton.ToString());
    }
}

And it does not reproduce in Develop

Unity_2024-02-29_13-37-32.mp4

Copy link
Collaborator

@Pauliusd01 Pauliusd01 left a comment

Choose a reason for hiding this comment

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

Updating status - bug above needs fixing

…to lazily initialize array if lost in domain reload.
@ekcoh
Copy link
Collaborator Author

ekcoh commented Feb 29, 2024

@Pauliusd01 The bug should be fixed via c1f4c9e

@ekcoh
Copy link
Collaborator Author

ekcoh commented Feb 29, 2024

I did not modify behavior so UX question remains:

a) Do we want current behavior where one aliased enumeration type is displayed e.g. "North" with aliased enums listed as e.g. "Triangle (North)" and "Y (North)". OR
b) Do we want behavior where we reduce number of items in list by collapsing them, e.g. "North / Triangle / Y".

Notice that this is due to they all mapping to same integer value so only different "labels / identifiers" for the same option (usages).

@ekcoh ekcoh requested a review from Billreyn February 29, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants