FIX: Drag-reordering actions changing IDs (case 1231233, #1118).

Rene Damm · web-flow · commit 585047ccb513 · 2020-04-14T12:58:43.000+02:00
diff --git a/Assets/Tests/InputSystem/CoreTests_Editor.cs b/Assets/Tests/InputSystem/CoreTests_Editor.cs
@@ -334,6 +334,82 @@ public void Editor_InputAsset_CanAddAndRemoveActionMapThroughSerialization()
         Assert.That(asset.actionMaps[0].name, Is.EqualTo(actionMap2Name));
     }
 
+    [Test]
+    [Category("Editor")]
+    public void Editor_InputAsset_CanAddAndRemoveElementThroughSerialization()
+    {
+        var map = new InputActionMap("map");
+        var action1 = map.AddAction(name: "action1", binding: "<Gamepad>/leftStick");
+        var action2 = map.AddAction(name: "action2", binding: "<Gamepad>/rightStick");
+        action2.AddBinding("<Gamepad>/dpad");
+        var asset = ScriptableObject.CreateInstance<InputActionAsset>();
+        asset.AddActionMap(map);
+
+        var mapId = map.id;
+        var action1Id = action1.id;
+        var action2Id = action2.id;
+        var binding1Id = map.bindings[0].id;
+        var binding2Id = map.bindings[1].id;
+        var binding3Id = map.bindings[2].id;
+
+        var obj = new SerializedObject(asset);
+
+        var maps = obj.FindProperty("m_ActionMaps");
+        InputActionSerializationHelpers.AddElement(maps, "new map", 0);
+
+        var actions = obj.FindProperty("m_ActionMaps").GetArrayElementAtIndex(1).FindPropertyRelative("m_Actions");
+        var bindings = obj.FindProperty("m_ActionMaps").GetArrayElementAtIndex(1).FindPropertyRelative("m_Bindings");
+        InputActionSerializationHelpers.AddElement(actions, "new action", 1);
+        InputActionSerializationHelpers.AddElement(bindings, "new binding", 1);
+
+        obj.ApplyModifiedPropertiesWithoutUndo();
+
+        // By the nature of Unity serialization, only the connection to UnityEngine.Objects is maintained
+        // for C# objects. So map, action1, and action2 are all no longer the objects inside the asset.
+        map = asset.actionMaps[1];
+
+        Assert.That(asset.actionMaps.Count, Is.EqualTo(2));
+        Assert.That(asset.actionMaps[0].name, Is.EqualTo("new map"));
+        Assert.That(asset.actionMaps[1].name, Is.EqualTo("map"));
+        Assert.That(asset.actionMaps[0].id, Is.Not.EqualTo(mapId));
+        Assert.That(asset.actionMaps[1].id, Is.EqualTo(mapId));
+
+        Assert.That(map.actions, Has.Count.EqualTo(3));
+        Assert.That(map.actions[0].name, Is.EqualTo("action1"));
+        Assert.That(map.actions[1].name, Is.EqualTo("new action"));
+        Assert.That(map.actions[2].name, Is.EqualTo("action2"));
+        Assert.That(map.actions[0].id, Is.EqualTo(action1Id));
+        Assert.That(map.actions[1].id, Is.Not.EqualTo(action1Id));
+        Assert.That(map.actions[1].id, Is.Not.EqualTo(action2Id));
+        Assert.That(map.actions[2].id, Is.EqualTo(action2Id));
+        Assert.That(map.actions[0].bindings, Has.Count.EqualTo(1));
+        Assert.That(map.actions[1].bindings, Has.Count.Zero);
+        Assert.That(map.actions[2].bindings, Has.Count.EqualTo(2));
+        Assert.That(map.actions[0].bindings[0].path, Is.EqualTo("<Gamepad>/leftStick"));
+        Assert.That(map.actions[2].bindings[0].path, Is.EqualTo("<Gamepad>/rightStick"));
+        Assert.That(map.actions[2].bindings[1].path, Is.EqualTo("<Gamepad>/dpad"));
+
+        Assert.That(map.bindings, Has.Count.EqualTo(4));
+        Assert.That(map.bindings[0].id, Is.EqualTo(binding1Id));
+        Assert.That(map.bindings[1].id, Is.Not.EqualTo(binding1Id));
+        Assert.That(map.bindings[1].id, Is.Not.EqualTo(binding2Id));
+        Assert.That(map.bindings[1].id, Is.Not.EqualTo(binding3Id));
+        Assert.That(map.bindings[2].id, Is.EqualTo(binding2Id));
+        Assert.That(map.bindings[3].id, Is.EqualTo(binding3Id));
+        Assert.That(map.bindings[0].name, Is.Not.EqualTo("new binding"));
+        Assert.That(map.bindings[1].name, Is.EqualTo("new binding"));
+        Assert.That(map.bindings[2].name, Is.Not.EqualTo("new binding"));
+        Assert.That(map.bindings[3].name, Is.Not.EqualTo("new binding"));
+        Assert.That(map.bindings[0].path, Is.EqualTo("<Gamepad>/leftStick"));
+        Assert.That(map.bindings[1].path, Is.Empty);
+        Assert.That(map.bindings[2].path, Is.EqualTo("<Gamepad>/rightStick"));
+        Assert.That(map.bindings[3].path, Is.EqualTo("<Gamepad>/dpad"));
+        Assert.That(map.bindings[0].action, Is.EqualTo("action1"));
+        Assert.That(map.bindings[1].action, Is.Empty);
+        Assert.That(map.bindings[2].action, Is.EqualTo("action2"));
+        Assert.That(map.bindings[3].action, Is.EqualTo("action2"));
+    }
+
     [Test]
     [Category("Editor")]
     public void Editor_InputAsset_CanAddAndRemoveActionThroughSerialization()
diff --git a/Packages/com.unity.inputsystem/CHANGELOG.md b/Packages/com.unity.inputsystem/CHANGELOG.md
@@ -33,6 +33,7 @@ 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 &ndash; 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)).
+- Drag-reordering actions no longer changes action IDs of some of the existing actions ([case 1231233](https://issuetracker.unity3d.com/issues/input-systems-action-ids-dont-stick-with-action-names-when-input-actions-are-reorganized)).
 - 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`.
 
diff --git a/Packages/com.unity.inputsystem/Documentation~/KnownLimitations.md b/Packages/com.unity.inputsystem/Documentation~/KnownLimitations.md
@@ -4,20 +4,23 @@ The following is a list of known limitations that the Input System currently has
 
 ## Compatibility with other Unity features
 
+* Input processing in the background is tied to `Application.runInBackground` (i.e. the "Run In Background" setting in "Player Preferences") which, however, Unity always forces to `true` in __development__ players. This means that in development players, input will always be processed, even if the app is in the background. Of course, this only pertains to platforms where the player can actually run in the background (iOS and Android are thus unaffected).
 * `PlayerInput` split-screen support does not work with Cinemachine virtual cameras.
-* The Input System cannot generate input for IMGUI or UIElements.
+* The Input System cannot generate input for IMGUI or UIElements. Support for the latter is being worked on.
 * The Input System does not yet support the new 2019.3 mode where domain reloads are disabled when entering play mode.
 
 ### uGUI
 
 * After enabling, the UI will not react to a pointer's position until the position is changed.
+* The new input system cannot yet feed text input into uGUI and TextMesh Pro input field components. This means that text input ATM is still picked up directly and internally from the Unity native runtime.
 
 ## Device support
 
 * (Desktop) We do not yet support distinguishing input from multiple pointers (mouse, pen, touch) or keyboards. There will be a single Mouse, Pen, Touch, and Keyboard device.
-* (Windows) Mouse input from remote desktop connections does not come through properly.
 * (Windows) Pen input will not work with Wacom devices if "Windows Ink" support is turned off.
+* (Windows) HID input is not currently supported in 32-bit players. This means that devices such as the PS4 controller will not work in 32-bit standalone players. Use the 64-bit standalone player instead.
 * (Android) We only support a single Touchscreen at the moment.
+* (Stadia) The Stadia controller is only supported __in__ the Stadia player at the moment. In the editor, use the generic `Gamepad` for bindings and use any Xbox or PS4 controller for testing.
 * Joy-Cons are only supported on Switch.
 * Sensors in the PS4 controller are currently only supported on PS4.
   >NOTE: Support for NDA platforms is distributed as separate packages due to licensing restrictions. The packages, at this point, are made available separately to licensees for download and installation.
diff --git a/Packages/com.unity.inputsystem/InputSystem/Actions/InputAction.cs b/Packages/com.unity.inputsystem/InputSystem/Actions/InputAction.cs
@@ -11,6 +11,19 @@
 ////       If it mentions the action, it appears on the action. Otherwise it doesn't. The controls should consistently appear on the
 ////       action based on what action the *composite* references.
 
+////REVIEW: Should we bring the checkboxes for actions back? We tried to "simplify" things by collapsing everything into a InputActionTypes
+////        and making the various behavior toggles implicit in that. However, my impression is that this has largely backfired by making
+////        it opaque what the choices actually entail and by giving no way out if the choices for one reason or another don't work out
+////        perfectly.
+////
+////        My impression is that at least two the following two checkboxes would make sense:
+////        1) Initial State Check? Whether the action should immediately sync to the current state of controls when enabled.
+////        2) Resolve Conflicting Inputs? Whether the action should try to resolve conflicts between multiple concurrent inputs.
+////
+////        I'm fine hiding this under an "Advanced" foldout or something. But IMO, control over this should be available to the user.
+////
+////        In the same vein, we probably also should expose control over how an action behaves on focus loss (https://forum.unity.com/threads/actions-canceled-when-game-loses-focus.855217/).
+
 ////REVIEW: I think the action system as it is today offers too many ways to shoot yourself in the foot. It has
 ////        flexibility but at the same time has abundant opportunity for ending up with dysfunction. Common setups
 ////        have to come preconfigured and work robustly for the user without requiring much understanding of how
@@ -188,17 +201,17 @@ public Guid id
             get
             {
                 MakeSureIdIsInPlace();
-                return m_Guid;
+                return new Guid(m_Id);
             }
         }
 
         internal Guid idDontGenerate
         {
             get
             {
-                if (m_Guid == Guid.Empty && !string.IsNullOrEmpty(m_Id))
-                    m_Guid = new Guid(m_Id);
-                return m_Guid;
+                if (string.IsNullOrEmpty(m_Id))
+                    return default;
+                return new Guid(m_Id);
             }
         }
 
@@ -973,7 +986,6 @@ public unsafe object ReadValueAsObject()
         [NonSerialized] internal int m_BindingsCount;
         [NonSerialized] internal int m_ControlStartIndex;
         [NonSerialized] internal int m_ControlCount;
-        [NonSerialized] internal Guid m_Guid;
 
         /// <summary>
         /// Index of the action in the <see cref="InputActionState"/> associated with the
@@ -1022,21 +1034,14 @@ private InputActionState.TriggerState currentState
 
         internal string MakeSureIdIsInPlace()
         {
-            if (m_Guid != Guid.Empty)
-                return m_Id;
-
             if (string.IsNullOrEmpty(m_Id))
                 GenerateId();
-            else
-                m_Guid = new Guid(m_Id);
-
             return m_Id;
         }
 
         internal void GenerateId()
         {
-            m_Guid = Guid.NewGuid();
-            m_Id = m_Guid.ToString();
+            m_Id = Guid.NewGuid().ToString();
         }
 
         internal InputActionMap GetOrCreateActionMap()
diff --git a/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionMap.cs b/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionMap.cs
@@ -100,28 +100,19 @@ public Guid id
         {
             get
             {
-                if (m_Guid == Guid.Empty)
-                {
-                    if (m_Id == null)
-                    {
-                        GenerateId();
-                    }
-                    else
-                    {
-                        m_Guid = new Guid(m_Id);
-                    }
-                }
-                return m_Guid;
+                if (string.IsNullOrEmpty(m_Id))
+                    GenerateId();
+                return new Guid(m_Id);
             }
         }
 
         internal Guid idDontGenerate
         {
             get
             {
-                if (m_Guid == Guid.Empty && !string.IsNullOrEmpty(m_Id))
-                    m_Guid = new Guid(m_Id);
-                return m_Guid;
+                if (string.IsNullOrEmpty(m_Id))
+                    return default;
+                return new Guid(m_Id);
             }
         }
 
@@ -405,23 +396,28 @@ internal int FindActionIndex(string nameOrId)
 
             if (string.IsNullOrEmpty(nameOrId))
                 return -1;
-
             if (m_Actions == null)
-                return InputActionState.kInvalidIndex;
+                return -1;
+
             var actionCount = m_Actions.Length;
 
-            // If it contains hyphens, it may be a GUID so try looking up that way.
-            if (nameOrId.Contains('-') && Guid.TryParse(nameOrId, out var id))
+            var isOldBracedFormat = nameOrId.StartsWith("{") && nameOrId.EndsWith("}");
+            if (isOldBracedFormat)
             {
+                var length = nameOrId.Length - 2;
                 for (var i = 0; i < actionCount; ++i)
-                    if (m_Actions[i].idDontGenerate == id)
+                {
+                    if (string.Compare(m_Actions[i].m_Id, 0, nameOrId, 1, length) == 0)
                         return i;
+                }
             }
 
-            // Default search goes by name (case insensitive).
             for (var i = 0; i < actionCount; ++i)
-                if (string.Compare(m_Actions[i].m_Name, nameOrId, StringComparison.InvariantCultureIgnoreCase) == 0)
+            {
+                var action = m_Actions[i];
+                if (action.m_Id == nameOrId || string.Compare(m_Actions[i].m_Name, nameOrId, StringComparison.InvariantCultureIgnoreCase) == 0)
                     return i;
+            }
 
             return InputActionState.kInvalidIndex;
         }
@@ -616,10 +612,7 @@ public InputActionMap Clone()
                 var bindings = new InputBinding[bindingCount];
                 Array.Copy(m_Bindings, 0, bindings, 0, bindingCount);
                 for (var i = 0; i < bindingCount; ++i)
-                {
                     bindings[i].m_Id = default;
-                    bindings[i].m_Guid = default;
-                }
                 clone.m_Bindings = bindings;
             }
 
@@ -727,11 +720,6 @@ IEnumerator IEnumerable.GetEnumerator()
         /// </remarks>
         [NonSerialized] internal int m_EnabledActionsCount;
 
-        /// <summary>
-        /// GUID converted from <see cref="m_Id"/>.
-        /// </summary>
-        [NonSerialized] private Guid m_Guid;
-
         // Action maps that are created internally by singleton actions to hold their data
         // are never exposed and never serialized so there is no point allocating an m_Actions
         // array.
@@ -984,8 +972,7 @@ internal void ClearPerActionCachedBindingData()
 
         internal void GenerateId()
         {
-            m_Guid = Guid.NewGuid();
-            m_Id = m_Guid.ToString();
+            m_Id = Guid.NewGuid().ToString();
         }
 
         /// <summary>
@@ -1218,12 +1205,12 @@ public InputBinding ToBinding()
                 };
             }
 
-            public static BindingJson FromBinding(InputBinding binding)
+            public static BindingJson FromBinding(ref InputBinding binding)
             {
                 return new BindingJson
                 {
                     name = binding.name,
-                    id = binding.id.ToString(),
+                    id = binding.m_Id,
                     path = binding.path,
                     action = binding.action,
                     interactions = binding.interactions,
@@ -1305,7 +1292,7 @@ public static WriteActionJson FromAction(InputAction action)
                 {
                     name = action.m_Name,
                     type = action.m_Type.ToString(),
-                    id = action.id.ToString(),
+                    id = action.m_Id,
                     expectedControlType = action.m_ExpectedControlType,
                     processors = action.processors,
                     interactions = action.interactions,
@@ -1352,7 +1339,7 @@ public static WriteMapJson FromMap(InputActionMap map)
                     jsonBindings = new BindingJson[bindingCount];
 
                     for (var i = 0; i < bindingCount; ++i)
-                        jsonBindings[i] = BindingJson.FromBinding(bindings[i]);
+                        jsonBindings[i] = BindingJson.FromBinding(ref bindings[i]);
                 }
 
                 return new WriteMapJson
diff --git a/Packages/com.unity.inputsystem/InputSystem/Actions/InputBinding.cs b/Packages/com.unity.inputsystem/InputSystem/Actions/InputBinding.cs
@@ -98,15 +98,12 @@ public Guid id
         {
             get
             {
-                if (m_Guid == Guid.Empty && !string.IsNullOrEmpty(m_Id))
-                    m_Guid = new Guid(m_Id);
-                return m_Guid;
-            }
-            set
-            {
-                m_Guid = value;
-                m_Id = m_Guid.ToString();
+                ////REVIEW: this is inconsistent with InputActionMap and InputAction which generate IDs, if necessary
+                if (string.IsNullOrEmpty(m_Id))
+                    return default;
+                return new Guid(m_Id);
             }
+            set => m_Id = value.ToString();
         }
 
         /// <summary>
@@ -359,7 +356,6 @@ public InputBinding(string path, string action = null, string groups = null, str
             m_Processors = processors;
             m_Interactions = interactions;
             m_Name = name;
-            m_Guid = default;
             m_Id = default;
             m_Flags = default;
             m_OverridePath = default;
@@ -376,21 +372,7 @@ public string GetNameOfComposite()
 
         internal void GenerateId()
         {
-            m_Guid = Guid.NewGuid();
-            m_Id = m_Guid.ToString();
-        }
-
-        internal string MakeSureIdIsInPlace()
-        {
-            if (m_Guid != Guid.Empty)
-                return m_Id;
-
-            if (string.IsNullOrEmpty(m_Id))
-                GenerateId();
-            else
-                m_Guid = new Guid(m_Id);
-
-            return m_Id;
+            m_Id = Guid.NewGuid().ToString();
         }
 
         public static InputBinding MaskByGroup(string group)
@@ -415,8 +397,6 @@ public static InputBinding MaskByGroups(params string[] groups)
         [NonSerialized] private string m_OverridePath;
         [NonSerialized] private string m_OverrideInteractions;
         [NonSerialized] private string m_OverrideProcessors;
-        ////REVIEW: do we actually need this or should we just convert from m_Id on the fly all the time?
-        [NonSerialized] internal Guid m_Guid;
 
         /// <summary>
         /// This is the bindings path which is effectively being used.
