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

ekcoh · 2023-10-05T12:28:32Z

Description

This PR provides a new object picker for InputActionReference that allows selecting Input Actions from regular .inputaction assets combined with matches from Project-Wide actions. The intention with this PR is to support the workflow with Project Settings based input actions and map.

Changes made

Implemented an object picker based on SearchProvider with the support and guidance of @lochrist that allows using a single object picker (Search window) to select InputActionReference assets from Project Settings folder asset and from regular .inputactions assets governed by the AssetDatabase. The functionality is wrapped into InputReferencePropertyDrawer which allows including it as an object picker in the Inspector. The component relies on UITK for property drawer.

Note that all property drawers above are conditionally included based on the feature flag for project wide action functionality. If this feature is not enabled, behaviour should be similar to prior this PR and default to existing or default object prickers and property drawers.

Extracted code from InputActionAssetImported relating to icons into new class InputActionAssetIconProvider to be able to reference this functionality without caring about implementation details from importer and also from new search/property drawers.

Notes

Impact on existing property drawers:

PR modifies InputActionReference property drawer to rely on a picker capable of selecting from Project Settings asset actions as well as regular .inputaction assets without requiring the user to make any decision outside of the picker.

InputActionProperty doesn't require any modifications since it exposes an editor for a InputActionReference when it is configured to use a reference. When configured to represent an InputAction there is no implication on the property drawer by Project Wide actions.

Previous modifications to InputActionAsset could still make sense since None may be confusing. Probably needs discussion if this PR would be merged, but likely should remain as-is.

InputActionMap property drawer remains unaffected since its similar to InputAction drawer and allows creating and editing a map owned by the component and not providing support for indirect references.

There currently is no InputActionMapReference support in Input System but if there was, such a picker could be implemented the same way as the InputActionReferencePropertyDrawer part of this PR with little effort.

Open issues:

Currently the asset loaded contains InputActionReference duplicates with different GUIDs. Everything points to this problem being unrelated to this PR or the editor functionality. Update: this have been resolved now by not attempting to load the InputActionReference assets directly, however it is unclear if the load is additive in some way leading to duplicates. If someone could help me understand as part of reviewing it would be helpful.
It seems that InputActionReferences referencing Project-wide actions are detached from Property drawer monitoring. This is visible by referencing an action and then delete the action via the editor. Alternatively, rename the referenced action. The inspector will not realize this nor update compared to the same inspector property drawer referencing a .inputactions asset action where it will show "Missing (InputActionReference)" for the same scenario. Not sure if something is broken with the InputManager.asset, if need custom monitoring/scanning of InputActionReferences in some way or how we could resolve this. It seems unrelated to the editor in this PR but might not be unrelated to how InputActionReferences are retrieved. It could be related to creating new references I assume, when picking using regular ADB I assume we fetch InputActionReferences directly without creating new ones but not sure how this could be achieved for InputManager.asset.
Minimum zoom level obfuscates items to only display description. Currently a fallback solution with a non-standard description is used that contains path and map/action name to allow the user to understand what asset it refers to at the minimum zoom level. For all other zoom levels this seem to not be an issue.
There seems to be some exceptions entering playmode where a null serialised property is reported in UI code. Not sure what is wrong here at the moment. This seems to be related to IMGUI since the UITK variant of this doesn't seem to suffer from the same issue. Needs more investigation. It seems this must have been related to something in /Library while working on this. Haven't seen it since I cleaned up the /Library folder.

Open issues (likely bugs) found in SearchProvider functionality - currently being investigated
(PENDING) Issue with description - scenarios:

It seems that if SearchProvider.CreateItem(...) IS NOT given a description, SearchProvider IS GIVEN a fetchDescription then...
...the given description is derived via fetchDescription at all times regardless of Window zoom-level.
It seems that if SearchProvider.CreateItem(...) IS GIVEN a description and NOT a fetchDescription then...
...the given description to CreateItem is used as description for all zoom-levels of the Window.
It seems that if SearchProvider.CreateItem(...) IS GIVEN a description and IS GIVEN a fetchDescription then...
...the given description to CreateItem is used as description for all zoom-levels of the Window EXCEPT minimum zoom-level where fetchDescription is used instead.
Based on this behaviour (3) should be used to provide desired behaviour but it is needed to verify that this undocumented behaviour is not a bug to be reliable.

(PENDING) Incorrect documentation.
SearchProvider.CreateItem has incorrect documentation for data argument which says its a thumbnail when it really is a "user-provided reference passed to function callbacks". This should be reported and correct.

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.

…cking InputReference

…allback)

…ed project-wide actions icon to use sub-asset icon from InputActionImporter

lyndon-unity

Overall this is what I was expecting from the original task on this area
(https://jira.unity3d.com/browse/ISX-1321)

So looks good IMO.

I would generally approved the PR with a small comment on some duplicate code.

We do need to agree where backend data is stored asset vs project settings as we still have other issues with this project settings storage

the bug with the UI when storing data in project settings (https://jira.unity3d.com/browse/ISX-1600)
the issue with code generation requiring a file in the assets folder (in part https://jira.unity3d.com/browse/ISX-1635)
potential standardisation for users with storing data as an asset.
Issue with template file showing up in asset selectors (https://jira.unity3d.com/browse/ISX-1588)

Assets/InputActionReferenceContainer.cs

lyndon-unity · 2023-10-06T09:02:33Z

Assets/InputActionReferenceContainer.cs

+using System.Collections.Generic;
+using UnityEngine;
+
+using System.Collections;


3 duplicated lines here

using System.Collections;
using System.Collections.Generic;
using UnityEngine;

Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/AssetSearchProviders.cs

lyndon-unity · 2023-10-06T09:10:57Z

Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/AssetSearchProviders.cs

+                    asset.GetInstanceID().ToString(),
+                    label,
+                    $"{AssetDatabase.GetAssetPath(asset)} ({label})",
+                    (thumbnail == null) ? AssetDatabase.GetCachedIcon(ProjectWideActionsAsset.kAssetPath) as Texture2D : thumbnail,


I like that we have a new icon to indicate the project wide ones

These icons seem to update at I'm viewing the UI which is a bit distracting.
(Would be nice if it was at least blank before updating to reduce distraction.)

I think this is potentially a UI issue outside the scope of this PR though.

Icons I'm seeing - opportunity to revise the icon for project wide input actions (currently seems to be a magnifying glass)

Thing is we do not have any new icons, I just used the one registered with asset database. The icon (blue+orange) is the .inputactions icon registered by InputActionImporter. This should really be the way to fetch it (the default value handling I have in the code). However the "Flash icon" is what is I currently set for the Project-wide in this search. That is currently used with .inputactions sub-assets (InputActions), but it could be debated what to use. I see the choice of icon as minor for this PR and could be addressed separately. However I propose here to differentiate assets and project-wide for increased visual cue.

The magnifying glass is a default icon from the SearchProvider

@lyndon-unity Do you see Input Action Icon (Flash) in Project-Wide Input Actions search group?

I like that we have a new icon to indicate the project wide ones

These icons seem to update at I'm viewing the UI which is a bit distracting. (Would be nice if it was at least blank before updating to reduce distraction.)

I think this is potentially a UI issue outside the scope of this PR though.

Not sure was this was referring to? I need to understand this to address it I believe

…n resulting in exceptions

Assets/SceneWithInputContainer.unity

Assets/SceneWithInputContainer.unity.meta

ekcoh · 2023-10-06T10:17:16Z

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

@@ -29,8 +29,8 @@ internal class InputActionImporter : ScriptedImporter
    {
        private const int kVersion = 13;

-        private const string kActionIcon = "Packages/com.unity.inputsystem/InputSystem/Editor/Icons/InputAction.png";
-        private const string kAssetIcon = "Packages/com.unity.inputsystem/InputSystem/Editor/Icons/InputActionAsset.png";
+        internal const string kActionIcon = "Packages/com.unity.inputsystem/InputSystem/Editor/Icons/InputAction.png";


Thinking about it, it likely makes sense to either have internal access on importer to load/fetch Texture2D instead or move these or such logic to its own class.

Even better is relying on AssetDatabase.GetCachedIcon but this doesn't seem to do the job for sub-assets. Might need more investigation.

lyndon-unity

Requesting a change of whats listed

Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/AssetSearchProviders.cs

…Reduced scopes and made internal dependencies restricted in scope. Moved icon handling out of InputActionAssetImporter.

ekcoh · 2023-10-09T08:49:58Z

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

@@ -10,18 +10,18 @@ namespace UnityEngine.InputSystem.Editor
 {
    internal static class ProjectWideActionsAsset


This refactoring doesn't provide value for the PR other than that it reduces use of internals and eliminates redundant similar code.

…detail moved to ProjectWideInputActionsAsset

…stem into inspector-poc

lyndon-unity

Updates looks good

ekcoh · 2023-10-09T13:31:25Z

Going to comment on this myself, this branch needs auto-tests to verify behaviour.

ekcoh · 2023-10-10T07:51:23Z

Transitioning this back to work-in-progress again unfortunately since I have detected issues with UI behavior that needs follow-up. Seems to be possible to work around but needs verification that we are not relying on side-effects from Search-related bugs.

jfreire-unity · 2023-10-19T14:32:45Z

Assets/Tests/InputSystem/CoreTests_Actions.cs

+    [Test]
+    [Category("Actions")]
+    public void Actions_CanResolveActionReference_WhenUsingToInputActionToConstructANewReference()
+    {
+        var map = new InputActionMap("map");
+        map.AddAction("action1");
+        var action2 = map.AddAction("action2");
+        var asset = ScriptableObject.CreateInstance<InputActionAsset>();
+        asset.AddActionMap(map);
+
+        var reference = ScriptableObject.CreateInstance<InputActionReference>();
+        reference.Set(asset, "map", "action2");
+
+        var copy1 = InputActionReference.Create(reference.action);
+        var copy2 = InputActionReference.Create(reference.ToInputAction());
+
+        // Expecting action to be the same
+        Assert.That(reference.action, Is.SameAs(copy1.action));
+        Assert.That(reference.action, Is.SameAs(copy2.action));
+    }
+
+    [Test]
+    [Category("Actions")]
+    public void Actions_CanImplicitlyConvertReferenceToAction_WhenAssigningActionFromReference()
+    {
+        var reference = ScriptableObject.CreateInstance<InputActionReference>();
+        InputAction action = reference; // implicit conversion
+        Assert.That(reference.action, Is.Null);
+    }


It's unclear to me why these tests are needed in this PR as they seem to be unrelated to the object picker. Maybe I'm missing something or this is just being used for testing.

…ovider Instead of relying in the default provider, a "custom" provider was created to search input action references on all assets in the asset DB.

ritamerkl · 2023-10-25T17:30:26Z

Packages/com.unity.inputsystem/InputSystem/Editor/Internal/InputActionReferenceValidator.cs

+    // TODO Instead we likely need to integrate all reference management into each step of the editor.
+    internal class InputActionReferenceValidator
+    {
+        public static void ValidateReferences(InputActionAsset asset)


Can we split this method up a bit to make it more readable?
Maybe just outsource some code to methods with meaningful names like "HandleAddedOrRemovedActions()"?

ritamerkl · 2023-10-25T17:34:02Z

Packages/com.unity.inputsystem/InputSystem/Editor/Internal/InputActionReferenceValidator.cs

+                    // Look for first duplicate reference referencing the same action and eliminate this reference
+                    // if duplicates exist (Should not happen, basically corrupt asset). Additional duplicates are
+                    // covered since this is evaluated for each element.
+                    for (var j = i - 1; j >= 0; --j)


for example here: calling a method like "EleminateDuplicateReferences" could help explain a lot without reading a comment

ekcoh · 2024-02-19T09:09:03Z

Majority of this PR was picked up in another PR attempting to resolve this issue. given #1834 these changes are no longer relevant so will close this PR.

ekcoh · 2024-02-19T09:09:37Z

Majority of this PR was picked up in another PR attempting to resolve this issue. given #1834 these changes are no longer relevant so will close this PR.

ekcoh added 2 commits October 5, 2023 08:48

wip

8a40bb6

wip object picker for Input Action References

c5b7cff

ekcoh requested review from jamesmcgill, jfreire-unity and ritamerkl October 5, 2023 12:28

lochrist and others added 6 commits October 5, 2023 13:04

Simplified the advance picker workflow. Add a MonoBehavior to test pi…

d7dd88c

…cking InputReference

Refactoring

feba8d4

Create advance picker in UITK

cece135

Modified description to be possible to decipher when min zoom mode (f…

48abd7f

…allback)

Fixed duplicates returned when loading project settings asset, modifi…

98d9b30

…ed project-wide actions icon to use sub-asset icon from InputActionImporter

Merge branch 'develop' into inspector-poc

54eb471

ekcoh marked this pull request as ready for review October 6, 2023 07:19

ekcoh changed the title ~~[DRAFT/WIP] Object picker for InputActionReference~~ [Revisted] Object picker for InputActionReference combining results from Project-wide actions and Project .inputactions assets Oct 6, 2023

ekcoh requested a review from lyndon-unity October 6, 2023 07:42

lyndon-unity approved these changes Oct 6, 2023

View reviewed changes

ekcoh added 2 commits October 6, 2023 11:54

Modified property drawer to rely on UITK to avoid bug in IMGUI versio…

260e8b3

…n resulting in exceptions

Added workaround for Unity older than 2022_2

b2ec69c

ekcoh commented Oct 6, 2023

View reviewed changes

Assets/SceneWithInputContainer.unity Outdated Show resolved Hide resolved

ekcoh commented Oct 6, 2023

View reviewed changes

Assets/SceneWithInputContainer.unity.meta Outdated Show resolved Hide resolved

ekcoh commented Oct 6, 2023

View reviewed changes

lyndon-unity requested changes Oct 6, 2023

View reviewed changes

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

ekcoh added 3 commits October 9, 2023 09:22

Minor refactoring and cleanup relating to implemented functionality. …

f261264

…Reduced scopes and made internal dependencies restricted in scope. Moved icon handling out of InputActionAssetImporter.

Fixed problem in InputActionAsset importer

ae426ee

Removed test files that should never have been part of PR

1fd79e5

ekcoh commented Oct 9, 2023

View reviewed changes

ekcoh added 4 commits October 9, 2023 10:56

Simplified code, adressed filter from review.

1181c67

Merge branch 'develop' into inspector-poc

46f677c

Removed comment that is obsolete due to it commenting on an internal …

efe707f

…detail moved to ProjectWideInputActionsAsset

Merge branch 'inspector-poc' of github.com:Unity-Technologies/InputSy…

33f11da

…stem into inspector-poc

lyndon-unity approved these changes Oct 9, 2023

View reviewed changes

Merge branch 'develop' into inspector-poc

909cfa2

ekcoh added the work in progress Indicates that the PR is work in progress and any review efforts can be post-poned. label Oct 10, 2023

ekcoh changed the title ~~[Revisted] Object picker for InputActionReference combining results from Project-wide actions and Project .inputactions assets~~ NEW: Object picker for InputActionReference combining results from Project-wide Input Actions Asset and Project Input Actions Assets Oct 11, 2023

Merge branch 'develop' into inspector-poc

f15abff

ekcoh self-assigned this Oct 17, 2023

wip

51b78c6

jfreire-unity reviewed Oct 19, 2023

View reviewed changes

jfreire-unity added 2 commits October 19, 2023 16:41

Merge branch 'develop' into inspector-poc

203e978

Change asset search provider to same logic as project-wide actions pr…

2c76576

…ovider Instead of relying in the default provider, a "custom" provider was created to search input action references on all assets in the asset DB.

jfreire-unity mentioned this pull request Oct 25, 2023

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

Merged

4 tasks

ritamerkl reviewed Oct 25, 2023

View reviewed changes

ekcoh closed this Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ekcoh commented Oct 5, 2023 •

edited

Loading

lyndon-unity left a comment •

edited

Loading

lyndon-unity Oct 6, 2023

lyndon-unity Oct 6, 2023

lyndon-unity Oct 6, 2023

lyndon-unity Oct 6, 2023

ekcoh Oct 6, 2023

ekcoh Oct 6, 2023

ekcoh Oct 6, 2023

ekcoh Oct 9, 2023

ekcoh Oct 6, 2023

ekcoh Oct 6, 2023

lyndon-unity left a comment

ekcoh Oct 9, 2023 •

edited

Loading

lyndon-unity left a comment

ekcoh commented Oct 9, 2023

ekcoh commented Oct 10, 2023

jfreire-unity Oct 19, 2023 •

edited

Loading

ritamerkl Oct 25, 2023

ritamerkl Oct 25, 2023 •

edited

Loading

ekcoh commented Feb 19, 2024

ekcoh commented Feb 19, 2024

		@@ -10,18 +10,18 @@ namespace UnityEngine.InputSystem.Editor
		{
		internal static class ProjectWideActionsAsset

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

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

Conversation

ekcoh commented Oct 5, 2023 • edited Loading

Description

Changes made

Notes

Checklist

lyndon-unity left a comment • edited Loading

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

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 Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

lyndon-unity left a comment

Choose a reason for hiding this comment

ekcoh commented Oct 9, 2023

ekcoh commented Oct 10, 2023

jfreire-unity Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritamerkl Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

ekcoh commented Feb 19, 2024

ekcoh commented Feb 19, 2024

ekcoh commented Oct 5, 2023 •

edited

Loading

lyndon-unity left a comment •

edited

Loading

ekcoh Oct 9, 2023 •

edited

Loading

jfreire-unity Oct 19, 2023 •

edited

Loading

ritamerkl Oct 25, 2023 •

edited

Loading