Skip to content

FIX: Implement caching for InputControlPath display name#2342

Open
josepmariapujol-unity wants to merge 6 commits intodevelopfrom
input/fixing-cache-path-input-control-path-editor
Open

FIX: Implement caching for InputControlPath display name#2342
josepmariapujol-unity wants to merge 6 commits intodevelopfrom
input/fixing-cache-path-input-control-path-editor

Conversation

@josepmariapujol-unity
Copy link
Collaborator

@josepmariapujol-unity josepmariapujol-unity commented Feb 5, 2026

Description

Purpose of this PR is to fix the TODO by caching per path value and only recompute when the string actually changes.

Testing status & QA

Check that it works as before, when inputing in the InputControlPath.

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: 0
  • Halo Effect: 0

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.

Cache the display name for InputControlPath to reduce GC churn.
@josepmariapujol-unity josepmariapujol-unity self-assigned this Feb 5, 2026
@josepmariapujol-unity josepmariapujol-unity changed the title Implement caching for InputControlPath display name FIX: Implement caching for InputControlPath display name Feb 5, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements caching for InputControlPath display names to reduce unnecessary garbage collection churn. The change addresses a TODO comment by storing the computed display name and only recalculating it when the input path string actually changes.

Changes:

  • Added caching mechanism with string comparison to avoid redundant ToHumanReadableString calls
  • Introduced two cache fields (m_CachedPath and m_CachedDisplayName) to track the last computed values

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@u-pr
Copy link
Contributor

u-pr bot commented Feb 5, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Remove conditional check to preserve formatting behavior

The explicit string.IsNullOrEmpty check forces the display name to be empty, which
might suppress specific formatting for empty paths (e.g., "") that
ToHumanReadableString might provide. Removing the check ensures consistency with the
previous implementation and simplifies the code.

Packages/com.unity.inputsystem/InputSystem/Editor/ControlPicker/InputControlPathEditor.cs [126-139]

 // Cache the display name per path value and only recompute when the string actually changes.
 if (!string.Equals(path, m_CachedPath, StringComparison.Ordinal))
 {
     m_CachedPath = path;
-
-    if (string.IsNullOrEmpty(path))
-    {
-        m_CachedDisplayName = string.Empty;
-    }
-    else
-    {
-        m_CachedDisplayName = InputControlPath.ToHumanReadableString(path);
-    }
+    m_CachedDisplayName = InputControlPath.ToHumanReadableString(path);
 }
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the explicit empty check might alter the original behavior if ToHumanReadableString handles empty strings specifically (e.g., returning ""). Removing the check restores consistency with the pre-PR logic and simplifies the code.

Low
  • More suggestions

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

Fixed caching for InputControlPath display name.
@josepmariapujol-unity josepmariapujol-unity marked this pull request as draft February 5, 2026 12:35
josepmariapujol-unity and others added 2 commits February 5, 2026 13:36
…r/InputControlPathEditor.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

This comment was marked as resolved.

@Unity-Technologies Unity-Technologies deleted a comment from u-pr bot Feb 5, 2026
@josepmariapujol-unity josepmariapujol-unity marked this pull request as ready for review February 6, 2026 08:16
@u-pr
Copy link
Contributor

u-pr bot commented Feb 6, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

The PR introduces a simple caching mechanism in a single file with straightforward logic, making it easy to review.
🏅 Score: 95

The code correctly resolves the TODO regarding GC churn with a clean implementation.
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Performance Verification

Verify that the InputControlPathEditor instance is not reused to draw multiple different properties within the same frame (e.g., inside a List PropertyDrawer). If the instance is shared and drawn with different paths in a loop, the cache will be invalidated on every call (thrashing), negating the performance benefits.

// Cache the display name per path value and only recompute when the string actually changes.
if (!string.Equals(path, m_CachedPath, StringComparison.Ordinal))
{
    m_CachedPath = path;

    if (string.IsNullOrEmpty(path))
    {
        m_CachedDisplayName = string.Empty;
    }
    else
    {
        m_CachedDisplayName = InputControlPath.ToHumanReadableString(path);
    }
}

var displayName = m_CachedDisplayName;
  • Update review

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

@u-pr
Copy link
Contributor

u-pr bot commented Feb 6, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Remove conditional check to preserve formatting behavior

The explicit string.IsNullOrEmpty check forces the display name to be empty, which
might suppress specific formatting for empty paths (e.g., "") that
ToHumanReadableString might provide. Removing the check ensures consistency with the
previous implementation and simplifies the code.

Packages/com.unity.inputsystem/InputSystem/Editor/ControlPicker/InputControlPathEditor.cs [126-139]

 // Cache the display name per path value and only recompute when the string actually changes.
 if (!string.Equals(path, m_CachedPath, StringComparison.Ordinal))
 {
     m_CachedPath = path;
-
-    if (string.IsNullOrEmpty(path))
-    {
-        m_CachedDisplayName = string.Empty;
-    }
-    else
-    {
-        m_CachedDisplayName = InputControlPath.ToHumanReadableString(path);
-    }
+    m_CachedDisplayName = InputControlPath.ToHumanReadableString(path);
 }
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the explicit empty check might alter the original behavior if ToHumanReadableString handles empty strings specifically (e.g., returning ""). Removing the check restores consistency with the pre-PR logic and simplifies the code.

Low
  • More suggestions

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

@codecov-github-com
Copy link

codecov-github-com bot commented Feb 6, 2026

Codecov Report

All modified and coverable lines are covered by tests ✅

@@           Coverage Diff            @@
##           develop    #2342   +/-   ##
========================================
  Coverage    77.95%   77.96%           
========================================
  Files          476      476           
  Lines        97453    97434   -19     
========================================
- Hits         75971    75964    -7     
+ Misses       21482    21470   -12     
Files with missing lines Coverage Δ
...tem/Editor/ControlPicker/InputControlPathEditor.cs 0.00% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

1 participant