NEW: Input consumption - Priorities instead of complexity [ISX-2510]#2402
NEW: Input consumption - Priorities instead of complexity [ISX-2510]#2402Darren-Kelly-Unity wants to merge 46 commits into
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? 👍/👎
| // 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? 👍/👎
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
?
| [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?
…e with other test criteria, added the use of passing in InputActions to the tests also.
…e InputManager. Still need another review to look into improvement.
Darren-Kelly-Unity
changed the title
Darren-Kelly-Unity
changed the title
|
@Darren-Kelly-Unity @K-Tone Is this ready for review? It looks to have multiple u-pr comments on it and failing tests indicating it might not be but at the same time it is not a Draft nor having the "work-in-progress" label? I would suggest to either convert back to Draft or add that label for clarity and follow up on outstanding comments to make it clear what the current state is. Feel free to re-request review when ready. Current things to address:
|
K-Tone
added
the
work in progress
# Conflicts: # Assets/Tests/InputSystem/CoreTests_Actions.cs # Packages/com.unity.inputsystem/InputSystem/Runtime/InputManager.cs
4 participants
Purpose of this PR
This work advances epic ISX-2510 [6.6] Reliable Interactions: shortcut overlap resolution no longer depends only on automatic composite complexity, which was hard to reason about and produced corner cases. The branch adds an opt-in action priority path (
InputSettings.shortcutKeysUseActionPriorityplusInputAction.Priority), wires it throughInputActionStategrouping andInputManagerStateMonitorsordering and consumption, and keeps complexity-based behavior when that setting is off. Design context for the epic is linked from the Jira description.Release Notes
InputSettings.shortcutKeysUseActionPriorityand per-actionInputAction.Priority. When enabled, higher-priority actions are ordered and can consume overlapping input before lower-priority actions in the same group; the Input Actions editor shows Priority only in this mode. (ISX-2510)InputSettings.shortcutKeysConsumeInputis documented as complexity-based shortcut resolution when action priority is disabled (prior develop behavior). Modifier composites use ordered evaluation when that complexity path is active.Functional Testing status
Automated coverage added and extended in
CoreTests_ActionsPriority.csand updates inCoreTests_Actions.cs(hold paths, repeated binding presses, priority vs complexity,DefaultInputActions, and related scenarios). Manual passes recommended for Project Settings toggles (shortcutKeysUseActionPriority/shortcutKeysConsumeInput), Input Actions editor visibility of Priority, and play-mode shortcuts that previously relied on complexity-only ordering.Performance Testing Status
Potential extra work when monitor lists are re-sorted (
needToUpdateOrderingOfMonitors) and when action priority changes trigger regrouping updates inInputActionState. No dedicated performance benchmarks were run for this draft; recommend a quick profile on a scene with many simultaneous action monitors if risk tolerance is low.Overall Product Risks
Technical Risk: 2 — Touches core action state, control grouping memory, and state-change monitor ordering and
handledpropagation; behavior is gated behind new settings but logic is subtle.Halo Effect: 2 — Affects any project using shortcut consumption or overlapping bindings across maps; editor UX and serialized assets gain priority fields that persist even when the setting is off.
Class diagram
Git range analyzed:
origin/develop...HEAD(merge-base toproof-of-concept/input-consumptiontip). Diagram emphasizes priority-based shortcut resolution: settings, action priority, packed monitor indices, action state grouping, and monitor dispatch.Expand class diagram (Mermaid)
classDiagram direction TB class InputSettings { +shortcutKeysConsumeInput bool +shortcutKeysUseActionPriority bool IsShortcutResolutionUsingActionPriority bool IsShortcutResolutionUsingComplexity bool } class InputAction { +Priority int } class InputActionMap { m_State InputActionState } class InputActionState { ControlGroupingTable OnActionPriorityChanged() GetComplexityFromMonitorIndex() int } class InputActionState_ControlGroupingTable { Stride int GroupElementIndex() int PriorityElementIndex() int } class InputActionStateMonitorIndex { <<struct>> +Packed long Create(map ctrl bind prio) InputActionStateMonitorIndex +Priority int } class IInputStateChangeMonitor { <<interface>> NotifyControlStateChanged() } class InputManagerStateMonitors { SortMonitorsForDeviceIfNeeded(device) FireStateChangeNotifications(device time event) } class StateChangeMonitorsForDevice { SortMonitorsByIndex() } class InputManager { m_StateMonitors InputManagerStateMonitors } InputAction --> InputActionMap : lives in InputActionMap --> InputActionState : owns InputAction ..> InputActionState : Priority notifies InputActionState ..|> IInputStateChangeMonitor InputActionState --> InputSettings : reads resolution mode InputActionState +-- InputActionState_ControlGroupingTable : uses table layout InputManager --> InputManagerStateMonitors : owns InputManagerStateMonitors --> StateChangeMonitorsForDevice : per device StateChangeMonitorsForDevice ..> InputActionStateMonitorIndex : unpack monitorIndex InputManagerStateMonitors ..> InputActionState : invokes monitorsDocumentation Impact
Changes analyzed:
origin/develop...HEADUser-facing surface: public
InputAction.Priority, publicInputSettings.shortcutKeysUseActionPriority, and clarified semantics forshortcutKeysConsumeInput.Already updated on this branch:
Documentation~/Settings.md(settings table and shortcut behavior),Documentation~/ActionBindings.md(shortcut section and cross-links), andCHANGELOG.mdunder Added / Changed.Residual: After review, confirm cross-references and wording match final API names in generated docs; add release-note polish if UX strings change.
Want draft doc edits beyond what is already in this branch? Reply if you want a pass on TOC, xref targets, or extra examples.