NEW: Support entering playmode with Domain Reload disabled ("Faster Enter Playmode")#2343
NEW: Support entering playmode with Domain Reload disabled ("Faster Enter Playmode")#2343
Conversation
- Rework InputSystem's Init functions to handle optional domain reloads - Bypass InitializeInPlayer() "Reset" and "Restore" functions if DRs disabled Only the first step to getting this working, but enables FEPM support for golden path scenarios.
- Move Reset/Restore state functionality out of InputSystem to the Test assembly (InputTestStateManager.cs)
- Refactor InputManager Init/Dispose to be cleaner and better abstracted:
* Adds CreateAndInitialize static method
* Replaces Destroy() with IDisposable implementation
* InputManager creates "default" InputSettings object if none provided
* Runtime, Settings, and Metrics fields now private
- Update InitializeInEditor() to incorporate changes
- Update and fix tests
For the most part, the logic should be mostly preserved. InitializeInEditor() has the biggest (logical)
change because Reset() (moved to Tests) contained some actual init calls that needed to be pulled out.
However, we *should* be making the same calls in the same order.
However, this change does seem to "break" some of the OnScreenTests(); they're now unstable.
This will need to be fixed in a later commit.
- Rework the class so it operates as a Singleton (basically how it's being used)
- Move actions get/set implementations from InputSystem to ProjectWideActions class
* Combine functionality for tighter cohesion despite being an Editor
* Update #ifefs to allow Player to access some of these members
- Call EnsureInitialized() from Editor init flows to fix the actual bug
Tested using ProjectWideActions sample and verified could move cube when domain reloads disabled.
Ran Package tests (Edit/Play/Player) to verify no (new) failures.
…ns (ISX-1842) - Make InputSystem static fields private with read-only accessors - Add InputSystemTestHooks.cs for "Test Hook" operations that need direct access to fields - Factor out InputSystemObject (and related State) and rename to InputSystemStateManager - Factor out "dirty asset tracking" functionality to a separate Utility class Despite touching a bunch of files, this is a low-risk set of changes; functions are moved to new files and types renamed, but no real changes to functionality. Validated changes with Edit/Play/Player tests.
- Most ProjectWideAction changes discarded since feature was re-implemented (no longer relevant) - Recreate InputSystem "Test Hook" for Enabling/Disabling Actions from tests - Small fixes from unresolved conflicts or bad merges during rebase - Various formatting changes (to be more consistant)
- Consolidate functionality into DeferBindingResolutionContext class - Instantiate Context object as a non-static InputManager field exposed via InputManager methods
- Static field (in each class) initialized via RuntimeInitializeOnLoadMethod() hook - This functionality is a bit clunky and should be refactored but that's out-of-scope for this work
- Add a flag to guard against re-initializing Plugins within PerformDefaultPluginInitialization() - Refactor SteamSupport to fix Init/Shutdown flows and improve state management - Refactor PlayerInput's static fields into a single GlobalState struct (matching other classes) - Move EnhancedTouch static fields into Touch's GlobalState struct - Minor refactoring of SteamSupport to improve init/cleanp-up flows (especially with tests)
…ts (ISX-1840) - Consolidate Touchscreen's cached settings into a separate struct - Rework NativeInputRuntime initialization to (fully) employ Singleton pattern - Refactor Actions_CanHandleModification TestCase generator to work without Domain Reloads - Fix Device static fields not getting reset during SimulateDomainReload()
- Rename some variables - Add update some comments - Other small tweaks.
# Conflicts: # Packages/com.unity.inputsystem/InputSystem/Actions/InputActionMap.cs # Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs # Packages/com.unity.inputsystem/InputSystem/InputAnalytics.cs # Packages/com.unity.inputsystem/InputSystem/InputManager.cs # Packages/com.unity.inputsystem/InputSystem/InputSystem.cs # Packages/com.unity.inputsystem/Tests/TestFixture/InputTestFixture.cs
…invalid formatting
…d from a serialisation thread and force create ScriptableObjects
K-Tone
changed the title
…er playmode anymore
|
Hey Hakan! Here is my branch that is based on yours really, with just a few minor corrections. Let's take a look at it together and define what are the larger bits of work that are still missing. |
Ask❓can you make a new entry to the CHANGELOG.md file describing the change happened in this PR. Make sure to keep it a one-liner in style with the rest in that file. Answer:Based on the PR information, here is a suggested entry for the - Added support for entering Play Mode with Domain Reload disabled ("Faster Enter Playmode") by ensuring internal static state is correctly reset.🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
|
/update_changelog |
|
Changelog updates: 🔄 [2026-02-25] *Added
Changed
|
There was a problem hiding this comment.
Looking through this diff it looks very good in my opinion but it is also difficult to review due to many scattered changes. I left a few smaller comments/questions but what I would recommend as way forward:
- Test that all our samples work as expected with CEPM on/off. Including samples changing scenes.
- Test that some external samples, e.g. Warriors or other internal more complex QA test projects work as expected with CEPM on/off.
- Involve integrating team packages, e.g. XR, Cinemachine to hopefully get QA perspective on this work as well from teams depending on Input System.
When we have built sufficient confidence in that this PR doesn't regress any CEPM off behaviour and support CEPM on with good quality I think this can land which will be beneficial for users suffering from domain reload overhead during their development.
| /// The s_ConnectedControllers array is allocated in response to setting event handlers and | ||
| /// so it can double as our "is installed" flag. | ||
| /// </remarks> | ||
| private static bool updateHooksInstalled => s_ConnectedControllers != null; |
There was a problem hiding this comment.
Should these steam-support related static also be grouped into an object, e.g. struct to reduce to a single static similar to other consolidation of static state?
Pauliusd01
left a comment
Pauliusd01
left a comment
There was a problem hiding this comment.
Seeing a ton of errors on the OnScreenControlsSample scene (Steps: disable domain and scene reloads -> open the scene -> enter play mode -> errors)
Awesome, thanks. Taking a look shortly |
Test Plan
Summary of Changes & Risk AssessmentSummary of ChangesThis PR implements support for entering Play Mode with Domain Reload disabled ("Faster Enter Playmode"). It refactors 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 |
K-Tone
changed the title
|
@u-pr Attached to this post you can find all the 87 issues that Unity's Project Auditor has found regarding the domain reload in this branch. Can you analyse them and see if any are still legit? I mean that some issues appear to be problems still, but the check itself is somewhat formal and doesn't appear to take into account that statics of a class can be initialised from other classes, not neccessarily from the dedicated methods of the class. |
|
I cannot access the external JSON attachment directly due to tool limitations. However, I have performed a manual audit of the static fields and state management logic in the current PR diff. Based on your description of the Auditor's behavior and the patterns in the code, I've identified several areas that are likely being flagged. Here is my analysis of which issues are likely False Positives and which ones appear to be Legitimate problems for Domain Reload. 🟢 Likely False PositivesThese are fields that the Auditor probably flags because they aren't reset in a standard
🔴 Legitimate Issues (Still Legit)These fields or patterns appear to have actual issues where state will persist incorrectly or initialization will fail when Domain Reload is disabled.
🟡 Needs Verification
If you can provide the specific list of flagged fields (even just a few examples of the most common ones), I can give you a more targeted assessment. 🤖 Helpful? 👍/👎 |
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
| // Plug-ins must only be initialized once, since many of them use static fields. | ||
| // When Domain Reloads are disabled, we must guard against this method being called a second time. | ||
| private static bool s_PluginsInitialized = false; | ||
| #endif |
There was a problem hiding this comment.
This static flag will persist across Play Mode sessions when Domain Reload is disabled. This means that after the first session, PerformDefaultPluginInitialization will always return early.
Since InitializeInEditor creates a new InputManager on every entry, the plugins will never be hooked to the new manager's events, rendering them inactive in subsequent Play sessions. This should be reset to false when globalReset is true in InitializeInEditor.
🤖 Helpful? 👍/👎
|
@u-pr See below all the 87 issues that Unity's Project Auditor has found regarding the domain reload in this branch. Can you analyse them and see if any are still legit? I mean that some issues are reported, but the check itself is somewhat formal and doesn't appear to take into account that statics of a class can be initialised from other classes, not necessarily from the dedicated methods of the class. {"ignored":false,"descriptorId":"UDR0001","category":"DomainReload","properties":["UDR0001","Unity.InputSystem"],"description":"No method with RuntimeInitializeOnLoadMethod attribute","location":{"IsValid":true,"line":207,"path":"Packages/com.unity.inputsystem/InputSystem/Devices/Sensor.cs"},"severity":"Moderate"}, |
|
I've analyzed the 87 issues reported by the Project Auditor in the context of this PR. Your intuition is correct: many of these are "formal" hits where the auditor's static analysis is too shallow to recognize the consolidated initialization pattern introduced in this branch. Key Findings
RecommendationMost of these hits can be safely ignored as "formal" issues. However, it might be worth adding explicit nullification for the 🤖 Helpful? 👍/👎 |
| internal static InputManager s_Manager; | ||
| internal static InputRemoting s_Remote; | ||
| internal static InputManager manager => s_Manager; | ||
| private static InputManager s_Manager; |
There was a problem hiding this comment.
The auditor flags this as not assigned in a RuntimeInitializeOnLoadMethod. While it is assigned via the GlobalInitialize call chain, the auditor fails to follow the nested calls. This is a false positive.
🤖 Helpful? 👍/👎
| @@ -520,6 +520,14 @@ protected TouchControl[] touchControlArray | |||
| /// <value>Current touch screen.</value> | |||
| public new static Touchscreen current { get; internal set; } | |||
There was a problem hiding this comment.
Since Touchscreen.current is a static property, it should ideally be reset to null during domain reload to avoid leaking references from a previous session, especially when Domain Reload is disabled. Consider adding current = null; to an initialization method.
🤖 Helpful? 👍/👎
| internal static GlobalState s_GlobalState; | ||
|
|
||
| [RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.BeforeSceneLoad)] | ||
| private static void InitializeGlobalActionState() |
There was a problem hiding this comment.
This centralized reset is exactly what is needed for Domain Reload safety. The auditor may still flag fields within InputActionState as UDR0002 if it doesn't recognize that ResetGlobals() is called from an attributed method.
🤖 Helpful? 👍/👎
|
/improve |
|
Persistent review updated to latest commit f38866b |
There was a problem hiding this comment.
Tested: Parts of the AI test plan here on github, the pre-production test plan, ran all of our sample scenes in all FEPM modes, ran the survival kids project in all modes looking for broken inputs. Checked performance via memory profiler by entering and leaving play mode a lot with reloads disabled, usage did not increase. (Windows and Mac)
If you have any other ideas that need testing let me know
| if (handle.IsAllocated) | ||
| globalList[i].Free(); | ||
| globalList.RemoveAtWithCapacity(i); | ||
| --i; |
There was a problem hiding this comment.
Does --i here avoid the collection modified during enumeration exception that could occur?
| continue; | ||
| } | ||
|
|
||
| for (var n = 0; n < state.totalMapCount; ++n) |
There was a problem hiding this comment.
Is this for loop at the right indentation level? It looks like this will be redone for each iteration of the globalList.length for loop above.
| { | ||
| if (globalReset) | ||
| { | ||
| ////FIXME: does not preserve action map state |
There was a problem hiding this comment.
Does this need to be done or will it be done in this PR or later on / it's known?
| s_TrackedDirtyAssets.Clear(); | ||
|
|
||
| // reload input assets marked as dirty from disk | ||
| DirtyAssetTracker.ReloadDirtyAssets(); |
There was a problem hiding this comment.
Really like the separation of logic here into it's own class, we definitely need more of this in classes like these :)
|
|
||
| #if UNITY_EDITOR | ||
| using UnityEngine.InputSystem.Editor; | ||
| using UnityEngine.Tilemaps; |
There was a problem hiding this comment.
| using UnityEngine.Tilemaps; |
| { | ||
| get | ||
| { | ||
| Debug.Assert(m_Settings != null); |
There was a problem hiding this comment.
Getter now allowed to return null but setter still throws on null?
|
|
||
| // For remoting tests, we need to create a "fake manager" that simulates a remote endpoint. | ||
| // In this case don't install globals as this will corrupt the "local" manager state. | ||
| if (!fakeManagerForRemotingTests) |
There was a problem hiding this comment.
Not a fan of mixing the mocking behaviour for testing. But I guess it's not worth creating a separated a test specific factory.
| public void OnBeforeSerialize() | ||
| { | ||
| // Save current system state. | ||
| if (InputSystem.s_Manager != null) |
There was a problem hiding this comment.
Should probably early out if InputSystem.s_Manager is not available.
| /// </summary> | ||
| internal void ApplySettings() | ||
| { | ||
| Debug.Assert(m_Settings != null); |
There was a problem hiding this comment.
Is it even for m_Settings to be null?
| } | ||
| } | ||
|
|
||
| internal sealed class DeferBindingResolutionContext : IDisposable |
There was a problem hiding this comment.
nit. Should probably lie in InputManager file as direct dependency, or it's own, but not in an extension file.
| ref var globalList = ref InputActionState.s_GlobalState.globalList; | ||
|
|
||
| public void Dispose() | ||
| for (var i = 0; i < globalList.length; ++i) |
There was a problem hiding this comment.
This loop should probably be own by InputActionState.
7 participants
Description
This branch aims to enable entering the play mode with Domain Reload disabled, it's also called Faster Enter Playmode sometimes.
The change is a bit non-trivial because we seem to have quite a bunch of statics, and all of them have to be reset the right way when Domain Reload is disabled.
The branch is basically a straightforward merge of the current develop branch into Hakan's #2016 branch, which in turn is based on Tim's incredible original branch: #1915.
Testing status & QA
Barely tested at this stage of affairs, but appears to be running kinda alright on Anthony's mac locally.
Overall Product Risks
Comments to reviewers
Please describe any additional information such as what to focus on, or historical info for the 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.