FIX: RemoveActionMap does not remove contained actions. Additional bug fixing of InputActionReference, added more tests.

Author: ekcoh

Assets/Tests/InputSystem/CoreTests_Actions_Reference.cs

@@ -1,6 +1,8 @@
+using System;
 using NUnit.Framework;
 using UnityEngine;
 using UnityEngine.InputSystem;
+using Object = System.Object;
 
 partial class CoreTests
 {
@@ -95,8 +97,10 @@ public void Actions_Reference_CanResolveActionAfterReassignment()
         Assert.That(reference.ToString(), Is.EqualTo(":map2/action3"));
     }
 
-    [Test]
-    public void Actions_Reference_NameShouldReflectReferencedAction()
+    [TestCase(typeof(InputAction))]
+    [TestCase(typeof(InputActionMap))]
+    [TestCase(typeof(InputActionAsset))]
+    public void Actions_Reference_NameShouldReflectReferencedAction(Type typeToDelete)
     {
         var map = new InputActionMap("map");
         var action1 = map.AddAction("action1");
@@ -112,15 +116,34 @@ public void Actions_Reference_NameShouldReflectReferencedAction()
         Assert.That(reference.ToDisplayName(), Is.EqualTo("map/action1"));
         Assert.That(reference.ToString(), Is.EqualTo(":map/action1"));
 
-        // Delete the referenced action
-
-        asset.RemoveAction("map/action1");
+        // Delete the referenced action directly or indirectly
+        if (typeToDelete == typeof(InputAction))
+            asset.RemoveAction("map/action1");
+        else if (typeToDelete == typeof(InputActionMap))
+            asset.RemoveActionMap("map");
+        else if (typeToDelete == typeof(InputActionAsset))
+            UnityEngine.Object.DestroyImmediate(asset);
 
         // TODO reference need to react to this
-        Assert.That(reference.action, Is.SameAs(action1));
+        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.ToString(), Is.EqualTo(":map/action1")); // Unexpected when no longer existing
+        //Assert.That(reference.ToString(), Is.EqualTo(":" + new Guid(action1.m_Id))); // Unexpected when no longer existing
     }
+
+    // List of bugs addressed on this branch:
+    // (FIXED) InputActionReference - Do not set m_Action when calling Set.
+    // (FIXED) InputActionReference.asset - Returns incorrect asset after being .Set if .action is called before Set.
+    // (FIXED) InputActionReference - Assigned with Set during play-mode corrupts InputActionAsset by mutating asset reference into pointing into potentially another asset and overwrites existign asset.
+    // (FIXED) InputActionReferencePropertyDrawer - Assigns direct reference into InputActionAsset to InputActionReference fields leading to corruption.
+    // InputActionReference - Contains stale reference and looks like a valid reference if the action is deleted. .action returns removed action.
+    // 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.
+
+
+    // TODO Make a test where action map is deleted
+    // TODO Make a test where asset is deleted
+    // TODO Make an undo resolve test
 }

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

@@ -53,11 +53,12 @@ public InputAction action
         {
             get
             {
-                if (m_Action != null)
+                // Note that we check m_Action since it would be null if we haven't yet resolved the action.
+                // We also need to check that m_Action.actionMap isn't null since it would be null if the
+                // action was deleted.
+                if (m_Action != null && m_Action.actionMap != null && m_Asset)
                     return m_Action;
-                if (!m_Asset)
-                    return null;
-                return (m_Action = m_Asset.FindAction(new Guid(m_ActionId)));;
+                return (m_Action = ResolveAction());
             }
         }
 
@@ -236,5 +237,20 @@ public InputAction ToInputAction()
         {
             return action;
         }
+
+        private InputAction ResolveAction()
+        {
+            return m_Asset == null ? null : m_Asset.FindAction(new Guid(m_ActionId));
+        }
+
+        // OnValidate() is stripped out from player builds (editor only).
+        /*private void OnValidate()
+        {
+            var resolvedAction = ResolveAction();
+            if (resolvedAction == null)
+            {
+                Debug.Log("RESOLVED TO NULL");
+            }
+        }*/
     }
 }

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

@@ -104,6 +104,13 @@ public static void RemoveActionMap(this InputActionAsset asset, InputActionMap m
             if (map.m_Asset != asset)
                 return;
 
+            // First remove all actions from map (They become "singleton actions" or may get collected if not referenced).
+            if (map.m_Actions != null)
+            {
+                for (var i = map.m_Actions.Length - 1; i >= 0; --i)
+                    RemoveAction(map.m_Actions[i]);
+            }
+
             ArrayHelpers.Erase(ref asset.m_ActionMaps, map);
             map.m_Asset = null;
             asset.OnSetupChanged();