Skip to content

Conversation

@josepmariapujol-unity
Copy link
Collaborator

@josepmariapujol-unity josepmariapujol-unity commented Jan 7, 2026

Description

Refactor MatchControlsRecursive to use the new PathParser.

Testing status & QA

Please describe the testing already done by you and what testing you request/recommend QA to execute. If you used or created any testing project please link them here too for QA.

Overall Product Risks

Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.

  • Complexity:
  • Halo Effect:

Comments to reviewers

Please describe any additional information such as what to focus on, or historical info for the reviewers.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

@josepmariapujol-unity josepmariapujol-unity self-assigned this Jan 7, 2026
@josepmariapujol-unity josepmariapujol-unity changed the title Input/removing legacy todos 4 CHANGE: Refactor MatchControlsRecursive to use the new PathParser Jan 8, 2026
@josepmariapujol-unity josepmariapujol-unity marked this pull request as ready for review January 8, 2026 17:25
@u-pr-agent
Copy link
Contributor

u-pr-agent bot commented Jan 8, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

The change replaces a large chunk of path-matching logic with new parsing behavior, so the main effort is validating functional parity across edge cases (escaping, wildcards, trailing slashes, aliases/layout inheritance/usages).
🏅 Score: 72

The refactor is a clear simplification, but it introduces extra substring allocations and appears to change matching semantics for several previously-supported path features, which needs verification.
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Behavior Change

The new MatchControlsRecursive parses only a single component string and matches via PathParser.current.Matches(control), which may not preserve prior semantics such as base-layout chain matching, alias matching, display-name #(...) matching, wildcard *, and accepting trailing slashes; confirm all previously supported path syntaxes still match the same controls.

private static TControl MatchControlsRecursive<TControl>(InputControl control, string path, int indexInPath,
    ref InputControlList<TControl> matches, bool matchMultiple)
    where TControl : InputControl
{
    if (control == null)
        throw new ArgumentNullException(nameof(control));
    if (path == null)
        throw new ArgumentNullException(nameof(path));

    var pathLength = path.Length;
    if (indexInPath < 0 || indexInPath > pathLength)
        throw new ArgumentOutOfRangeException(nameof(indexInPath));

    // Create substring from current index so we can use PathParser for the next component.
    var subPath = path.Substring(indexInPath);
    if (subPath.Length == 0)
        return null;

    // Find end of first component in subPath (stop at unescaped '/').
    var compEnd = 0;
    while (compEnd < subPath.Length)
    {
        if (subPath[compEnd] == '\\' && compEnd + 1 < subPath.Length)
        {
            // Skip escaped char.
            compEnd += 2;
            continue;
        }
        if (subPath[compEnd] == '/')
            break;
        compEnd++;
    }

    var componentStr = subPath.Substring(0, compEnd);

    // Parse the single component.
    var componentParser = new PathParser(componentStr);
    if (!componentParser.MoveToNextComponent())
        return null; // malformed/empty component

    var component = componentParser.current;

    // Use ParsedPathComponent.Matches to test if current control satisfies component.
    if (!component.Matches(control))
        return null;

    // If there's no more path after this component, we matched.
    var restStart = compEnd;
    // If there's a separating '/', skip it for the remainder.
    if (restStart < subPath.Length && subPath[restStart] == '/')
        restStart++;

    var remainder = restStart >= subPath.Length ? string.Empty : subPath.Substring(restStart);

    // If remainder is empty, we've reached the end -> success.
    if (string.IsNullOrEmpty(remainder))
    {
        if (!(control is TControl match))
            return null;
        if (matchMultiple)
            matches.Add(match);
        return match;
    }

    // Otherwise, dive into children or route by usage depending on next char.
    TControl lastMatch;
    if (remainder[0] == '{')
    {
        // Usage-based routing from the device root. Pass remainder as a new path starting at 0.
        lastMatch = MatchByUsageAtDeviceRootRecursive(control.device, remainder, 0, ref matches, matchMultiple);
    }
    else
    {
        // Recurse into children. Pass remainder as a new path starting at 0.
        lastMatch = MatchChildrenRecursive(control, remainder, 0, ref matches, matchMultiple);
    }

    return lastMatch;
}
Performance

The implementation allocates multiple substrings per recursion (path.Substring(indexInPath), component substring, remainder substring), which can regress allocations on frequently-called matching paths; consider a span/index-based approach or a PathParser that can advance on the original string without creating new strings.

// Create substring from current index so we can use PathParser for the next component.
var subPath = path.Substring(indexInPath);
if (subPath.Length == 0)
    return null;

// Find end of first component in subPath (stop at unescaped '/').
var compEnd = 0;
while (compEnd < subPath.Length)
{
    if (subPath[compEnd] == '\\' && compEnd + 1 < subPath.Length)
    {
        // Skip escaped char.
        compEnd += 2;
        continue;
    }
    if (subPath[compEnd] == '/')
        break;
    compEnd++;
}

var componentStr = subPath.Substring(0, compEnd);

// Parse the single component.
var componentParser = new PathParser(componentStr);
if (!componentParser.MoveToNextComponent())
    return null; // malformed/empty component

var component = componentParser.current;

// Use ParsedPathComponent.Matches to test if current control satisfies component.
if (!component.Matches(control))
    return null;

// If there's no more path after this component, we matched.
var restStart = compEnd;
// If there's a separating '/', skip it for the remainder.
if (restStart < subPath.Length && subPath[restStart] == '/')
    restStart++;

var remainder = restStart >= subPath.Length ? string.Empty : subPath.Substring(restStart);

// If remainder is empty, we've reached the end -> success.
if (string.IsNullOrEmpty(remainder))
{
Index Handling

The logic resets recursion to index 0 by passing remainder into child/usage matching, which can diverge from the original index-based traversal (e.g., escaping rules or component boundary assumptions) and makes correctness dependent on how MatchChildrenRecursive/MatchByUsageAtDeviceRootRecursive interpret the new substring; validate with complex paths containing escapes and multiple components.

// If there's no more path after this component, we matched.
var restStart = compEnd;
// If there's a separating '/', skip it for the remainder.
if (restStart < subPath.Length && subPath[restStart] == '/')
    restStart++;

var remainder = restStart >= subPath.Length ? string.Empty : subPath.Substring(restStart);

// If remainder is empty, we've reached the end -> success.
if (string.IsNullOrEmpty(remainder))
{
    if (!(control is TControl match))
        return null;
    if (matchMultiple)
        matches.Add(match);
    return match;
}

// Otherwise, dive into children or route by usage depending on next char.
TControl lastMatch;
if (remainder[0] == '{')
{
    // Usage-based routing from the device root. Pass remainder as a new path starting at 0.
    lastMatch = MatchByUsageAtDeviceRootRecursive(control.device, remainder, 0, ref matches, matchMultiple);
}
else
{
    // Recurse into children. Pass remainder as a new path starting at 0.
    lastMatch = MatchChildrenRecursive(control, remainder, 0, ref matches, matchMultiple);
}
  • Update review

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent

@u-pr-agent
Copy link
Contributor

u-pr-agent bot commented Jan 8, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Treat end-of-path as match

Returning null when indexInPath points at the end of the string changes behavior
versus the old implementation, which treated "end of path" as a successful match of
the current control. This will break callers that pass indexInPath == path.Length to
mean "already fully matched". Handle this case by returning the current control
(type-checked) and adding it to matches if requested.

Packages/com.unity.inputsystem/InputSystem/Controls/InputControlPath.cs [802-805]

 // Create substring from current index so we can use PathParser for the next component.
 var subPath = path.Substring(indexInPath);
+
+// If we're already at the end of the path, treat it as a successful match.
 if (subPath.Length == 0)
-    return null;
+{
+    if (!(control is TControl endMatch))
+        return null;
+    if (matchMultiple)
+        matches.Add(endMatch);
+    return endMatch;
+}
Suggestion importance[1-10]: 6

__

Why: In MatchControlsRecursive, returning null when indexInPath == path.Length is a behavior change that could break callers relying on the previous “already fully matched” semantics; however, the new code already validates indexInPath and likely expects to always parse at least one component, so impact depends on external call patterns.

Low
  • More suggestions

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent

@codecov-github-com
Copy link

codecov-github-com bot commented Jan 8, 2026

Codecov Report

Attention: Patch coverage is 87.17949% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...putsystem/InputSystem/Controls/InputControlPath.cs 87.17% 5 Missing ⚠️
@@             Coverage Diff             @@
##           develop    #2312      +/-   ##
===========================================
- Coverage    77.95%   77.81%   -0.15%     
===========================================
  Files          476      476              
  Lines        97443    97385      -58     
===========================================
- Hits         75961    75778     -183     
- Misses       21482    21607     +125     
Flag Coverage Δ
inputsystem_MacOS_2022.3 5.54% <0.00%> (+<0.01%) ⬆️
inputsystem_MacOS_2022.3_project 74.25% <87.17%> (-1.22%) ⬇️
inputsystem_MacOS_6000.0 5.32% <0.00%> (+<0.01%) ⬆️
inputsystem_MacOS_6000.0_project 76.13% <87.17%> (-1.23%) ⬇️
inputsystem_MacOS_6000.2 5.32% <0.00%> (-0.01%) ⬇️
inputsystem_MacOS_6000.2_project 76.14% <87.17%> (-1.22%) ⬇️
inputsystem_MacOS_6000.3 5.32% <0.00%> (+<0.01%) ⬆️
inputsystem_MacOS_6000.3_project 76.13% <87.17%> (-1.24%) ⬇️
inputsystem_MacOS_6000.4 5.33% <0.00%> (+<0.01%) ⬆️
inputsystem_MacOS_6000.4_project 76.14% <87.17%> (-1.23%) ⬇️
inputsystem_MacOS_6000.5 5.33% <0.00%> (+<0.01%) ⬆️
inputsystem_MacOS_6000.5_project 76.14% <87.17%> (-1.23%) ⬇️
inputsystem_Ubuntu_2022.3 5.54% <0.00%> (+<0.01%) ⬆️
inputsystem_Ubuntu_2022.3_project 74.08% <87.17%> (-1.19%) ⬇️
inputsystem_Ubuntu_6000.0 5.32% <0.00%> (+<0.01%) ⬆️
inputsystem_Ubuntu_6000.0_project 75.96% <87.17%> (-1.20%) ⬇️
inputsystem_Ubuntu_6000.2 5.32% <0.00%> (+<0.01%) ⬆️
inputsystem_Ubuntu_6000.2_project 75.96% <87.17%> (-1.20%) ⬇️
inputsystem_Ubuntu_6000.3 5.32% <0.00%> (+<0.01%) ⬆️
inputsystem_Ubuntu_6000.3_project 75.96% <87.17%> (-1.20%) ⬇️
inputsystem_Ubuntu_6000.4 5.33% <0.00%> (+<0.01%) ⬆️
inputsystem_Ubuntu_6000.4_project 75.97% <87.17%> (-1.20%) ⬇️
inputsystem_Ubuntu_6000.5 5.33% <0.00%> (+<0.01%) ⬆️
inputsystem_Ubuntu_6000.5_project 75.97% <87.17%> (-1.20%) ⬇️
inputsystem_Windows_2022.3 5.54% <0.00%> (+<0.01%) ⬆️
inputsystem_Windows_2022.3_project 74.38% <87.17%> (-1.22%) ⬇️
inputsystem_Windows_6000.0 5.32% <0.00%> (+<0.01%) ⬆️
inputsystem_Windows_6000.0_project 76.26% <87.17%> (-1.23%) ⬇️
inputsystem_Windows_6000.2 5.32% <0.00%> (+<0.01%) ⬆️
inputsystem_Windows_6000.2_project 76.26% <87.17%> (-1.23%) ⬇️
inputsystem_Windows_6000.3 5.32% <0.00%> (+<0.01%) ⬆️
inputsystem_Windows_6000.3_project 76.26% <87.17%> (-1.23%) ⬇️
inputsystem_Windows_6000.4 5.33% <0.00%> (+<0.01%) ⬆️
inputsystem_Windows_6000.4_project 76.27% <87.17%> (-1.23%) ⬇️
inputsystem_Windows_6000.5 5.33% <0.00%> (+<0.01%) ⬆️
inputsystem_Windows_6000.5_project 76.27% <87.17%> (-1.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...putsystem/InputSystem/Controls/InputControlPath.cs 91.90% <87.17%> (+0.02%) ⬆️

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@josepmariapujol-unity josepmariapujol-unity marked this pull request as draft January 9, 2026 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants