CHANGE: Make ToHumanReadableString() respect display names (#804).

Author: Rene Damm

Assets/Tests/InputSystem/CoreTests_Controls.cs

@@ -996,11 +996,20 @@ public void Controls_DisplayNameForNestedControls_IncludesNameOfParentControl()
     public void Controls_CanTurnControlPathIntoHumanReadableText()
     {
         Assert.That(InputControlPath.ToHumanReadableString("*/{PrimaryAction}"), Is.EqualTo("PrimaryAction [Any]"));
-        Assert.That(InputControlPath.ToHumanReadableString("<Gamepad>/leftStick"), Is.EqualTo("leftStick [Gamepad]"));
-        Assert.That(InputControlPath.ToHumanReadableString("<Gamepad>/leftStick/x"), Is.EqualTo("leftStick/x [Gamepad]"));
+        Assert.That(InputControlPath.ToHumanReadableString("<Gamepad>/leftStick"), Is.EqualTo("Left Stick [Gamepad]"));
+        Assert.That(InputControlPath.ToHumanReadableString("<Gamepad>/leftStick/x"), Is.EqualTo("Left Stick/X [Gamepad]"));
         Assert.That(InputControlPath.ToHumanReadableString("<XRController>{LeftHand}/position"), Is.EqualTo("position [LeftHand XRController]"));
         Assert.That(InputControlPath.ToHumanReadableString("*/leftStick"), Is.EqualTo("leftStick [Any]"));
         Assert.That(InputControlPath.ToHumanReadableString("*/{PrimaryMotion}/x"), Is.EqualTo("PrimaryMotion/x [Any]"));
+        Assert.That(InputControlPath.ToHumanReadableString("<Gamepad>/buttonSouth"), Is.EqualTo("Button South [Gamepad]"));
+        Assert.That(InputControlPath.ToHumanReadableString("<XInputController>/buttonSouth"), Is.EqualTo("A [Xbox Controller]"));
+
+        Assert.That(
+            InputControlPath.ToHumanReadableString("<Gamepad>/buttonSouth",
+                InputControlPath.HumanReadableStringOptions.OmitDevice), Is.EqualTo("Button South"));
+        Assert.That(
+            InputControlPath.ToHumanReadableString("*/{PrimaryAction}",
+                InputControlPath.HumanReadableStringOptions.OmitDevice), Is.EqualTo("PrimaryAction"));
     }
 
     [Test]

Packages/com.unity.inputsystem/CHANGELOG.md

@@ -15,6 +15,8 @@ however, it has to be formatted properly to pass verification tests.
 
 #### Actions
 
+- Binding paths now show the same way in the action editor UI as they do in the control picker.
+  * For example, where before a binding to `<XInputController>/buttonSouth` was shown as `rightShoulder [XInputController]`, the same binding will now show as `A [Xbox Controller]`.
 - 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.
@@ -26,6 +28,8 @@ however, it has to be formatted properly to pass verification tests.
 - `InputUser.onUnpairedDeviceUsed` now receives a 2nd argument which is the event that triggered the callback.
   * Also, the callback is now triggered __BEFORE__ the given event is processed rather than after the event has already been written to the device. This allows updating the pairing state of the system before input is processed.
   * In practice, this means that, for example, if the user switches from keyboard&mouse to gamepad, the initial input that triggered the switch will get picked up right away.
+- `InputControlPath.ToHumanReadableString` now takes display names from registered `InputControlLayout` instances into account.
+  * This means that the method can now be used to generate strings to display in rebinding UIs.
 
 #### Actions
 

Packages/com.unity.inputsystem/InputSystem/Controls/InputControlLayout.cs

@@ -343,6 +343,7 @@ public ControlItem this[string path]
                 if (string.IsNullOrEmpty(path))
                     throw new ArgumentNullException(nameof(path));
 
+                // Does not use FindControl so that we don't force-intern the given path string.
                 if (m_Controls != null)
                 {
                     for (var i = 0; i < m_Controls.Length; ++i)
@@ -1825,5 +1826,48 @@ public InputControlLayout FindOrLoadLayout(string name)
                 throw new LayoutNotFoundException(name);
             }
         }
+
+        internal static Cache s_CacheInstance;
+        internal static int s_CacheInstanceRef;
+
+        // Constructing InputControlLayouts is very costly as it tends to involve lots of reflection and
+        // piecing data together. Thus, wherever possible, we want to keep layouts around for as long as
+        // we need them yet at the same time not keep them needlessly around while we don't.
+        //
+        // This property makes a cache of layouts available globally yet implements a resource acquisition
+        // based pattern to make sure we keep the cache alive only within specific execution scopes.
+        internal static ref Cache cache
+        {
+            get
+            {
+                Debug.Assert(s_CacheInstanceRef > 0, "Must hold an instance reference");
+                return ref s_CacheInstance;
+            }
+        }
+
+        internal static CacheRefInstance CacheRef()
+        {
+            ++s_CacheInstanceRef;
+            return new CacheRefInstance {valid = true};
+        }
+
+        internal struct CacheRefInstance : IDisposable
+        {
+            public bool valid; // Make sure we can distinguish default-initialized instances.
+            public void Dispose()
+            {
+                if (!valid)
+                    return;
+
+                --s_CacheInstanceRef;
+                if (s_CacheInstanceRef <= 0)
+                {
+                    s_CacheInstance = default;
+                    s_CacheInstanceRef = 0;
+                }
+
+                valid = false;
+            }
+        }
     }
 }

Packages/com.unity.inputsystem/InputSystem/Controls/InputControlPath.cs

@@ -113,51 +113,106 @@ public static string Combine(InputControl parent, string path)
             return $"{parent.path}/{path}";
         }
 
+        /// <summary>
+        /// Options for customizing the behavior of <see cref="ToHumanReadableString"/>.
+        /// </summary>
+        [Flags]
+        public enum HumanReadableStringOptions
+        {
+            /// <summary>
+            /// The default behavior.
+            /// </summary>
+            None = 0,
+
+            /// <summary>
+            /// Do not mention the device of the control. For example, instead of "A [Gamepad]",
+            /// return just "A".
+            /// </summary>
+            OmitDevice = 1 << 1,
+        }
+
+        ////TODO: factor out the part that looks up an InputControlLayout.ControlItem from a given path
+        ////      and make that available as a stand-alone API
+        ////TODO: add option to customize path separation character
         /// <summary>
         /// Create a human readable string from the given control path.
         /// </summary>
         /// <param name="path">A control path such as "&lt;XRController>{LeftHand}/position".</param>
-        /// <returns>A string such as "leftStick/x [Gamepad]".</returns>
-        public static string ToHumanReadableString(string path)
+        /// <param name="options">Customize the resulting string.</param>
+        /// <returns>A string such as "Left Stick/X [Gamepad]".</returns>
+        /// <remarks>
+        /// This function is most useful for turning binding paths (see <see cref="InputBinding.path"/>)
+        /// into strings that can be displayed in UIs (such as rebinding screens). It is used by
+        /// the Unity editor itself to display binding paths in the UI.
+        ///
+        /// The method uses display names (see <see cref="InputControlAttribute.displayName"/>,
+        /// <see cref="InputControlLayoutAttribute.displayName"/>, and <see cref="InputControlLayout.ControlItem.displayName"/>)
+        /// where possible. For example, "&lt;XInputController&gt;/buttonSouth" will be returned as
+        /// "A [Xbox Controller]" as the display name of <see cref="XInput.XInputController"/> is "XBox Controller"
+        /// and the display name of its "buttonSouth" control is "A".
+        ///
+        /// Note that these lookups depend on the currently registered control layouts (see <see
+        /// cref="InputControlLayout"/>) and different strings may thus be returned for the same control
+        /// path depending on the layouts registered with the system.
+        ///
+        /// <example>
+        /// <code>
+        /// InputControlPath.ToHumanReadableString("*/{PrimaryAction"); // -> "PrimaryAction [Any]"
+        /// InputControlPath.ToHumanReadableString("&lt;Gamepad&gt;/buttonSouth"); // -> "Button South [Gamepad]"
+        /// InputControlPath.ToHumanReadableString("&lt;XInputController&gt;/buttonSouth"); // -> "A [Xbox Controller]"
+        /// InputControlPath.ToHumanReadableString("&lt;Gamepad&gt;/leftStick/x"); // -> "Left Stick/X [Gamepad]"
+        /// </code>
+        /// </example>
+        /// </remarks>
+        /// <seealso cref="InputBinding.path"/>
+        public static string ToHumanReadableString(string path,
+            HumanReadableStringOptions options = HumanReadableStringOptions.None)
         {
             if (string.IsNullOrEmpty(path))
                 return string.Empty;
 
             var buffer = new StringBuilder();
             var parser = new PathParser(path);
 
-            ////REVIEW: ideally, we'd use display names of controls rather than the control paths directly from the path
-
-            // First level is taken to be device.
-            if (parser.MoveToNextComponent())
+            // For display names of controls and devices, we need to look at InputControlLayouts.
+            // If none is in place here, we establish a temporary layout cache while we go through
+            // the path. If one is in place already, we reuse what's already there.
+            using (InputControlLayout.CacheRef())
             {
-                var device = parser.current.ToHumanReadableString();
-
-                // Any additional levels (if present) are taken to form a control path on the device.
-                var isFirstControlLevel = true;
-                while (parser.MoveToNextComponent())
+                // First level is taken to be device.
+                if (parser.MoveToNextComponent())
                 {
-                    if (!isFirstControlLevel)
-                        buffer.Append('/');
+                    // Keep track of which control layout we're on (if any) as we're crawling
+                    // down the path.
+                    var device = parser.current.ToHumanReadableString(null, out var currentLayoutName);
 
-                    buffer.Append(parser.current.ToHumanReadableString());
-                    isFirstControlLevel = false;
-                }
+                    // Any additional levels (if present) are taken to form a control path on the device.
+                    var isFirstControlLevel = true;
+                    while (parser.MoveToNextComponent())
+                    {
+                        if (!isFirstControlLevel)
+                            buffer.Append('/');
 
-                if (!string.IsNullOrEmpty(device))
-                {
-                    buffer.Append(" [");
-                    buffer.Append(device);
-                    buffer.Append(']');
+                        buffer.Append(parser.current.ToHumanReadableString(
+                            currentLayoutName, out currentLayoutName));
+                        isFirstControlLevel = false;
+                    }
+
+                    if ((options & HumanReadableStringOptions.OmitDevice) == 0 && !string.IsNullOrEmpty(device))
+                    {
+                        buffer.Append(" [");
+                        buffer.Append(device);
+                        buffer.Append(']');
+                    }
                 }
-            }
 
-            // If we didn't manage to figure out a display name, default to displaying
-            // the path as is.
-            if (buffer.Length == 0)
-                return path;
+                // If we didn't manage to figure out a display name, default to displaying
+                // the path as is.
+                if (buffer.Length == 0)
+                    return path;
 
-            return buffer.ToString();
+                return buffer.ToString();
+            }
         }
 
         public static string[] TryGetDeviceUsages(string path)
@@ -171,7 +226,7 @@ public static string[] TryGetDeviceUsages(string path)
 
             if (parser.current.usages != null && parser.current.usages.Length > 0)
             {
-                return Array.ConvertAll<Substring, string>(parser.current.usages, i => { return i.ToString(); });
+                return Array.ConvertAll(parser.current.usages, i => { return i.ToString(); });
             }
 
             return null;
@@ -219,11 +274,6 @@ public static string TryGetDeviceLayout(string path)
         ////TODO: return Substring and use path parser; should get rid of allocations
 
         // From the given control path, try to determine the control layout being used.
-        //
-        // NOTE: This function will only use information available in the path itself or
-        //       in layouts referenced by the path. It will not look at actual devices
-        //       in the system. This is to make the behavior predictable and not dependent
-        //       on whether you currently have the right device connected or not.
         // NOTE: Allocates!
         public static string TryGetControlLayout(string path)
         {
@@ -956,8 +1006,10 @@ internal struct ParsedPathComponent
             public bool isWildcard => name == Wildcard;
             public bool isDoubleWildcard => name == DoubleWildcard;
 
-            public string ToHumanReadableString()
+            public string ToHumanReadableString(string parentLayoutName, out string referencedLayoutName)
             {
+                referencedLayoutName = null;
+
                 var result = string.Empty;
                 if (isWildcard)
                     result += "Any";
@@ -987,18 +1039,60 @@ public string ToHumanReadableString()
 
                 if (!layout.isEmpty)
                 {
+                    referencedLayoutName = layout.ToString();
+
+                    // Where possible, use the displayName of the given layout rather than
+                    // just the internal layout name.
+                    string layoutString;
+                    var referencedLayout = InputControlLayout.cache.FindOrLoadLayout(referencedLayoutName);
+                    if (referencedLayout != null && !string.IsNullOrEmpty(referencedLayout.m_DisplayName))
+                        layoutString = referencedLayout.m_DisplayName;
+                    else
+                        layoutString = ToHumanReadableString(layout);
+
                     if (!string.IsNullOrEmpty(result))
-                        result += ' ' + ToHumanReadableString(layout);
+                        result += ' ' + layoutString;
                     else
-                        result += ToHumanReadableString(layout);
+                        result += layoutString;
                 }
 
                 if (!name.isEmpty && !isWildcard)
                 {
+                    // If we have a layout from a preceding path component, try to find
+                    // the control by name on the layout. If we find it, use its display
+                    // name rather than the name referenced in the binding.
+                    string nameString = null;
+                    if (!string.IsNullOrEmpty(parentLayoutName))
+                    {
+                        // NOTE: This produces a fully merged layout. We should thus pick up display names
+                        //       from base layouts automatically wherever applicable.
+                        var parentLayout = InputControlLayout.cache.FindOrLoadLayout(new InternedString(parentLayoutName));
+                        if (parentLayout != null)
+                        {
+                            var controlName = new InternedString(name.ToString());
+                            var control = parentLayout.FindControl(controlName);
+                            if (control != null)
+                            {
+                                if (!string.IsNullOrEmpty(control.Value.displayName))
+                                    nameString = control.Value.displayName;
+
+                                // If we don't have an explicit <layout> part in the component,
+                                // remember the name of the layout referenced by the control name so
+                                // that path components further down the line can keep looking up their
+                                // display names.
+                                if (string.IsNullOrEmpty(referencedLayoutName))
+                                    referencedLayoutName = control.Value.layout;
+                            }
+                        }
+                    }
+
+                    if (nameString == null)
+                        nameString = ToHumanReadableString(name);
+
                     if (!string.IsNullOrEmpty(result))
-                        result += ' ' + ToHumanReadableString(name);
+                        result += ' ' + nameString;
                     else
-                        result += ToHumanReadableString(name);
+                        result += nameString;
                 }
 
                 if (!displayName.isEmpty)

Packages/com.unity.inputsystem/InputSystem/Devices/InputDeviceBuilder.cs

@@ -43,11 +43,13 @@ namespace UnityEngine.InputSystem.Layouts
     /// Existing controls may be reused while at the same time the hierarchy and even the device instance
     /// itself may change.
     /// </remarks>
-    internal struct InputDeviceBuilder
+    internal struct InputDeviceBuilder : IDisposable
     {
         public void Setup(InternedString layout, InternedString variants,
             InputDeviceDescription deviceDescription = default)
         {
+            m_LayoutCacheRef = InputControlLayout.CacheRef();
+
             InstantiateLayout(layout, variants, new InternedString(), null);
             FinalizeControlHierarchy();
 
@@ -67,12 +69,16 @@ public InputDevice Finish()
             return device;
         }
 
-        internal InputDevice m_Device;
+        public void Dispose()
+        {
+            m_LayoutCacheRef.Dispose();
+        }
+
+        private InputDevice m_Device;
 
-        // We construct layouts lazily as we go but keep them cached while we
-        // set up hierarchies so that we don't re-construct the same Button layout
-        // 256 times for a keyboard.
-        private InputControlLayout.Cache m_LayoutCache;
+        // Make sure the global layout cache sticks around for at least as long
+        // as the device builder so that we don't load layouts over and over.
+        private InputControlLayout.CacheRefInstance m_LayoutCacheRef;
 
         // Table mapping (lower-cased) control paths to control layouts that contain
         // overrides for the control at the given path.
@@ -725,7 +731,8 @@ private static void SetFormat(InputControl control, InputControlLayout.ControlIt
 
         private InputControlLayout FindOrLoadLayout(string name)
         {
-            return m_LayoutCache.FindOrLoadLayout(name);
+            Debug.Assert(InputControlLayout.s_CacheInstanceRef > 0, "Should have acquired layout cache reference");
+            return InputControlLayout.cache.FindOrLoadLayout(name);
         }
 
         private static void ComputeStateLayout(InputControl control)
@@ -896,7 +903,7 @@ internal static ref InputDeviceBuilder instance
             }
         }
 
-        public static RefInstance Ref()
+        internal static RefInstance Ref()
         {
             Debug.Assert(s_Instance.m_Device == null,
                 "InputDeviceBuilder is already in use! Cannot use the builder recursively");
@@ -911,8 +918,12 @@ internal struct RefInstance : IDisposable
             public void Dispose()
             {
                 --s_InstanceRef;
-                if (s_InstanceRef == 0)
+                if (s_InstanceRef <= 0)
+                {
+                    s_Instance.Dispose();
                     s_Instance = default;
+                    s_InstanceRef = 0;
+                }
                 else
                     // Make sure we reset when there is an exception.
                     s_Instance.Reset();