Skip to content

Conversation

@ekcoh
Copy link
Collaborator

@ekcoh ekcoh commented Nov 2, 2023

Description

Restructured main Input System Package settings node to be Input Actions and Settings to be a child node when Project Wide Actions are available. If Project Wide Actions is not available structure is unchanged.

This implements ISX-1554.

Changes made

Extracted common settings path string into its own constant.
Introduced preconditional compile time checks to use different paths depending on Project Wide Actions availability derived from preprocessor symbol defines.

Notes

Recommendations for QA/testing:

  • Visual inspection of desired result
  • Double check editors open as expected via SettingsService, e.g. when clicking "Edit/Open Asset" in Inspectors.
  • Double check visual appearance/structure on Unity version supporting Project Wide Actions and on at least one version that do not.

Unfortunately not covered by automated tests.

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.

…put Actions and Settings to be a child node when Project Wide Actions are available.
Copy link
Collaborator

@jamesmcgill jamesmcgill left a comment

Choose a reason for hiding this comment

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

I think this should be tested to make sure existing projects with InputSettings still use those settings in the Player.
Otherwise lgtm.

@ekcoh
Copy link
Collaborator Author

ekcoh commented Nov 3, 2023

@jamesmcgill Cannot see why the UI structure path would affect that but why not test it.

@ekcoh ekcoh requested a review from stefanunity November 3, 2023 11:37
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.

LGTM (?) Here's what I checked (Tested on both 2020.3 and latest trunk Editor):

  • Checked that Input Manager, Input System Settings tabs still work correctly (Input Manager tab still had the UUM-53925 bug but this should already be fixed in up to date Editors)
  • Checked that Input System Settings asset Edit button works and takes you to the right place
  • Checked that PlayerInput and Input SystemUI module components still find ProjectWideActions correctly when selecting it in the dropdown and that its open button works

Things I have to note, I noticed that on the trunk Editor I can sometimes get this UITK error when pressing "Open Input Settings Window" for the input settings asset:

 Expected a visual element called 'control-schemes-toolbar-menu' of type 'UnityEditor.UIElements.ToolbarMenu' to exist but none was found.
UnityEngine.Debug:ExtractStackTraceNoAlloc (byte*,int,string)
UnityEngine.StackTraceUtility:ExtractStackTrace () (at D:/Gitrepo/unity/Runtime/Export/Scripting/StackTrace.cs:37)
UnityEngine.DebugLogHandler:Internal_Log (UnityEngine.LogType,UnityEngine.LogOption,string,UnityEngine.Object)
UnityEngine.DebugLogHandler:LogFormat (UnityEngine.LogType,UnityEngine.Object,string,object[])
UnityEngine.Logger:Log (UnityEngine.LogType,object)
UnityEngine.Debug:LogError (object)
UnityEngine.InputSystem.Editor.StateContainer:<Dispatch>b__6_0 () (at ./Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/StateContainer.cs:49)
UnityEngine.UIElements.VisualElement/SimpleScheduledItem:PerformTimerUpdate (UnityEngine.UIElements.TimerState) (at D:/Gitrepo/unity/Modules/UIElements/Core/VisualElementScheduler.cs:346)
UnityEngine.UIElements.TimerEventScheduler:UpdateScheduledEvents () (at D:/Gitrepo/unity/Modules/UIElements/Core/Scheduler.cs:363)
UnityEngine.UIElements.UIElementsUtility:UnityEngine.UIElements.IUIElementsUtility.UpdateSchedulers () (at D:/Gitrepo/unity/Modules/UIElements/Core/UIElementsUtility.cs:273)
UnityEngine.UIElements.UIEventRegistration:UpdateSchedulers () (at D:/Gitrepo/unity/Modules/UIElements/Core/UIElementsUtility.cs:105)
UnityEditor.RetainedMode:UpdateSchedulers () (at D:/Gitrepo/unity/Modules/UIElementsEditor/RetainedMode.cs:55)

Do you think this could be related to your changes? It sounded so generic so I brushed it off but still mentioning anyway just in case

@stefanunity stefanunity removed their request for review November 3, 2023 13:35
@ekcoh
Copy link
Collaborator Author

ekcoh commented Nov 3, 2023

@Pauliusd01 I believe that log message is not related to changes on this branch, but rather to our uxml setup, I believe we should file it separately

@ekcoh
Copy link
Collaborator Author

ekcoh commented Nov 4, 2023

@Pauliusd01 I filed ISX-1721 with the issue you reported. Assigned it to you if you have additional information.

@ekcoh ekcoh merged commit fbee9b1 into develop Nov 4, 2023
@ekcoh ekcoh deleted the reorganized-settings-structure branch November 4, 2023 14:46
jamesmcgill pushed a commit that referenced this pull request Nov 8, 2023
…oject wide actions are available (#1791)

* CHANGE: Restructured main Input System Package settings node to be Input Actions and Settings to be a child node when Project Wide Actions are available.
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.

5 participants