Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Special chars in HID names causing rebinding to not work (case 1335465). #1451

Merged
merged 8 commits into from Nov 29, 2021
Merged
51 changes: 50 additions & 1 deletion Assets/Tests/InputSystem/Plugins/HIDTests.cs
Expand Up @@ -13,7 +13,6 @@
using UnityEngine.InputSystem.Layouts;
using UnityEngine.InputSystem.Processors;
using UnityEngine.InputSystem.Utilities;
using UnityEngine.TestTools;
using UnityEngine.TestTools.Utils;

////TODO: add test to make sure we're not grabbing HIDs that have more specific layouts
Expand Down Expand Up @@ -1312,5 +1311,55 @@ public void Utilities_CanRecognizeVendorDefinedUsages()
Assert.That(HID.UsagePageToString((HID.UsagePage) 0xff01), Is.EqualTo("Vendor-Defined"));
Assert.That(HID.UsageToString((HID.UsagePage) 0xff01, 0x33), Is.Null);
}

// https://fogbugz.unity3d.com/f/cases/1335465/
[Test]
[Category("Devices")]
[TestCase('/')]
[TestCase('\\')]
[TestCase('>')]
[TestCase('<')]
Rene-Damm marked this conversation as resolved.
Show resolved Hide resolved
[TestCase(' ')]
public void Devices_CanHaveReservedCharactersInHIDDeviceNames(char character)
{
var hidDescriptor = new HID.HIDDeviceDescriptor
{
usage = (int)HID.GenericDesktop.Joystick,
usagePage = HID.UsagePage.GenericDesktop,
elements = new[]
{
new HID.HIDElementDescriptor { usage = (int)HID.GenericDesktop.X, usagePage = HID.UsagePage.GenericDesktop, reportType = HID.HIDReportType.Input, reportId = 1, reportOffsetInBits = 0, reportSizeInBits = 16 },
new HID.HIDElementDescriptor { usage = (int)HID.GenericDesktop.Y, usagePage = HID.UsagePage.GenericDesktop, reportType = HID.HIDReportType.Input, reportId = 1, reportOffsetInBits = 16, reportSizeInBits = 16 },
new HID.HIDElementDescriptor { usage = (int)HID.Button.Primary, usagePage = HID.UsagePage.Button, reportType = HID.HIDReportType.Input, reportId = 1, reportOffsetInBits = 32, reportSizeInBits = 1 },
new HID.HIDElementDescriptor { usage = (int)HID.Button.Secondary, usagePage = HID.UsagePage.Button, reportType = HID.HIDReportType.Input, reportId = 1, reportOffsetInBits = 33, reportSizeInBits = 1 },
}
};

var device = InputSystem.AddDevice(
new InputDeviceDescription
{
interfaceName = HID.kHIDInterface,
manufacturer = "TestVendor",
product = "Test" + character + "HID",
capabilities = hidDescriptor.ToJson()
});

Assert.That(device.name, Is.EqualTo("TestVendor Test" + (character == '/' ? InputControlPath.SeparatorReplacement : character) + "HID"));
Assert.That(device.displayName, Is.EqualTo("Test" + character + "HID"));

var action = new InputAction(binding: "<Keyboard>/space");

// Interactively rebind the action to refer to the device's button.
using (action.PerformInteractiveRebinding()
.OnMatchWaitForAnother(0)
.Start())
{
Press((ButtonControl)device["button2"]);
}

Assert.That(InputControlPath.TryGetDeviceLayout(action.bindings[0].effectivePath), Is.EqualTo(device.layout));
Assert.That(InputControlPath.TryGetControlLayout(action.bindings[0].effectivePath), Is.EqualTo("Button"));
Assert.That(action.controls, Is.EquivalentTo(new[] { device["button2"]}));
}
}
#endif
1 change: 1 addition & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
Expand Up @@ -29,6 +29,7 @@ however, it has to be formatted properly to pass verification tests.
- Fixed DualSense on iOS not inheriting from `DualShockGamepad` ([case 1378308](https://issuetracker.unity3d.com/issues/input-dualsense-detection-ios)).
- Fixed a device becoming `.current` (e.g. `Gamepad.current`, etc) when sending a new state event that contains no control changes (case 1377952).
- Fixed calling `IsPressed` on an entire device returning `true` ([case 1374024](https://issuetracker.unity3d.com/issues/inputcontrol-dot-ispressed-always-returns-true-when-using-new-input-system)).
- Fixed HIDs having blackslashes in their vendor or product names leading to binding paths generated by interactive rebinding that failed to resolve to controls and thus lead to no input being received ([case 1335465](https://issuetracker.unity3d.com/product/unity/issues/guid/1335465/)).

#### Actions

Expand Down
Expand Up @@ -646,7 +646,7 @@ private void EnableControls(InputAction action)

var map = action.m_ActionMap;
var mapIndex = map.m_MapIndexInState;
Debug.Assert(mapIndex >= 0 && mapIndex < totalMapCount, "Map index out of range");
Debug.Assert(mapIndex >= 0 && mapIndex < totalMapCount, "Map index out of range in EnableControls");

// Go through all bindings in the map and for all that belong to the given action,
// enable the associated controls.
Expand Down Expand Up @@ -682,7 +682,7 @@ public void DisableAllActions(InputActionMap map)

// Mark all actions as disabled.
var mapIndex = map.m_MapIndexInState;
Debug.Assert(mapIndex >= 0 && mapIndex < totalMapCount, "Map index out of range");
Debug.Assert(mapIndex >= 0 && mapIndex < totalMapCount, "Map index out of range in DisableAllActions");
var actionStartIndex = mapIndices[mapIndex].actionStartIndex;
var actionCount = mapIndices[mapIndex].actionCount;
for (var i = 0; i < actionCount; ++i)
Expand All @@ -708,7 +708,7 @@ private void DisableControls(InputActionMap map)
Debug.Assert(maps.Contains(map), "Map must be contained in state");

var mapIndex = map.m_MapIndexInState;
Debug.Assert(mapIndex >= 0 && mapIndex < totalMapCount, "Map index out of range");
Debug.Assert(mapIndex >= 0 && mapIndex < totalMapCount, "Map index out of range in DisableControls(InputActionMap)");

// Remove state monitors from all controls.
var controlCount = mapIndices[mapIndex].controlCount;
Expand Down Expand Up @@ -742,7 +742,7 @@ private void DisableControls(InputAction action)

var map = action.m_ActionMap;
var mapIndex = map.m_MapIndexInState;
Debug.Assert(mapIndex >= 0 && mapIndex < totalMapCount, "Map index out of range");
Debug.Assert(mapIndex >= 0 && mapIndex < totalMapCount, "Map index out of range in DisableControls(InputAction)");

// Go through all bindings in the map and for all that belong to the given action,
// disable the associated controls.
Expand Down Expand Up @@ -1012,7 +1012,7 @@ private static long ToCombinedMapAndControlAndBindingIndex(int mapIndex, int con
/// </remarks>
private void ProcessControlStateChange(int mapIndex, int controlIndex, int bindingIndex, double time, InputEventPtr eventPtr)
{
Debug.Assert(mapIndex >= 0 && mapIndex < totalMapCount, "Map index out of range");
Debug.Assert(mapIndex >= 0 && mapIndex < totalMapCount, "Map index out of range in ProcessControlStateChange");
Debug.Assert(controlIndex >= 0 && controlIndex < totalControlCount, "Control index out of range");
Debug.Assert(bindingIndex >= 0 && bindingIndex < totalBindingCount, "Binding index out of range");

Expand Down
Expand Up @@ -919,22 +919,24 @@ internal static string BuildPath(this InputControl control, string deviceLayout,
var device = control.device;

builder.Append('<');
builder.Append(deviceLayout);
builder.Append(deviceLayout.Escape("\\>", "\\>"));
builder.Append('>');

// Add usages of device, if any.
var deviceUsages = device.usages;
for (var i = 0; i < deviceUsages.Count; ++i)
{
builder.Append('{');
builder.Append(deviceUsages[i]);
builder.Append(deviceUsages[i].ToString().Escape("\\}", "\\}"));
builder.Append('}');
}

builder.Append('/');
builder.Append(InputControlPath.Separator);

var devicePath = device.path;
var controlPath = control.path;
// If any of the components contains a backslash, double it up as in control paths,
// these serve as escape characters.
var devicePath = device.path.Replace("\\", "\\\\");
var controlPath = control.path.Replace("\\", "\\\\");
builder.Append(controlPath, devicePath.Length + 1, controlPath.Length - devicePath.Length - 1);

return builder.ToString();
Expand Down
Expand Up @@ -97,6 +97,19 @@ public static class InputControlPath

public const char Separator = '/';

// We consider / a reserved character in control names. So, when this character does creep
Rene-Damm marked this conversation as resolved.
Show resolved Hide resolved
// in there (e.g. from a device product name), we need to do something about it. We replace
// it with this character here.
// NOTE: Display names have no such restriction.
// NOTE: There are some Unicode characters that look sufficiently like a slash (e.g. FULLWIDTH SOLIDUS)
// but that only makes for rather confusing output. So we just replace with a blank.
internal const char SeparatorReplacement = ' ';

internal static string CleanSlashes(this String pathComponent)
{
return pathComponent.Replace(Separator, SeparatorReplacement);
}

public static string Combine(InputControl parent, string path)
{
if (parent == null)
Expand All @@ -116,7 +129,7 @@ public static string Combine(InputControl parent, string path)
}

/// <summary>
/// Options for customizing the behavior of <see cref="ToHumanReadableString"/>.
/// Options for customizing the behavior of <see cref="ToHumanReadableString(string,HumanReadableStringOptions,InputControl)"/>.
/// </summary>
[Flags]
public enum HumanReadableStringOptions
Expand Down Expand Up @@ -356,7 +369,7 @@ public static string TryGetDeviceLayout(string path)
return null;

if (parser.current.m_Layout.length > 0)
return parser.current.m_Layout.ToString();
return parser.current.m_Layout.ToString().Unescape();

if (parser.current.isWildcard)
return Wildcard;
Expand Down Expand Up @@ -410,7 +423,7 @@ public static string TryGetControlLayout(string path)
if (parser.current.isWildcard)
return Wildcard;

return FindControlLayoutRecursive(ref parser, deviceLayoutName);
return FindControlLayoutRecursive(ref parser, deviceLayoutName.Unescape());
}

private static string FindControlLayoutRecursive(ref PathParser parser, string layoutName)
Expand Down Expand Up @@ -535,6 +548,8 @@ private static bool StringMatches(Substring str, InternedString matchTo)
while (posInStr < strLength && posInMatchTo < matchToLength)
{
var nextChar = str[posInStr];
if (nextChar == '\\' && posInStr + 1 < strLength)
nextChar = str[++posInStr];
if (nextChar == '*')
{
////TODO: make sure we don't end up with ** here
Expand Down Expand Up @@ -1047,7 +1062,7 @@ private static bool MatchPathComponent(string component, string path, ref int in
}
else
{
if (nextCharInPath == '/')
if (nextCharInPath == '/' && componentType == PathComponentType.Name)
break;
if ((nextCharInPath == '>' && componentType == PathComponentType.Layout)
|| (nextCharInPath == '}' && componentType == PathComponentType.Usage)
Expand Down Expand Up @@ -1329,17 +1344,13 @@ public bool Matches(InputControl control)
if (!m_Layout.isEmpty)
{
// Check for direct match.
var layoutMatches = Substring.Compare(m_Layout, control.layout,
StringComparison.OrdinalIgnoreCase) == 0;
var layoutMatches = ComparePathElementToString(m_Layout, control.layout);
if (!layoutMatches)
{
// No direct match but base layout may match.
var baseLayout = control.m_Layout;
while (InputControlLayout.s_Layouts.baseLayoutTable.TryGetValue(baseLayout, out baseLayout) && !layoutMatches)
{
layoutMatches = Substring.Compare(m_Layout, baseLayout.ToString(),
StringComparison.OrdinalIgnoreCase) == 0;
}
layoutMatches = ComparePathElementToString(m_Layout, baseLayout.ToString());
}

if (!layoutMatches)
Expand All @@ -1356,7 +1367,7 @@ public bool Matches(InputControl control)
var controlUsages = control.usages;
var haveUsageMatch = false;
for (var ci = 0; ci < controlUsages.Count; ++ci)
if (Substring.Compare(controlUsages[ci].ToString(), m_Usages[i], StringComparison.OrdinalIgnoreCase) == 0)
if (ComparePathElementToString(m_Usages[i], controlUsages[ci]))
{
haveUsageMatch = true;
break;
Expand All @@ -1372,15 +1383,39 @@ public bool Matches(InputControl control)
if (!m_Name.isEmpty && !isWildcard)
{
////FIXME: unlike the matching path we have in MatchControlsRecursive, this does not take aliases into account
if (Substring.Compare(control.name, m_Name, StringComparison.OrdinalIgnoreCase) != 0)
if (!ComparePathElementToString(m_Name, control.name))
return false;
}

// Match display name.
if (!m_DisplayName.isEmpty)
{
if (Substring.Compare(control.displayName, m_DisplayName,
StringComparison.OrdinalIgnoreCase) != 0)
if (!ComparePathElementToString(m_DisplayName, control.displayName))
return false;
}

return true;
}

// In a path, characters may be escaped so in those cases, we can't just compare
// character-by-character.
private static bool ComparePathElementToString(Substring pathElement, string element)
{
var pathElementLength = pathElement.length;
var elementLength = element.Length;

// `element` is expected to not include escape sequence. `pathElement` may.
// So if `element` is longer than `pathElement`, the two can't be a match.
if (elementLength > pathElementLength)
return false;

for (var i = 0; i < pathElementLength && i < elementLength; ++i)
{
var ch = pathElement[i];
if (ch == '\\' && i + 1 < pathElementLength)
ch = pathElement[++i];

if (char.ToLowerInvariant(ch) != char.ToLowerInvariant(element[i]))
return false;
}

Expand Down Expand Up @@ -1515,7 +1550,11 @@ private Substring ParseComponentPart(char terminator)

var partStartIndex = rightIndexInPath;
while (rightIndexInPath < length && path[rightIndexInPath] != terminator)
{
if (path[rightIndexInPath] == '\\' && rightIndexInPath + 1 < length)
++rightIndexInPath;
++rightIndexInPath;
}

var partLength = rightIndexInPath - partStartIndex;
if (rightIndexInPath < length && terminator != '/')
Expand Down
Expand Up @@ -173,6 +173,10 @@ private InputControl InstantiateLayout(InternedString layout, InternedString var
name = new InternedString(name.ToString().Substring(indexOfLastColon + 1));
}

// Make sure name does not contain any slashes.
if (name.ToString().IndexOf(InputControlPath.Separator) != -1)
name = new InternedString(name.ToString().CleanSlashes());

// Variant defaults to variants of layout.
if (variants.IsEmpty())
{
Expand Down