InputSystem
InputSystem copied to clipboard
FIX: disabling the InputSystemUIInputModule component resets all actions to None
Description
This PR fixes a problem where disabling the InputSystemUIInputModule component would reset all the actions, even if they had been priorly set by user.
This started to happen after a fix for another regression landed, where we unassigned all actions from the components OnDisable. I believe this is wrong, since the only thing we really need to be resetting is the default actions asset that shouldn't normally survive re-reentering playmode.
Testing status & QA
Local testing, a new regression test, pending a QA pass
Overall Product Risks
- Complexity: Low
- Halo Effect: Low
Checklist
Before review:
- [ ] Changelog entry added.
- Explains the change in
Changed,Fixed,Addedsections. - For API change contains an example snippet and/or migration example.
- JIRA ticket linked, example (case %<ID>%). If it is a private issue, just add the case ID without a link.
- Jira port for the next release set as "Resolved".
- Explains the change in
- [x] Tests added/changed, if applicable.
- Functional tests
Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult. - Performance tests.
- Integration tests.
- Functional tests
- [ ] Docs for new/changed API's.
- Xmldoc cross references are set correctly.
- Added explanation how the API works.
- Usage code examples added.
- The manual is updated, if needed.
During merge:
- [x] Commit message for squash-merge is prefixed with one of the list:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.
After merge:
- [ ] Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.
Codecov Report
Attention: Patch coverage is 36.36364% with 7 lines in your changes missing coverage. Please review.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| ...InputSystem/Plugins/UI/InputSystemUIInputModule.cs | 36.36% | 7 Missing :warning: |
@@ Coverage Diff @@
## develop #2192 +/- ##
===========================================
- Coverage 67.78% 67.78% -0.01%
===========================================
Files 367 367
Lines 53526 53536 +10
===========================================
+ Hits 36284 36287 +3
- Misses 17242 17249 +7
| Flag | Coverage Δ | |
|---|---|---|
| mac_2021.3_pkg | 5.41% <0.00%> (-0.01%) |
:arrow_down: |
| mac_2021.3_project | 70.42% <36.36%> (-0.02%) |
:arrow_down: |
| mac_2022.3_pkg | 5.19% <ø> (ø) |
|
| mac_2022.3_project | 65.28% <36.36%> (-0.01%) |
:arrow_down: |
| mac_6000.0_pkg | 5.20% <ø> (ø) |
|
| mac_6000.0_project | 67.70% <36.36%> (-0.01%) |
:arrow_down: |
| mac_6000.1_pkg | 5.20% <ø> (ø) |
|
| mac_6000.1_project | 67.70% <36.36%> (-0.01%) |
:arrow_down: |
| mac_6000.2_pkg | 5.20% <ø> (ø) |
|
| mac_6000.2_project | 67.70% <36.36%> (-0.01%) |
:arrow_down: |
| mac_trunk_pkg | 5.20% <ø> (ø) |
|
| mac_trunk_project | 67.70% <36.36%> (-0.01%) |
:arrow_down: |
| win_2021.3_pkg | 5.41% <0.00%> (-0.01%) |
:arrow_down: |
| win_2021.3_project | 70.49% <36.36%> (-0.02%) |
:arrow_down: |
| win_2022.3_pkg | 5.19% <ø> (ø) |
|
| win_2022.3_project | 65.36% <36.36%> (-0.01%) |
:arrow_down: |
| win_6000.0_pkg | 5.20% <ø> (ø) |
|
| win_6000.0_project | 67.77% <36.36%> (-0.01%) |
:arrow_down: |
| win_6000.1_pkg | 5.20% <ø> (ø) |
|
| win_6000.1_project | 67.77% <36.36%> (-0.01%) |
:arrow_down: |
| win_6000.2_pkg | 5.20% <ø> (ø) |
|
| win_6000.2_project | 67.77% <36.36%> (-0.01%) |
:arrow_down: |
| win_trunk_pkg | 5.20% <ø> (ø) |
|
| win_trunk_project | 67.77% <36.36%> (-0.01%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Files with missing lines | Coverage Δ | |
|---|---|---|
| ...InputSystem/Plugins/UI/InputSystemUIInputModule.cs | 93.17% <36.36%> (-0.59%) |
:arrow_down: |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
As to the catches by Copilot:
(1) This is fine. Historically, we've only had UnassignActions since the beginning of the year, and that was a mistake, so I think future maintainers don't need to know we made that mistake and corrected it quickly
(2) There is a short, but to the point comment actually. Not sure I'd like to expand it further.
Hey @Pauliusd01, could you try it again please? With this new patch, I tried:
(1) With actions asset set before play mode
- Disabling component doesn't lead to all actions lost any more
- Entering playmode and loading the project-wide test scene and navigating the menu doesn't duplicate actions
(2) With no actions asset set before play mode
- Entering playmode sets the default actions correctly
- Disabling domain reload and entering play mode twice doesn't result in exception
Note that when you enter play mode with InputSystemUIInputModule that had no actions asset assigned it results in us creating some default actions asset behind the scenes and using it to assign all actions. Disabling this component results in us destroying the default asset, and thus the actions get unassigned as well, so you do get "None" in this case, which I guess is kinda by design (though unfortunate). Enabling it back will restore defaults. If I were to redesign this component from scratch, I would make an explicit checkbox in the component called "Use defaults", and in this case I would hide all the actions, so that the user doesn't get confused by what is going on behind the scenes. This would also simplify code a little bit.
@K-Tone Looks like you need to rerun the formatter on this branch before CI will be happier. Other failures look like instabilities.