InputSystem icon indicating copy to clipboard operation
InputSystem copied to clipboard

FIX: disabling the InputSystemUIInputModule component resets all actions to None

Open K-Tone opened this issue 6 months ago • 2 comments

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, Added sections.
    • 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".
  • [x] Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration 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.

K-Tone avatar Jun 05 '25 09:06 K-Tone

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.

codecov-github-com[bot] avatar Jun 05 '25 09:06 codecov-github-com[bot]

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.

K-Tone avatar Jun 06 '25 09:06 K-Tone

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 avatar Jun 25 '25 08:06 K-Tone

@K-Tone Looks like you need to rerun the formatter on this branch before CI will be happier. Other failures look like instabilities.

ekcoh avatar Jun 25 '25 19:06 ekcoh