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 62 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? 👍/👎
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
…e with other test criteria, added the use of passing in InputActions to the tests also.
|
/review |
PR Reviewer Guide 🔍(Review updated until commit ac5d101)Here are some key observations to aid the review process:
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
…te the monitor's packed priority.
|
/review |
|
Persistent review updated to latest commit e52d920 |
1 similar comment
|
Persistent review updated to latest commit e52d920 |
…ontrolType` (`Button`) in the built-in asset and embedded C# JSON, so validation, rebinding filters, and UI that rely on expected control layout recognize it as a button action again.
|
Persistent review updated to latest commit ced22e0 |
For issue 1: For issue 2: As a product risk for shipping players, it is low today because production code does not use reInstallRuntime after initial creation.
I looked into the Hold leakage mentioned and it doesn't seem to be much of an issue: From the current implementation: the code knowingly avoids setting handled when there is no valid event—likely to avoid writing handled on invalid/stale memory—so this is an intentional safety tradeoff with observable leakage for timeouts. |
|
Persistent review updated to latest commit ac5d101 |
|
/test_plan |
Test Plan
Summary of Changes & Risk AssessmentSummary of ChangesThis PR introduces a new opt-in Action Priority system for resolving overlapping input shortcuts (e.g., Risk Assessment
Test ScenariosFunctional Testing
Regression Testing
🔍 Regression Deep Dive (additional risks identified)
Edge Cases
💡 This test plan updates automatically when 🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
This comment was marked as resolved.
This comment was marked as resolved.
The naming should be tweaked as the text is too long and does not fit (I assume we don't want to move the checkmark forward and make it out of line with the others) How about dropping the "shortcuts" part in the end, they're already under the shortcut support category. For example: Complexity-based & Action Priority. Also, some subjective tooltip suggestions below: "When several enabled actions share the same control, the action with the higher Priority value is ordered first and consume input so lower-priority actions do not trigger on the same event. Set Priority on each in the Input Actions editor. Serialized priority values are kept when the option is disabled" and "Composite bindings can consume overlapping input using binding-chain depth (complexity): actions with more composite parts are considered before simpler bindings on the same controls. This works well for shortcut keys; when two composites have the same depth, resolution can be non-deterministic. Conflicts can occur between different action maps (for example UI navigation versus gameplay on the same keys) but not between different action assets. Since consumption applies to enabled actions only, disable maps or actions you do not need in the current context to reduce surprises" |
|
I've tested the plan scenarios from here: #2402 (comment) and everything besides this part passes:
Although I don't think I would agree that's a real bug, probably needs clarity. Otherwise seems good to me, does what it is supposed to do and much better than the old one Tests if you want to double check: |
Hi Paulius, I also looked into this Test the "Hold leakage" scenario mentioned in PR comments. Verify that a higher-priority action suppresses a lower-priority action even when the transition is triggered by a timer (e.g., two overlapping Holds). But I didn't see it as a bug either to be honest. The fix would be messy and change a lot so it wouldn't be worth it in my opinion to try to solve this. |
Darren-Kelly-Unity
removed
the
work in progress
5 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.