NEW: Added Focus Events to Input Event Queue#2365
NEW: Added Focus Events to Input Event Queue#2365VeraMommersteeg wants to merge 21 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 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? 👍/👎
This comment was marked as off-topic.
This comment was marked as off-topic.
# Conflicts: # Packages/com.unity.inputsystem/InputSystem/InputManager.cs # Packages/com.unity.inputsystem/Tests/TestFixture/InputTestFixture.cs
|
@VeraMommersteeg Looks like the PR currently has conflicts that are in need of being resolved:
Likely due to CEPM/FEPM branch being merged. |
|
@VeraMommersteeg Noticed there are multiple test failures, I suspect you have already noticed and is looking into it? |
|
Yes Im aware I have changes to fix it locally |
ekcoh
left a comment
ekcoh
left a comment
There was a problem hiding this comment.
Just commenting for now as a half-way review. Need to clone this PR in order to have a realistic chance to provide relevant feedback on InputManager.cs diff. Looking at the diff alone is very complex.
|
|
||
| // And make sure we're not seeing the data in the editor. | ||
| runtime.PlayerFocusLost(); | ||
| ScheduleFocusEvent(false); |
There was a problem hiding this comment.
Minor: It is only semantics, by I personally think a test fixture helper like is should be named what it does instead of reflecting implementation details (that changing focus leads to scheduling an event). E.g. ChangeFocus(applicationHasFocus: false) is a lot clearer. The fact that this leads to a deferred change event is a detail. But the actual change happens when that line is executed.
Nitpick: When using trivial arguments like this, I think it's helpful to pass as named argument since false could be mean many things. It makes it a lot easier to read the code, especially in web-interface where there are no IDE smartness that highlights the name of the argument.
There was a problem hiding this comment.
I only write this comment once even though it applies to all tests here, e.g. row 174 ScheduleFocusEvent(true) etc.
| // will be Editor, which means that we will end up swapping buffers to the editor buffer, and retreiving the wrong active touch | ||
| // The only way to properly fix this, is to remove defaultUpdateType, and split the player/editor update loops into separate methods, which would be a breaking change. | ||
| // Until then, we have to make sure to schedule the correct update type here. | ||
| switch (updateMode) |
There was a problem hiding this comment.
I think it would be cleaner, and separate concerns, if the type transform from InputSettings.UpdateMode to InputUpdateType would happen in its own single-purpose (internal) extension method. This whole switch statement would then effectively be part of that static method and this would instead become:
InputSystem.Update(updateMode.ToInputUpdateType());
I suspect that conversion happens in multiple places so such a conversion is likely reusable to reduce code-bloat as well.
|
|
||
| runtime.PlayerFocusLost(); | ||
|
|
||
| // We now have to run a dynamic update before the press, to make sure the focus state is up to date. |
There was a problem hiding this comment.
Thanks for adding this comment, very helpful. A slight improvement could be to reference what you actually refer to by "pre-update". It is not currently clear exactly what this means. It might help developers circling back to this in the future.
|
|
||
| var eventCount = 0; | ||
| InputSystem.onEvent += (eventPtr, _) => ++ eventCount; | ||
| InputSystem.onEvent += (eventPtr, _) => |
There was a problem hiding this comment.
Nitpick: I have no problem with this approach or filtering at all. The first thing that strikes me though is that it sounds like the test is based on observing an exact number of events. This means that the test is very brittle and would brake down when any new (or user plugin injected) event is added. To avoid that, maybe it would make sense to instead only count the events relevant to the test to make it future proof? The system should be resilient to new events being added IMO. What do you think?
| "SoftReset Mouse1", "SoftReset Mouse3", "HardReset Joystick", "SoftReset TrackedDevice2" | ||
| "Sync Keyboard" | ||
| })); | ||
| // Enabled devices that don't support syncs dont get reset for Ignore Focus as we do not want to cancel any actions. |
| @@ -398,12 +431,13 @@ public bool runPlayerUpdatesInEditMode | |||
| #else | |||
| true; | |||
| #endif | |||
| private bool applicationHasFocus => (focusState & FocusFlags.ApplicationFocus) != 0; | |||
There was a problem hiding this comment.
Nitpick: This is fine, but the C# standard way of such a check is typically (focusState & FocusFlags.ApplicationFocus) != FocusFlags.None. Happy to see you do not use flags extension since I learned it applies boxing under the hood.
| k_InputAddDeviceMarker.End(); | ||
| return null; | ||
| var device = AddDevice(layout, deviceId, deviceName, description, deviceFlags); | ||
| device.m_Description = description; |
There was a problem hiding this comment.
Not introduced by your PR but refactored, but isn't this device.m_Description = description; redundant? It seems to be passed in AddDevice on the line above? (I would assume it is the same)
| None = 0, | ||
|
|
||
| /// <summary> | ||
| /// The application has focus. |
There was a problem hiding this comment.
Maybe we should make this comment more crisp (potentially both here and in module), to clarify that ApplicationFocus means "GameView focus" when running in the editor, see https://docs.unity3d.com/6000.3/Documentation/ScriptReference/MonoBehaviour.OnApplicationFocus.html. This could avoid confusion and strengthen the definition.
| @@ -0,0 +1,267 @@ | |||
| using System.Runtime.CompilerServices; | |||
There was a problem hiding this comment.
The name of this class can be very confusing since it is similar to what Input Manager in Unity engine is often called. Since this is a partial class containing a selected set of functionality I would suggest renaming it to InputManager.LegacyFocusHandling.cs or similar instead.
There was a problem hiding this comment.
(But it make a lot of sense to extract this functionality into its own file)
| get => m_FocusState; | ||
| set => m_FocusState = value; | ||
| } | ||
| public bool isPlayerFocused => (m_FocusState & FocusFlags.ApplicationFocus) != 0; |
There was a problem hiding this comment.
Nitpick: Compare FocusFlags.None instead of 0?
3 participants
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.