[Draft ]Proof of concept, input consumption#2402
[Draft ]Proof of concept, input consumption#2402Darren-Kelly-Unity wants to merge 17 commits intodevelopfrom
Conversation
…ort and clamping logic. Add tooltip for priority field and update related classes to handle priority in bindings.
This reverts commit 28640eb
…r the shortcut again.
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
The review identified several issues ranging from potential runtime exceptions and logic regressions to documentation mismatches. Most notably, there's a priority clamping issue that causes value wrap-around and a potential logic regression in how composite bindings are handled compared to previous versions.
🤖 Helpful? 👍/👎
| if (shiftB.IsPressed()) | ||
| { | ||
| Debug.Log("SHIFT + B WAS PRESSED"); | ||
| } | ||
|
|
||
| if (b.IsPressed()) | ||
| { | ||
| Debug.Log("B WAS PRESSED"); | ||
| } |
There was a problem hiding this comment.
These calls to IsPressed() lack null checks. Since shiftB and b are resolved via FindAction in Start and are guarded by null checks elsewhere (lines 49 and 55), they could potentially be null. Accessing them here without a check will cause a NullReferenceException every frame if the actions are missing from the asset.
Additionally, polling IsPressed() and logging here spams the console every frame while the keys are held. This is likely redundant since the same messages are already logged once per press via the event callbacks (OnB, OnShiftB).
🤖 Helpful? 👍/👎
| } | ||
| } | ||
| controlGroupingAndComplexity[i * 2 + 1] = (ushort)complexity; | ||
| var priority = Math.Clamp(action != null && !disableControlGrouping ? action.Priority : 0, 0, 65536); |
There was a problem hiding this comment.
The upper bound of the clamp here is 65536, but the resulting value is cast to a ushort on the next line. Since the maximum value of a ushort is 65535, a priority of 65536 will wrap around to 0 after the cast. This means a high priority intended by the user would be treated as the lowest possible priority. The clamp should be to 65535 to match the documentation and the ushort range.
| var priority = Math.Clamp(action != null && !disableControlGrouping ? action.Priority : 0, 0, 65536); | |
| var priority = Math.Clamp(action != null && !disableControlGrouping ? action.Priority : 0, 0, 65535); |
🤖 Helpful? 👍/👎
| // can then reset state monitors in the same group. | ||
| // NOTE: We don't consume for controls at binding complexity 1. Those we fire in unison. | ||
| if (controlGroupingAndComplexity[trigger.controlIndex * 2 + 1] > 1 && | ||
| if (controlGroupingAndPriority[trigger.controlIndex * 2 + 1] > 0 && |
There was a problem hiding this comment.
There is a contradiction between this code and the documentation. The documentation in InputAction.cs (line 207) and the tooltip in InputActionsEditorConstants.cs (line 40) state that "Values 0–1 follow one path; values greater than 1 follow another". However, this check uses > 0, meaning priority 1 will be treated as high priority and mark the event as handled.
Furthermore, this change appears to be a regression for composite bindings. Previously, composites had a "complexity" > 1 and would mark events as handled by default (suppressing simple bindings on the same key). Now, they default to priority 0 and will no longer suppress other actions unless the user manually sets a higher priority. If the intention was to maintain existing behavior while introducing explicit priorities, composites should likely default to a priority > 0 (or the check should remain > 1 with a higher default).
🤖 Helpful? 👍/👎
| @@ -2478,7 +2469,7 @@ private void ChangePhaseOfActionInternal(int actionIndex, TriggerState* actionSt | |||
| // When we perform an action, we mark the event handled such that FireStateChangeNotifications() | |||
| // can then reset state monitors in the same group. | |||
| // NOTE: We don't consume for controls at binding complexity 1. Those we fire in unison. | |||
There was a problem hiding this comment.
This comment still references 'binding complexity 1'. Since the implementation has shifted from complexity-based grouping to priority-based grouping, this comment should be updated to reflect the new criteria (e.g., 'priority 0') to stay consistent with the code.
🤖 Helpful? 👍/👎
|
|
||
| // Two shorts per control. First one is group number. Second one is complexity count. | ||
| // Two shorts per control. First one is group number. Second one is priority. | ||
| public ushort* controlGroupingAndComplexity; |
There was a problem hiding this comment.
For better maintainability, consider renaming the internal field controlGroupingAndComplexity in the UnmanagedMemory struct (and its related variable names like on line 1382) to 'priority'. The current naming is misleading now that it primarily stores priority values instead of complexity counts.
🤖 Helpful? 👍/👎
There was a problem hiding this comment.
Thanks for putting up this draft PR. I looked through the tests only so far and I think we should have a discussion around it which would be more helpful than doing asynchronous feedback on the comments. Test cases generally make sense but I fail to decipher some differences and rules in them I fail to understand. Also I am missing tests using priorities when interaction is not a shortcut modifier, I would assume the same rules apply regardless of binding? I think there is opportunity to reduce code diff here by doing some code reuse in the tests without having tests full of if statements.
I also think it would be good to have tests with repeated input which I mentioned before. I didn't spot any such tests unless I missed something. This would look like e.g. with a binding 'X', here value of X button provided as binary
X: 0 1 1
Action 1 (Prio 1): 0 - 0
Action 2 (Prio 2): 0 1 0
a b c
a: button up, nothing happens
b: button down, triggers both, but only action 2 fires notification since higher prio
c: button down again (repeat), triggers none since action 1 saw and reacted to state transition but did not fire
| } | ||
|
|
||
| [Test] | ||
| [Category("Actions Priority")] |
There was a problem hiding this comment.
If "Actions Priority" is its own category (it makes sense so I like it), could we also move out those tests into e.g. "CoreTests_Actions_Priority.cs" (since partial), to simplify finding these and working with these. This file is way too large.
| [Category("Actions Priority")] | ||
| [TestCase("ctrl", "x", true)] | ||
| [TestCase("ctrl", "x", false)] | ||
| public void Actions_Priority_PressingModifierShortcutWithSameBinding_TriggersHighestPriorityAction(string sharedModifier, string binding1, bool legacyComposites) |
There was a problem hiding this comment.
We some inline doc be added here to explain the arguments? It's not clear to me what binding1 or legacyComposites mean unless I read the whole test. And reading it I am still confused what legacyComposites mean/imply?
|
|
||
| map.Enable(); | ||
|
|
||
| Assert.That(action1.WasPerformedThisFrame(), Is.False); |
There was a problem hiding this comment.
Remove these asserts, what do they test? Seems like a sanity check or another test? "Actions should not trigger if there is no input"
| .With("Modifier", "<Keyboard>/" + sharedModifier) | ||
| .With(legacyComposites ? "Button" : "Binding", "<Keyboard>/" + binding1); | ||
|
|
||
| action1.Priority = 9; |
There was a problem hiding this comment.
Is this also valid for
action1.Priority = -2
action2.Priority = -1
?
|
|
||
| [Test] | ||
| [Category("Actions Priority")] | ||
| [TestCase("ctrl", "x", true)] |
There was a problem hiding this comment.
I find this choice of parameterisation confusing, but maybe it's just me. However it looks like the test case structure here is generally:
public void Actions_PriorityFirstActionShouldFire_IfFirstActionHasHigherPriority(Action first, Action second)
and just keep the last part of the test. Actions could be constructed as a one-liner instead and then the same test could test multiple pairs that should fulfil the condition. Worthwhile?
| [Category("Actions Priority")] | ||
| [TestCase("ctrl", "shift", "x", false)] | ||
| [TestCase("ctrl", "shift", "x", true)] | ||
| public void Actions_Priority_BothPrioritiesZero_ConflictingShortcuts_BothPerform(string sharedModifier, string sharedModifier2, string binding1, bool legacyComposites) |
There was a problem hiding this comment.
This is also true if both priorities are e.g. 3 right?
| [Category("Actions Priority")] | ||
| [TestCase("ctrl", "shift", "x", false)] | ||
| [TestCase("ctrl", "shift", "x", true)] | ||
| public void Actions_Priority_ShortcutConsumeDisabled_BothPerformDespiteDifferentPriorities(string sharedModifier, string sharedModifier2, string binding1, bool legacyComposites) |
There was a problem hiding this comment.
I was under the impression that shortcutKeysConsumeInput would be irrelevant after adding priority but it seems that might have changed along the way?
There was a problem hiding this comment.
We added it back for now while debugging. We need to have a task in the epic around how we migrate from the current input consumption to the new one.
|
|
||
| [UnityTest] | ||
| [Category("Actions Priority")] | ||
| public IEnumerator Actions_Priority_TwoNonConflictingShortcuts_ReversedPriorityOrder_BothStillPerform() |
There was a problem hiding this comment.
This could probably be combined with the other similar test and just pass parameterised actions in different order? Test logic can be fixed and just alter argument order to test?
|
|
||
| [UnityTest] | ||
| [Category("Actions Priority")] | ||
| public IEnumerator Actions_Priority_TwoNonConflictingShortcuts_ReverseActionDeclarationOrder_BothPerform() |
There was a problem hiding this comment.
Again, just a small mutation but code bloat
|
|
||
| [UnityTest] | ||
| [Category("Actions Priority")] | ||
| public IEnumerator Actions_Priority_TwoNonConflictingShortcuts_EqualHighPriority_BothPerform() |
There was a problem hiding this comment.
Just parameter difference so why another test?
3 participants
Description
Please fill this section with a description what the pull request is trying to address and what changes were made.
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.
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.