More bug fixes and test cases for input action reference

Author: ekcoh

Assets/Tests/InputSystem/CoreTests_Actions_Reference.cs

@@ -6,6 +6,8 @@
 
 partial class CoreTests
 {
+    #region Helpers
+
     private static InputActionAsset CreateAssetWithTwoActions()
     {
         var map1 = new InputActionMap("map1");
@@ -25,68 +27,130 @@ private static InputActionAsset CreateAssetWithSingleAction()
         return asset;
     }
 
-    [Test]
-    public void Actions_Reference_AssetAndActionsReturnsNull_IfNotSet()
+    private void AssertDefaults(InputActionReference reference)
     {
-        var reference = ScriptableObject.CreateInstance<InputActionReference>();
         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]
-    public void Actions_Reference_SetNull()
+    [Category("Actions")]
+    public void Actions_Reference_Defaults()
     {
+        AssertDefaults(ScriptableObject.CreateInstance<InputActionReference>());
+    }
+
+    [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<InputActionReference>();
-        reference.Set(null);
+        Assert.Throws<ArgumentNullException>(() => reference.Set(validAsset ? asset : null, mapName, actionName));
     }
 
-    [Test]
+    [TestCase("doesNotExist", "action1")]
+    [TestCase("map1", "doesNotExist")]
     [Category("Actions")]
-    public void Actions_Reference_CanResolveAction()
+    public void Actions_Reference_SetByNameThrows_IfNoMatchingMapOrActionExists(string mapName, string actionName)
     {
         var asset = CreateAssetWithTwoActions();
         var reference = ScriptableObject.CreateInstance<InputActionReference>();
+        Assert.Throws<ArgumentException>(() => reference.Set(asset, mapName, actionName));
+    }
 
-        reference.Set(asset, "map1", "action2");
+    [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<InputActionReference>();
+        Assert.Throws<InvalidOperationException>(() => reference.Set(action));
 
-        Assert.That(reference.action, Is.SameAs(asset.FindAction("map1/action2")));
+        // 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<InvalidOperationException>(() => reference.Set(action1));
     }
 
     [Test]
     [Category("Actions")]
-    public void Actions_Reference_CanResolveAction_EvenAfterActionHasBeenRenamed()
+    public void Actions_Reference_CreateReturnsReference_WhenCreatedFromValidAction()
     {
-        var map = new InputActionMap("map");
-        var action = map.AddAction("oldName");
-        var asset = ScriptableObject.CreateInstance<InputActionAsset>();
-        asset.AddActionMap(map);
+        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");
 
         var reference = ScriptableObject.CreateInstance<InputActionReference>();
-        reference.Set(asset, "map", "oldName");
+        if (setByReference)
+            reference.Set(action);
+        else
+            reference.Set(asset, "map1", "action2");
 
-        action.Rename("newName");
+        Assert.That(reference.action, Is.SameAs(action));
+    }
+
+    [Test]
+    [Category("Actions")]
+    public void Actions_Reference_CanResolveAction_AfterActionHasBeenRenamed()
+    {
+        var asset = CreateAssetWithSingleAction();
+        var reference = ScriptableObject.CreateInstance<InputActionReference>();
+        reference.Set(asset, "map2", "action3");
+        var action = asset.FindAction("map2/action3");
 
-        var referencedAction = reference.action;
+        action.Rename("newName");
+        Assert.That(reference.action, Is.SameAs(action));
+    }
 
-        Assert.That(referencedAction, 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_CanResolveActionAfterReassignment()
+    public void Actions_Reference_CanResolveActionAfterReassignmentToActionFromAnotherAsset()
     {
         var asset1 = CreateAssetWithTwoActions();
         var asset2 = CreateAssetWithSingleAction();
 
         var reference = ScriptableObject.CreateInstance<InputActionReference>();
         reference.Set(asset1, "map1", "action1");
-        Assert.That(reference.action, Is.Not.Null);
-        Assert.That(reference.action, Is.SameAs(asset1.FindAction("map1/action1"))); // Redundant, but important for test case
-        Assert.That(reference.asset, Is.SameAs(asset1));
-        Assert.That(reference.name, Is.EqualTo("map1/action1"));
-        Assert.That(reference.ToDisplayName(), Is.EqualTo("map1/action1"));
-        Assert.That(reference.ToString(), Is.EqualTo(":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);
@@ -100,35 +164,38 @@ public void Actions_Reference_CanResolveActionAfterReassignment()
     [TestCase(typeof(InputAction))]
     [TestCase(typeof(InputActionMap))]
     [TestCase(typeof(InputActionAsset))]
-    public void Actions_Reference_NameShouldReflectReferencedAction(Type typeToDelete)
+    public void Actions_Reference_ShouldReevaluateIfAssociatedEntityIsDeleted(Type typeToDelete)
     {
-        var map = new InputActionMap("map");
-        var action1 = map.AddAction("action1");
-        var asset = ScriptableObject.CreateInstance<InputActionAsset>();
-        asset.AddActionMap(map);
+        var asset = CreateAssetWithTwoActions();
+        var action = asset.FindAction("map1/action1");
+
+        // var map = new InputActionMap("map");
+        // var action1 = map.AddAction("action1");
+        // var asset = ScriptableObject.CreateInstance<InputActionAsset>();
+        // asset.AddActionMap(map);
 
         var reference = ScriptableObject.CreateInstance<InputActionReference>();
-        reference.Set(asset, "map", "action1");
+        reference.Set(asset, "map1", "action1");
 
-        Assert.That(reference.action, Is.SameAs(action1));
+        Assert.That(reference.action, Is.SameAs(action));
         Assert.That(reference.asset, Is.SameAs(asset));
-        Assert.That(reference.name, Is.EqualTo("map/action1"));
-        Assert.That(reference.ToDisplayName(), Is.EqualTo("map/action1"));
-        Assert.That(reference.ToString(), Is.EqualTo(":map/action1"));
+        Assert.That(reference.name, Is.EqualTo("map1/action1"));
+        Assert.That(reference.ToDisplayName(), Is.EqualTo("map1/action1"));
+        Assert.That(reference.ToString(), Is.EqualTo(":map1/action1"));
 
         // Delete the referenced action directly or indirectly
         if (typeToDelete == typeof(InputAction))
-            asset.RemoveAction("map/action1");
+            asset.RemoveAction("map1/action1");
         else if (typeToDelete == typeof(InputActionMap))
-            asset.RemoveActionMap("map");
+            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("map/action1")); // Unexpected when no longer existing
-        Assert.That(reference.ToDisplayName(), Is.EqualTo("map/action1")); // Unexpected when no longer existing
+        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
     }
 
@@ -141,7 +208,8 @@ public void Actions_Reference_NameShouldReflectReferencedAction(Type typeToDelet
     // InputActionReference - Do not invalidate if action, action map or asset is deleted/destroyed. .action still returns the action.
     // (FIXED) InputActionAsset.RemoveActionMap - Do not remove actions within the map and they keep a stale reference to the removed map.
     // (FIXED) InputActionAsset.RemoveAction - Throws exception if action do not have any bindings.
-
+    // (FIXED) InputActionReference.Set(null) - Does not update m_Action. Hence .actions returns an incorrect reference. Also doesn't update ScriptableObject.name consistently with default ScriptableObject.
+    // (FIXED) InputActionReference.Create - Returns null if action is null which is not inline with xmldoc description and inconsistent since it is allowed to create a reference and set it to null. If you want a null reference you should not call Create in the first place.
 
     // TODO Make a test where action map is deleted
     // TODO Make a test where asset is deleted

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

@@ -72,12 +72,12 @@ public InputAction action
         /// <see cref="InputActionMap"/> that is itself contained in an <see cref="InputActionAsset"/>.</exception>
         public void Set(InputAction action)
         {
-            // TODO Throw if attempting to set reference on an input action reference that resides in an asset.
-
             if (action == null)
             {
                 m_Asset = null;
                 m_ActionId = null;
+                m_Action = null;
+                name = string.Empty; // Scriptable object default name is empty string.
                 return;
             }
 
@@ -116,27 +116,27 @@ 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))
+            var actionMap = actionArg.actionMap;
+            if (!assetArg.actionMaps.Contains(actionMap))
                 throw new ArgumentException(
-                    $"Action '{action}' is not contained in asset '{asset}'", nameof(action));
+                    $"Action '{actionArg}' is not contained in asset '{assetArg}'", nameof(actionArg));
 
             // 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 = asset;
-            m_ActionId = action.id.ToString();
-            name = GetDisplayName(action);
-            m_Action = action;
+            m_Asset = assetArg;
+            m_ActionId = actionArg.id.ToString();
+            m_Action = actionArg;
+            name = GetDisplayName(actionArg);
 
             ////REVIEW: should this dirty the asset if IDs had not been generated yet?
         }
@@ -147,23 +147,19 @@ private void SetInternal(InputActionAsset asset, InputAction action)
         /// <returns>A string representation of the reference.</returns>
         public override string ToString()
         {
-            try
-            {
-                var value = action;
+            var value = action; // Indirect resolve
+            if (value == null)
+                return base.ToString();
+            if (value.actionMap != null)
                 return $"{m_Asset.name}:{value.actionMap.name}/{value.name}";
-            }
-            catch
-            {
-                if (m_Asset != null)
-                    return $"{m_Asset.name}:{m_ActionId}";
-            }
-
-            return base.ToString();
+            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;
         }
 
         /// <summary>
@@ -193,8 +189,6 @@ public static implicit operator InputAction(InputActionReference reference)
         /// <returns>A new InputActionReference referencing <paramref name="action"/>.</returns>
         public static InputActionReference Create(InputAction action)
         {
-            if (action == null)
-                return null;
             var reference = CreateInstance<InputActionReference>();
             reference.Set(action);
             return reference;
@@ -232,15 +226,17 @@ internal static void ResetCachedAction()
         /// </summary>
         [NonSerialized] private InputAction m_Action;
 
-        // Make annoying Microsoft code analyzer happy.
+        /// <summary>
+        /// Equivalent to <see cref="InputActionReference.action"/>.
+        /// </summary>
         public InputAction ToInputAction()
         {
             return action;
         }
 
         private InputAction ResolveAction()
         {
-            return m_Asset == null ? null : m_Asset.FindAction(new Guid(m_ActionId));
+            return m_Asset ? m_Asset.FindAction(new Guid(m_ActionId)) : null;
         }
 
         // OnValidate() is stripped out from player builds (editor only).