Skip to content

Conversation

@jfreire-unity
Copy link
Collaborator

Description

https://jira.unity3d.com/browse/ISX-1699

Changes made

  • Sets the default value for the PlayerInput asset on Reset(), which is called when the component is first added in the inspector.
  • Checks for changes not captured by EditorGUI.Begin/EndChangeCheck when users call "Reset
    image

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.

Copy link
Collaborator

@ekcoh ekcoh left a comment

Choose a reason for hiding this comment

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

In general looks good, and excellent you thought of Reset feature, but confused why we need to have these editor changes? Realize know that we maybe cannot do this since it would not be backwards compatible if introduced as the default value inside the component so its solved via editor?

@jfreire-unity
Copy link
Collaborator Author

jfreire-unity commented Nov 3, 2023

In general looks good, and excellent you thought of Reset feature, but confused why we need to have these editor changes? Realize know that we maybe cannot do this since it would not be backwards compatible if introduced as the default value inside the component so its solved via editor?

@ekcoh Yeah, I should have explained better the reasoning for adding the CheckIfActionAssetChanged().
Reset() is called both when the component is first created, and when users press "Reset" to that component in the UI.
When pressing Reset(), the OnActionAssetChange() method was not being called which populates this part of the UI:
image
This would result in just showing a partial part of the UI until you disable/enable the Inspector. E.g. opening a different GameObject in the scene which, and going back to the GameObject would show the correct UI.
See below the behavior that I'm fixing with my editor changes:

Kapture 2023-11-03 at 13 08 58

The PR fixes the above behavior. Once you press Reset, OnActonAssetChange is called and the UI is populated accordingly.

@jfreire-unity jfreire-unity force-pushed the isx-1699-player-input-default-project-wide-actions branch from 9540ccd to 7217ebd Compare November 6, 2023 09:31
@Pauliusd01
Copy link
Collaborator

Pauliusd01 commented Nov 6, 2023

Found this ArgumentException, steps to reproduce are niche so not sure if It's important/worth fixing if It's too hard. Steps to repro: Open the properties for the PlayerInput component -> Reset the component in the properties window (the original inspector does not need to be opened, this is not a 2 inspector bug) -> observe the error:


ArgumentException: Getting control 1's position in a group with only 1 controls when doing repaint
Aborting
UnityEngine.GUILayoutGroup.GetNext () (at D:/Gitrepo/unity/Modules/IMGUI/LayoutGroup.cs:129)
UnityEngine.GUILayoutUtility.DoGetRect (UnityEngine.GUIContent content, UnityEngine.GUIStyle style, UnityEngine.GUILayoutOption[] options) (at D:/Gitrepo/unity/Modules/IMGUI/GUILayoutUtility.cs:462)
UnityEngine.GUILayoutUtility.GetRect (UnityEngine.GUIContent content, UnityEngine.GUIStyle style, UnityEngine.GUILayoutOption[] options) (at D:/Gitrepo/unity/Modules/IMGUI/GUILayoutUtility.cs:419)
UnityEngine.GUILayout.DoButton (UnityEngine.GUIContent content, UnityEngine.GUIStyle style, UnityEngine.GUILayoutOption[] options) (at D:/Gitrepo/unity/Modules/IMGUI/GUILayout.cs:36)
UnityEngine.GUILayout.Button (UnityEngine.GUIContent content, UnityEngine.GUILayoutOption[] options) (at D:/Gitrepo/unity/Modules/IMGUI/GUILayout.cs:30)
UnityEngine.InputSystem.Editor.InputActionAssetDrawer.DoOpenAssetButtonUI (UnityEditor.SerializedProperty property, UnityEngine.InputSystem.Editor.AssetOptions selected) (at ./Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/InputActionAssetDrawer.cs:68)
UnityEngine.InputSystem.Editor.InputActionAssetDrawer.OnGUI (UnityEngine.Rect position, UnityEditor.SerializedProperty property, UnityEngine.GUIContent label) (at ./Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/InputActionAssetDrawer.cs:38)
UnityEditor.PropertyDrawer.OnGUISafe (UnityEngine.Rect position, UnityEditor.SerializedProperty property, UnityEngine.GUIContent label) (at D:/Gitrepo/unity/Editor/Mono/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/ScriptAttributeGUI/PropertyHandler.cs:193)
UnityEditor.PropertyHandler.OnGUI (UnityEngine.Rect position, UnityEditor.SerializedProperty property, UnityEngine.GUIContent label, System.Boolean includeChildren) (at D:/Gitrepo/unity/Editor/Mono/ScriptAttributeGUI/PropertyHandler.cs:153)
UnityEditor.PropertyHandler.OnGUILayout (UnityEditor.SerializedProperty property, UnityEngine.GUIContent label, System.Boolean includeChildren, UnityEngine.GUILayoutOption[] options) (at D:/Gitrepo/unity/Editor/Mono/ScriptAttributeGUI/PropertyHandler.cs:307)
UnityEditor.EditorGUILayout.PropertyField (UnityEditor.SerializedProperty property, UnityEngine.GUIContent label, System.Boolean includeChildren, UnityEngine.GUILayoutOption[] options) (at D:/Gitrepo/unity/Editor/Mono/EditorGUILayout.cs:2086)
UnityEditor.EditorGUILayout.PropertyField (UnityEditor.SerializedProperty property, UnityEngine.GUILayoutOption[] options) (at D:/Gitrepo/unity/Editor/Mono/EditorGUILayout.cs:2070)
UnityEngine.InputSystem.Editor.PlayerInputEditor.OnInspectorGUI () (at ./Packages/com.unity.inputsystem/InputSystem/Plugins/PlayerInput/PlayerInputEditor.cs:75)
UnityEditor.UIElements.InspectorElement+<>c__DisplayClass74_0.<CreateInspectorElementUsingIMGUI>b__0 () (at D:/Gitrepo/unity/Editor/Mono/UIElements/Inspector/InspectorElement.cs:727)
UnityEngine.DebugLogHandler:Internal_LogException_Injected(Exception, IntPtr)
UnityEngine.DebugLogHandler:Internal_LogException(Exception, Object)
UnityEngine.DebugLogHandler:LogException(Exception, Object)
UnityEngine.Logger:LogException(Exception, Object)
UnityEngine.Debug:LogException(Exception)
UnityEditor.UIElements.<>c__DisplayClass74_0:<CreateInspectorElementUsingIMGUI>b__0() (at D:/Gitrepo/unity/Editor/Mono/UIElements/Inspector/InspectorElement.cs:738)
UnityEngine.UIElements.IMGUIContainer:DoOnGUI(Event, Matrix4x4, Rect, Boolean, Rect, Action, Boolean) (at D:/Gitrepo/unity/Modules/UIElements/Core/IMGUIContainer.cs:394)
UnityEngine.UIElements.IMGUIContainer:HandleIMGUIEvent(Event, Matrix4x4, Rect, Action, Boolean) (at D:/Gitrepo/unity/Modules/UIElements/Core/IMGUIContainer.cs:709)
UnityEngine.UIElements.IMGUIContainer:DoIMGUIRepaint() (at D:/Gitrepo/unity/Modules/UIElements/Core/IMGUIContainer.cs:574)
UnityEngine.UIElements.UIR.RenderChainCommand:ExecuteNonDrawMesh(DrawParams, Single, Exception&) (at D:/Gitrepo/unity/Modules/UIElements/Core/Renderer/UIRenderer/UIRenderers.cs:134)
UnityEngine.UIElements.UIR.UIRenderDevice:EvaluateChain(RenderChainCommand, Material, Material, Texture, Texture, Single, Exception&) (at D:/Gitrepo/unity/Modules/UIElements/Core/Renderer/UIRenderer/UIRenderDevice.cs:1017)
UnityEngine.UIElements.UIR.RenderChain:Render() (at D:/Gitrepo/unity/Modules/UIElements/Core/Renderer/UIRRenderChain.cs:464)
UnityEngine.UIElements.UIRRepaintUpdater:Update() (at D:/Gitrepo/unity/Modules/UIElements/Core/Renderer/UIRRepaintUpdater.cs:84)
UnityEngine.UIElements.VisualTreeUpdater:UpdateVisualTreePhase(VisualTreeUpdatePhase) (at D:/Gitrepo/unity/Modules/UIElements/Core/VisualTreeUpdater.cs:114)
UnityEngine.UIElements.Panel:UpdateForRepaint() (at D:/Gitrepo/unity/Modules/UIElements/Core/Panel.cs:1187)
UnityEngine.UIElements.Panel:Repaint(Event) (at D:/Gitrepo/unity/Modules/UIElements/Core/Panel.cs:1251)
UnityEngine.UIElements.UIElementsUtility:DoDispatch(BaseVisualElementPanel) (at D:/Gitrepo/unity/Modules/UIElements/Core/UIElementsUtility.cs:475)
UnityEngine.UIElements.UIElementsUtility:UnityEngine.UIElements.IUIElementsUtility.ProcessEvent(Int32, IntPtr, Boolean&) (at D:/Gitrepo/unity/Modules/UIElements/Core/UIElementsUtility.cs:225)
UnityEngine.UIElements.UIEventRegistration:ProcessEvent(Int32, IntPtr) (at D:/Gitrepo/unity/Modules/UIElements/Core/UIElementsUtility.cs:74)
UnityEngine.UIElements.<>c:<.cctor>b__1_2(Int32, IntPtr) (at D:/Gitrepo/unity/Modules/UIElements/Core/UIElementsUtility.cs:28)
UnityEngine.GUIUtility:ProcessEvent(Int32, IntPtr, Boolean&) (at D:/Gitrepo/unity/Modules/IMGUI/GUIUtility.cs:206)

Unity_2023-11-06_11-48-46.mp4

(In the video I do the steps with your branch and then the same with Develop)

@jfreire-unity
Copy link
Collaborator Author

jfreire-unity commented Nov 6, 2023

Found this ArgumentException, steps to reproduce are niche so not sure if It's important/worth fixing if It's too hard. Steps to repro: Open the properties for the PlayerInput component -> Reset the component in the properties window (the original inspector does not need to be opened, this is not a 2 inspector bug) -> observe the error:

(In the video I do the steps with your branch and then the same with Develop)

Ahh, good catch. However, I didn't find a quick solution and, like you said, it's not critical. Seems to be very specific to that new property window and related to the "Open" button of the "project-wide actions". If I remove it the code that it's drawing the button, there's no error. Can you open a JIRA issue for it? I can investigate and fix it later.

@jfreire-unity
Copy link
Collaborator Author

Bypassed last step of CI because I only did a changelog update.

@jfreire-unity jfreire-unity merged commit 70d769a into develop Nov 6, 2023
@jfreire-unity jfreire-unity deleted the isx-1699-player-input-default-project-wide-actions branch November 6, 2023 11:26
jamesmcgill pushed a commit that referenced this pull request Nov 8, 2023
* Add Reset() to default values

This event is called once the component is added for the first time https://docs.unity3d.com/2022.3/Documentation/ScriptReference/MonoBehaviour.Reset.html

* Check for changes on "Reset" not captured by EndChangeCheck()

Without this, changes will not be detected when a user selects "Reset" in the Component Inspector.

* Update CHANGELOG

* Add project-wide actions define

* Only reference InputSystem.Editor on the Editor
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.

6 participants