CHANGE: ISX-1865 Switch from InputManager.asset to asset-based storage for Project-wide actions #1834

ekcoh · 2024-02-07T15:17:27Z

Description

This branch switches from using InputManager.asset for storing project-wide actions to instead store them in an appointed .inputactions asset.

Changes made

Introduces new UX in Project Settings > Input Actions that prompts the user to either select an existing asset or to create a new one in order to get setup with Project-wide actions. If a new asset is created or selected this is at the same time assigned to the static property InputSystem.actions. If the default target filename already exist the user is prompted to overwrite or cancel the operation.
Introduces new UX via Inspector when a .inputactions asset is selected to display whether its the current Project-wide actions or not and to also make it the active asset if its not currently the active asset. Open button for regular assets opens assets in a Input Actions Editor window. When the project-wide asset is selected there is also an option to Edit In Project Settings.
Double clicking a .inputactions asset opens it in a Input Actions Editor window regardless if its the active Project-wide asset or not.
When InputSystem.actions is assigned either via editor or via code, the appointed project-wide asset is changed and also persisted in Editor Build Settings as an asset reference (Config-object). This object reference is fetched during build to embed a preloaded asset for player builds that can be loaded via Resources. This is similar to how InputSettings are handled.
After a round of Unity assert import has finished, Input System now attempts to extract any actions residing in InputManager.asset into a project-wide Input Actions asset instead. This is currently guarded with a static variable to only happen once per domain reload (throttling).
All write operations to InputManager.asset have been removed.
Adds a new API InputSystem.onActionsChanged which allows monitoring any changes to the property.
All previous code for writing Project-wide actions into ProjectSettings/InputManager.asset has been removed.
Project-wide actions are enabled on entering play-mode () and disabled when exiting play-mode (OnEnterEditMode).
Added "Remove All Action Maps" option to Project-wide Input Action Editor context menu to allow a user to quickly remove default template if desired.
Removed tests from CoreTests_ProjectWideActions which no longer makes sense to tests.
Removed tests from CoreTests_ProjectWideActions which overlap with tests in CoreTests_Actions.cs.
Modified Input Actions Editor Reset operation to only operate on the current in-memory version of the asset making it Undoable.
Added logic to handle the case of Project-wide .inputactions being deleted while the editor is open.
Reworked object pickers for InputActionAsset (affecting e.g. PlayerInput editor) and InputActionReference to also visualise Project-wide assets in a separate tab compared to regular assets.

Minor/technical:

Removed redundant code for saving InputActionAssets and relocated to InputActionAssetManager. Eliminated redundant dialog strings by utilising a common utility function regardless of editor framework.
Refactored InputActionImporter.
Modified CopyPasteHelpers, SerializationHelpers to allow using buffer modification for any string buffer to enable utilising this functionality when replacing action maps within an existing asset.
InputForUI monitors changes to project-wide actions configuration and handles case of an initial null-asset

Open/known issues that might need fixing before merge:

Changelog needs update based on this list.
Temporarily disabled test CoreTests_ProjectWideActions#ProjectWideActions_ThrowsWhenAddingOrRemovingWhileEnabled (added recently by @timkeo) since its unclear how it should be treated in this new concepts. Needs to be determined.
Not directly related to changes on this branch but it seems InputForUI assumes Project-wide actions is available and the separate assembly lacks any symbol definition to conditionally compile.
Consider adding a mechanic to handle the scenario of project-wide asset being open simultaneously in regular editor and Project-Settings. In this case, and to simplify implementation, we could hide the editor in Project Settings and replace it with a disclaimer, e.g. "The asset is currently being edited in a separate Input Action Editor" with a shortcut to move to that window. Basically Project Settings could back off if another editor with the asset is open and otherwise show the UI again. Probably a UX discussion topic @Billreyn .
Need to investigate/test properly that Project-Settings editor and windowed-editor both react to external asset updates when already open. Basically, if source file is modified while editor is open there are two scenarios. 1) Asset unmodified, and editor should reload latest content and mark itself as not-dirty. Other scenario is that in-memory changes exist and in that case user need to be prompted whether to overwrite file on disc with editor version or discard changes and replace editor content with asset from disk.

Notes

Suggestion for QA:

Make sure to cover usage of Project-wide actions in built player.
Make sure to cover usage of Project-wide actions when entering/exiting play-mode.
Make sure to cover editor workflows relating to Project-wide asset.
Good to check corner cases like moving, deleting, renaming project-wide-asset and doing such operations when editors are open.
Test in game UI toolkit control with valid and with null Project-wide actions

For any issues found we need to decide on a case-by-case basis whether it should be handled as a separate issue (and get this merged) or whether its an issue that may risk this concept not holding up and then we should fix it on this branch before merge.

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.

lyndon-unity · 2024-02-08T20:01:08Z

If I follow these steps I see a problem:

Create new project and add input system package
Open project settings and click the create asset button - this works to create the asset, but the create button remains as the asset contents are displayed
Close project settings
Reopen project settings - asset is found and shown, no button for create
Delete the asset from the database
Open project settings again and as no asset, click create button - works to create it and show it
Close project settings
Reopen - asset is not found/shown in UI - so create button still present on its own (but asset is present in the database)

Problem seems to be related to the InputSystem.asset assignment. The check to see if the asset is already assigned passes as the assigned value has a instanceId that matches.

Note - restarting editor and the asset is found again when opening the project settings
9 - restart editor
10 - open project settings - asset is found !

In step 5 deleting the asset from the database, the InputSystem.actions still has a reference to that removed file.
It seems that when assigning the newly created file in 6 it checks if InputSystem.actions is the same as the new asset before assigning and the instanceID matches so it doesn't assign it over.

if we selected 'none' or a different asset and then reselect the newly created asset then the UI updates.

lyndon-unity · 2024-02-08T20:02:38Z

Also the UX is different to the flow for creating the input settings - these don't pop up a location dialog

Input settings don't ask for location when button clicked - they only ask for location when using the version from the menu on the cog icon (which isn't present in the project wide input actions editor ATM)

^ I've addressed this by making the button write the asset to a fixed location.
Adding the selector to be able to pick an asset from other locations has also helped.
Currently you still can't create an asset in a random location (unless you use the project browser)

Adding a drop down like the IMGUI settings version is more work and not critical in first pass.

Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/AssetContext.cs

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Commands/Commands.cs

Pauliusd01 · 2024-02-09T12:07:12Z

Was asked to take a look early, here's my initial notes:

Does the pre-filled asset name have to be a generic "InputSystem.Actions.inputactions" ? Why not something more clear? [Optional]
ProjectWideActions test scene in main repo does not work with this PR's changes (with newly created asset that has the default name)
PlayerInput component is broken if you don't already have the ProjectWideActions asset created. You should be able to use the dropdown to use a custom actions asset if you don't want to create a projectwideactions but currently dropdown does nothing. Also, if you do use the built in button to create a projectwideactions asset then the component is still broken - showing you the custom actions view instead.
ProjectWideActions reset button does nothing.
The name of the currently used asset should be visible somewhere in the Project Settings -> ProjectWideActions view as you can load different ones now. [Optional]
Text explaining what ProjectWideActions are and why the user might want it next to the "Create new a new Project Wide Actions Asset" button would be good to have [Optional]

Fixed up some styles for the header

lyndon-unity · 2024-02-09T13:56:30Z

The name of the currently used asset should be visible somewhere in the Project Settings -> ProjectWideActions view as you can load different ones now. [Optional]

This is now added

Fixed name in the json data when a new input action asset is created from the template

lyndon-unity · 2024-02-09T15:06:05Z

ProjectWideActions reset button does nothing.

This is now fixed

lyndon-unity · 2024-02-09T15:43:04Z

I'm seeing some issues in 2022.3.12f1 when selecting an input action asset

NullReferenceException: Object reference not set to an instance of an object
UnityEngine.InputSystem.Editor.InputActionImporterEditor.OnInspectorGUI () (at ./Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporterEditor.cs:47)
UnityEditor.UIElements.InspectorElement+<>c__DisplayClass72_0.b__0 () (at <53ddbed73faf4fe3b980a493ab4e6639>:0)
UnityEngine.GUIUtility:ProcessEvent(Int32, IntPtr, Boolean&)

Appears to be an import error

My latest commit fixes this

…putSystem.actions is null InputSystem.actions is null at the start when no project wide input actions is set

(Default project wide input actions could be added to the project, but a user won't necessarily have them so adding messages so the user would know what to do if they are missing)

lyndon-unity · 2024-02-09T17:04:56Z

ProjectWideActions test scene in main repo does not work with this PR's changes (with newly created asset that has the default name)

I think you mean ProjectWideActionsSampleScene
I've just fixed this to show a message if the InputSystem.actions is null and also to enabled them if they are not enabled (which they are not in the PR at present)

…yEngine.Object

ritamerkl

Hard to grasp the full logic at once. Just leaving minor comments, I think this is worth testing wisely (also considering the changes in CopyPaste).

Packages/com.unity.inputsystem/InputSystem/Editor/ProjectWideActions/ProjectWideActionsAsset.cs

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Commands/Commands.cs

Packages/com.unity.inputsystem/InputSystem/InputManager.cs

AlexTyrer · 2024-02-16T17:33:20Z

First pass read-through looks good (long though, there's a lot to look at!)

Will try some more testing on Monday.

Packages/com.unity.inputsystem/InputSystem/Editor/ProjectWideActions/ProjectWideActionsAsset.cs

...m.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/InputActionsEditorSettingsProvider.cs

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

ekcoh · 2024-02-17T06:37:29Z

Assets/Tests/InputSystem/CoreTests_ProjectWideActions.cs

@@ -116,7 +116,7 @@ private void Destroy(Object obj)
 #if UNITY_EDITOR
        Object.DestroyImmediate(obj);
 #else
-        Object.Destroy(actions);
+        Object.DestroyImmediate(actions);


According to documentation of Object.DestroyImmediate and Object.Destroy that should not be done? https://docs.unity3d.com/ScriptReference/Object.DestroyImmediate.html

ekcoh · 2024-02-17T06:39:48Z

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

@@ -3680,9 +3680,6 @@ private static void InitializeInPlayer(IInputRuntime runtime = null, InputSettin
            if (settings == null)
                settings = Resources.FindObjectsOfTypeAll<InputSettings>().FirstOrDefault() ?? ScriptableObject.CreateInstance<InputSettings>();

-            if (actions == null)


Removing this would completely bypass player loading of preloaded asset? Cannot see how this change is valid and would like to revert it. This area anyway need to be addressed additionally since there are existing problems with this an InputForUI integration. There are also related problems to static object stack push/pop within InputSystem/InputManager.

Good spot.

I've reveted this and fixed 2 of the tests instead, with the knowledge that this can pickup an Asset from the test itself.

I am concerned with this way of loading the InputActionAsset though. If there are multiple in the project then it seems this could load the wrong one.

Agree that this is too brittle, seems to be the same case for InputSwttings from previously. Likely needs an internal property serialized that allows tagging it unless there is a better way or use a dedicated path. This needs a ticket and a test case.

ekcoh · 2024-02-17T06:53:06Z

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

@@ -3524,6 +3555,12 @@ internal static void InitializeInEditor(IInputRuntime runtime = null)
            }
            s_SystemObject.newInputBackendsCheckedAsEnabled = true;

+#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
+            // Make sure project wide input actions are enabled
+            if (actions != null)


This is the second time actions are enabled in this code path since also done via Reset above. Needs to be checked which of these is correct since Reset is also invoked when static state stack is pushed/popped.

The one in Reset is not used in Editor during the initialisation flow as its too early and at that point the value is null. So this line is needed then.

ekcoh · 2024-02-17T07:25:59Z

Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs

@@ -80,6 +80,8 @@ public void Initialize()

            m_Cfg = Configuration.GetDefaultConfiguration();
            RegisterActions(m_Cfg);
+


InputSystem.actions and InputSystem.onActionsChange are only available when UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS is defined, but there are no symbolic checks here. In addition this is compiled into its own assembly so cannot detect symbols from Input System defines in asmdef and this seems to only rely on UNITY version which makes this broken if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS is undefined?

True, but that issue is not introduced by this PR.

…logies/InputSystem into isx-1865-project-wide-asset

lyndon-unity

Approving to land. This addresses a lot of the main issues of the InputManager.asset storage.
Any further improvements can land in follow up PR's

ekcoh added 2 commits February 5, 2024 23:54

Replacing use of Inputmanager.asset with promoted InputActionsAsset

85c5cf7

WIP: More changes towards asset based workflow

66a8f3a

ekcoh added the work in progress Indicates that the PR is work in progress and any review efforts can be post-poned. label Feb 7, 2024

ekcoh requested review from AlexTyrer, lyndon-unity, timkeo, stefanunity, Pauliusd01, graham-huws and ritamerkl February 7, 2024 15:17

Fixes to allow to compile

879857d

lyndon-unity reviewed Feb 8, 2024

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/AssetContext.cs Outdated Show resolved Hide resolved

lyndon-unity reviewed Feb 8, 2024

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/AssetContext.cs Outdated Show resolved Hide resolved

lyndon-unity reviewed Feb 8, 2024

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Commands/Commands.cs Outdated Show resolved Hide resolved

lyndon-unity added 2 commits February 9, 2024 10:06

Improved UX for the project wide input actions in project settings

5aba932

Added missing changes for UX updates

8182b37

lyndon-unity mentioned this pull request Feb 9, 2024

ISX-1851 Remove title from editor window #1822

Closed

4 tasks

Added indication of selected project wide input actions asset

b09d43b

Fixed up some styles for the header

Enabled reset button to work on project wide input assets

ee24377

Fixed name in the json data when a new input action asset is created from the template

Fixes for package to load in 2019

92b2f30

lyndon-unity added 3 commits February 9, 2024 16:00

Fixed asset import

b17423e

Fixed Editor_LeavingPlayMode_DestroysAllActionStates test for when In…

2a5078f

…putSystem.actions is null InputSystem.actions is null at the start when no project wide input actions is set

Sanity checks added to project wide input actions

7abb437

(Default project wide input actions could be added to the project, but a user won't necessarily have them so adding messages so the user would know what to do if they are missing)

FIX: Correct improper use of operator for null checks when using Unit…

621bcb2

…yEngine.Object

ritamerkl reviewed Feb 16, 2024

View reviewed changes

AlexTyrer reviewed Feb 16, 2024

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/InputManager.cs Outdated Show resolved Hide resolved

Fixed 2021.3 compilation issue

aa7ca47

lyndon-unity reviewed Feb 16, 2024

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Editor/ProjectWideActions/ProjectWideActionsAsset.cs Outdated Show resolved Hide resolved

lyndon-unity reviewed Feb 16, 2024

View reviewed changes

...m.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/InputActionsEditorSettingsProvider.cs Outdated Show resolved Hide resolved

lyndon-unity reviewed Feb 16, 2024

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/InputSystem.cs Outdated Show resolved Hide resolved

Lyndon Homewood added 3 commits February 16, 2024 21:56

Fixes to make tests pass on standalone player

e6e1f03

Fixed InputForUI tests and InputForUI to handle null InputSystem.actions

502c022

InputForUI hook added to update when InputSystem.actions changes

ba5bddd

ekcoh commented Feb 17, 2024

View reviewed changes

ekcoh and others added 9 commits February 17, 2024 08:28

FIX: Removed TODOs and uncommented code

52e17e7

Merge branch 'develop' into isx-1865-project-wide-asset

255f83e

Restored code to set InputSystem.actions at runtime and fixed tests

eb7b70d

Merge branch 'isx-1865-project-wide-asset' of github.com:Unity-Techno…

930a6cf

…logies/InputSystem into isx-1865-project-wide-asset

Formatting fix

513fdf9

FIX: Restore Defaults now also restores control schemes (binding groups)

5389210

Merge branch 'isx-1865-project-wide-asset' of github.com:Unity-Techno…

a316861

…logies/InputSystem into isx-1865-project-wide-asset

Removed one test which is causing next two to fail

3b33251

Graceful disable actions to avoid asset on destruction

7e44945

lyndon-unity approved these changes Feb 17, 2024

View reviewed changes

lyndon-unity merged commit 241946a into develop Feb 18, 2024
65 of 66 checks passed

lyndon-unity deleted the isx-1865-project-wide-asset branch February 18, 2024 14:14

ekcoh mentioned this pull request Feb 19, 2024

NEW: Object picker for InputActionReference combining results from Project-wide Input Actions Asset and Project Input Actions Assets #1768

Closed

4 tasks

graham-huws restored the isx-1865-project-wide-asset branch March 19, 2024 11:51

graham-huws deleted the isx-1865-project-wide-asset branch March 19, 2024 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CHANGE: ISX-1865 Switch from InputManager.asset to asset-based storage for Project-wide actions #1834

CHANGE: ISX-1865 Switch from InputManager.asset to asset-based storage for Project-wide actions #1834

ekcoh commented Feb 7, 2024 •

edited

lyndon-unity commented Feb 8, 2024 •

edited

lyndon-unity commented Feb 8, 2024 •

edited

Pauliusd01 commented Feb 9, 2024 •

edited

lyndon-unity commented Feb 9, 2024

lyndon-unity commented Feb 9, 2024

lyndon-unity commented Feb 9, 2024 •

edited

lyndon-unity commented Feb 9, 2024

ritamerkl left a comment

AlexTyrer commented Feb 16, 2024

ekcoh Feb 17, 2024

ekcoh Feb 17, 2024

lyndon-unity Feb 17, 2024

ekcoh Feb 19, 2024

ekcoh Feb 17, 2024

lyndon-unity Feb 17, 2024

ekcoh Feb 17, 2024

lyndon-unity Feb 17, 2024

lyndon-unity left a comment

		@@ -80,6 +80,8 @@ public void Initialize()

		m_Cfg = Configuration.GetDefaultConfiguration();
		RegisterActions(m_Cfg);

CHANGE: ISX-1865 Switch from InputManager.asset to asset-based storage for Project-wide actions #1834

CHANGE: ISX-1865 Switch from InputManager.asset to asset-based storage for Project-wide actions #1834

Conversation

ekcoh commented Feb 7, 2024 • edited

Description

Changes made

Notes

Checklist

lyndon-unity commented Feb 8, 2024 • edited

lyndon-unity commented Feb 8, 2024 • edited

Pauliusd01 commented Feb 9, 2024 • edited

lyndon-unity commented Feb 9, 2024

lyndon-unity commented Feb 9, 2024

lyndon-unity commented Feb 9, 2024 • edited

lyndon-unity commented Feb 9, 2024

ritamerkl left a comment

Choose a reason for hiding this comment

AlexTyrer commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyndon-unity left a comment

Choose a reason for hiding this comment

ekcoh commented Feb 7, 2024 •

edited

lyndon-unity commented Feb 8, 2024 •

edited

lyndon-unity commented Feb 8, 2024 •

edited

Pauliusd01 commented Feb 9, 2024 •

edited

lyndon-unity commented Feb 9, 2024 •

edited