NEW: Allow selection of InputActionReferences from project-wide actions #1779

jfreire-unity · 2023-10-25T16:35:17Z

Description

https://jira.unity3d.com/browse/ISX-1660
https://jira.unity3d.com/browse/ISX-1664

Changes made

This PR allows users to select InputActionReferences from project-wide actions.

It is based on most of the POC work done in #1768 but with some refactoring:

It adds a new UI advanced picker for the InputActionReference property
Deals with reference management issues caused by having an InputActionAsset outside of the Assets folder. Project-wide actions are part of "ProjectSettings/InputManager.asset" which abides by a different workflow as .inputaction assets. More specifically, project-wide actions don't have InputActionReferences as sub-assets, which means they are not updated once the ScriptedImporter.OnImportAsset runs. This means we need to "manually" update the InputActionReferences to match the existing input actions in the project-wide actions asset. Essentially, making sure that input actions in the asset have references, and also making sure that the references point to the correct input action.

Notes

Feel free to capture issues regarding the Undo system so that we can deal with them in a separate PR.
I believe if users can use action references they added, renamed, or deleted in project-wide actions, it is still valuable to pre.2, even if there's some stabilization to be done regarding Undo actions.

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.

This new picker allows us to search InputActionReferences from the project wide actions asset. Since this is asset is in ProjectSettings, a normal object picker doesn't work.

A dangling action reference means a actin reference that point to a non-existent InputAction. This needs to be checked here in case an input action is removed from the project-wide actions asset. Otherwise the UI will still hold on to a dangling action reference.

lyndon-unity · 2023-10-26T15:29:09Z

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

+        {
+            //Get all InputActionReferences are stored at the same asset path as InputActionAsset
+            return AssetDatabase.LoadAllAssetsAtPath(AssetDatabase.GetAssetPath(asset)).Where(
+                o => o is InputActionReference && o.name != "InputManager").Cast<InputActionReference>();


Do we need this part?
&& o.name != "InputManager

InputManager data shouldn't be an InputActionReference should it ?

You're right, in theory it shouldn't.
But when removing an input action reference from the asset, it seems to show InputManage in the picker window. This was a just a quick fix. Something to improve, depending on the time we have to understand what's happening in InputManager.asset.

lyndon-unity · 2023-10-26T15:31:24Z

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

+
+        private static void CreateInputActionReferences(InputActionAsset asset)
+        {
+            var maps = asset.actionMaps;


Do we need a check here to make sure the asset doesn't already have the InputActionReferences before they are created?

var actionReference = ScriptableObject.CreateInstance();

I guess not as its only called from CreateNewActionAsset

lyndon-unity · 2023-10-26T16:04:10Z

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

+            // Check if all actions have a reference
+            foreach (var action in asset)
+            {
+                var actionReference = existingReferences.FirstOrDefault(r => r.m_ActionId == action.id.ToString());


Feels like we could do this faster, e.g. by generating a hash in the above foreach look and then checking if its in the hash here - rather than splining the eixtingReferences list on every action in the asset.

Thanks. We probably can. I'll leave it for the end in case the direction in this PR is accepted.
Are you suggesting to create some kind of dictionary to associate the action ID hash of the reference with the reference itself ? So that we can then use the action.id here as a key ?

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

...m.unity.inputsystem/InputSystem/Editor/PropertyDrawers/InputActionReferencePropertyDrawer.cs

ekcoh

I believe this could be a valuable change to enable testing of pre2 but I do not think its a solution for InputActionReference (nor is the branch it is based on). I still believe that a way forward could/would be either integrating InputActionReference into UITK editor workflow or use OnValidate() on related assets to fix InputActionReferences. Likely a combination is needed and have a serialised array in the InputActionAsset instead of Load to track them in order to allow that array to follow the same Undo operations and the ScriptableObjects themselves.

I added some questions/comments below relating to the implementation. My main comments for feedback with Request changes are:

This PR (as the referenced PoC PR) needs unit tests for the added functionality to at least have some test coverage as part of CI.

ekcoh · 2023-10-29T21:25:52Z

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

+            // Check if referenced input action exists in the asset and remove the reference if it doesn't.
+            foreach (var actionReference in existingReferences)
+            {
+                var action = asset.FindAction(actionReference.action.id);


When testing the branch I have a NullReferenceException here in PlayMode after some edits and entering PlayMode (Not sure about exact steps to reproduce, will update his comment if I can reproduce again):

NullReferenceException: Object reference not set to an instance of an object UnityEngine.InputSystem.Editor.ProjectWideActionsAsset.UpdateInputActionReferences () (at ./Packages/com.unity.inputsystem/InputSystem/Editor/ProjectWideActions/ProjectWideActionsAsset.cs:123) UnityEngine.InputSystem.Editor.InputActionsEditorWindowUtils.SaveAsset (UnityEditor.SerializedObject serializedAsset) (at ./Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/InputActionsEditorWindowUtils.cs:21) UnityEngine.InputSystem.Editor.InputActionsEditorSettingsProvider.OnEditFocusLost (UnityEngine.UIElements.FocusOutEvent event) (at ./Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/InputActionsEditorSettingsProvider.cs:79) UnityEngine.InputSystem.Editor.InputActionsEditorSettingsProvider.OnDeactivate () (at ./Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/InputActionsEditorSettingsProvider.cs:54) UnityEditor.SettingsWindow.OnDisable () (at /Users/bokken/build/output/unity/unity/Editor/Mono/Settings/SettingsWindow.cs:130) UnityEditor.ContainerWindow:InternalCloseWindow() (at /Users/bokken/build/output/unity/unity/Editor/Mono/ContainerWindow.cs:360)

Hm, interesting. Good catch! Was this when doing a bunch of Undo/Redo actions? I'll try to replicate it as well.

Not sure, my main test case have been creating, deleting, undo, redo, renaming, select again in picker, repeat.... Haven't found this particular one again though.

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

...m.unity.inputsystem/InputSystem/Editor/PropertyDrawers/InputActionReferencePropertyDrawer.cs

....unity.inputsystem/InputSystem/Editor/PropertyDrawers/InputActionReferenceSearchProviders.cs

ekcoh · 2023-10-30T08:28:38Z

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

@@ -35,7 +35,7 @@ public override void OnActivate(string searchContext, VisualElement rootElement)
            // Note that focused element will be set if we are navigating back to
            // an existing instance when switching setting in the left project settings panel since
            // this doesn't recreate the editor.
-            if (m_RootVisualElement.focusController.focusedElement != null)
+            if (m_RootVisualElement?.focusController?.focusedElement != null)


I noticed you picked up this diff from the original branch, just wanted to clarify this is a general bug fix relating to a post around potential focus controller bug and isn't really related to the subject of the PR, I think it is fine to add anyway but might require its own FIX line in the CHANGELOG / commit message.

...es/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/InputActionsEditorWindowUtils.cs

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

ekcoh · 2023-10-30T08:51:14Z

Did some additional testing of this branch but failed to trigger the NullReference exception again. In general it seems that this is more reliable than behavior on the PoC branch being referenced and hence this is likely a better choice for pre2.
I noticed however in one case I managed to get into a situation where I could no longer assign any Project-wide action to via the picker and the search result from the picker and the UITK editor window was out of sync. Unfortunately I haven't been able to reproduce but may open an issue if I manage to do this again.

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

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

...es/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/InputActionsEditorWindowUtils.cs

jfreire-unity · 2023-10-30T17:54:14Z

This PR (as the referenced PoC PR) needs unit tests for the added functionality to at least have some test coverage as part of CI.

Added at least one test now

Assets/Tests/InputSystem/CoreTests_ProjectWideActions.cs

jfreire-unity · 2023-10-31T11:46:03Z

Known issues to follow up:

Undo issues to be fixed: https://jira.unity3d.com/browse/ISX-1682
Check if it's possible to use Asset Database search and customize the items icons: https://jira.unity3d.com/browse/ISX-1681

jfreire-unity · 2023-10-31T12:41:15Z

Adding @Pauliusd01 for some testing. Heads up that Undo/Redo doesn't work properly and was already captured as a bug

Pauliusd01 · 2023-10-31T13:41:47Z

Unity_2023-10-31_15-40-21.mp4

Are we able to keep these tabs always visible no matter the size of the window? Seems very odd in its current state

Also, a question about the tabs - why is NONE option only available in ALL and not in every tab? Is it because the tabs are not really tabs but just pre-made searches and it doesn't find "none" in the filter? :D

jfreire-unity · 2023-10-31T13:47:21Z

Unity_2023-10-31_15-40-21.mp4
Are we able to keep these tabs always visible no matter the size of the window? Seems very odd in its current state

I'll have a look if it's easy, otherwise, I'll poke the Search team about this. I wouldn't consider it a blocker but definitely something to improve.

Pauliusd01 · 2023-10-31T13:59:25Z

Is there a reason why if an action is deleted in Project Wide Actions the reference defaults to NONE instead of Missing (like for custom actions assets)? Could give the user a different impression - unfinished work vs broken work

Pauliusd01 · 2023-10-31T14:03:35Z

Unity_2023-10-31_16-02-44.mp4

Are these area tags the same as the existing tabs? If so, why are they returning nothing?

Pauliusd01

LGTM. Had non blocking minor things listed in comments (I can report those separately if needed, just let me know) Other than those and the undo redo issues did not notice anything broken, references seem to handle deleting,duplicating, basic renaming just fine for both ProjectWideActions and custom ones

jfreire-unity · 2023-10-31T14:07:08Z

LGTM. Had non blocking minor things listed in comments (I can report those separately if needed, just let me know) Other than those and the undo redo issues did not notice anything broken, reference seem to handle deleting,duplicating, basic renaming just fine for both ProjectWideActions and custom ones

Thanks a lot @Pauliusd01 🥇

I'll have a look at the issues you reported and try to address them today. The ones that are not easily fixed, I'll create a ticket.

… query OpenInBuilder mode allows to search by "tags". But no implementation was done so they did not work.

jfreire-unity · 2023-10-31T14:30:55Z

Unity_2023-10-31_16-02-44.mp4
Are these area tags the same as the existing tabs? If so, why are they returning nothing?

I removed this from the search in my last commit.

chris-massie · 2023-11-08T17:44:47Z

....unity.inputsystem/InputSystem/Editor/PropertyDrawers/InputActionReferenceSearchProviders.cs

+
+        private static string FetchLabel(SearchItem item, SearchContext context)
+        {
+            return FetchLabel((item.data as Object) !);


This line doesn't compile.

Could you clarify in what circumstances (editor version, OS, etc) ? I haven't seen any compile errors on my side and others, or CI.

Sorry this was my mistake, I was trying to enable global actions on an earlier Unity Editor version that isn't supported.

jfreire-unity requested review from jamesmcgill and ekcoh October 25, 2023 16:35

jfreire-unity force-pushed the isx-1660-action-references-advanced-picker branch 4 times, most recently from 33c8e54 to 371ae05 Compare October 26, 2023 11:20

jfreire-unity self-assigned this Oct 26, 2023

jfreire-unity added 3 commits October 26, 2023 16:19

Add InputActionReference advanced picker

698dc8c

This new picker allows us to search InputActionReferences from the project wide actions asset. Since this is asset is in ProjectSettings, a normal object picker doesn't work.

Update InputActionReferences of project-wide actions on save

f23f587

jfreire-unity force-pushed the isx-1660-action-references-advanced-picker branch from eefd9bc to 1b22b98 Compare October 26, 2023 14:19

lyndon-unity reviewed Oct 26, 2023

View reviewed changes

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

lyndon-unity reviewed Oct 26, 2023

View reviewed changes

...m.unity.inputsystem/InputSystem/Editor/PropertyDrawers/InputActionReferencePropertyDrawer.cs Show resolved Hide resolved

ekcoh requested changes Oct 30, 2023

View reviewed changes

Remove unused code

bdda2ec

jamesmcgill approved these changes Oct 30, 2023

View reviewed changes

jfreire-unity requested a review from ekcoh October 30, 2023 17:54

ekcoh approved these changes Oct 30, 2023

View reviewed changes

Assets/Tests/InputSystem/CoreTests_ProjectWideActions.cs Outdated Show resolved Hide resolved

jfreire-unity added 4 commits October 31, 2023 13:07

Fix comments references

0193ad2

Correct variables naming

3d3ec21

Test updating references when asset actions are changed

4e16a51

Only check dangling project-wide action references

5fb9517

jfreire-unity force-pushed the isx-1660-action-references-advanced-picker branch from 5d67ea4 to 5fb9517 Compare October 31, 2023 12:31

Merge branch 'develop' into isx-1660-action-references-advanced-picker

7b3f850

Update CHANGELOG

48f17b5

jfreire-unity requested a review from Pauliusd01 October 31, 2023 12:39

Pauliusd01 approved these changes Oct 31, 2023

View reviewed changes

jfreire-unity added 2 commits October 31, 2023 15:13

Make InputActionAssetIconLoader methods internal

bdc0d07

Make advanced picker open by default with list view and simple search…

560e8c4

… query OpenInBuilder mode allows to search by "tags". But no implementation was done so they did not work.

jfreire-unity added 2 commits October 31, 2023 17:04

Run added tests only on the Editor

a26eace

Merge branch 'develop' into isx-1660-action-references-advanced-picker

1fe575a

jfreire-unity merged commit 4aa72ef into develop Nov 1, 2023

jfreire-unity deleted the isx-1660-action-references-advanced-picker branch November 1, 2023 12:12

chris-massie reviewed Nov 8, 2023

View reviewed changes

NEW: Allow selection of InputActionReferences from project-wide actions #1779

NEW: Allow selection of InputActionReferences from project-wide actions #1779

Uh oh!

Conversation

jfreire-unity commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes made

Notes

Checklist

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!

Uh oh!

Uh oh!

ekcoh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfreire-unity Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ekcoh commented Oct 30, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jfreire-unity commented Oct 30, 2023

Uh oh!

Uh oh!

jfreire-unity commented Oct 31, 2023

Uh oh!

jfreire-unity commented Oct 31, 2023

Uh oh!

Pauliusd01 commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jfreire-unity commented Oct 31, 2023

Uh oh!

Pauliusd01 commented Oct 31, 2023

Uh oh!

Pauliusd01 commented Oct 31, 2023

Uh oh!

Pauliusd01 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfreire-unity commented Oct 31, 2023

Uh oh!

jfreire-unity commented Oct 31, 2023

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!

Uh oh!

jfreire-unity commented Oct 25, 2023 •

edited

Loading

jfreire-unity Oct 30, 2023 •

edited

Loading

Pauliusd01 commented Oct 31, 2023 •

edited

Loading

Pauliusd01 left a comment •

edited

Loading