FIX: GetBindingDisplayString returns empty for composites filtered by group-based binding mask [UUM-141423]#2414
Conversation
… group-based binding mask [UUM-141423] Auto-generated by input-fix-pr-prep after input-fix-judge approved the hypothesis from input-fix-deep-dive (after triage Step 5b was rejected on pass 1). InputActionRebindingExtensions.GetBindingDisplayString(InputAction, InputBinding, InputBinding.DisplayStringOptions) returns empty when the binding mask filters by group, because the per-binding loop skips isPartOfComposite parts but then tests the mask against composite headers which carry no group metadata, so the whole composite drops out. Jira: https://issuetracker.unity3d.com/product/unity/issues/guid/UUM-141423 Judge rationale: file and method exist, the mechanism is in the method body (lines 321-323), the fall-through reuses the existing scan idiom at lines 444-448 of the same file with no signature/exception/serialization change, no nearby test asserts the buggy current behaviour, no native/threading/unsafe touches in the method or its direct callees at one level, and the previously-cited mixed-group composite edge case is now explicitly addressed via documented atomic-composite semantics grounded in the line-440-492 renderer's existing convention. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@u-pr can you review this PR |
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
|
The fix correctly addresses the issue where composite bindings were being filtered out by group-based masks because the composite header itself lacks group metadata. By scanning the composite's parts and "promoting" the composite if any part matches the mask, you've ensured that the The implementation follows existing patterns in 🤖 Helpful? 👍/👎 |
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2414 +/- ##
===========================================
+ Coverage 78.13% 78.17% +0.04%
===========================================
Files 483 761 +278
Lines 98770 139073 +40303
===========================================
+ Hits 77169 108721 +31552
- Misses 21601 30352 +8751
Flags with carried forward coverage won't be shown. Click here to find out more.
|
- Collapse the composite-part scan into a single loop. The original two-pass
scan computed lastPartIndex but did not use it for outer-loop advancement,
so merging the bounds check into the for-loop condition eliminates the
redundant first pass without changing behaviour.
- Reword the inline comment to reference the index-based GetBindingDisplayString
overload by name instead of by line number, which goes stale as the file
is edited.
- Add Actions_WhenGettingDisplayTextForBindingsOnAction_MixedGroupCompositeIsRenderedAtomicallyWhenAnyPartMatchesBindingMask
covering a mixed-group 1DAxis composite (Keyboard + Mouse parts) under both
MaskByGroup("Keyboard") and MaskByGroup("Mouse"). Both masks must render the
whole composite ("A/Left Button"), exercising the atomic-promotion claim.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@u-pr has the feedback been addressed fully? Any other issues? |
|
Yes, you've addressed the previous feedback effectively. The loop simplification in Regarding the Codecov report, there are still two lines in the new logic missing coverage. These likely correspond to the cases where a binding is skipped (the 🤖 Helpful? 👍/👎 |
|
/test_plan |
Test Plan
Summary of Changes & Risk AssessmentSummary of ChangesThis PR fixes a bug where Risk Assessment
Test ScenariosFunctional Testing
Regression Testing
Edge Cases
💡 This test plan updates automatically when 🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
|
@copilot resolve the merge conflicts in this pull request |
…bindingdisplaystring-composite-mask # Conflicts: # Packages/com.unity.inputsystem/CHANGELOG.md Co-authored-by: Pauliusd01 <54306142+Pauliusd01@users.noreply.github.com>
|
|
There was a problem hiding this comment.
No new issues were identified in this review pass. The PR correctly addresses the composite binding display issue (UUM-141423) and incorporates previous feedback regarding logic simplification and test coverage for atomic promotion.
🤖 Helpful? 👍/👎
Update unit tests to match the new binding display format. Change assertions from "A/Left Button" to "A/LMB" for both Keyboard and Mouse group masks so the tests reflect the updated display string formatting.
Pauliusd01
deleted the
fix/uum-141423-getbindingdisplaystring-composite-mask
branch
Description
Note
This pull request was generated automatically. Please review carefully before merging.
InputActionRebindingExtensions.GetBindingDisplayString(InputAction, InputBinding, InputBinding.DisplayStringOptions)returned an empty string for composite bindings (1DAxis,2DVector,ButtonWithOneModifier, etc.) under group-based binding masks because the per-binding loop tested the mask only against the composite header — which carries no group metadata of its own — and dropped the whole composite out of the result.The fix scans the composite's parts when the header fails the mask check and promotes the composite atomically when at least one part matches, mirroring how the index-based
GetBindingDisplayStringoverload below already treats composites as a single display unit. Per-part filtering would require a separate API and is intentionally out of scope.Testing status & QA
Actions_WhenGettingDisplayTextForBindingsOnAction_CompositeIsIncludedWhenAtLeastOnePartMatchesBindingMaskandActions_WhenGettingDisplayTextForBindingsOnAction_MixedGroupCompositeIsRenderedAtomicallyWhenAnyPartMatchesBindingMaskinAssets/Tests/InputSystem/CoreTests_Actions.cs.Overall Product Risks
Comments to reviewers
N/A
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.