FIX: disabling the InputSystemUIInputModule component resets all actions to None #2192
Conversation
…ving junk after reloading
| @@ -1665,7 +1676,6 @@ protected override void OnDisable() | |||
| InputActionState.s_GlobalState.onActionControlsChanged.RemoveCallback(m_OnControlsChangedDelegate); | |||
| DisableAllActions(); | |||
| UnhookActions(); | |||
| UnassignActions(); | |||
There was a problem hiding this comment.
^ This line is very wrong, it results in user data being lost on component deactivation.
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2192 +/- ##
===========================================
- Coverage 65.45% 65.45% -0.01%
===========================================
Files 367 367
Lines 53505 53511 +6
===========================================
- Hits 35024 35023 -1
- Misses 18481 18488 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
K-Tone
changed the title
K-Tone
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an issue where disabling the InputSystemUIInputModule component inadvertently resets all actions, including user-configured ones, instead of just the default actions asset. Key changes include:
- Adding a static ResetDefaultActions method that disposes the default actions asset on domain reload.
- Removing the UnassignActions() call from OnDisable to prevent resetting user actions.
Comments suppressed due to low confidence (2)
Packages/com.unity.inputsystem/InputSystem/Plugins/UI/InputSystemUIInputModule.cs:1678
- [nitpick] Removing 'UnassignActions()' is the intended change to prevent resetting user-assigned actions; consider adding an inline comment to clearly explain this design decision for future maintainers.
UnassignActions();
Packages/com.unity.inputsystem/InputSystem/Plugins/UI/InputSystemUIInputModule.cs:1532
- [nitpick] While the ResetDefaultActions method properly disposes the default actions, adding a summary comment that explains its role during domain reloads would improve clarity and maintainability.
[RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.SubsystemRegistration)]
|
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. |
There was a problem hiding this comment.
Found an issue with the UI default actions getting added in after a scene load. Steps are: Open our main repo as a unity project -> top dropdown QA Tools -> Add all core samples / open core scene -> import the textmeshpro essentials if needed -> make sure the event system is set to use a project wide actions asset if it isn't already -> enter play mode and observe the action count -> pick anything from the scene loader -> esc -> return to scene picker and observe the action count again. This does not reproduce in Develop
06.06.2025.-.Unity.197.mp4
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
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.After merge: