ISXB-358 wildcards not working #1665
Conversation
There was a problem hiding this comment.
Looks like various tests are failing.
I think at minimum MatchesMask needs a path!=null check at the start
[Error] NullReferenceException while resolving binding 'Move:/leftStick[Gamepad]' in action map 'InGameHintsActions (UnityEngine.InputSystem.InputActionAsset):Gameplay''.
https://unity-ci.cds.internal.unity3d.com/job/22912980
I'd also like to see more detail in the PR description about cases checked.
It would be useful to also add a new test for the specific case we are fixing in this PR
Packages/com.unity.inputsystem/InputSystem/Actions/InputBinding.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Actions/InputBinding.cs
Outdated
Show resolved
Hide resolved
|
There are several test failures which seems to highlight problems with this change. |
| Assert.That(mask.MatchesMask(ref binding), Is.False); | ||
| mask.path = "<Keyboard>/*"; | ||
| Assert.That(mask.MatchesMask(ref binding), Is.True); | ||
| mask.path = null; |
There was a problem hiding this comment.
I'd be tempted to split some out into a new tests to improve readability
This is a good guide on test naming
https://internaldocs.unity.com/automation/Guidelines/naming/
UnitOfWork_WhenStateUnderTest_ProducesExpectedBehavior()
Perhaps some like:
Controls_BindingWithoutMask_Matches
Controls_BindingMaskWithSpecificKey_IgnoresOtherKey
Controls_BindingMaskWithWildcard_Matches
Controls_BindingMaskWithNullPath_Ignored
Controls_BindingMaskWithNullName_Ignored
Controls_BindingMaskWithMousePath_FiltersOutKeyboard
| @@ -842,6 +842,14 @@ public bool Matches(InputBinding binding) | |||
| return Matches(ref binding); | |||
| } | |||
|
|
|||
| internal bool MatchesMask(ref InputBinding binding) | |||
| { | |||
| if (path != null) | |||
There was a problem hiding this comment.
Minor point but with nested ifs I usually add braces around the code within the outer if, just to make it clear and avoid errors if someone adds to the inner if.
However in this case you could also collapse into a single if (path != null && InputControlPath....)
Minor point and not needing to hold up PR.
| @@ -481,6 +481,12 @@ private static string FindControlLayoutRecursive(ref PathParser parser, InputCon | |||
| return currentResult; | |||
| } | |||
|
|
|||
| internal static bool BindingPathMatchesMask(string mask, string bindingPath) | |||
| { | |||
| var bindingPathString = new InternedString(bindingPath); | |||
There was a problem hiding this comment.
I'm a little worried about the InternedString creation here which is creating a lowercase variant of the bindingPath string which will be doing a GC allocation
This BindingPathMatchesMask is called inside a loop over all bindings so this will be generating a lot of garbage.
Perhaps StringMatches could be altered to take a string variant. However I think it uses InternedString for the speed of the internal checks (as we've seen in the separate PR about turkish letter support)
I'm also seeing that StringMatches says the second 'matchTo' parameter may not contain wildcards and I think the bindingPath is allowed to contain wildcards.
There was a problem hiding this comment.
Yes binding path can contain wildcards
https://docs.unity3d.com/Packages/[email protected]/manual/ActionBindings.html
A Binding path can also contain wildcards, such as /button*
Description
Fixes the wildcard not working bug
Changes made
Changed:
BindingMasks get compared to Bindings with another method (StringMatches)
Fixed:
Wildcards working now
Added:
tests for wildcards
cases checked:
Notes
Please write down any additional notes, remove the section if not applicable.
Checklist
Before review:
Changed,Fixed,Addedsections.([case %number%](https://issuetracker.unity3d.com/issues/...)).Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.