FIX: Binding not getting sync'd when renaming/deleting control schemes (#802).

Author: Rene Damm

Assets/Tests/InputSystem/CoreTests_Actions.cs

@@ -2243,7 +2243,7 @@ public void Actions_CanConvertAssetToAndFromJson()
         map2.AddAction("action3", binding: "<Gamepad>/leftTrigger");
 
         asset.AddControlScheme("scheme1").WithBindingGroup("group1").WithRequiredDevice("<Gamepad>");
-        asset.AddControlScheme("scheme2").BasedOn("scheme1").WithBindingGroup("group2")
+        asset.AddControlScheme("scheme2").WithBindingGroup("group2")
             .WithOptionalDevice("<Keyboard>").WithRequiredDevice("<Mouse>").OrWithRequiredDevice("<Pen>");
 
         var json = asset.ToJson();
@@ -2279,8 +2279,6 @@ public void Actions_CanConvertAssetToAndFromJson()
         Assert.That(asset.controlSchemes[1].name, Is.EqualTo("scheme2"));
         Assert.That(asset.controlSchemes[0].bindingGroup, Is.EqualTo("group1"));
         Assert.That(asset.controlSchemes[1].bindingGroup, Is.EqualTo("group2"));
-        Assert.That(asset.controlSchemes[0].baseScheme, Is.Null);
-        Assert.That(asset.controlSchemes[1].baseScheme, Is.EqualTo("scheme1"));
         Assert.That(asset.controlSchemes[0].deviceRequirements, Has.Count.EqualTo(1));
         Assert.That(asset.controlSchemes[1].deviceRequirements, Has.Count.EqualTo(3));
         Assert.That(asset.controlSchemes[0].deviceRequirements[0].controlPath, Is.EqualTo("<Gamepad>"));

Assets/Tests/InputSystem/CoreTests_Editor.cs

@@ -451,6 +451,40 @@ public void Editor_InputAsset_CanChangeCompositeType()
         Assert.That(action1.bindings[5].name, Is.Empty);
     }
 
+    [Test]
+    [Category("Editor")]
+    public void Editor_InputAsset_CanReplaceBindingGroupThroughSerialization()
+    {
+        var map = new InputActionMap("map");
+        var action = map.AddAction(name: "action1");
+        action.AddBinding("Foo", groups: "A");
+        action.AddBinding("Bar", groups: "B");
+        action.AddBinding("Flub", groups: "A;B");
+        var asset = ScriptableObject.CreateInstance<InputActionAsset>();
+        asset.AddActionMap(map);
+
+        var obj = new SerializedObject(asset);
+        InputActionSerializationHelpers.ReplaceBindingGroup(obj, "A", "C");
+        obj.ApplyModifiedPropertiesWithoutUndo();
+
+        Assert.That(action.bindings[0].groups, Is.EqualTo("C"));
+        Assert.That(action.bindings[1].groups, Is.EqualTo("B"));
+        Assert.That(action.bindings[2].groups, Is.EqualTo("C;B"));
+
+        InputActionSerializationHelpers.ReplaceBindingGroup(obj, "C", "");
+        obj.ApplyModifiedPropertiesWithoutUndo();
+
+        Assert.That(action.bindings[0].groups, Is.EqualTo(""));
+        Assert.That(action.bindings[1].groups, Is.EqualTo("B"));
+        Assert.That(action.bindings[2].groups, Is.EqualTo("B"));
+
+        InputActionSerializationHelpers.ReplaceBindingGroup(obj, "B", "", deleteOrphanedBindings: true);
+        obj.ApplyModifiedPropertiesWithoutUndo();
+
+        Assert.That(map.bindings, Has.Count.EqualTo(1));
+        Assert.That(map.bindings[0].groups, Is.EqualTo(""));
+    }
+
     private class MonoBehaviourWithEmbeddedAction : MonoBehaviour
     {
         public InputAction action;

Packages/com.unity.inputsystem/CHANGELOG.md

@@ -15,6 +15,9 @@ however, it has to be formatted properly to pass verification tests.
 
 #### Actions
 
+- When deleting a control scheme, bindings are now updated. A dialog is presented that allows choosing between deleting the bindings or just unassigning them from the control scheme.
+- When renaming a control scheme, bindings are now updated. Previously the old name was in place on bindings.
+- Control scheme names can no longer be set to empty strings.
 - `PlayerInput.Instantiate` now correctly sets up a given control scheme, if specified.
   * When passing a `controlScheme:` argument, the result used to be a correctly assigned control scheme at the `InputUser` level but no restrictions being actually applied to the bindings, i.e. every single binding was active regardless of the specified control scheme.
 
@@ -27,6 +30,7 @@ however, it has to be formatted properly to pass verification tests.
 #### Actions
 
 - When switching devices/controls on actions, the system will no longer subsequently force an initial state check on __all__ actions. Instead, every time an action's bindings get re-resolved, the system will simply cancel all on-going actions and then re-enable them the same way it would happen by manually calling `InputAction.Enable`.
+- Removed non-functional `InputControlScheme.baseScheme` API and `basedOn` serialized property. This was never fully implemented.
 
 ### Added
 

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

@@ -571,19 +571,6 @@ internal ControlSchemeSyntax(InputControlScheme controlScheme)
                 m_ControlScheme = controlScheme;
             }
 
-            public ControlSchemeSyntax BasedOn(string baseControlScheme)
-            {
-                if (string.IsNullOrEmpty(baseControlScheme))
-                    throw new ArgumentNullException(nameof(baseControlScheme));
-
-                if (m_Asset == null)
-                    m_ControlScheme.m_BaseSchemeName = baseControlScheme;
-                else
-                    m_Asset.m_ControlSchemes[m_ControlSchemeIndex].m_BaseSchemeName = baseControlScheme;
-
-                return this;
-            }
-
             public ControlSchemeSyntax WithBindingGroup(string bindingGroup)
             {
                 if (string.IsNullOrEmpty(bindingGroup))

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

@@ -8,9 +8,6 @@
 
 ////REVIEW: allow associating control schemes with platforms, too?
 
-////REVIEW: move `baseScheme` entirely into JSON data only such that we resolve it during loading?
-////        (and thus support it only input assets only)
-
 namespace UnityEngine.InputSystem
 {
     /// <summary>
@@ -35,19 +32,6 @@ public struct InputControlScheme : IEquatable<InputControlScheme>
         /// <seealso cref="InputActionAsset.AddControlScheme"/>
         public string name => m_Name;
 
-        ////REVIEW: is this actually functional? if not, kill
-        //problem: how do you do any subtractive operation? should we care?
-        //problem: this won't allow resolving things on just an InputControlScheme itself; needs context
-        /// <summary>
-        /// Name of control scheme that this scheme is based on.
-        /// </summary>
-        /// <remarks>
-        /// When the control scheme is enabled, all bindings from the base control
-        /// scheme will also be enabled. At the same time, bindings act as overrides on
-        /// bindings coming through from the base scheme.
-        /// </remarks>
-        public string baseScheme => m_BaseSchemeName;
-
         /// <summary>
         /// Binding group that is associated with the control scheme.
         /// </summary>
@@ -74,14 +58,12 @@ public string bindingGroup
         /// </remarks>
         public ReadOnlyArray<DeviceRequirement> deviceRequirements => new ReadOnlyArray<DeviceRequirement>(m_DeviceRequirements);
 
-        public InputControlScheme(string name, string basedOn = null, IEnumerable<DeviceRequirement> devices = null)
+        public InputControlScheme(string name, IEnumerable<DeviceRequirement> devices = null)
+            : this()
         {
-            m_Name = name;
-            m_BaseSchemeName = string.Empty;
-            m_BindingGroup = name; // Defaults to name.
-            m_BaseSchemeName = basedOn;
-            m_DeviceRequirements = null;
+            SetNameAndBindingGroup(name);
 
+            m_DeviceRequirements = null;
             if (devices != null)
             {
                 m_DeviceRequirements = devices.ToArray();
@@ -90,6 +72,14 @@ public InputControlScheme(string name, string basedOn = null, IEnumerable<Device
             }
         }
 
+        internal void SetNameAndBindingGroup(string name)
+        {
+            m_Name = name;
+            bindingGroup = name.Contains(InputBinding.Separator)
+                ? name.Replace(InputBinding.kSeparatorString, "")
+                : name;
+        }
+
         public static InputControlScheme? FindControlSchemeForDevice<TList>(InputDevice device, TList schemes)
             where TList : IEnumerable<InputControlScheme>
         {
@@ -288,7 +278,6 @@ public MatchResult PickDevicesFrom<TDevices>(TDevices devices)
         public bool Equals(InputControlScheme other)
         {
             if (!(string.Equals(m_Name, other.m_Name, StringComparison.InvariantCultureIgnoreCase) &&
-                  string.Equals(m_BaseSchemeName, other.m_BaseSchemeName, StringComparison.InvariantCultureIgnoreCase) &&
                   string.Equals(m_BindingGroup, other.m_BindingGroup, StringComparison.InvariantCultureIgnoreCase)))
                 return false;
 
@@ -332,7 +321,6 @@ public override int GetHashCode()
             unchecked
             {
                 var hashCode = (m_Name != null ? m_Name.GetHashCode() : 0);
-                hashCode = (hashCode * 397) ^ (m_BaseSchemeName != null ? m_BaseSchemeName.GetHashCode() : 0);
                 hashCode = (hashCode * 397) ^ (m_BindingGroup != null ? m_BindingGroup.GetHashCode() : 0);
                 hashCode = (hashCode * 397) ^ (m_DeviceRequirements != null ? m_DeviceRequirements.GetHashCode() : 0);
                 return hashCode;
@@ -376,7 +364,6 @@ public override string ToString()
         }
 
         [SerializeField] internal string m_Name;
-        [SerializeField] internal string m_BaseSchemeName;
         [SerializeField] internal string m_BindingGroup;
         [SerializeField] internal DeviceRequirement[] m_DeviceRequirements;
 
@@ -770,8 +757,6 @@ public override int GetHashCode()
         internal struct SchemeJson
         {
             public string name;
-            ////TODO: nuke 'basedOn'
-            public string basedOn;
             public string bindingGroup;
             public DeviceJson[] devices;
 
@@ -817,7 +802,6 @@ public InputControlScheme ToScheme()
                 return new InputControlScheme
                 {
                     m_Name = string.IsNullOrEmpty(name) ? null : name,
-                    m_BaseSchemeName = string.IsNullOrEmpty(basedOn) ? null : basedOn,
                     m_BindingGroup = string.IsNullOrEmpty(bindingGroup) ? null : bindingGroup,
                     m_DeviceRequirements = deviceRequirements,
                 };
@@ -837,7 +821,6 @@ public static SchemeJson ToJson(InputControlScheme scheme)
                 return new SchemeJson
                 {
                     name = scheme.m_Name,
-                    basedOn = scheme.m_BaseSchemeName,
                     bindingGroup = scheme.m_BindingGroup,
                     devices = devices,
                 };

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

@@ -204,8 +204,10 @@ private void OnDeleteControlScheme()
         {
             Debug.Assert(m_SelectedControlSchemeIndex >= 0, "Control scheme must be selected");
 
-            // Ask for confirmation.
             var name = m_ControlSchemes[m_SelectedControlSchemeIndex].name;
+            var bindingGroup = m_ControlSchemes[m_SelectedControlSchemeIndex].bindingGroup;
+
+            // Ask for confirmation.
             if (!EditorUtility.DisplayDialog("Delete scheme?", $"Do you want to delete control scheme '{name}'?",
                 "Delete", "Cancel"))
                 return;
@@ -222,8 +224,10 @@ private void OnDeleteControlScheme()
 
             onControlSchemesChanged?.Invoke();
             onSelectedSchemeChanged?.Invoke();
+            onControlSchemeDeleted?.Invoke(name, bindingGroup);
         }
 
+        ////REVIEW: this does nothing to bindings; should this ask to duplicate bindings as well?
         private void OnDuplicateControlScheme(object position)
         {
             Debug.Assert(m_SelectedControlSchemeIndex >= 0, "Control scheme must be selected");
@@ -236,7 +240,6 @@ private void OnDuplicateControlScheme(object position)
                 (s, _) => AddAndSelectControlScheme(s));
         }
 
-        ////REVIEW: renaming a control scheme should probably update the binding group (also on all bindings)
         private void OnEditSelectedControlScheme(object position)
         {
             Debug.Assert(m_SelectedControlSchemeIndex >= 0, "Control scheme must be selected");
@@ -266,19 +269,30 @@ private void UpdateControlScheme(InputControlScheme scheme, int index)
         {
             Debug.Assert(index >= 0 && index < m_ControlSchemes.LengthSafe(), "Control scheme index out of range");
 
+            var renamed = false;
+            string oldBindingGroup = null;
+            string newBindingGroup = null;
+
             // If given scheme has no name, preserve the existing one on the control scheme.
             if (string.IsNullOrEmpty(scheme.name))
                 scheme.m_Name = m_ControlSchemes[index].name;
 
             // If name is changing, make sure it's unique.
             else if (scheme.name != m_ControlSchemes[index].name)
             {
+                renamed = true;
+                oldBindingGroup = m_ControlSchemes[index].bindingGroup;
                 m_ControlSchemes[index].m_Name = ""; // Don't want this to interfere with finding a unique name.
-                m_ControlSchemes[index].m_Name = MakeUniqueControlSchemeName(scheme.name);
+                var newName = MakeUniqueControlSchemeName(scheme.name);
+                m_ControlSchemes[index].SetNameAndBindingGroup(newName);
+                newBindingGroup = m_ControlSchemes[index].bindingGroup;
             }
 
             m_ControlSchemes[index] = scheme;
             onControlSchemesChanged?.Invoke();
+
+            if (renamed)
+                onControlSchemeRenamed?.Invoke(oldBindingGroup, newBindingGroup);
         }
 
         private void SelectControlScheme(int index)
@@ -319,6 +333,8 @@ private static string DeviceRequirementToDisplayString(InputControlScheme.Device
         public Action onSelectedSchemeChanged;
         public Action onSelectedDeviceChanged;
         public Action onControlSchemesChanged;
+        public Action<string, string> onControlSchemeRenamed;
+        public Action<string, string> onControlSchemeDeleted;
         public Action onSave;
 
         [SerializeField] private bool m_IsDirty;
@@ -478,11 +494,20 @@ private void DrawConfirmationButton()
                 }
                 if (GUILayout.Button("Save", GUILayout.ExpandWidth(true)))
                 {
-                    m_ControlScheme = new InputControlScheme(m_ControlScheme.name,
-                        devices: m_DeviceList.Select(a => a.deviceRequirement));
+                    // Don't allow control scheme name to be empty.
+                    if (string.IsNullOrEmpty(m_ControlScheme.name))
+                    {
+                        ////FIXME: On 2019.1 this doesn't display properly in the window; check 2019.3
+                        editorWindow.ShowNotification(new GUIContent("Control scheme must have a name"));
+                    }
+                    else
+                    {
+                        m_ControlScheme = new InputControlScheme(m_ControlScheme.name,
+                            devices: m_DeviceList.Select(a => a.deviceRequirement));
 
-                    editorWindow.Close();
-                    m_OnApply(m_ControlScheme, m_ControlSchemeIndex);
+                        editorWindow.Close();
+                        m_OnApply(m_ControlScheme, m_ControlSchemeIndex);
+                    }
                 }
                 if (Event.current.type == EventType.Repaint)
                     m_ButtonsAndLabelsHeights += GUILayoutUtility.GetLastRect().height;

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

@@ -203,6 +203,8 @@ private void OnEnable()
             m_Toolbar.onSelectedDeviceChanged = OnControlSchemeSelectionChanged;
             m_Toolbar.onSave = SaveChangesToAsset;
             m_Toolbar.onControlSchemesChanged = OnControlSchemesModified;
+            m_Toolbar.onControlSchemeRenamed = OnControlSchemeRenamed;
+            m_Toolbar.onControlSchemeDeleted = OnControlSchemeDeleted;
             EditorApplication.wantsToQuit += EditorWantsToQuit;
 
             // Initialize after assembly reload.
@@ -286,6 +288,43 @@ private void OnControlSchemeSelectionChanged()
         private void OnControlSchemesModified()
         {
             TransferControlSchemes(save: true);
+
+            // Control scheme changes may affect the search filter.
+            OnToolbarSearchChanged();
+
+            ApplyAndReloadTrees();
+        }
+
+        private void OnControlSchemeRenamed(string oldBindingGroup, string newBindingGroup)
+        {
+            InputActionSerializationHelpers.ReplaceBindingGroup(m_ActionAssetManager.serializedObject,
+                oldBindingGroup, newBindingGroup);
+            ApplyAndReloadTrees();
+        }
+
+        private void OnControlSchemeDeleted(string name, string bindingGroup)
+        {
+            Debug.Assert(!string.IsNullOrEmpty(name), "Control scheme name should not be empty");
+            Debug.Assert(!string.IsNullOrEmpty(bindingGroup), "Binding group should not be empty");
+
+            var asset = m_ActionAssetManager.m_AssetObjectForEditing;
+
+            var bindingMask = InputBinding.MaskByGroup(bindingGroup);
+            var schemeHasBindings = asset.actionMaps.Any(m => m.bindings.Any(b => bindingMask.Matches(ref b)));
+            if (!schemeHasBindings)
+                return;
+
+            ////REVIEW: offer to do nothing and leave all bindings as is?
+            var deleteBindings =
+                EditorUtility.DisplayDialog("Delete Bindings?",
+                    $"Delete bindings for '{name}' as well? If you select 'No', the bindings will only "
+                    + $"be unassigned from the '{name}' control scheme but otherwise left as is. Note that bindings "
+                    + $"that are assigned to '{name}' but also to other control schemes will be left in place either way.",
+                    "Yes", "No");
+
+            InputActionSerializationHelpers.ReplaceBindingGroup(m_ActionAssetManager.serializedObject, bindingGroup, "",
+                deleteOrphanedBindings: deleteBindings);
+
             ApplyAndReloadTrees();
         }
 
@@ -441,7 +480,6 @@ private void LoadPropertiesForSelection()
                 // Grab the action for the binding and see if we have an expected control layout
                 // set on it. Pass that on to the control picking machinery.
                 var isCompositePartBinding = item is PartOfCompositeBindingTreeItem;
-                var isCompositeBinding = item is CompositeBindingTreeItem;
                 var actionItem = (isCompositePartBinding ? item.parent.parent : item.parent) as ActionTreeItem;
                 Debug.Assert(actionItem != null);
 
@@ -451,7 +489,7 @@ private void LoadPropertiesForSelection()
                 // The toolbar may constrain the set of devices we're currently interested in by either
                 // having one specific device selected from the current scheme or having at least a control
                 // scheme selected.
-                var controlPathsToMatch = (IEnumerable<string>)null;
+                IEnumerable<string> controlPathsToMatch;
                 if (m_Toolbar.selectedDeviceRequirement != null)
                 {
                     // Single device selected from set of devices in control scheme.
@@ -478,7 +516,8 @@ private void LoadPropertiesForSelection()
                         {
                             if (change == InputBindingPropertiesView.k_PathChanged ||
                                 change == InputBindingPropertiesView.k_CompositePartAssignmentChanged ||
-                                change == InputBindingPropertiesView.k_CompositeTypeChanged)
+                                change == InputBindingPropertiesView.k_CompositeTypeChanged ||
+                                change == InputBindingPropertiesView.k_GroupsChanged)
                             {
                                 ApplyAndReloadTrees();
                             }