diff --git a/Assets/Editor/DumpInputActionReferences.cs b/Assets/Editor/DumpInputActionReferences.cs new file mode 100644 index 0000000000..c3703b453b --- /dev/null +++ b/Assets/Editor/DumpInputActionReferences.cs @@ -0,0 +1,33 @@ +using System.Collections; +using System.Collections.Generic; +using System.Text; +using UnityEngine; +using UnityEngine.InputSystem; + +internal static class DumpInputActionReferences +{ + private static void DumpReferences(StringBuilder sb, string prefix, InputActionReference[] references) + { + sb.Append(prefix + ":\n"); + foreach (var reference in references) + { + var s = reference.action != null ? "Yes" : "No"; + sb.Append($"- {reference.name} (Resolved: {s}, Asset: {reference.asset})\n"); + } + } + + private static void DumpReferences() + { + var sb = new StringBuilder(); + DumpReferences(sb, "Loaded objects", Object.FindObjectsByType( + FindObjectsInactive.Include, FindObjectsSortMode.InstanceID)); + DumpReferences(sb, "All objects:", Resources.FindObjectsOfTypeAll()); + Debug.Log(sb.ToString()); + } + + [UnityEditor.MenuItem("QA Tools/Dump Input Action References to Console", false, 100)] + private static void Dump() + { + DumpReferences(); + } +} diff --git a/Assets/Editor/DumpInputActionReferences.cs.meta b/Assets/Editor/DumpInputActionReferences.cs.meta new file mode 100644 index 0000000000..7f1391d91d --- /dev/null +++ b/Assets/Editor/DumpInputActionReferences.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 68f73e449eb4b41ee946193cedeface2 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/InputSystem.Editor/EditorPrefsTestUtils.cs b/Assets/Tests/InputSystem.Editor/EditorPrefsTestUtils.cs new file mode 100644 index 0000000000..bedc2009e0 --- /dev/null +++ b/Assets/Tests/InputSystem.Editor/EditorPrefsTestUtils.cs @@ -0,0 +1,76 @@ +using UnityEditor; + +namespace Tests.InputSystem.Editor +{ + /// + /// Utility to simplify editor tests with respect to editor preferences. + /// + internal static class EditorPrefsTestUtils + { + private const string EnterPlayModeOptionsEnabledKey = "EnterPlayModeOptionsEnabled"; + private const string EnterPlayModeOptionsKey = "EnterPlayModeOptions"; + + private static bool _savedEnterPlayModeOptionsEnabled; + private static int _savedEnterPlayModeOptions; + + /// + /// Call this from a tests SetUp routine to save editor preferences so they can be restored after the test. + /// + public static void SaveEditorPrefs() + { + _savedEnterPlayModeOptionsEnabled = EditorPrefs.GetBool(EnterPlayModeOptionsEnabledKey, false); + _savedEnterPlayModeOptions = EditorPrefs.GetInt(EnterPlayModeOptionsKey, (int)EnterPlayModeOptions.None); + } + + /// + /// Call this from a tests TearDown routine to restore editor preferences to the state it had before the test. + /// + /// Note that if domain reloads have not been disabled and you have a domain reload mid-test, + /// this utility will fail to restore editor preferences since the saved data will be lost. + public static void RestoreEditorPrefs() + { + EditorPrefs.SetBool(EnterPlayModeOptionsEnabledKey, _savedEnterPlayModeOptionsEnabled); + EditorPrefs.SetInt(EnterPlayModeOptionsKey, _savedEnterPlayModeOptions); + } + + /// + /// Returns whether domain reloads are disabled. + /// + /// true if domain reloads have been disabled, else false. + public static bool IsDomainReloadsDisabled() + { + return EditorPrefs.GetBool(EnterPlayModeOptionsEnabledKey, false) && + (EditorPrefs.GetInt(EnterPlayModeOptionsKey, (int)EnterPlayModeOptions.None) & + (int)EnterPlayModeOptions.DisableDomainReload) != 0; + } + + /// + /// Returns whether scene reloads are disabled. + /// + /// true if scene reloads have been disabled, else false. + public static bool IsSceneReloadsDisabled() + { + return EditorPrefs.GetBool(EnterPlayModeOptionsEnabledKey, false) && + (EditorPrefs.GetInt(EnterPlayModeOptionsKey, (int)EnterPlayModeOptions.None) & + (int)EnterPlayModeOptions.DisableSceneReload) != 0; + } + + /// + /// Call this from within a test to temporarily enable domain reload. + /// + public static void EnableDomainReload() + { + EditorPrefs.SetBool(EnterPlayModeOptionsEnabledKey, false); + } + + /// + /// Call this from within a test to temporarily disable domain reload (and scene reloads). + /// + public static void DisableDomainReload() + { + EditorPrefs.SetInt(EnterPlayModeOptionsKey, (int)(EnterPlayModeOptions.DisableDomainReload | + EnterPlayModeOptions.DisableSceneReload)); + EditorPrefs.SetBool(EnterPlayModeOptionsEnabledKey, true); + } + } +} diff --git a/Assets/Tests/InputSystem.Editor/EditorPrefsTestUtils.cs.meta b/Assets/Tests/InputSystem.Editor/EditorPrefsTestUtils.cs.meta new file mode 100644 index 0000000000..0a1780f95f --- /dev/null +++ b/Assets/Tests/InputSystem.Editor/EditorPrefsTestUtils.cs.meta @@ -0,0 +1,3 @@ +fileFormatVersion: 2 +guid: d50a191facb44cd39e68985750838db9 +timeCreated: 1759311333 \ No newline at end of file diff --git a/Assets/Tests/InputSystem.Editor/InputActionReferenceEditorTests.cs b/Assets/Tests/InputSystem.Editor/InputActionReferenceEditorTests.cs new file mode 100644 index 0000000000..66c3463e0d --- /dev/null +++ b/Assets/Tests/InputSystem.Editor/InputActionReferenceEditorTests.cs @@ -0,0 +1,197 @@ +using System; +using System.Collections; +using System.Linq; +using NUnit.Framework; +using Tests.InputSystem.Editor; +using UnityEditor; +using UnityEditor.SceneManagement; +using UnityEngine; +using UnityEngine.InputSystem; +using UnityEngine.InputSystem.Editor; +using UnityEngine.SceneManagement; +using UnityEngine.TestTools; +using Object = UnityEngine.Object; + +/// +/// Editor tests for . +/// +/// +/// This test need fixed asset paths since mid-test domain reloads would otherwise discard data. +/// +/// Be aware that if you get failures in editor tests that switch between play mode and edit mode via coroutines +/// you might get misleading stack traces that indicate errors in different places than they actually happen. +/// At least this have been observered for exception stack traces. +/// +internal class InputActionReferenceEditorTestsWithScene +{ + private const string TestCategory = "Editor"; + + private Scene m_Scene; + + private const string assetPath = "Assets/__InputActionReferenceEditorTests.inputactions"; + private const string dummyPath = "Assets/__InputActionReferenceEditorTestsDummy.asset"; + private const string scenePath = "Assets/__InputActionReferenceEditorTestsScene.unity"; + + private void CreateAsset() + { + var asset = ScriptableObject.CreateInstance(); + + var map1 = new InputActionMap("map1"); + map1.AddAction("action1"); + map1.AddAction("action2"); + asset.AddActionMap(map1); + + System.IO.File.WriteAllText(assetPath, asset.ToJson()); + Object.DestroyImmediate(asset); + AssetDatabase.ImportAsset(assetPath); + } + + [SetUp] + public void SetUp() + { + // This looks odd, but when we yield into play mode from our test coroutine we may get a domain reload + // (depending on editor preferences) which will trigger another SetUp() mid-test. + if (Application.isPlaying) + return; + + EditorPrefsTestUtils.SaveEditorPrefs(); + + m_Scene = EditorSceneManager.NewScene(NewSceneSetup.EmptyScene); + CreateAsset(); + + var go = new GameObject("Root"); + var behaviour = go.AddComponent(); + var reference = InputActionImporter.LoadInputActionReferencesFromAsset(assetPath).First( + r => "action1".Equals(r.action.name)); + behaviour.referenceAsField = reference; + behaviour.referenceAsReference = reference; + + TestUtils.SaveScene(m_Scene, scenePath); + } + + [TearDown] + public void TearDown() + { + // This looks odd, but when we yield into play mode from our test coroutine we may get a domain reload + // (depending on editor preferences) which will trigger another TearDown() mid-test. + if (Application.isPlaying) + return; + + EditorPrefsTestUtils.RestoreEditorPrefs(); + + // Close scene + EditorSceneManager.CloseScene(m_Scene, true); + + // Clean-up + AssetDatabase.DeleteAsset(dummyPath); + AssetDatabase.DeleteAsset(assetPath); + AssetDatabase.DeleteAsset(scenePath); + } + + private void DisableDomainReloads() + { + // Assumes off before running tests. + Debug.Assert(!EditorPrefsTestUtils.IsDomainReloadsDisabled()); + Debug.Assert(!EditorPrefsTestUtils.IsSceneReloadsDisabled()); + + // Safe to store since state wouldn't be reset by domain reload. + EditorPrefsTestUtils.DisableDomainReload(); + } + + private static InputActionBehaviour GetBehaviour() => Object.FindObjectOfType(); + private static InputActionAsset GetAsset() => AssetDatabase.LoadAssetAtPath(assetPath); + + // For unclear reason, NUnit fails to assert throwing exceptions after transition into play-mode. + // So until that can be sorted out, we do it manually (in the same way) ourselves. + private static void AssertThrows(Action action) where T : Exception + { + var exceptionThrown = false; + try + { + action(); + } + catch (InvalidOperationException) + { + exceptionThrown = true; + } + Assert.IsTrue(exceptionThrown, $"Expected exception of type {typeof(T)} to be thrown but it was not."); + } + + private static bool[] _disableDomainReloadsValues = new bool[] { false, true }; + + [UnityTest] + [Category(TestCategory)] + [Description("https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1584")] + public IEnumerator ReferenceSetInPlaymodeShouldBeRestored_WhenExitingPlaymode( + [ValueSource(nameof(_disableDomainReloadsValues))] bool disableDomainReloads) + { + if (disableDomainReloads) + DisableDomainReloads(); + + // Edit-mode section + { + // Sanity check our initial edit-mode state + var obj = GetBehaviour(); + Assert.That(obj.referenceAsField.action, Is.SameAs(GetAsset().FindAction("map1/action1"))); + Assert.That(obj.referenceAsReference.action, Is.SameAs(GetAsset().FindAction("map1/action1"))); + + // Enter play-mode (This will lead to domain reload by default). + yield return new EnterPlayMode(); + } + + // Play-mode section + { + var obj = GetBehaviour(); + var editModeAction = GetAsset().FindAction("map1/action1"); + var playModeAction = GetAsset().FindAction("map1/action2"); + + // Make sure our action reference is consistent in play-mode + Assert.That(obj.referenceAsField.action, Is.SameAs(editModeAction)); + + // ISXB-1584: Attempting assignment of persisted input action reference in play-mode in editor. + // Rationale: We cannot allow this since it would corrupt the source asset since changes applied to SO + // mapped to an asset isn't reverted when exiting play-mode. + // + // Here we would like to do: + // Assert.Throws(() => obj.reference.Set(null)); + // + // But we can't since it would fail with NullReferenceException. + // It turns out that because of the domain reload / Unity’s internal serialization quirks, the obj is + // sometimes null inside the lambda when NUnit captures it for execution. So to work around this we + // instead do the same kind of check manually for now which doesn't seem to have this problem. + // + // It is odd since NUnit does basically does the same thing (apart from wrapping the lambda as a + // TestDelegate). So the WHY for this problem remains unclear for now. + AssertThrows(() => obj.referenceAsField.Set(playModeAction)); + AssertThrows(() => obj.referenceAsReference.Set(editModeAction)); + + // Make sure there were no side-effects. + Assert.That(obj.referenceAsField.action, Is.SameAs(editModeAction)); + Assert.That(obj.referenceAsReference.action, Is.SameAs(editModeAction)); + + // Correct usage is to use a run-time assigned input action reference instead. It is up to the user + // to decide whether this reference should additionally be persisted (which is possible by saving it to + // and asset, or by using SerializeReference). + obj.referenceAsField = InputActionReference.Create(playModeAction); + obj.referenceAsReference = InputActionReference.Create(playModeAction); + + // Makes sure we have the expected reference. + Assert.That(obj.referenceAsField.action, Is.SameAs(playModeAction)); + Assert.That(obj.referenceAsReference.action, Is.SameAs(playModeAction)); + + // Exit play-mode (This will lead to domain reload by default). + yield return new ExitPlayMode(); + } + + // Edit-mode section + { + // Make sure our reference is back to its initial edit mode state + var obj = GetBehaviour(); + var editModeAction = GetAsset().FindAction("map1/action1"); + Assert.That(obj.referenceAsField.action, Is.SameAs(editModeAction)); + Assert.That(obj.referenceAsReference.action, Is.SameAs(editModeAction)); + } + + yield return null; + } +} diff --git a/Assets/Tests/InputSystem.Editor/InputActionReferenceEditorTests.cs.meta b/Assets/Tests/InputSystem.Editor/InputActionReferenceEditorTests.cs.meta new file mode 100644 index 0000000000..2493eb6d72 --- /dev/null +++ b/Assets/Tests/InputSystem.Editor/InputActionReferenceEditorTests.cs.meta @@ -0,0 +1,3 @@ +fileFormatVersion: 2 +guid: a468197148d54148907ffb0e1c053c65 +timeCreated: 1759304679 \ No newline at end of file diff --git a/Assets/Tests/InputSystem.Editor/InputActionReferencePropertyDrawerEditorTests.cs b/Assets/Tests/InputSystem.Editor/InputActionReferencePropertyDrawerEditorTests.cs new file mode 100644 index 0000000000..8fffb97f8d --- /dev/null +++ b/Assets/Tests/InputSystem.Editor/InputActionReferencePropertyDrawerEditorTests.cs @@ -0,0 +1,168 @@ +#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS // Mimic implementation guard + +using System; +using System.Collections; +using NUnit.Framework; +using UnityEditor; +using UnityEngine; +using UnityEngine.InputSystem; +using UnityEngine.InputSystem.Editor; +using UnityEngine.TestTools; +using UnityEngine.UIElements; +using Object = UnityEngine.Object; + +/// +/// Tests for custom property drawer associated with InputActionReference. +/// +internal sealed class InputActionReferencePropertyDrawerEditorTests +{ + private const string assetPath = "Assets/__InputActionReferencePropertyDrawerEditorTests.inputactions"; + + private GameObject go; + private InputActionBehaviour behaviour; + private SerializedObject serializedObject; + private SerializedProperty serializedProperty; + private bool onGuiCalled; + private bool fieldCalled; + private EditorWindow window; + + private Object fieldObj; + private Object fieldResult; + + [SetUp] + public void SetUp() + { + go = new GameObject("Host"); + behaviour = go.AddComponent(); + serializedObject = new SerializedObject(behaviour); + serializedProperty = serializedObject.FindProperty(nameof(InputActionBehaviour.referenceAsField)); + window = EditorWindow.GetWindow(); + } + + [TearDown] + public void TearDown() + { + window.Close(); + serializedObject.Dispose(); + Object.DestroyImmediate(go); + + AssetDatabase.DeleteAsset(assetPath); + } + + private void CreateAsset() + { + var asset = ScriptableObject.CreateInstance(); + + var map = new InputActionMap("map"); + map.AddAction("action1"); + map.AddAction("action2"); + asset.AddActionMap(map); + + System.IO.File.WriteAllText(assetPath, asset.ToJson()); + Object.DestroyImmediate(asset); + AssetDatabase.ImportAsset(assetPath); + } + + private Object Field(Rect position, Object obj, Type type, GUIContent label) + { + fieldObj = obj; + fieldCalled = true; + return fieldResult; + } + + private IEnumerator RenderDrawer(InputActionReferencePropertyDrawer drawer, bool mockField = false) + { + onGuiCalled = false; + fieldCalled = false; + + var container = new IMGUIContainer(() => + { + var rect = new Rect(0, 0, 200, 20); + var label = new GUIContent("Reference"); + if (mockField) + drawer.OnGUI(rect, serializedProperty, label, Field); + else + drawer.OnGUI(rect, serializedProperty, label); + onGuiCalled = true; + }); + + window.rootVisualElement.Add(container); + window.Repaint(); + + var timeoutTime = Time.realtimeSinceStartupAsDouble + 3.0; + do + { + yield return null; // let Unity process a frame + } + while (!onGuiCalled && Time.realtimeSinceStartupAsDouble < timeoutTime); + } + + private class TestHostWindow : EditorWindow {} + + [UnityTest] + [Description("This is a weak test but is mainly for coverage of the regular OnGUI")] + public IEnumerator ExecutesWithoutExceptions() + { + var drawer = new InputActionReferencePropertyDrawer(); + yield return RenderDrawer(drawer); + Assert.That(onGuiCalled, Is.True); + } + + [UnityTest] + [Description("Verifies that when InputActionReference field is null, null is passed to Inspector field UI")] + public IEnumerator FieldObjectIsNullWhenReferenceIsNull() + { + var drawer = new InputActionReferencePropertyDrawer(); + yield return RenderDrawer(drawer, true); + Assert.That(onGuiCalled, Is.True); + Assert.That(fieldObj, Is.Null); + } + + [UnityTest] + [Description("Verifies that when InputActionReference field is not null and reference is valid, " + + "null is passed to Inspector field UI")] + public IEnumerator FieldObjectIsReferenceWhenReferenceIsValid() + { + CreateAsset(); + var asset = AssetDatabase.LoadAssetAtPath(assetPath); + var action = asset.FindAction("map/action1"); + Assert.That(action, Is.Not.Null); // Sanity check + + // Simulate reference being assigned + var reference = InputActionReference.Create(action); + serializedProperty.objectReferenceValue = reference; + serializedObject.ApplyModifiedPropertiesWithoutUndo(); + + var drawer = new InputActionReferencePropertyDrawer(); + yield return RenderDrawer(drawer, true); + Assert.That(onGuiCalled, Is.True); + Assert.That(fieldObj, Is.SameAs(reference)); + } + + [UnityTest] + [Description("Verifies that when InputActionReference field is not null and reference is broken, " + + "null is passed to Inspector field UI")] + public IEnumerator FieldObjectIsNullWhenReferenceIsInvalid() + { + CreateAsset(); + var asset = AssetDatabase.LoadAssetAtPath(assetPath); + var action = asset.FindAction("map/action1"); + Assert.That(action, Is.Not.Null); // Sanity check + + // Simulate reference being assigned + var reference = InputActionReference.Create(action); + serializedProperty.objectReferenceValue = reference; + serializedObject.ApplyModifiedPropertiesWithoutUndo(); + + // Delete the action to make the reference invalid + asset.RemoveAction("map/action1"); + InputActionAssetManager.SaveAsset(assetPath, asset.ToJson()); + + var drawer = new InputActionReferencePropertyDrawer(); + yield return RenderDrawer(drawer, true); + Assert.That(onGuiCalled, Is.True); + Assert.That(fieldObj, Is.Null); + } +} + +#endif // UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS diff --git a/Assets/Tests/InputSystem.Editor/InputActionReferencePropertyDrawerEditorTests.cs.meta b/Assets/Tests/InputSystem.Editor/InputActionReferencePropertyDrawerEditorTests.cs.meta new file mode 100644 index 0000000000..8a84ff2313 --- /dev/null +++ b/Assets/Tests/InputSystem.Editor/InputActionReferencePropertyDrawerEditorTests.cs.meta @@ -0,0 +1,3 @@ +fileFormatVersion: 2 +guid: b84098284c2a46bb914384e5b682b950 +timeCreated: 1759473326 \ No newline at end of file diff --git a/Assets/Tests/InputSystem.Editor/InputActionsEditorTests.cs.meta b/Assets/Tests/InputSystem.Editor/InputActionsEditorTests.cs.meta index abaccf56c6..6ce6403d42 100644 --- a/Assets/Tests/InputSystem.Editor/InputActionsEditorTests.cs.meta +++ b/Assets/Tests/InputSystem.Editor/InputActionsEditorTests.cs.meta @@ -1,2 +1,11 @@ fileFormatVersion: 2 -guid: 2a061f6298b7df240a1842d1e02a957d \ No newline at end of file +guid: 2a061f6298b7df240a1842d1e02a957d +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/InputSystem.Editor/TestUtils.cs b/Assets/Tests/InputSystem.Editor/TestUtils.cs index 5650acc2f8..bb7e884aec 100644 --- a/Assets/Tests/InputSystem.Editor/TestUtils.cs +++ b/Assets/Tests/InputSystem.Editor/TestUtils.cs @@ -1,7 +1,12 @@ +using System; +using System.Linq; +using UnityEditor; +using UnityEditor.SceneManagement; using UnityEngine.InputSystem; using UnityEngine.InputSystem.Editor; +using UnityEngine.SceneManagement; -// Replicated in this test assembly to avoid building public API picked up by PackageValidator +// Replicated in this test assembly to avoid building public API picked up by PackageValidator. internal class TestUtils { #if UNITY_EDITOR @@ -24,5 +29,85 @@ public static void RestoreDialogs() Dialog.ControlScheme.SetDeleteControlScheme(null); } + /// + /// Returns the (default) name of a scene asset created at the given path. + /// + /// The path to the asset. + /// Scene name. + public static string GetSceneName(string scenePath) + { + return System.IO.Path.GetFileNameWithoutExtension(scenePath); + } + + /// + /// Saves a scene to the given path. + /// + /// The scene to be saved. + /// The target scene path. + /// If true, force saves the scene by making it explicitly dirty. + /// Throws exception if failed to save scene + /// (if dirty or was true) + public static void SaveScene(Scene scene, string scenePath, bool makeDirty = true) + { + var wasDirty = scene.isDirty; + if (makeDirty) + EditorSceneManager.MarkSceneDirty(scene); + var wasSaved = EditorSceneManager.SaveScene(scene, scenePath); + if (!wasSaved && (wasDirty || makeDirty)) + throw new Exception($"Failed to save scene to {scenePath}"); + } + + /// + /// Adds a scene asset to build settings unless it is already present. + /// + /// The scene path to the scene asset. + /// Value tuple where first argument (int) specifies the build index of the scene and the second + /// argument (bool) specifies whether the scene was added (true) or already present (false). + public static ValueTuple AddSceneToEditorBuildSettings(string scenePath) + { + // Determine if scene is already present or not. + var existingScenes = EditorBuildSettings.scenes; + var index = FindSceneIndexByPath(existingScenes, scenePath); + if (index >= 0) + return new ValueTuple(index, false); + + // Add the new scene. + var newScenes = new EditorBuildSettingsScene[existingScenes.Length + 1]; + Array.Copy(existingScenes, newScenes, existingScenes.Length); + newScenes[existingScenes.Length] = new EditorBuildSettingsScene(scenePath, true); + EditorBuildSettings.scenes = newScenes; + + return new ValueTuple(existingScenes.Length, true); + } + + /// + /// Removes a scene asset from editor build settings given its path. + /// + /// The asset path of the scene to be removed. + /// true if the scene was removed, false if it do not exist within editor build settings scenes. + public static bool RemoveSceneFromEditorBuildSettings(string scenePath) + { + var existingScenes = EditorBuildSettings.scenes; + var index = FindSceneIndexByPath(existingScenes, scenePath); + if (index < 0) + return false; + + var newScenes = new EditorBuildSettingsScene[existingScenes.Length - 1]; + Array.Copy(existingScenes, newScenes, index); + Array.Copy(existingScenes, index + 1, newScenes, index, existingScenes.Length - index - 1); + EditorBuildSettings.scenes = newScenes; + return true; + } + + private static int FindSceneIndexByPath(EditorBuildSettingsScene[] scenes, string scenePath) + { + for (var i = 0; i < scenes.Length; ++i) + { + if (scenes[i].path == scenePath) + return i; + } + return -1; + } + #endif // UNITY_EDITOR } diff --git a/Assets/Tests/InputSystem.Editor/Unity.InputSystem.Tests.Editor.asmdef b/Assets/Tests/InputSystem.Editor/Unity.InputSystem.Tests.Editor.asmdef index c5ecbdbc5d..0ddc350ce0 100644 --- a/Assets/Tests/InputSystem.Editor/Unity.InputSystem.Tests.Editor.asmdef +++ b/Assets/Tests/InputSystem.Editor/Unity.InputSystem.Tests.Editor.asmdef @@ -5,7 +5,8 @@ "UnityEngine.TestRunner", "UnityEditor.TestRunner", "Unity.InputSystem", - "Unity.InputSystem.TestFramework" + "Unity.InputSystem.TestFramework", + "Unity.InputSystem.TestSupport" ], "includePlatforms": [ "Editor" diff --git a/Assets/Tests/InputSystem.TestSupport.meta b/Assets/Tests/InputSystem.TestSupport.meta new file mode 100644 index 0000000000..631710d522 --- /dev/null +++ b/Assets/Tests/InputSystem.TestSupport.meta @@ -0,0 +1,8 @@ +fileFormatVersion: 2 +guid: 4e07537d3c95f4d54842701a8eda9588 +folderAsset: yes +DefaultImporter: + externalObjects: {} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/InputSystem.TestSupport/InputActionBehaviour.cs b/Assets/Tests/InputSystem.TestSupport/InputActionBehaviour.cs new file mode 100644 index 0000000000..bc64dd0ff2 --- /dev/null +++ b/Assets/Tests/InputSystem.TestSupport/InputActionBehaviour.cs @@ -0,0 +1,18 @@ +using UnityEngine; +using UnityEngine.InputSystem; + +// This class and this assembly should NOT be considered API, it is only public and its own assembly for the following +// reasons: +// 1) Unity only supports serialization of public types stored in a file with the same name. +// If this wasn't the case, this type would be internal to the test assembly. +// 2) Editor test assemblies cannot contain MonoBehaviour that should be added to scene game objects. +// Hence, this assembly needs to be a regular assembly and referenced by editor test assembly to use this +// MonoBehaviour in a scene. +// +// Note that we serialize both as field and reference. Note that this should not make any difference for Unity.Object +// types, but is included for completeness. +public sealed class InputActionBehaviour : MonoBehaviour +{ + [SerializeField] public InputActionReference referenceAsField; + [SerializeReference] public InputActionReference referenceAsReference; +} diff --git a/Assets/Tests/InputSystem.TestSupport/InputActionBehaviour.cs.meta b/Assets/Tests/InputSystem.TestSupport/InputActionBehaviour.cs.meta new file mode 100644 index 0000000000..9827fbe6af --- /dev/null +++ b/Assets/Tests/InputSystem.TestSupport/InputActionBehaviour.cs.meta @@ -0,0 +1,3 @@ +fileFormatVersion: 2 +guid: ca0cb2a0912943afa6dd586c959f79e3 +timeCreated: 1759319676 \ No newline at end of file diff --git a/Assets/Tests/InputSystem.TestSupport/Unity.InputSystem.TestSupport.asmdef b/Assets/Tests/InputSystem.TestSupport/Unity.InputSystem.TestSupport.asmdef new file mode 100644 index 0000000000..25833308dd --- /dev/null +++ b/Assets/Tests/InputSystem.TestSupport/Unity.InputSystem.TestSupport.asmdef @@ -0,0 +1,16 @@ +{ + "name": "Unity.InputSystem.TestSupport", + "rootNamespace": "", + "references": [ + "GUID:75469ad4d38634e559750d17036d5f7c" + ], + "includePlatforms": [], + "excludePlatforms": [], + "allowUnsafeCode": true, + "overrideReferences": true, + "precompiledReferences": [], + "autoReferenced": false, + "defineConstraints": [], + "versionDefines": [], + "noEngineReferences": false +} \ No newline at end of file diff --git a/Assets/Tests/InputSystem.TestSupport/Unity.InputSystem.TestSupport.asmdef.meta b/Assets/Tests/InputSystem.TestSupport/Unity.InputSystem.TestSupport.asmdef.meta new file mode 100644 index 0000000000..484a730ede --- /dev/null +++ b/Assets/Tests/InputSystem.TestSupport/Unity.InputSystem.TestSupport.asmdef.meta @@ -0,0 +1,7 @@ +fileFormatVersion: 2 +guid: 94b8393c82ebb4677a21b5ab4ab8019b +AssemblyDefinitionImporter: + externalObjects: {} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/InputSystem/CoreTests_Actions.cs b/Assets/Tests/InputSystem/CoreTests_Actions.cs index 0a4d551fdd..2e28963bb0 100644 --- a/Assets/Tests/InputSystem/CoreTests_Actions.cs +++ b/Assets/Tests/InputSystem/CoreTests_Actions.cs @@ -8239,10 +8239,12 @@ public void Actions_CanRemoveActionMapFromAsset() { var asset = ScriptableObject.CreateInstance(); - asset.AddActionMap(new InputActionMap("test")); + var map = new InputActionMap("test"); + asset.AddActionMap(map); asset.RemoveActionMap("test"); Assert.That(asset.actionMaps, Is.Empty); + Assert.That(map.asset, Is.Null); } [Test] @@ -11539,43 +11541,6 @@ public void Actions_CanCloneActionMaps() Assert.That(clone.actions[1].bindings[0].path, Is.EqualTo("/gamepad/rightStick")); } - [Test] - [Category("Actions")] - public void Actions_CanResolveActionReference() - { - var map = new InputActionMap("map"); - map.AddAction("action1"); - var action2 = map.AddAction("action2"); - var asset = ScriptableObject.CreateInstance(); - asset.AddActionMap(map); - - var reference = ScriptableObject.CreateInstance(); - reference.Set(asset, "map", "action2"); - - var referencedAction = reference.action; - - Assert.That(referencedAction, Is.SameAs(action2)); - } - - [Test] - [Category("Actions")] - public void Actions_CanResolveActionReference_EvenAfterActionHasBeenRenamed() - { - var map = new InputActionMap("map"); - var action = map.AddAction("oldName"); - var asset = ScriptableObject.CreateInstance(); - asset.AddActionMap(map); - - var reference = ScriptableObject.CreateInstance(); - reference.Set(asset, "map", "oldName"); - - action.Rename("newName"); - - var referencedAction = reference.action; - - Assert.That(referencedAction, Is.SameAs(action)); - } - [Test] [Category("Actions")] public void Actions_CanDisableAllEnabledActionsInOneGo() diff --git a/Assets/Tests/InputSystem/CoreTests_Actions_Reference.cs b/Assets/Tests/InputSystem/CoreTests_Actions_Reference.cs new file mode 100644 index 0000000000..dfb3b6b2a7 --- /dev/null +++ b/Assets/Tests/InputSystem/CoreTests_Actions_Reference.cs @@ -0,0 +1,201 @@ +using System; +using NUnit.Framework; +using UnityEngine; +using UnityEngine.InputSystem; + +partial class CoreTests +{ + #region Helpers + + private static InputActionAsset CreateAssetWithTwoActions() + { + var map1 = new InputActionMap("map1"); + map1.AddAction("action1"); + map1.AddAction("action2"); + var asset = ScriptableObject.CreateInstance(); + asset.AddActionMap(map1); + return asset; + } + + private static InputActionAsset CreateAssetWithSingleAction() + { + var map2 = new InputActionMap("map2"); + map2.AddAction("action3"); + var asset = ScriptableObject.CreateInstance(); + asset.AddActionMap(map2); + return asset; + } + + private void AssertDefaults(InputActionReference reference) + { + Assert.That(reference.asset, Is.Null); + Assert.That(reference.action, Is.Null); + Assert.That(reference.name, Is.EqualTo(string.Empty)); + Assert.That(reference.ToDisplayName(), Is.Null); + Assert.That(reference.ToString(), Is.EqualTo($" ({typeof(InputActionReference).FullName})")); + } + + #endregion + + [Test] + [Category("Actions")] + public void Actions_Reference_Defaults() + { + AssertDefaults(ScriptableObject.CreateInstance()); + } + + [TestCase(false, "map1", "action1")] + [TestCase(true, null, "action1")] + [TestCase(true, "map1", null)] + [Category("Actions")] + public void Actions_Reference_SetByNameThrows_IfAnyArgumentIsNull(bool validAsset, string mapName, string actionName) + { + var asset = CreateAssetWithTwoActions(); + var reference = ScriptableObject.CreateInstance(); + Assert.Throws(() => reference.Set(validAsset ? asset : null, mapName, actionName)); + } + + [TestCase("doesNotExist", "action1")] + [TestCase("map1", "doesNotExist")] + [Category("Actions")] + public void Actions_Reference_SetByNameThrows_IfNoMatchingMapOrActionExists(string mapName, string actionName) + { + var asset = CreateAssetWithTwoActions(); + var reference = ScriptableObject.CreateInstance(); + Assert.Throws(() => reference.Set(asset, mapName, actionName)); + } + + [Test] + [Category("Actions")] + public void Actions_Reference_SetByReferenceThrows_IfActionDoNotBelongToAnAsset() + { + // Case 1: Action was never part of any asset + var action = new InputAction("action"); + var reference = ScriptableObject.CreateInstance(); + Assert.Throws(() => reference.Set(action)); + + // Case 2: Action was part of an asset but then removed + var asset = CreateAssetWithTwoActions(); + var action1 = asset.FindAction("map1/action1"); + asset.RemoveAction("map1/action1"); + Assert.Throws(() => reference.Set(action1)); + } + + [Test] + [Category("Actions")] + public void Actions_Reference_CreateReturnsReference_WhenCreatedFromValidAction() + { + var asset = CreateAssetWithTwoActions(); + var action = asset.FindAction("map1/action2"); + + var reference = InputActionReference.Create(action); + Assert.That(reference.action, Is.SameAs(action)); + } + + [Test] + [Category("Actions")] + public void Actions_Reference_CreateReturnsNullReferenceObject_IfActionIsNull() + { + var reference = InputActionReference.Create(null); + Assert.That(reference.action, Is.Null); + } + + [TestCase(false)] + [TestCase(true)] + [Category("Actions")] + public void Actions_Reference_CanResolveAction_WhenSet(bool setByReference) + { + var asset = CreateAssetWithTwoActions(); + var action = asset.FindAction("map1/action2"); + + // Note that setting reference is allowed when its an in-memory object and not a reference object backed + // by an asset. + var reference = ScriptableObject.CreateInstance(); + if (setByReference) + reference.Set(action); + else + reference.Set(asset, "map1", "action2"); + + Assert.That(reference.action, Is.SameAs(action)); + } + + [Test] + [Category("Actions")] + public void Actions_Reference_CanResolveAction_AfterActionHasBeenRenamed() + { + var asset = CreateAssetWithSingleAction(); + var reference = ScriptableObject.CreateInstance(); + reference.Set(asset, "map2", "action3"); + var action = asset.FindAction("map2/action3"); + + action.Rename("newName"); + Assert.That(reference.action, Is.SameAs(action)); + } + + [Test] + [Category("Actions")] + public void Actions_Reference_CanResolveToDefaultState_AfterActionHasBeenSetToNull() + { + var asset = CreateAssetWithTwoActions(); + var reference = InputActionReference.Create(asset.FindAction("map1/action1")); + reference.Set(null); + AssertDefaults(reference); + } + + [Test(Description = "https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1584")] + [Category("Actions")] + public void Actions_Reference_CanResolveActionAfterReassignmentToActionFromAnotherAsset() + { + var asset1 = CreateAssetWithTwoActions(); + var asset2 = CreateAssetWithSingleAction(); + + var reference = ScriptableObject.CreateInstance(); + reference.Set(asset1, "map1", "action1"); + Assert.That(reference.action, Is.Not.Null); // Looks redundant, but important for test case to resolve + + reference.Set(asset2, "map2", "action3"); + Assert.That(reference.action, Is.Not.Null); + Assert.That(reference.action, Is.SameAs(asset2.FindAction("map2/action3"))); + Assert.That(reference.asset, Is.SameAs(asset2)); + Assert.That(reference.name, Is.EqualTo("map2/action3")); + Assert.That(reference.ToDisplayName(), Is.EqualTo("map2/action3")); + Assert.That(reference.ToString(), Is.EqualTo(":map2/action3")); + } + + [TestCase(typeof(InputAction))] + [TestCase(typeof(InputActionMap))] + [TestCase(typeof(InputActionAsset))] + public void Actions_Reference_ShouldReevaluateIfAssociatedEntityIsDeleted(Type typeToDelete) + { + var asset = CreateAssetWithTwoActions(); + var action = asset.FindAction("map1/action1"); + + var reference = ScriptableObject.CreateInstance(); + reference.Set(asset, "map1", "action1"); + + Assert.That(reference.action, Is.SameAs(action)); + Assert.That(reference.asset, Is.SameAs(asset)); + Assert.That(reference.name, Is.EqualTo("map1/action1")); + Assert.That(reference.ToDisplayName(), Is.EqualTo("map1/action1")); + Assert.That(reference.ToString(), Is.EqualTo(":map1/action1")); + + // Remove the referenced action directly or indirectly. Note that this doesn't destroy an action or action map + // since they are regular reference types. However, an InputActionReference is based on asset an and action ID + // So if a map is removed from an asset but remains valid in memory it is no longer a valid reference. + if (typeToDelete == typeof(InputAction)) + asset.RemoveAction("map1/action1"); + else if (typeToDelete == typeof(InputActionMap)) + asset.RemoveActionMap("map1"); + else if (typeToDelete == typeof(InputActionAsset)) + UnityEngine.Object.DestroyImmediate(asset); + + // TODO reference need to react to this + Assert.That(reference.action, Is.Null); + Assert.That(reference.asset, Is.SameAs(asset)); + Assert.That(reference.name, Is.EqualTo("map1/action1")); // Unexpected when no longer existing + Assert.That(reference.ToDisplayName(), Is.EqualTo("map1/action1")); // Unexpected when no longer existing + //Assert.That(reference.ToString(), Is.EqualTo(":" + new Guid(action1.m_Id))); // Unexpected when no longer existing + } + + // TODO Make an undo resolve test that destroys reference sub-asset, breaks reference, then undo and reference is valid again. +} diff --git a/Assets/Tests/InputSystem/CoreTests_Actions_Reference.cs.meta b/Assets/Tests/InputSystem/CoreTests_Actions_Reference.cs.meta new file mode 100644 index 0000000000..e5be0ba2f8 --- /dev/null +++ b/Assets/Tests/InputSystem/CoreTests_Actions_Reference.cs.meta @@ -0,0 +1,3 @@ +fileFormatVersion: 2 +guid: af3ddc97be234713880f2a5a2b348259 +timeCreated: 1759228337 \ No newline at end of file diff --git a/Packages/com.unity.inputsystem/CHANGELOG.md b/Packages/com.unity.inputsystem/CHANGELOG.md index a691fdd3c9..a684729db3 100644 --- a/Packages/com.unity.inputsystem/CHANGELOG.md +++ b/Packages/com.unity.inputsystem/CHANGELOG.md @@ -31,7 +31,13 @@ however, it has to be formatted properly to pass verification tests. - Fixed an issue where `InputSystemUIInputModule.localMultiPlayerRoot` could not be set to `null` when using `MultiplayerEventSystem`. [ISXB-1610](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1610) - Fixed an issue in `Keyboard` where the sub-script operator would return a `null` key control for the deprecated key `Key.IMESelected`. Now, an aliased `KeyControl`mapping to the IMESelected bit is returned for compability reasons. It is still strongly advised to not rely on this key since `IMESelected` bit isn't strictly a key and will be removed from the `Key` enumeration type in a future major revision. [ISXB-1541](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1541). - Fixed InputControl picker not updating correctly when the Input Actions Window was dirty. [ISXB-1221](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1221) -- Fixed formatting issues on processor documentation page +- Fixed formatting issues on processor documentation page. +- Fixed an issue in `InputActionReference` where `Set` was not updating the cached action leading to incorrect evaluation of `InputActionReference.action` after first being cached, then set, then read again. This could lead to reference not being reset if set programmatically during play-mode. [ISXB-1584](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1584). +- Fixed an issue where `InputActionSetupExtensions.Remove` will result in `NullReferenceException` if the action being removed has no bindings. (ISXB-1688). +- Fixed an issue where assigning a ``MonoBehaviour` field of type `InputActionReference` to an instance stored in `.inputactions` asset via `InputActionReference.Set` in playmode corrupts the originally referenced asset. (ISXB-1692). +- Fixed an issue where `InputActionReference.Create(null)` returns `null` instead "of a reference object to the given action" as documented behaviour for the API. (ISXB-1693) +- Fixed an issue where `InputActtionReference.Set(null)` does not update the cached input action reference nor the `ScriptableObject.name` leading to incorrect action being returned and reference showing up as the path to the previously assigned action in Inspector. (ISXB-1694) +- Fixed an issue where Inspector field of type `InputActionReference` isn't reflecting broken reference in case action is deleted from asset, action map is deleted from asset, or the asset itself is deleted. (ISXB-1701) ## [1.14.2] - 2025-08-05 diff --git a/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionReference.cs b/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionReference.cs index 5b56e431a9..8698b0efdb 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionReference.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionReference.cs @@ -1,5 +1,4 @@ using System; -using System.Linq; ////REVIEW: Can we somehow make this a simple struct? The one problem we have is that we can't put struct instances as sub-assets into //// the import (i.e. InputActionImporter can't do AddObjectToAsset with them). However, maybe there's a way around that. The thing @@ -40,11 +39,10 @@ public class InputActionReference : ScriptableObject /// is not initialized or if the asset has been deleted. /// /// InputActionAsset of the referenced action. - public InputActionAsset asset => m_Asset; + public InputActionAsset asset => m_Action?.m_ActionMap != null ? m_Action.m_ActionMap.asset : m_Asset; /// - /// The action that the reference resolves to. Null if the action - /// cannot be found. + /// The action that the reference resolves to. Null if the action cannot be found. /// /// The action that reference points to. /// @@ -54,15 +52,20 @@ public InputAction action { get { - if (m_Action == null) - { - if (m_Asset == null) - return null; + // Note that we need to check multiple things here that could invalidate the validity of the reference: + // 1) m_Action != null, this indicates if we have a resolved cached reference. + // 2) m_Action.actionMap != null, this would fail if the action has been removed from an action map + // and converted into a "singleton action". This would render the reference invalid since the action + // is no longer indirectly bound to m_Asset. + // 3) m_Action.actionMap.asset == m_Asset, needs to be checked to make sure that its action map + // have not been moved to another asset which would invalidate the reference since reference is + // defined by action GUID and asset reference. + // 4) m_Asset, a Unity object life-time check that would fail if the asset has been deleted. + if (m_Action != null && m_Action.actionMap != null && m_Action.actionMap.asset == m_Asset && m_Asset) + return m_Action; - m_Action = m_Asset.FindAction(new Guid(m_ActionId)); - } - - return m_Action; + // Attempt to resolve action based on asset and GUID. + return (m_Action = m_Asset ? m_Asset.FindAction(new Guid(m_ActionId)) : null); } } @@ -74,12 +77,16 @@ public InputAction action /// case the reference is reset to its default state which does not reference an action. /// is not contained in an /// that is itself contained in an . + /// If attempting to mutate a reference object + /// that is backed by an .inputactions asset. This is not allowed to prevent side-effects. public void Set(InputAction action) { if (action == null) { - m_Asset = default; - m_ActionId = default; + m_Asset = null; + m_ActionId = null; + m_Action = null; + name = string.Empty; // Scriptable object default name is empty string. return; } @@ -102,6 +109,8 @@ public void Set(InputAction action) /// is null -or- /// is null or empty -or- /// is null or empty. + /// If attempting to mutate a reference object + /// that is backed by by .inputactions asset. This is not allowed to prevent side-effects. /// No action map called could /// be found in -or- no action called /// could be found in the action map called in . @@ -118,26 +127,24 @@ public void Set(InputActionAsset asset, string mapName, string actionName) if (actionMap == null) throw new ArgumentException($"No action map '{mapName}' in '{asset}'", nameof(mapName)); - var action = actionMap.FindAction(actionName); - if (action == null) + var foundAction = actionMap.FindAction(actionName); + if (foundAction == null) throw new ArgumentException($"No action '{actionName}' in map '{mapName}' of asset '{asset}'", nameof(actionName)); - SetInternal(asset, action); + SetInternal(asset, foundAction); } - private void SetInternal(InputActionAsset asset, InputAction action) + private void SetInternal(InputActionAsset assetArg, InputAction actionArg) { - var actionMap = action.actionMap; - if (!asset.actionMaps.Contains(actionMap)) - throw new ArgumentException( - $"Action '{action}' is not contained in asset '{asset}'", nameof(action)); - - m_Asset = asset; - m_ActionId = action.id.ToString(); - name = GetDisplayName(action); + CheckImmutableReference(); - ////REVIEW: should this dirty the asset if IDs had not been generated yet? + // If we are setting the reference in edit-mode, we want the state to be reflected in the serialized + // object and hence assign serialized fields. This is a destructive operation. + m_Asset = assetArg; + m_ActionId = actionArg.id.ToString(); + m_Action = actionArg; + name = GetDisplayName(actionArg); } /// @@ -146,23 +153,19 @@ private void SetInternal(InputActionAsset asset, InputAction action) /// A string representation of the reference. public override string ToString() { - try - { - var action = this.action; - return $"{m_Asset.name}:{action.actionMap.name}/{action.name}"; - } - catch - { - if (m_Asset != null) - return $"{m_Asset.name}:{m_ActionId}"; - } - - return base.ToString(); + var value = action; // Indirect resolve + if (value == null) + return base.ToString(); + if (value.actionMap != null) + return $"{m_Asset.name}:{value.actionMap.name}/{value.name}"; + return $"{m_Asset.name}:{m_ActionId}"; } - internal static string GetDisplayName(InputAction action) + private static string GetDisplayName(InputAction action) { - return !string.IsNullOrEmpty(action?.actionMap?.name) ? $"{action.actionMap?.name}/{action.name}" : action?.name; + return !string.IsNullOrEmpty(action?.actionMap?.name) + ? $"{action.actionMap?.name}/{action.name}" + : action?.name; } /// @@ -192,8 +195,6 @@ public static implicit operator InputAction(InputActionReference reference) /// A new InputActionReference referencing . public static InputActionReference Create(InputAction action) { - if (action == null) - return null; var reference = CreateInstance(); reference.Set(action); return reference; @@ -203,18 +204,31 @@ public static InputActionReference Create(InputAction action) /// Clears the cached field for all current objects. /// /// - /// After calling this, the next call to will retrieve a new reference from the existing just as if - /// using it for the first time. The serialized and fields are not touched and will continue to hold their current values. - /// - /// This method is used to clear the Action references when exiting PlayMode since those objects are no longer valid. + /// This method is used to clear the Action references when exiting PlayMode since those objects are no + /// longer valid. /// - internal static void ResetCachedAction() + internal static void InvalidateAll() { + // It might be possible that Object.FindObjectOfTypeAll(true) would be sufficient here since we only + // need to invalidate non-serialized data on active/loaded objects. This returns a lot more, but for + // now we keep it like this to not change more than we have to. Optimizing this can be done separately. var allActionRefs = Resources.FindObjectsOfTypeAll(typeof(InputActionReference)); - foreach (InputActionReference obj in allActionRefs) - { - obj.m_Action = null; - } + foreach (var obj in allActionRefs) + ((InputActionReference)obj).Invalidate(); + } + + /// + /// Clears the cached field for this instance. + /// + /// + /// After calling this, the next call to will resolve a new + /// reference from the existing just as if using it for the first time. + /// The serialized and fields are not touched and will continue + /// to hold their current values. Also the name remains valid since the underlying reference data is unmodified. + /// + internal void Invalidate() + { + m_Action = null; } [SerializeField] internal InputActionAsset m_Asset; @@ -227,10 +241,72 @@ internal static void ResetCachedAction() /// [NonSerialized] private InputAction m_Action; - // Make annoying Microsoft code analyzer happy. + /// + /// Equivalent to . + /// + /// The associated action reference if its a valid reference, else null. public InputAction ToInputAction() { return action; } + + /// + /// Checks if this input action reference instance can be safely mutated without side effects. + /// + /// + /// This check isn't needed in player builds since ScriptableObject would never be persisted if mutated + /// in a player. + /// + /// Thrown if this input action reference is part of an + /// input actions asset and mutating it would have side-effects on the projects assets. + private void CheckImmutableReference() + { + #if UNITY_EDITOR + // Note that we do a lot of checking here, but it is only for a rather slim (unintended) use case in + // editor and not in final builds. The alternative would be to set a non-serialized field on the reference + // when importing assets which would simplify this class, but it adds complexity to import stage and + // is more difficult to assess from a asset version portability perspective. + static bool CanSetReference(InputActionReference reference) + { + // If the asset isn't persisted it cannot be part of an InputActionAsset file. + // We use this check first since it can allow us to avoid GC pressure. + var instanceID = reference.GetInstanceID(); + var isPersistent = UnityEditor.AssetDatabase.TryGetGUIDAndLocalFileIdentifier(instanceID, out _, out long _); + if (!isPersistent) + return true; + + // "Immutable" input action references are always sub-assets of InputActionAsset. + var isSubAsset = UnityEditor.AssetDatabase.IsSubAsset(instanceID); + if (!isSubAsset) + return true; + + // If we cannot get the path of our reference, we cannot be a persisted asset within an InputActionAsset. + var path = UnityEditor.AssetDatabase.GetAssetPath(reference); + if (path == null) + return true; + + // If we cannot get the main asset we cannot be a persisted asset within an InputActionAsset. + // Also we check that it is the expected type. + var mainAsset = UnityEditor.AssetDatabase.LoadMainAssetAtPath(path); + if (mainAsset == null) + return true; + + // We can only allow setting the reference if it is not part of an persisted InputActionAsset. + return (mainAsset is not InputActionAsset); + } + + // Prevent accidental mutation of the source asset if this InputActionReference is a persisted object + // residing as a sub-asset within a .inputactions asset. + // This is not needed for players since scriptable objects aren't serialized back from within a player. + if (!CanSetReference(this)) + { + throw new InvalidOperationException("Attempting to mutate an immutable InputActionReference instance. " + + "This is not allowed since it would modify the source asset." + + "Instead use InputActionReference.Create(action) to create a new " + + "in-memory instance or serialize it as a separate asset if it " + + "survive domain reloads."); + } + #endif // UNITY_EDITOR + } } } diff --git a/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionSetupExtensions.cs b/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionSetupExtensions.cs index 6a1aa3e7a5..df13f2979e 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionSetupExtensions.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionSetupExtensions.cs @@ -81,9 +81,13 @@ public static void AddActionMap(this InputActionAsset asset, InputActionMap map) /// /// Remove the given action map from the asset. /// - /// Asset to add the action map to. + /// The asset to remove the action from. /// An action map. If the given map is not part of the asset, the method /// does nothing. + /// Any action contained in the removed map will remain unchanged and still be + /// associated with its parent input action map. If this operation is successful, map will no + /// longer be associated with and will + /// return null. /// or is null. /// is currently enabled (see ) or is part of an that has s @@ -116,6 +120,10 @@ public static void RemoveActionMap(this InputActionAsset asset, InputActionMap m /// The name or ID (see ) of a map in the /// asset. Note that lookup is case-insensitive. If no map with the given name or ID is found, /// the method does nothing. + /// Any action contained in the removed map will remain unchanged and still be + /// associated with its parent input action map. If this operation is successful, map will no + /// longer be associated with and will + /// return null. /// or is null. /// The map referenced by is currently enabled /// (see ). @@ -240,7 +248,7 @@ public static void RemoveAction(this InputAction action) action.m_SingletonActionBindings = bindingsForAction; // Remove bindings to action from map. - var newActionMapBindingCount = actionMap.m_Bindings.Length - bindingsForAction.Length; + var newActionMapBindingCount = actionMap.m_Bindings != null ? actionMap.m_Bindings.Length - bindingsForAction.Length : 0; if (newActionMapBindingCount == 0) { actionMap.m_Bindings = null; diff --git a/Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporter.cs b/Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporter.cs index ddc2ef6430..0fb8416436 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporter.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporter.cs @@ -180,6 +180,8 @@ private static void CreateInputActionReferences(InputActionAsset asset, AddObjec { foreach (var action in map.actions) { + // Note that it is important action is set before being added to asset, otherwise the immutability + // check within InputActionReference would throw. var actionReference = ScriptableObject.CreateInstance(); actionReference.Set(action); addObjectToAsset(action.m_Id, actionReference, actionIcon); @@ -275,28 +277,34 @@ private static void GenerateWrapperCode(AssetImportContext ctx, InputActionAsset } } -#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS internal static IEnumerable LoadInputActionReferencesFromAsset(string assetPath) { // Get all InputActionReferences are stored at the same asset path as InputActionAsset // Note we exclude 'hidden' action references (which are present to support one of the pre releases) return AssetDatabase.LoadAllAssetsAtPath(assetPath).Where( - o => o is InputActionReference && !((InputActionReference)o).hideFlags.HasFlag(HideFlags.HideInHierarchy)) + o => o is InputActionReference && + !((InputActionReference)o).hideFlags.HasFlag(HideFlags.HideInHierarchy)) .Cast(); } - // Get all InputActionReferences from assets in the project. By default it only gets the assets in the "Assets" folder. - internal static IEnumerable LoadInputActionReferencesFromAssetDatabase(string[] foldersPath = null, bool skipProjectWide = false) +#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS + private static readonly string[] s_DefaultAssetSearchFolders = new string[] { "Assets" }; + + /// + /// Gets all instances available in assets in the project. + /// By default, it only gets the assets located in the "Assets" folder. + /// + /// Optional array of directory paths to be searched. + /// If true, excludes project-wide input actions from the result. + /// + internal static IEnumerable LoadInputActionReferencesFromAssetDatabase( + string[] foldersPath = null, bool skipProjectWide = false) { - string[] searchFolders = null; - // If folderPath is null, search in "Assets" folder. - if (foldersPath == null) - { - searchFolders = new string[] { "Assets" }; - } - - // Get all InputActionReference from assets in "Asset" folder. It does not search inside "Packages" folder. - var inputActionReferenceGUIDs = AssetDatabase.FindAssets($"t:{typeof(InputActionReference).Name}", searchFolders); + // Get all InputActionReference from assets in "Asset" folder by default. + // It does not search inside "Packages" folder. + const string inputActionReferenceFilter = "t:" + nameof(InputActionReference); + var inputActionReferenceGUIDs = AssetDatabase.FindAssets(inputActionReferenceFilter, + foldersPath ?? s_DefaultAssetSearchFolders); // To find all the InputActionReferences, the GUID of the asset containing at least one action reference is // used to find the asset path. This is because InputActionReferences are stored in the asset database as sub-assets of InputActionAsset. @@ -307,15 +315,13 @@ internal static IEnumerable LoadInputActionReferencesFromA foreach (var guid in inputActionReferenceGUIDs) { var assetPath = AssetDatabase.GUIDToAssetPath(guid); - var assetInputActionReferenceList = LoadInputActionReferencesFromAsset(assetPath).ToList(); - - if (skipProjectWide && assetInputActionReferenceList.Count() > 0) + foreach (var assetInputActionReference in LoadInputActionReferencesFromAsset(assetPath)) { - if (assetInputActionReferenceList[0].m_Asset == InputSystem.actions) + if (skipProjectWide && assetInputActionReference.m_Asset == InputSystem.actions) continue; - } - inputActionReferencesList.AddRange(assetInputActionReferenceList); + inputActionReferencesList.Add(assetInputActionReference); + } } return inputActionReferencesList; } @@ -346,6 +352,17 @@ public static string NameFromAssetPath(string assetPath) return Path.GetFileNameWithoutExtension(assetPath); } + private static bool ContainsInputActionAssetPath(string[] assetPaths) + { + foreach (var assetPath in assetPaths) + { + if (IsInputActionAssetPath(assetPath)) + return true; + } + + return false; + } + // This processor was added to address this issue: // https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-749 // @@ -373,11 +390,29 @@ private static void OnPostprocessAllAssets(string[] importedAssets, string[] del string[] movedAssets, string[] movedFromAssetPaths) #endif { + var needToInvalidate = false; foreach (var assetPath in importedAssets) { if (IsInputActionAssetPath(assetPath)) + { + needToInvalidate = true; CheckAndRenameJsonNameIfDifferent(assetPath); + } } + + if (!needToInvalidate) + needToInvalidate = ContainsInputActionAssetPath(deletedAssets); + if (!needToInvalidate) + needToInvalidate = ContainsInputActionAssetPath(movedAssets); + if (!needToInvalidate) + needToInvalidate = ContainsInputActionAssetPath(movedFromAssetPaths); + + // Invalidate all references to make sure there are no dangling references after reimport among + // our "live objects. We only need to invalidate loaded sub-assets and not assets/prefabs/SO etc + // since they may still contain effectively obsolete InputActionReferences but those references + // will resolve. + if (needToInvalidate) + InputActionReference.InvalidateAll(); } private static void CheckAndRenameJsonNameIfDifferent(string assetPath) diff --git a/Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/InputActionReferencePropertyDrawer.cs b/Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/InputActionReferencePropertyDrawer.cs index 398c99b14b..8f459e035d 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/InputActionReferencePropertyDrawer.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Editor/PropertyDrawers/InputActionReferencePropertyDrawer.cs @@ -3,6 +3,7 @@ #if UNITY_EDITOR && UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS using UnityEditor; using UnityEditor.Search; +using ObjectField = UnityEditor.Search.ObjectField; namespace UnityEngine.InputSystem.Editor { @@ -12,37 +13,53 @@ namespace UnityEngine.InputSystem.Editor [CustomPropertyDrawer(typeof(InputActionReference))] internal sealed class InputActionReferencePropertyDrawer : PropertyDrawer { + // Custom search providers for asset-based, and project-wide actions input action reference sub-assets. private readonly SearchContext m_Context = UnityEditor.Search.SearchService.CreateContext(new[] { InputActionReferenceSearchProviders.CreateInputActionReferenceSearchProviderForAssets(), InputActionReferenceSearchProviders.CreateInputActionReferenceSearchProviderForProjectWideActions(), }, string.Empty, SearchConstants.PickerSearchFlags); - - public override void OnGUI(Rect position, SerializedProperty property, GUIContent label) + public void OnGUI(Rect position, SerializedProperty property, GUIContent label, + System.Func doField = null) { - // Sets the property to null if the action is not found in the asset. - ValidatePropertyWithDanglingInputActionReferences(property); + EditorGUI.BeginProperty(position, label, property); + EditorGUI.BeginChangeCheck(); - ObjectField.DoObjectField(position, property, typeof(InputActionReference), label, - m_Context, SearchConstants.PickerViewFlags); - } + var current = property.objectReferenceValue; - static void ValidatePropertyWithDanglingInputActionReferences(SerializedProperty property) - { - if (property?.objectReferenceValue is InputActionReference reference) + // If the reference is set but cannot be resolved, we want to be consistent with standard unity objects + // which resolve to display "Missing (Type)". This isn't really feasible since our SO isn't destroyed + // directly if referenced from other assets and we have two-layer indirection (InputAction isn't an asset). + // Hence, the next best thing we can do is pass null to object field to show "None (Type)" to indicate + // a broken reference that need to be replaced. If however, other code would manage to delete the SO, + // "Missing (Type)" should be the result. + var obj = current; + var currentReference = current as InputActionReference; + if (currentReference && currentReference.action == null) + obj = null; // none/missing + + // Pick an InputActionReference using custom picker. We need to use this overload taking an object + // in order to be in control of the actual assignment to the serialized property so we can substitute. + // We allow substituting the object field here to at least enable some test coverage. + var candidate = doField != null ? doField(position, obj, typeof(InputActionReference), label) + : ObjectField.DoObjectField(position, obj, typeof(InputActionReference), + label, m_Context, SearchConstants.PickerViewFlags); + + // Only assign the value was actually changed by the user. + if (EditorGUI.EndChangeCheck() && !Equals(candidate, current)) { - // Check only if the reference is a project-wide action. - if (reference?.asset == InputSystem.actions) - { - var action = reference?.asset?.FindAction(reference.action.id); - if (action is null) - { - property.objectReferenceValue = null; - property.serializedObject.ApplyModifiedProperties(); - } - } + var reference = candidate as InputActionReference; + property.objectReferenceValue = reference ? + InputActionReference.Create(reference.action) : null; } + + EditorGUI.EndProperty(); + } + + public override void OnGUI(Rect position, SerializedProperty property, GUIContent label) + { + OnGUI(position, property, label); } } } diff --git a/Packages/com.unity.inputsystem/InputSystem/InputSystem.cs b/Packages/com.unity.inputsystem/InputSystem/InputSystem.cs index 34d4462e22..67989918e9 100644 --- a/Packages/com.unity.inputsystem/InputSystem/InputSystem.cs +++ b/Packages/com.unity.inputsystem/InputSystem/InputSystem.cs @@ -3665,7 +3665,7 @@ internal static void OnPlayModeChange(PlayModeStateChange change) InputActionState.DestroyAllActionMapStates(); // Clear the Action reference from all InputActionReference objects - InputActionReference.ResetCachedAction(); + InputActionReference.InvalidateAll(); // Restore settings. if (!string.IsNullOrEmpty(s_SystemObject.settings)) diff --git a/Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs b/Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs index 98dec05a6b..c92abe6643 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs @@ -18,14 +18,18 @@ internal class InputSystemProvider : IEventProviderImpl DefaultInputActions m_DefaultInputActions; InputActionAsset m_InputActionAsset; - InputActionReference m_PointAction; - InputActionReference m_MoveAction; - InputActionReference m_SubmitAction; - InputActionReference m_CancelAction; - InputActionReference m_LeftClickAction; - InputActionReference m_MiddleClickAction; - InputActionReference m_RightClickAction; - InputActionReference m_ScrollWheelAction; + // Note that these are plain action references instead since InputActionReference do + // not provide any value when this integration doesn't have any UI. If this integration + // later gets a UI replace these by InputActionReference and remember to check for e.g. + // m_ActionReference != null && m_ActionReference.action != null. + InputAction m_PointAction; + InputAction m_MoveAction; + InputAction m_SubmitAction; + InputAction m_CancelAction; + InputAction m_LeftClickAction; + InputAction m_MiddleClickAction; + InputAction m_RightClickAction; + InputAction m_ScrollWheelAction; InputAction m_NextPreviousAction; @@ -221,7 +225,7 @@ InputDevice GetActiveDeviceFromDirection(NavigationEvent.Direction direction) case NavigationEvent.Direction.Right: case NavigationEvent.Direction.Down: if (m_MoveAction != null) - return m_MoveAction.action.activeControl.device; + return m_MoveAction.activeControl.device; break; case NavigationEvent.Direction.Next: case NavigationEvent.Direction.Previous: @@ -242,9 +246,9 @@ InputDevice GetActiveDeviceFromDirection(NavigationEvent.Direction direction) if (m_MoveAction == null) return (default, default); - var move = m_MoveAction.action.ReadValue(); + var move = m_MoveAction.ReadValue(); // Check if the action was "pressed" this frame to deal with repeating events - var axisWasPressed = m_MoveAction.action.WasPressedThisFrame(); + var axisWasPressed = m_MoveAction.WasPressedThisFrame(); return (move, axisWasPressed); } @@ -610,43 +614,29 @@ void UnregisterFixedActions() } } + InputAction FindActionAndRegisterCallback(string actionNameOrId, Action callback = null) + { + var action = m_InputActionAsset.FindAction(actionNameOrId); + if (action != null && callback != null) + action.performed += callback; + return action; + } + void RegisterActions() { // Invoke potential lister observing registration s_OnRegisterActions?.Invoke(m_InputActionAsset); - m_PointAction = InputActionReference.Create(m_InputActionAsset.FindAction(Actions.PointAction)); - m_MoveAction = InputActionReference.Create(m_InputActionAsset.FindAction(Actions.MoveAction)); - m_SubmitAction = InputActionReference.Create(m_InputActionAsset.FindAction(Actions.SubmitAction)); - m_CancelAction = InputActionReference.Create(m_InputActionAsset.FindAction(Actions.CancelAction)); - m_LeftClickAction = InputActionReference.Create(m_InputActionAsset.FindAction(Actions.LeftClickAction)); - m_MiddleClickAction = InputActionReference.Create(m_InputActionAsset.FindAction(Actions.MiddleClickAction)); - m_RightClickAction = InputActionReference.Create(m_InputActionAsset.FindAction(Actions.RightClickAction)); - m_ScrollWheelAction = InputActionReference.Create(m_InputActionAsset.FindAction(Actions.ScrollWheelAction)); - - if (m_PointAction != null && m_PointAction.action != null) - m_PointAction.action.performed += OnPointerPerformed; - - if (m_SubmitAction != null && m_SubmitAction.action != null) - m_SubmitAction.action.performed += OnSubmitPerformed; - - if (m_CancelAction != null && m_CancelAction.action != null) - m_CancelAction.action.performed += OnCancelPerformed; - - if (m_LeftClickAction != null && m_LeftClickAction.action != null) - m_LeftClickAction.action.performed += OnLeftClickPerformed; - - if (m_MiddleClickAction != null && m_MiddleClickAction.action != null) - m_MiddleClickAction.action.performed += OnMiddleClickPerformed; - - if (m_RightClickAction != null && m_RightClickAction.action != null) - m_RightClickAction.action.performed += OnRightClickPerformed; - - if (m_ScrollWheelAction != null && m_ScrollWheelAction.action != null) - m_ScrollWheelAction.action.performed += OnScrollWheelPerformed; + m_PointAction = FindActionAndRegisterCallback(Actions.PointAction, OnPointerPerformed); + m_MoveAction = FindActionAndRegisterCallback(Actions.MoveAction); // No callback for this action + m_SubmitAction = FindActionAndRegisterCallback(Actions.SubmitAction, OnSubmitPerformed); + m_CancelAction = FindActionAndRegisterCallback(Actions.CancelAction, OnCancelPerformed); + m_LeftClickAction = FindActionAndRegisterCallback(Actions.LeftClickAction, OnLeftClickPerformed); + m_MiddleClickAction = FindActionAndRegisterCallback(Actions.MiddleClickAction, OnMiddleClickPerformed); + m_RightClickAction = FindActionAndRegisterCallback(Actions.RightClickAction, OnRightClickPerformed); + m_ScrollWheelAction = FindActionAndRegisterCallback(Actions.ScrollWheelAction, OnScrollWheelPerformed); // When adding new actions, don't forget to add them to UnregisterActions - if (InputSystem.actions == null) { // If we've not loaded a user-created set of actions, just enable the UI actions from our defaults. @@ -656,37 +646,23 @@ void RegisterActions() m_InputActionAsset.Enable(); } - void UnregisterActions() + void UnregisterAction(ref InputAction action, Action callback = null) { - if (m_PointAction != null && m_PointAction.action != null) - m_PointAction.action.performed -= OnPointerPerformed; - - if (m_SubmitAction != null && m_SubmitAction.action != null) - m_SubmitAction.action.performed -= OnSubmitPerformed; - - if (m_CancelAction != null && m_CancelAction.action != null) - m_CancelAction.action.performed -= OnCancelPerformed; - - if (m_LeftClickAction != null && m_LeftClickAction.action != null) - m_LeftClickAction.action.performed -= OnLeftClickPerformed; - - if (m_MiddleClickAction != null && m_MiddleClickAction.action != null) - m_MiddleClickAction.action.performed -= OnMiddleClickPerformed; - - if (m_RightClickAction != null && m_RightClickAction.action != null) - m_RightClickAction.action.performed -= OnRightClickPerformed; - - if (m_ScrollWheelAction != null && m_ScrollWheelAction.action != null) - m_ScrollWheelAction.action.performed -= OnScrollWheelPerformed; + if (action != null && callback != null) + action.performed -= callback; + action = null; + } - m_PointAction = null; - m_MoveAction = null; - m_SubmitAction = null; - m_CancelAction = null; - m_LeftClickAction = null; - m_MiddleClickAction = null; - m_RightClickAction = null; - m_ScrollWheelAction = null; + void UnregisterActions() + { + UnregisterAction(ref m_PointAction, OnPointerPerformed); + UnregisterAction(ref m_MoveAction); // No callback for this action + UnregisterAction(ref m_SubmitAction, OnSubmitPerformed); + UnregisterAction(ref m_CancelAction, OnCancelPerformed); + UnregisterAction(ref m_LeftClickAction, OnLeftClickPerformed); + UnregisterAction(ref m_MiddleClickAction, OnMiddleClickPerformed); + UnregisterAction(ref m_RightClickAction, OnRightClickPerformed); + UnregisterAction(ref m_ScrollWheelAction, OnScrollWheelPerformed); if (m_InputActionAsset != null) m_InputActionAsset.Disable();