FIX: Input devices being lost up on upgrade to the new input version [ISX-2569]#2438
Conversation
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2438 +/- ##
============================================
+ Coverage 58.58% 79.01% +20.43%
============================================
Files 738 766 +28
Lines 135831 140426 +4595
============================================
+ Hits 79570 110964 +31394
+ Misses 56261 29462 -26799 Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Not sure I fully understand this fix but looks like fixing broken serialisation. The key takeaway here IMO is that this (not this PR; but how this work) is the wrong solution to the problem. I will link this issue/PR as a motivational case for why device ownership, enumeration, reconnects should live in the module (native) and not in the package. This would make this + any serialisation push/pop disappear from the package. Good fix!
|
Thanks for the input Hakan! I would have totally loved if we could poll data from native at certain points instead of messing with ScriptableObjects. We do know keeping data is needed, just that if we keep copies in managed that appears to cost much more. I would have totally loved trying to minimise our managed state that needs to survive domain reload. |
|
Just leaving my status that I am currently busy with our module port, it is unlikely to be checked this week |
|
Simply to have it said somewhere, I'm on a PTO in July so if it's not this week then either someone else will have to address feedback on this PR or still myself but in August. |
|
I'll give it a go today |
|
/test_plan |
Test Plan
Summary of Changes & Risk AssessmentSummary of ChangesThis PR addresses a critical bug [ISX-2569] where input devices were lost during package upgrades. The issue stemmed from renaming the internal state-tracking class from Risk Assessment
Test ScenariosFunctional Testing
Regression Testing
Edge Cases
💡 This test plan updates automatically when 🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
Pauliusd01
left a comment
Pauliusd01
left a comment
There was a problem hiding this comment.
LGTM, checked upgrading, reimporting and did some basic fast enter play mode rechecks.
K-Tone
force-pushed
the
anthony/isx-2569-fix-fepm-upgrade-device-loss
branch
from
a8a5599 to
b32a0f9
Compare
3 participants
Description
So the problem we're trying to fix here is that when you have a working project with pre-FEPM InputSystem and you then update the input package to the one that has the FEPM change, all input comes broken. The root problem appeared that all devices were lost during upgrade.
The reason devices got lost during upgrade is somewhat tricky to explain, but it all comes down to the fact that native will only communicate the current devices once per Unity process. If you lose devices once, it's permanent - entering playmode doesn't list devices again.
The way we lost devices was due to us renaming an in-memory scriptable object class that was used to keep state from InputSystemObject to InputSystemStateManager. Up on upgrade recompilation, Unity realised that the class for the InputSystemObject SO is no longer available so it shall be dropped, and all the devices died with it.
Hereby, we bring back the old class with the old meta info so that the forward upgrade can work for now.
Testing status & QA
Tried locally.
Overall Product Risks
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.