FIX: [XR-10725] Fixed regression with initial focus causing input devices to always be disabled#2447
FIX: [XR-10725] Fixed regression with initial focus causing input devices to always be disabled#2447chris-massie wants to merge 9 commits into
Conversation
… disabled - Fixed regression caused by #2365 where the focus state of the application was not being synced to the input runtime correctly, causing all input devices to be initially disabled until the game window was clicked away and then back. - Fixed some incorrect setting of the `focusState` by just setting or clearing the ApplicationFocus flag instead of setting the entire value. In effect this has no current difference because the flags enum only has two values, but makes it future-proof.
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Harness Review
The focus-state sync change is directionally right, but the new runtime-install path now overwrites injected runtimes with the host application's focus state, which regresses a real code path in this repo.
Reviewed commit f36bf495
🤖 Helpful? 👍/👎
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2447 +/- ##
============================================
+ Coverage 58.58% 78.94% +20.36%
============================================
Files 738 767 +29
Lines 135831 140741 +4910
============================================
+ Hits 79570 111107 +31537
+ Misses 56261 29634 -26627 Flags with carried forward coverage won't be shown. Click here to find out more.
|
- This doesn't actually seem correctly formatted, but it's what the tool wants
…ent of the Application.isFocused property - Since the runtime is what triggers the callback that the InputManager uses to respond to the focus change, we don't need to have the InputManager set the focusState. We can just rely on the runtime itself to set the focus state after triggering the Action. - TODO: The InputTestRuntime.InvokePlayerFocusChanged does the opposite order of NativeInputRuntime.OnFocusChanged when setting the field. However, if I update the test runtime to set the `m_FocusState` after invoking the `onPlayerFocusChanged`, the `UI_WhenAppLosesAndRegainsFocus_WhileUIButtonIsPressed_UIButtonClickBehaviorShouldDependOnIfDeviceCanRunInBackground(false)` test fails. More investigation is needed to identify if the test is not setup correctly or if the difference in order is intended.
| if (m_Runtime != null) | ||
| return m_Runtime.focusState; | ||
|
|
||
| return Application.isFocused ? FocusFlags.ApplicationFocus : FocusFlags.None; |
There was a problem hiding this comment.
My worry with this would be that if m_runtime is null and we are using focus events, that could potentially mean that whatever focus state we receive could be wrong in high noise cases, as we are processing focus events based on the Input update processing buffer, not the application focus. I know there were some prs that improved the m_runtime lifecycle that were done later, so maybe this isnt a problem anymore now
There was a problem hiding this comment.
I don't think there is ever a case where there isn't a runtime installed in the InputManager now, so I don't think that fallback case would ever be hit. But anyway, there wasn't any place in the code where this getter was being used after this refactor, so I removed the property and updated the one line where the setter was being used to just directly set it in the runtime. Done in 67f6c04.
| /// In editor this means the GameView has focus. In a built player this means the player has focus. | ||
| /// </summary> | ||
| ApplicationFocus = (1 << 0) | ||
| ApplicationFocus = (1 << 0), |
There was a problem hiding this comment.
dont need comma here if we dont have more entries here
There was a problem hiding this comment.
Our Unity style guide says to use a trailing comma. I think the reasoning is because it keeps the diff cleaner if another entry is added since it will only be new lines instead of also being a modification to an existing line.
Trail last element in a list with ','
…used - Replaced the one line where the setter was used with directly updating the runtime's focus state. This was done to remove questions about the fallback case for the getter if there was no runtime set.
|
@VeraMommersteeg I addressed your feedback. However there's still something that I wanted to call out in the PR description under Comments to reviewers:
The I think it would be worth taking a look at these 3 cases to determine what the correct functionality should be while we're looking at all this focus stuff. The u-pr bot even called this out below: #2447 (comment) |
…focus change to restore old functionality - Since the `applicationHasFocus` property was updated with this branch to return the runtime's value instead of having the duplicate field, it now caused the defaultUpdateType to be different during the method. The goal of this change is to restore the old timing of when that property changes until after the `OnFocusChanged` method finishes executing.
There was a problem hiding this comment.
💡 Harness Review
The focus-state ownership cleanup is directionally sound, but the legacy focus handler now has a cleanup hole that can leave the manager stuck in the wrong focus/update state after an exception.
Reviewed commit 488a7fd7
🤖 Helpful? 👍/👎
…ge` is cleared before returning
This comment was marked as off-topic.
This comment was marked as off-topic.
Test Plan
Summary of Changes & Risk AssessmentSummary of ChangesThis PR fixes a regression (XR-10725) where input devices (specifically XR hand devices) remained disabled on application startup because the initial focus state was not correctly synced to the input runtime. The fix moves the source of truth for 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 |
There was a problem hiding this comment.
I've ran the /test_plan suggested tests (without XR devices) as well as some from the original focus PR in playmode and player looking for oddities with different focus settings. Saw improvements in device discoverability for the player only, otherwise it was the same as develop. But I have to be honest that the whole focus setting matrix was very confusing and I relied heavily on passing it to claude and constantly asking it whether x or y is expected.
Adding the test scenes I used below if you want to double check them:
InputPRTests.zip
FocusEvents.zip
The InputPRTests one has some of the matrix documented in the scene itself and you can double check if what you're seeing is what you're supposed to be getting (it's split into tabs for each /test_plan test, pick one and toggle it on)
3 participants
Description
Fixed regression caused by #2365 where the focus state of the application was not being synced to the input runtime correctly, causing all input devices to be initially disabled until the game window was clicked away and then back.
Testing status & QA
I tested with an XR project with a PC standalone build. Launching the built .exe now causes the hand devices (
<MetaAimHand>and<XRHandDevice>) to update correctly without needing to refresh focus by clicking away and back to the game. Since those currently do not have the run in background device flags (and thuscanDeviceRunInBackgroundis false), they correctly stop updating when the game window loses focus by clicking away. Tested with 6000.0.79f1 and 6000.5.0f1 to test both paths ofUNITY_INPUTSYSTEM_SUPPORTS_FOCUS_EVENTS.I also tested in the Unity Editor with 6.0 and 6000.5.0f1 and the hand devices update correctly. However due to how the Input System handles
canRunInBackgroundvscanDeviceRunInBackgroundwhile in the Editor, the hand devices continue to update even while the game is not focused (Application.isFocused), but that has been the case for many years.Note that I did see a difference in behavior between 6.0 and 6.5 in the Editor where
Application.isFocusedwould become False after clicking Play only on Unity 6.5 and only when the XR subsystem is initialized even though "Play Focused" was enabled in the Game view. That may be an existing problem though.Overall Product Risks
InputManager.focusStateno longer has its own state different from theIInputRuntime.focusState.IInputRuntime. This does not affect input tests since theInputTestRuntimeinitializesm_FocusState = FocusFlags.ApplicationFocusand thus is independent of theApplication.isFocusedproperty. This could also affectInputSystemUIInputModuleand UI Toolkit since theisPlayerFocusedis now initialized/updated differently, however the 1.19.0 version of that property was always usingApplication.isFocusedfor that property so it should be functionally the same as compared to that version.Comments to reviewers
There does seem to be a difference in the order in which the native runtime and the test runtime invokes theAddressed in #2447 (comment)onPlayerFocusChangedaction. TheNativeInputRuntimeinvokes the callback first and then sets thefocusStatefield, but theInputTestRuntimesets the field first and then invokes the callback. When I tried updating the test to order it the same as the real native input runtime, theUI_WhenAppLosesAndRegainsFocus_WhileUIButtonIsPressed_UIButtonClickBehaviorShouldDependOnIfDeviceCanRunInBackground(false)test in UITests.cs started failing. This needs to be investigated if the test is at fault with a faulty setup or if the difference in runtimes is intentional.I also fixed some incorrect setting of the
focusStateby just setting or clearing the ApplicationFocus flag instead of setting the entire value. This has no current difference because the flags enum only has two values, but makes it future-proof and makes it consistent in this codebase.No changelog entry is needed since this is a fix to a regression in develop's 1.20.0 which has not yet been released.
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.