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}
| anyPartMatches = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Although this logic intentionally reuses the scan idiom from the integer-index renderer below, it computes lastPartIndex to find the end of the composite parts and then iterates through the parts a second time to find a match.
Since lastPartIndex is not used further in this block (e.g., it is not used to advance the outer loop's i), you can simplify this into a single loop. This avoids evaluating the composite parts twice and makes the code slightly more concise.
Have you considered merging the bounds check into the for loop condition?
| } | |
| var anyPartMatches = false; | |
| for (var partIndex = i + 1; partIndex < bindings.Count && bindings[partIndex].isPartOfComposite; ++partIndex) | |
| { | |
| if (bindingMask.Matches(bindings[partIndex])) | |
| { | |
| anyPartMatches = true; | |
| break; | |
| } | |
| } |
🤖 Helpful? 👍/👎
| continue; | ||
| { | ||
| // Composites are filtered atomically: any matching part promotes the whole | ||
| // composite, consistent with how the integer-index renderer at lines 440-492 |
There was a problem hiding this comment.
Have you considered referencing the method name or logical section instead of specific line numbers (lines 440-492)? Hardcoded line numbers in comments tend to become outdated quickly as the file is modified and code is added or removed above this section.
🤖 Helpful? 👍/👎
|
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? 👍/👎 |
| continue; | ||
| var lastPartIndex = i + 1; | ||
| while (lastPartIndex < bindings.Count && bindings[lastPartIndex].isPartOfComposite) | ||
| ++lastPartIndex; |
There was a problem hiding this comment.
This scan idiom is consistent with the implementation at line 444 (in the index-based GetBindingDisplayString overload).
One minor detail: since the outer loop already skips isPartOfComposite (line 321), this inner loop correctly identifies the boundaries of the current composite without interfering with the outer loop's progression.
📚 Additional Context
The following files were referenced for additional context:
Packages/com.unity.inputsystem/InputSystem/Runtime/Actions/InputActionRebindingExtensions.cs
🤖 Helpful? 👍/👎
| .With("Positive", "<Keyboard>/d", groups: "Keyboard"); | ||
|
|
||
| Assert.That(action.GetBindingDisplayString(InputBinding.MaskByGroup("Keyboard")), | ||
| Is.EqualTo("A/D")); |
There was a problem hiding this comment.
This test confirms the fix for the common case where all parts share the same group. To fully verify the "atomic promotion" logic mentioned in the PR description, consider adding a test case for a composite with mixed groups, for example:
[Test]
public void Actions_WhenGettingDisplayTextForMixedGroupComposite_IsIncludedWhenAnyPartMatches()
{
var action = new InputAction();
action.AddCompositeBinding("2DVector")
.With("Up", "<Keyboard>/w", groups: "Keyboard")
.With("Down", "<Gamepad>/dpad/down", groups: "Gamepad");
// Should return "W/D-Pad Down" even though only the 'Up' part matches the 'Keyboard' mask
Assert.That(action.GetBindingDisplayString(InputBinding.MaskByGroup("Keyboard")),
Is.EqualTo("W/D-Pad Down"));
}🤖 Helpful? 👍/👎
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2414 +/- ##
===========================================
+ Coverage 78.13% 78.15% +0.02%
===========================================
Files 483 761 +278
Lines 98770 138878 +40108
===========================================
+ Hits 77169 108542 +31373
- Misses 21601 30336 +8735 Flags with carried forward coverage won't be shown. Click here to find out more.
|
1 participant
Description
InputActionRebindingExtensions.GetBindingDisplayString(InputAction, InputBinding, InputBinding.DisplayStringOptions)was returning an empty string for composite bindings (e.g.1DAxis,2DVector,ButtonWithOneModifier) whenever the supplied binding mask filtered by control-scheme group, because the per-binding loop skippedisPartOfCompositeentries and then tested the mask directly against the composite header — which carries no group metadata of its own — causing the whole composite to drop out of the result. The fix performs a fall-through scan over the composite's part siblings and promotes the composite atomically when at least one of its parts matches the mask, reusing the exact scan idiom already used by the integer-index renderer further down the same file. The user-visible effect is that rebinding UIs and similar callers now correctly render composite display strings under group-based masks. Hypothesis was approved by input-fix-judge on pass 2 — pass 1 rejected the original Step 5b Potential fix, input-fix-deep-dive then re-investigated and proposed this alternative, and pass 2 of the judge approved this hypothesis before the user invoked this skill directly.Testing status & QA
Actions_WhenGettingDisplayTextForBindingsOnAction_CompositeIsIncludedWhenAtLeastOnePartMatchesBindingMaskinAssets/Tests/InputSystem/CoreTests_Actions.cs([Test],CoreTestsFixture).Overall Product Risks
Comments to reviewers
Auto-approved by input-fix-judge (pass-2): 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. This hypothesis came from input-fix-deep-dive's independent root-cause analysis after the original triage Step 5b Potential fix was rejected on pass 1; pass 2 of the judge approved this alternative. The chain only routes hypotheses that the judge could not find any concrete reason to reject (read-only checklist of file/method existence, mechanism location, public API impact, existing test coverage, side-effects, and ignored edge cases). Reviewers should still treat this as a normal review — the chain skips anything the judge rejects but does not certify correctness.
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.