FIX: ISXB-1319 Input recorder game view focus#2407
FIX: ISXB-1319 Input recorder game view focus#2407MorganHoarau wants to merge 11 commits intodevelopfrom
Conversation
Bypasses several game focused related logic gates to let replay input event reach playmode session.
Bit of logics to track replay controllers and bridge with InputManager.
Uses InputManager.isReplayActive flag
MorganHoarau
changed the title
![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 critical issues, including potential compilation failures in non-Editor builds and logic bugs related to event subscription leaks. Additionally, there are minor suggestions for documentation and architectural improvements.
🤖 Helpful? 👍/👎
WinPlayer build made to ensure compilation
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2407 +/- ##
========================================
Coverage 78.13% 78.13%
========================================
Files 483 483
Lines 98770 98806 +36
========================================
+ Hits 77169 77202 +33
- Misses 21601 21604 +3 Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
/crc |
There was a problem hiding this comment.
The Pull Request quality is high, but there are a few remaining logic and safety issues to address. Specifically, the replay bypass in InputManager is currently too broad and may affect unintended devices, and a potential null reference should be guarded against in InputEventTrace.
🤖 Helpful? 👍/👎
Co-authored-by: u-pr[bot] <205906871+u-pr[bot]@users.noreply.github.com>
|
waiting for CI to be green before adding final reviewers |
|
Thanks for a very good and descriptive PR description. For issue #2 I would recommend filing a backlog ticket referencing the findings from this investigation and possibly this PR as well so this can be looked into in the future. |
ekcoh
left a comment
ekcoh
left a comment
There was a problem hiding this comment.
Generally very nice and clean PR with a great description. I have no reason to not approve but I would urge considering the questions and design aspects raised in comments since I believe this can be further improved by handling the ignoreFocusFiltering as a clear internal feature (that might become relevant for more things) and consider if editor/source splitting of the replay logic is potentially possible to make cleaner by adding (or relying on existing) events tied to the replay mechanism to avoid as much #if UNITY_EDITOR checks. The only thing I am missing here is a test case since this adds a "new internal feature" which doesn't seem to be covered for regressions which indicates it needs to be rested manually which is costly in the long run.
| public ReplayController PlayAllFramesOneByOne() | ||
| { | ||
| finished = false; | ||
| #if UNITY_EDITOR |
There was a problem hiding this comment.
Generally, when this kind of behaviour is needed (with these editor checks all over the place), I wonder if we shouldn't add a proper optional callback for these replay events, e.g. ReplayBegun, there already seems to be an m_OnFinished that could be used to handle the call to editor code ScheduleEndReplayBypass()?
There was a problem hiding this comment.
e.g. this could be e.g. (not sure about naming - conceptually) m_OnReplayStart?();
| #if UNITY_EDITOR | ||
| // Schedule bypass removal for after the next OnUpdate, so any events already | ||
| // queued into the native buffer this frame are still processed with the bypass active. | ||
| ScheduleEndReplayBypass(); |
There was a problem hiding this comment.
Couldn't editor code hook to m_OnFinished in this case handle it that way?
| // focus loss — the replay intentionally re-injects events for those devices. | ||
| if (device != null && !device.enabled && | ||
| #if UNITY_EDITOR | ||
| !isReplayActive && |
There was a problem hiding this comment.
Minor, but I don't specifically see why this is "tightly replay", it looks like an internal feature "ignoreGameViewFocus". That might age/scale better and better indicates what this is all about. It's a specific mode where game-view focus is ignored in event filtering, or what do you think?
3 participants
Description
PR addresses https://jira.unity3d.com/browse/ISXB-1319
Tech Roundtable Slide-deck: https://docs.google.com/presentation/d/18zL10TRcnVfBiSSBe_YT5EYVkJLyt7gFvD3s7EL0JKY/edit?usp=drive_link
Note: the fix adds a tight coupling between
InputManagerandInputEventTrace.ReplayController.Issue 1: replay requires Game View focus
Root cause: three independent focus-gating layers that block replayed events when Game View is unfocused (event deferral, device-disabled discard, UI module skip).
The fix implemented uses an InputManager-level
isReplayActiveflag that bypasses all three (Editor-only).Issues 1 Details
When Game View loses focus ->
applicationHasFocus=false->gameHasFocus=false. Replay events are blocked at three independent layers:sequenceDiagram participant IM as InputManager participant RC as ReplayController participant Dev as Mouse/Keyboard participant UIIM as InputSystemUIInputModule participant ES as EventSystem Note over IM,ES: Game View loses focus IM->>IM: gameHasFocus = false IM->>Dev: DisableDevice(DisabledWhileInBackground) ES->>ES: OnApplicationFocus(false) → m_HasFocus = false Note over IM,ES: Replay starts RC->>IM: QueueEvent(mouseStateEvent) IM->>IM: ❌ A — event deferred (ShouldDefer) IM->>IM: ❌ B — event discarded (device disabled) ES->>UIIM: Process() UIIM->>UIIM: ❌ C — !isFocused → skip ProcessPointer/ProcessNavigationKnown trade-off: The bypass doesn't distinguish replayed events from real hardware input, so real keyboard/mouse input also reaches the game during replay even with Game View unfocused. This is acceptable for typical usage (replay in isolation in Editor).
Issue 2: TextField text not replayed
Architectural limitation. UIToolkit TextField receives text through the native IMGUI event queue (
Event.PopEvent), which is a separate pipeline from InputSystem. Replayed TextEvents fire correctly throughKeyboard.OnTextInputbut have zero subscribers. The IMGUI queue is only populated by native OS input. Keyboard state (key presses) does replay correctly; only character delivery to TextFields is broken. Fixing this would require engine-side changes (InputForUI / IMGUI bridge), outside scope of this ticket.This is a consistent pattern across Unity's UI framework: IMGUI, uGUI, and UIToolkit all use
Event.PopEvent()for text processing.JIRA user story: https://jira.unity3d.com/browse/ISX-2562
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.
input-recorder-half-fix.mp4
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.