You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Here are some key observations to aid the review process:
⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
The changes are localized to a single method in one file, implementing a straightforward caching pattern to optimize string generation in the editor UI.
🏅 Score: 100
The PR effectively addresses the performance issue (GC churn) by caching the display name based on the path string. The implementation correctly handles nulls, uses efficient string comparison, and follows Unity editor coding standards.
🧪 No relevant tests
🔒 No security concerns identified
⚡ No major issues detected
Update review
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr
Latest suggestions up to 8086d03
Explore these optional code suggestions:
Category
Suggestion
Impact
General
Skip cache update when in manual edit mode
The display name calculation is unnecessary when m_PickerState.manualPathEditMode is active, as the UI displays the raw path string in that mode. Add a check to skip the cache update when manual edit mode is enabled to avoid needless GC churn and calculations during editing.
// Cache the display name per path value and only recompute when the string actually changes.
- if (!string.Equals(path, m_CachedPath, StringComparison.Ordinal))+ if (!m_PickerState.manualPathEditMode && !string.Equals(path, m_CachedPath, StringComparison.Ordinal))
{
m_CachedPath = path;
m_CachedDisplayName = InputControlPath.ToHumanReadableString(path);
}
Suggestion importance[1-10]: 6
__
Why: The suggestion properly optimizes the code by skipping an unnecessary string calculation when manualPathEditMode is active, as the display name is not used in that mode. This aligns with the PR's goal of reducing GC allocations, though the actual performance gain is likely minor since DelayedTextField limits the frequency of path updates.
Low
More suggestions
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr
Restore missing displayName variable to fix compilation
The local variable displayName was removed, but it is implied that subsequent code in the method (not visible in the diff) depends on it. This will likely cause a compilation error. You should declare displayName and assign it the value from m_CachedDisplayName to maintain compatibility with the rest of the method.
// 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;
m_CachedDisplayName = InputControlPath.ToHumanReadableString(path);
}
+var displayName = m_CachedDisplayName;
Suggestion importance[1-10]: 10
__
Why: The PR removes the local variable displayName which was previously defined and used. Since the subsequent code in the method (which is unchanged and not fully visible in the diff) likely relies on displayName, removing its definition will cause a compilation error.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Comments to reviewers
Please describe any additional information such as what to focus on, or historical info for the reviewers.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.