NEW: Added Focus Events to Input Event Queue#2365
NEW: Added Focus Events to Input Event Queue#2365VeraMommersteeg wants to merge 18 commits intodevelopfrom
Conversation
…background device and all input goes to gameview are set
|
|
VeraMommersteeg
changed the title
VeraMommersteeg
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.
This review identified several issues ranging from critical bugs in event handling to minor code cleanup items. The most significant concerns involve potential stale state during event processing and improper event discarding logic during focus changes in InputManager.cs, which could lead to stuck input states. Additionally, there are some minor performance improvements, concerns regarding profiler marker reliability, and several typos/commented-out code blocks in the test suite.
🤖 Helpful? 👍/👎
| var currentEventType = currentEventReadPtr->type; | ||
|
|
||
| Debug.Assert(!currentEventReadPtr->handled, "Event in buffer is already marked as handled"); | ||
|
|
||
| // In before render updates, we only take state events and only those for devices | ||
| // that have before render updates enabled. | ||
| if (updateType == InputUpdateType.BeforeRender) | ||
| { | ||
| while (m_InputEventStream.remainingEventCount > 0) | ||
| { | ||
| Debug.Assert(!currentEventReadPtr->handled, | ||
| "Iterated to event in buffer that is already marked as handled"); | ||
|
|
||
| device = TryGetDeviceById(currentEventReadPtr->deviceId); | ||
| if (device != null && device.updateBeforeRender && | ||
| (currentEventReadPtr->type == StateEvent.Type || | ||
| currentEventReadPtr->type == DeltaStateEvent.Type)) | ||
| break; | ||
|
|
||
| currentEventReadPtr = m_InputEventStream.Advance(leaveEventInBuffer: true); | ||
| } | ||
| } | ||
| ProcessBeforeRenderStateEvents(out device, out currentEventReadPtr); | ||
|
|
||
| if (m_InputEventStream.remainingEventCount == 0) | ||
| break; | ||
|
|
||
| var currentEventTimeInternal = currentEventReadPtr->internalTime; |
There was a problem hiding this comment.
If ProcessBeforeRenderStateEvents skips over events (advancing currentEventReadPtr), currentEventType becomes stale because it still holds the type of the first evaluated event (e.g., FocusEvent or DeviceRemoveEvent) rather than the newly found StateEvent.
This stale currentEventType will be incorrectly passed into subsequent checks like SkipEventDueToEditorBehaviour and ShouldDeferEventBetweenEditorAndPlayerUpdates, potentially causing the StateEvent to be erroneously dropped or deferred based on the rules meant for the skipped event.
You should update currentEventType after calling ProcessBeforeRenderStateEvents, or declare it after the BeforeRender block:
if (updateType == InputUpdateType.BeforeRender)
ProcessBeforeRenderStateEvents(out device, out currentEventReadPtr);
if (m_InputEventStream.remainingEventCount == 0)
break;
var currentEventTimeInternal = currentEventReadPtr->internalTime;
var currentEventType = currentEventReadPtr->type; // Grab type here!🤖 Helpful? 👍/👎
| var focusEventType = new FourCC(FocusConstants.kEventType); | ||
| var possibleFocusEvent = m_InputEventStream.Peek(); | ||
|
|
||
| case IMECompositionEvent.Type: | ||
| { | ||
| var imeEventPtr = (IMECompositionEvent*)currentEventReadPtr; | ||
| var textInputReceiver = device as ITextInputReceiver; | ||
| textInputReceiver?.OnIMECompositionChanged(imeEventPtr->compositionString); | ||
| break; | ||
| } | ||
| if (possibleFocusEvent != null) | ||
| { | ||
| if (possibleFocusEvent->type == focusEventType && !gameShouldGetInputRegardlessOfFocus) | ||
| { | ||
| // If the next event is a focus event and we're not supposed to get input of the current update type in the next one, drop current event. | ||
| // This ensures that we don't end up with a half processed events due to swapping buffers between editor and player, | ||
| // such as InputActionPhase.Started not being finished by a InputActionPhase.Performed and ending up in a pressed state in the previous update type | ||
| m_InputEventStream.Advance(false); | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
This unconditionally drops the current event if the next event in the buffer is a FocusEvent and !gameShouldGetInputRegardlessOfFocus.
This causes the exact "half processed" states the comment mentions it wants to prevent, because it only drops the single event immediately preceding the focus change. For example, if a queue contains [StateEvent(Button Down), StateEvent(Button Up), FocusEvent(Lost)], only the Button Up event will be dropped, leaving the button permanently stuck down!
Furthermore, this will recklessly discard critical system events if they happen to precede a focus change (e.g. dropping a DeviceRemoveEvent or even another FocusEvent if multiple focus changes occur in one frame).
Consider handling cleanup explicitly via device reset/sync upon focus changes instead of arbitrarily dropping the event directly before the focus event.
🤖 Helpful? 👍/👎
| return m_CurrentUpdate; | ||
|
|
||
| #if UNITY_EDITOR | ||
| #if UNITY_EDITOR | ||
| // We can no longer rely on checking the curent focus state, due to this check being used pre-update |
There was a problem hiding this comment.
Typo in comment: "curent" should be "current".
🤖 Helpful? 👍/👎
| focusState = Application.isFocused ? focusState |= FocusFlags.ApplicationFocus | ||
| : focusState &= ~FocusFlags.ApplicationFocus; |
There was a problem hiding this comment.
This ternary assignment is redundant and potentially inefficient. Since focusState is a property whose setter performs logic (updating the runtime state), using |= or &= inside the ternary branches causes the setter to be invoked twice—once for the compound assignment and once for the ternary's final assignment.
Consider simplifying to:
focusState = Application.isFocused
? focusState | FocusFlags.ApplicationFocus
: focusState & ~FocusFlags.ApplicationFocus;🤖 Helpful? 👍/👎
| k_InputUpdateProfilerMarker.Begin(); | ||
|
|
||
| if (m_InputEventStream.isOpen) | ||
| using (k_InputUpdateProfilerMarker.Auto()) |
There was a problem hiding this comment.
The previous implementation included a cautionary comment specifically warning against using try/finally with this profiler marker due to unreliability in the EndSample execution. The new code uses marker.Auto(), which uses try/finally under the hood. Have you verified that this unreliability is no longer a concern for the Unity versions targeted by this package?
🤖 Helpful? 👍/👎
| InputSystem.Update(); | ||
| ScheduleFocusEvent(false); | ||
| InputSystem.Update(InputUpdateType.Dynamic); | ||
| //InputSystem.Update(); |
There was a problem hiding this comment.
This commented-out call to InputSystem.Update() appears to be leftover debugging code. Consider removing it.
🤖 Helpful? 👍/👎
| InputSystem.Update(); | ||
| ScheduleFocusEvent(true); | ||
| InputSystem.Update(InputUpdateType.Dynamic); | ||
| //InputSystem.Update(); |
There was a problem hiding this comment.
This commented-out call to InputSystem.Update() appears to be leftover debugging code. Consider removing it.
🤖 Helpful? 👍/👎
| "SoftReset Mouse1", "SoftReset Mouse3", "HardReset Joystick", "SoftReset TrackedDevice2" | ||
| "Sync Keyboard" | ||
| })); | ||
| // Enabled devices that don't support syncs dont get reset for Ignore Forcus as we do not want to cancel any actions. |
There was a problem hiding this comment.
Typo in comment: "Forcus" should be "Focus".
🤖 Helpful? 👍/👎
| InputSystem.Update(); | ||
| ScheduleFocusEvent(true); | ||
|
|
||
| // We have to schedule the specificic update type that the player is configured to process events in, |
There was a problem hiding this comment.
Typo in comment: "specificic" should be "specific".
🤖 Helpful? 👍/👎
| { | ||
| // When not running in the background, the same thing happens but only on focus gain. | ||
| runtime.PlayerFocusGained(); | ||
| //runtime.PlayerFocusGained(); |
There was a problem hiding this comment.
Commented-out code //runtime.PlayerFocusGained(); should be removed to keep the test source clean.
🤖 Helpful? 👍/👎
1 participant
Description
These are the managed changes for the package to introduce a focus event into the event buffer. It no longer listens to the Application.OnFocusChanged and instead changes focus based on the queued event from native. This will help us deal with desync focus state issues to make sure events are being processed in the right state.
Jira tickets:
ISX-2426
ISX-2427
Testing status & QA
Testing done:
Testing by QA:
Overall Product Risks
Comments to reviewers
I've spend some time cleaning up the OnUpdate a bit, didn't go too crazy and mostly just split up the code a bit in a couple of functions. But since the git diff makes it a bit difficult to see the changes, it might be easier to pull the branch and do a diff that way.
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.