FIX: Avoid disabling project-wide actions on shutdown#2346
FIX: Avoid disabling project-wide actions on shutdown#2346AswinRajGopal wants to merge 6 commits intodevelopfrom
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
PR Code Suggestions ✨Explore these optional code suggestions:
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
|||||||||
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2346 +/- ##
=========================================
Coverage 77.90% 77.91%
=========================================
Files 476 482 +6
Lines 97613 97780 +167
=========================================
+ Hits 76048 76187 +139
- Misses 21565 21593 +28 Flags with carried forward coverage won't be shown. Click here to find out more.
|
| UnregisterAction(ref m_ScrollWheelAction, OnScrollWheelPerformed); | ||
|
|
||
| if (m_InputActionAsset != null) | ||
| if (m_InputActionAsset != null && m_InputActionAsset != InputSystem.actions) |
There was a problem hiding this comment.
Do we actually have to disable the actions? I'm not a huge fan of disabling this for a particular asset, like project wide actions. I also think the logic we implemented in the past might not be the most correct.
I believe we should always make sure that the UI action map is enabled when registering the actions. Do you know what's the impact of not disabling this asset for a normal asset?
There was a problem hiding this comment.
@jfreire-unity Thanks for the review, I agree that the current change only addresses the reported bug. I’ll improve the fix so that we explicitly ensure the UI action map is enabled when registering actions, and on unregister we only clean up state owned by the provider instead of disabling shared asset state.
|
/test_plan |
Test Plan
Summary of Changes & Risk AssessmentSummary of ChangesThis PR fixes a bug where the Risk Assessment
Test ScenariosFunctional Testing
Regression Testing
🔍 Regression Deep Dive (additional risks identified)
Edge Cases
💡 This test plan updates automatically when 🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
|
I'm not entirely sure what the expected outcome should be. Is it expected that most actions including UI remain active when deleting the UI object? What about this one in the screenshot, what makes it special and why is it the only one that gets removed? |
Hi @Pauliusd01 |
|
Sorry I'll try to have a look at this in the next couple of days. Need to "load the context" of this area to make sure we're not introducing any other regression |
|
/crc |
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
The review has identified several significant issues that need to be addressed. The current implementation introduces a regression risk with a potential ArgumentException when custom assets are used. Additionally, there is a logic discrepancy in the fix itself that results in disabling portions of the project-wide action asset, which will likely cause the newly added test case to fail. Some minor improvements to test code consistency and maintenance are also recommended.
🤖 Helpful? 👍/👎
| m_InputActionAsset.Enable(); | ||
| // Only touch the UI map so we don't change the enabled state of unrelated maps. | ||
| m_UIActionMap = m_InputActionAsset?.FindActionMap("UI", true); | ||
| m_ShouldDisableUIActionMapOnUnregister = m_UIActionMap != null && !m_UIActionMap.enabled; |
There was a problem hiding this comment.
This call will throw an ArgumentException if the m_InputActionAsset does not contain a map named "UI". This is a regression for scenarios where a user provides a custom InputActionAsset (via InputSystem.actions) that uses different map names (e.g., "Default", "GameUI", etc.).
Additionally, if the user's UI actions are located in a map with a different name, only enabling the "UI" map will result in the integration failing to work because the relevant actions will remain disabled. The previous implementation was safer as it enabled the entire asset when a user-provided asset was detected.
| m_ShouldDisableUIActionMapOnUnregister = m_UIActionMap != null && !m_UIActionMap.enabled; | |
| m_UIActionMap = m_InputActionAsset?.FindActionMap("UI", false); |
🤖 Helpful? 👍/👎
| if (m_InputActionAsset != null) | ||
| m_InputActionAsset.Disable(); | ||
| if (m_ShouldDisableUIActionMapOnUnregister && m_UIActionMap != null) | ||
| m_UIActionMap.Disable(); |
There was a problem hiding this comment.
There is a logic mismatch between this implementation and the newly added test case Shutdown_DoesNotDisableProjectWideActionsAsset.
In the test, the asset is not enabled initially. RegisterActions will set m_ShouldDisableUIActionMapOnUnregister to true and enable the "UI" map. When UnregisterActions is called, it will then call m_UIActionMap.Disable(). Since "UI" is the only map in the test asset, asset.enabled will become false, causing the assertion at line 121 in InputForUITests.cs to fail.
If the intention is to prevent side-effects on the project-wide asset, you should consider skipping the Disable() call if m_InputActionAsset is the global InputSystem.actions asset.
🤖 Helpful? 👍/👎
|
|
||
| EventProvider.ClearMockProvider(); | ||
| m_ClearedMockProvider = true; | ||
| Assert.That(asset.enabled, Is.True, "Project-wide actions must remain enabled after provider shutdown."); |
There was a problem hiding this comment.
Based on the implementation in InputSystemProvider.UnregisterActions, this assertion will likely fail.
If the provider was responsible for enabling the "UI" map (which it will be here since the asset is newly created and its maps are disabled by default), it will disable that map during shutdown. Since no other maps are enabled in this test asset, asset.enabled will return false after the provider is cleared.
🤖 Helpful? 👍/👎
| uiMap.AddAction("ScrollWheel", InputActionType.PassThrough, "<Mouse>/scroll"); | ||
| asset.AddActionMap(uiMap); | ||
|
|
||
| InputSystem.s_Manager.actions = asset; |
There was a problem hiding this comment.
For better maintainability and consistency with the rest of the test class, consider using the public InputSystem.actions property instead of the internal s_Manager field. This aligns with the access pattern used in the Setup method.
| InputSystem.s_Manager.actions = asset; | |
| InputSystem.actions = asset; |
🤖 Helpful? 👍/👎
3 participants
Description
case: https://issuetracker.unity3d.com/product/unity/issues/guid/UUM-134130
Bug:
UI Toolkit now destroys its internal objects when there is no UI left in the scene, so InputSystemProvider.Shutdown() can run during play. During shutdown we unregister the UI actions, but the old code was
managing the whole action asset. With project-wide actions (InputSystem.actions), that meant we could affect input outside UI. With the default internal asset, not cleaning up at all could leave the UI map
enabled longer than intended.
Fix:
Scope the lifecycle to the UI action map only. On registration, InputSystemProvider now makes sure the UI map is enabled. On unregister/shutdown, it disables only that UI map, and only if this provider
enabled it. This avoids changing the enabled state of unrelated maps in InputSystem.actions while still cleaning up the default/internal UI actions correctly.
Testing status & QA
Added new test.
Verified manually using repro project.
Overall Product Risks
Comments to reviewers
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.