FIX: Missing InputActionReferences when renaming actions (case 1129145, #1117).

Author: Rene Damm

Packages/com.unity.inputsystem/CHANGELOG.md

@@ -32,6 +32,8 @@ however, it has to be formatted properly to pass verification tests.
 - The importer for `.inputactions` assets will now check out from version control the generated .cs file when overwriting it – which only happens if the contents differ ([case 1222972](https://issuetracker.unity3d.com/issues/inputsystem-editor-generated-c-number-file-is-not-checked-out-when-overwriting)).
 - The editor for `.inputactions` assets will now check out from version control the asset before saving it.
 - Drag-reordering action maps no longer throws "Should have drop target" asserts in the console (case [1229146](https://issuetracker.unity3d.com/issues/inputsystem-reordering-of-actionmaps-in-input-action-window-fails-and-throws-should-have-drop-target-error)).
+- References to `InputActionReference` objects created by the importer for `.inputactions` files are no longer broken when the action referenced by the object is renamed ([case 1229145](https://issuetracker.unity3d.com/issues/inputsystem-inputactionreference-loses-guid-when-its-action-is-moved-or-renamed-in-the-inputaction-asset)).
+  * __NOTE: This fix does not apply to existing `InputActionReference` instances.__ The problem was inherent in the internal file IDs generated for actions &ndash; which were affected by action and map names. Thus, changing the name of an action or map would change the resulting file ID of the `InputActionReference`.<br>However, changing file IDs will break any existing reference to the object. Thus we had to preserve the existing `InputActionReference` objects under their original file ID. We hide them in the Project Browser, however. The ones that are visible now have the new, fixed file IDs.<br>To switch existing `InputActionReference` properties to the new file IDs, simply replace them with the newly created `InputActionReference`.
 
 ### Changed
 

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

@@ -1,6 +1,16 @@
 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
+////        is that we really want to store the asset reference plus the action GUID on the *user* side, i.e. the referencing side. Right
+////        now, what happens is that InputActionImporter puts these objects along with the reference and GUID they contain in the
+////        *imported* object, i.e. right with the asset. This partially defeats the whole purpose of having these objects and it means
+////        that now the GUID doesn't really matter anymore. Rather, it's the file ID that now has to be stable.
+////
+////        If we always store the GUID and asset reference on the user side, we can put the serialized data *anywhere* and it'll remain
+////        save and proper no matter what we do in InputActionImporter.
+
 ////REVIEW: should this throw if you try to assign an action that is not a singleton?
 
 ////REVIEW: akin to this, also have an InputActionMapReference?

Packages/com.unity.inputsystem/InputSystem/Editor/AssetEditor/InputActionAssetManager.cs

@@ -3,6 +3,8 @@
 using System.IO;
 using UnityEditor;
 
+////TODO: ensure that GUIDs in the asset are unique
+
 namespace UnityEngine.InputSystem.Editor
 {
     /// <summary>

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

@@ -1,6 +1,7 @@
 #if UNITY_EDITOR
 using System;
 using System.IO;
+using System.Linq;
 using UnityEditor;
 using UnityEditor.Experimental.AssetImporters;
 using UnityEngine.InputSystem.Utilities;
@@ -22,7 +23,7 @@ namespace UnityEngine.InputSystem.Editor
     [ScriptedImporter(kVersion, InputActionAsset.Extension)]
     internal class InputActionImporter : ScriptedImporter
     {
-        private const int kVersion = 10;
+        private const int kVersion = 11;
 
         private const string kActionIcon = "Packages/com.unity.inputsystem/InputSystem/Editor/Icons/InputAction.png";
         private const string kAssetIcon = "Packages/com.unity.inputsystem/InputSystem/Editor/Icons/InputActionAsset.png";
@@ -90,25 +91,27 @@ public override void OnImportAsset(AssetImportContext ctx)
             ctx.AddObjectToAsset("<root>", asset, assetIcon);
             ctx.SetMainObject(asset);
 
-            // Make sure all the elements in the asset have GUIDs.
+            // Make sure all the elements in the asset have GUIDs and that they are indeed unique.
             var maps = asset.actionMaps;
             foreach (var map in maps)
             {
                 // Make sure action map has GUID.
-                if (string.IsNullOrEmpty(map.m_Id))
+                if (string.IsNullOrEmpty(map.m_Id) || asset.actionMaps.Count(x => x.m_Id == map.m_Id) > 1)
                     map.GenerateId();
 
                 // Make sure all actions have GUIDs.
                 foreach (var action in map.actions)
                 {
-                    if (string.IsNullOrEmpty(action.m_Id))
+                    var actionId = action.m_Id;
+                    if (string.IsNullOrEmpty(actionId) || asset.actionMaps.Sum(m => m.actions.Count(a => a.m_Id == actionId)) > 1)
                         action.GenerateId();
                 }
 
                 // Make sure all bindings have GUIDs.
                 for (var i = 0; i < map.m_Bindings.LengthSafe(); ++i)
                 {
-                    if (string.IsNullOrEmpty(map.m_Bindings[i].m_Id))
+                    var bindingId = map.m_Bindings[i].m_Id;
+                    if (string.IsNullOrEmpty(bindingId) || asset.actionMaps.Sum(m => m.bindings.Count(b => b.m_Id == bindingId)) > 1)
                         map.m_Bindings[i].GenerateId();
                 }
             }
@@ -128,7 +131,23 @@ public override void OnImportAsset(AssetImportContext ctx)
                         objectName = $"{map.name}/{action.name}";
 
                     actionReference.name = objectName;
-                    ctx.AddObjectToAsset(objectName, actionReference, actionIcon);
+                    ctx.AddObjectToAsset(action.m_Id, actionReference, actionIcon);
+
+                    // Backwards-compatibility (added for 1.0.0-preview.7).
+                    // We used to call AddObjectToAsset using objectName instead of action.m_Id as the object name. This fed
+                    // the action name (*and* map name) into the hash generation that was used as the basis for the file ID
+                    // object the InputActionReference object. Thus, if the map and/or action name changed, the file ID would
+                    // change and existing references to the InputActionReference object would become invalid.
+                    //
+                    // What we do here is add another *hidden* InputActionReference object with the same content to the
+                    // asset. This one will use the old file ID and thus preserve backwards-compatibility. We should be able
+                    // to remove this for 2.0.
+                    //
+                    // Case: https://fogbugz.unity3d.com/f/cases/1229145/
+                    var backcompatActionReference = Instantiate(actionReference);
+                    backcompatActionReference.name = objectName; // Get rid of the (Clone) suffix.
+                    backcompatActionReference.hideFlags = HideFlags.HideInHierarchy;
+                    ctx.AddObjectToAsset(objectName, backcompatActionReference, actionIcon);
                 }
             }