FIX: disabling the InputSystemUIInputModule component resets all actions to None #2192
Conversation
| @@ -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 67.79% 67.77% -0.02%
===========================================
Files 367 367
Lines 53528 53538 +10
===========================================
- Hits 36290 36288 -2
- Misses 17238 17250 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 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
|
Hey @Pauliusd01, could you try it again please? With this new patch, I tried: (1) With actions asset set before play mode
(2) With no actions asset set before play mode
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. |
| @@ -1665,7 +1676,12 @@ protected override void OnDisable() | |||
| InputActionState.s_GlobalState.onActionControlsChanged.RemoveCallback(m_OnControlsChangedDelegate); | |||
| DisableAllActions(); | |||
| UnhookActions(); | |||
| UnassignActions(); | |||
|
|
|||
| // In the case we've been initialialised with default actions, we want to unalloc them | |||
There was a problem hiding this comment.
We want to do this because if we don't dealloc them, then no one would
| { | ||
| if (defaultActions != null) | ||
| { | ||
| defaultActions.Dispose(); |
There was a problem hiding this comment.
I guess, calling Dispose is just to stay on the safe side since the only data default actions stores is a ref to ScriptableObject that won't survive even with domain reload off. It will be in this intermediate state though, where its native data is gone but the instance is not null until the next gc run (however you'll see it as "null" in debugger, and ==null will be true as well, thanks to Unity's Equals overload for all Object descendants).
…ving junk after reloading
K-Tone
force-pushed
the
anthony/isxb-1493-deactivation-resets-actions
branch
from
bb42698 to
e4eba87
Compare
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: