CHANGE: Split Runtime and Editor code into separate folders to prepare for Module migration#2321
CHANGE: Split Runtime and Editor code into separate folders to prepare for Module migration#2321jfreire-unity wants to merge 114 commits intodevelopfrom
Conversation
- Move InputSystem/* to Runtime/ (preserving history) - Move InputSystem/Editor to Editor/ (preserving history) - Add meta files for new folder structure - Follows Unity package layout conventions - All file history preserved via git mv
Also exposes the required internals between assemblies.
This is done to avoid calling into Edito specific code. Instead, it will be called if Editor code has registered a callback.
Due to refactoring, a lot of paths still contained the InputSystem folder path that no longer exists. We only have Editor and Runtime folders.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
Some files were moved to other folders, after merging conflicts arise but since they showed as removed changes didn't land on the moved files. This commit is fixing this
| UnityEngine.TestTools.LogAssert.Expect(LogType.Error, "ScriptableSingleton already exists. Did you query the singleton in a constructor?"); | ||
| UnityEngine.TestTools.LogAssert.Expect(LogType.Error, "ScriptableSingleton already exists. Did you query the singleton in a constructor?"); | ||
| // Ensure the singleton is initialized deterministically in editor. | ||
| _ = RemoteInputPlayerConnection.instance; |
There was a problem hiding this comment.
what is the the underscore thing here in this expression?
There was a problem hiding this comment.
The _ is the C# discard pattern (introduced in C# 7). Writing _ = expr; evaluates the expression (triggering any side-effects, in this case ensuring the singleton is initialized) while explicitly discarding the result. It's equivalent to calling RemoteInputPlayerConnection.instance; as a statement, but avoids the unused-expression compiler warning.
Before it was a nested private class inside VirtualMouseInput class-
| } | ||
|
|
||
| // We have this function to hide away instanceId -> entityId migration that happened in Unity 6.3 | ||
| public static bool HasNativeObject(Object obj) |
There was a problem hiding this comment.
We have removed this API so we need to reintroduce-it somehow/deprecate it.
There was a problem hiding this comment.
Reintroduced in 8e248a9 as a [Obsolete] public method under #if UNITY_EDITOR. The implementation delegates to a s_HasNativeObjectCallback registered by InputSystemEditorInitializer, keeping the UnityEditor dependency out of the runtime file. Users who call it will get a compile-time warning pointing to EditorUtility.InstanceIDToObject as the replacement.
VeraMommersteeg
left a comment
There was a problem hiding this comment.
Overall looks good, I have a few questions and some minor feedback. I noticed there were quite a few changes unrelated to the editor/runtime split. It might've been better to do those changes in a separate pr to create less noise.
| /// </remarks> | ||
| /// <exception cref="InvalidOperationException">Thrown if this input action reference is part of an | ||
| /// input actions asset and mutating it would have side-effects on the projects assets.</exception> | ||
| private void CheckImmutableReference() |
There was a problem hiding this comment.
wouldnt it make more sense to move this into some form of editor utility file and then only wrap the callsite in an ifdef for now. that way it will be easier to deal with in the future
There was a problem hiding this comment.
The editor-specific logic in CheckImmutableReference is still in the runtime file due to the #if UNITY_EDITOR guard pattern used throughout this PR. Moving it to a separate editor utility file would require either making more of InputActionReference's internals accessible from the editor or wiring up another callback — both of which were deemed out of scope for this PR. The call site and implementation remain clearly marked with #if UNITY_EDITOR / #endif.
| // when importing assets which would simplify this class, but it adds complexity to import stage and | ||
| // is more difficult to assess from a asset version portability perspective. | ||
| static bool CanSetReference(InputActionReference reference) | ||
| bool CanSetReference(InputActionReference reference) |
There was a problem hiding this comment.
why do we still need a function inside a function? this is no longer static
There was a problem hiding this comment.
Good catch. Refactored in 8e248a9 – CanSetReference is now a regular private bool CanSetReference() instance method instead of a local function.
| } | ||
|
|
||
| #endif | ||
| private static void RegisterApiUsage(int api) |
There was a problem hiding this comment.
This function doesnt properly describe what it does. RegisterAnalyticsApi would be clearer. before you could see that it was an analytics call being registered, but now RegisterApiUsage((int)Api.AddBinding) doesnt say much
There was a problem hiding this comment.
Renamed to RegisterAnalyticsApi in 8e248a9.
| @@ -41,41 +37,13 @@ public void OnPointerDown(PointerEventData eventData) | |||
|
|
|||
| [InputControl(layout = "Button")] | |||
| [SerializeField] | |||
| private string m_ControlPath; | |||
| internal string m_ControlPath; | |||
There was a problem hiding this comment.
was there a reason to make this internal vs private?
There was a problem hiding this comment.
Changed back to private in 8e248a9. The editor's OnScreenButtonEditor.cs now uses the string literal "m_ControlPath" for FindProperty instead of nameof(OnScreenButton.m_ControlPath), since the two classes are now in separate files.
| @@ -187,26 +183,37 @@ public Action<int, string> onDeviceDiscovered | |||
| set => NativeInputSystem.onDeviceDiscovered = value; | |||
| } | |||
|
|
|||
| // Callbacks set by Editor to handle shutdown subscription | |||
| // In Editor, we use EditorApplication.wantsToQuit which expects Func<bool> | |||
| internal Action<Func<bool>> m_RegisterWantsToQuit; | |||
There was a problem hiding this comment.
why not make this private as this only needs to exist in the current scope
There was a problem hiding this comment.
Changed to private in 8e248a9. Added a new internal void SetWantsToQuitCallbacks(register, unregister) method that the editor initializer calls instead of setting the fields directly.
| /// Callback for play mode state changes. The int parameter corresponds to PlayModeStateChange enum values: | ||
| /// 0 = EnteredEditMode, 1 = ExitingEditMode, 2 = EnteredPlayMode, 3 = ExitingPlayMode | ||
| /// </summary> | ||
| Action<int> onPlayModeChanged { get; set; } |
There was a problem hiding this comment.
why not keep the enum instead, and have one in runtime?
There was a problem hiding this comment.
Introduced in 8e248a9 – added an internal enum InputPlayModeChange (in IInputRuntime.cs, guarded by #if UNITY_EDITOR) that mirrors UnityEditor.PlayModeStateChange with the same int values. onPlayModeChanged is now typed Action<InputPlayModeChange>. The editor casts between the two enums via (InputPlayModeChange)(int)change.
| @@ -418,7 +419,7 @@ public void SetUnityRemoteGyroUpdateInterval(float interval) | |||
| unityRemoteGyroUpdateInterval = interval; | |||
| } | |||
|
|
|||
| public Action<PlayModeStateChange> onPlayModeChanged { get; set; } | |||
| public Action<int> onPlayModeChanged { get; set; } | |||
There was a problem hiding this comment.
why was this changed to an int? why not have an enum in runtime?
There was a problem hiding this comment.
Updated in 8e248a9 – onPlayModeChanged is now Action<InputPlayModeChange> in both IInputRuntime and InputTestRuntime.
This comment was marked as off-topic.
This comment was marked as off-topic.
Test Plan
Summary of Changes & Risk AssessmentSummary of ChangesThis PR performs a major organizational refactor of the Input System package by splitting source code into 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 |
|
@u-pr "PR Reviewer Guide" seems outdated. Can you update it based on latest commit or can this only be done manually by author @jfreire-unity? |
|
I can provide an updated guide here in the comments based on the latest commit. However, I do not have the permissions to modify the PR description or existing comments; those must be updated manually by the author @jfreire-unity. Updated PR Reviewer Guide 🔍This PR is a major structural refactor aimed at decoupling 📂 Folder & Assembly Reorganization
🔌 Decoupling Pattern (The "Hook" System)All direct calls to Areas to verify usage of this pattern:
|
|
@copilot resolve the merge conflicts in this pull request |
…ame, etc. Agent-Logs-Url: https://github.com/Unity-Technologies/InputSystem/sessions/277fda1c-71db-41b9-8d8c-a918b3c36b9b Co-authored-by: josepmariapujol-unity <59828124+josepmariapujol-unity@users.noreply.github.com>
|
|
No merge conflicts were found in the branch – |
|
/crc |
There was a problem hiding this comment.
This review identified several new unique issues, including a potential compilation error in test utility code, missing null-safety in composite editor classes, and a resource path typo in a style sheet. Previously reported critical issues regarding initialization race conditions and native buffer management are currently being tracked in separate discussion threads.
🤖 Helpful? 👍/👎
| { | ||
| s_Manager = otherManager; | ||
| InputStateBuffers.SwitchTo(otherManager.m_StateBuffers, otherManager.defaultUpdateType); | ||
| } |
| modeField.RegisterValueChangedCallback(evt => | ||
| { | ||
| target.whichSideWins = (AxisComposite.WhichSideWins)evt.newValue; | ||
| onChangedCallback(); |
There was a problem hiding this comment.
The onChangedCallback delegate is invoked here without a null check. While some other editors implemented in this PR (such as MultiTapInteractionEditor and PressInteractionEditor) use the null-conditional operator ?., this one does not. To avoid a potential NullReferenceException if a null callback is passed, consider using onChangedCallback?.Invoke();.
| onChangedCallback(); | |
| onChangedCallback?.Invoke(); |
🤖 Helpful? 👍/👎
| modeField.RegisterValueChangedCallback(evt => | ||
| { | ||
| target.mode = (Vector2Composite.Mode)evt.newValue; | ||
| onChangedCallback(); |
There was a problem hiding this comment.
| modeField.RegisterValueChangedCallback(evt => | ||
| { | ||
| target.mode = (Vector3Composite.Mode)evt.newValue; | ||
| onChangedCallback(); |
There was a problem hiding this comment.
| "versionDefines": [] | ||
| "defineConstraints": [ | ||
| "UNITY_INCLUDE_TESTS", | ||
| "UNITY_INCLUDE_TESTS" |
…tor/PackageResources/InputActionsEditorStyles.uss Co-authored-by: u-pr[bot] <205906871+u-pr[bot]@users.noreply.github.com>
Description
Background & Goal
NOTE 🤯: This PR is huge and hard to review, but a lot of the changes are moving files and folders around. I know it's not easy to navigate these huge changes so I tried my best to do a good PR description.
The Unity Input System package historically had
UnityEditor.*references scattered throughout its runtime code, guarded by#if UNITY_EDITORpreprocessor directives, but still in the same files as runtime logic.This was making the codebase hard to port into a Unity module in trunk (
unity/unity), and it was also harder to reason about the Editor vs Standalone player boundary.This PR tries to break the dependency of Editor references in Runtime code to help us migrate to a Unity module and does the following changes:
UnityEditor.*references remain in the runtime source files.Unity.InputSystemassembly is kept . The folder reorganisation (Runtime/andEditor/subfolders) is the externally visible result, making forward-ports and movinggithistory to trunk and eventual module migration easier. We decided not to introduce 2 assemblies as this would duplicate our upgrade path when projects upgrade to a Unity version using the new Input System module.Ideally, we would have liked to also have a Runtime and Editor assembly.
That work was initially done but then we reverted it due to the last point above.
This PR was done with the help of agentic coding as well.
High-Level Changes
InputSystem/Runtime/, all editor source toInputSystem/Editor/. 1,201 files renamed (history preserved viagit mv).Unity.InputSystem.asmdefkept as single monolith. AUnity.InputSystem.Editorasmdef was prototyped and then rolled back — the folder split remains as foundation for a future module migration.UnityEditor.*call in runtime files with internalstaticcallback fields (Action,Func). The Editor registers these at startup via[InitializeOnLoad]. In player builds these fields arenulland all call sites are null-safe (?.Invoke()).InputSystemEditorInitializer.cs[InitializeOnLoad]class inEditor/that owns all editor lifecycle concerns previously scattered across the runtime (play-mode changes, domain reload, asset tracking, analytics, Unity Remote, remoting, etc.).InputSystemState.csInputSystemObjectand editor-specific paths) into its own runtime class to help with the dependency boundary.*Editorclasses (Interaction editors, Composite editors, Processor editors, OnScreen editors) removed from runtime files and placed in dedicated files underEditor/.#if UNITY_EDITORmoved toEditor/(e.g.PlayerInputEditor,UnityRemoteSupport,iOSPostProcessBuild).InputTestRuntime,InputTestStateManagerupdated to reflect the new interface contracts (e.g.onPlayModeChanged: Action<int>instead ofAction<PlayModeStateChange>).Pattern used for these changes
The approach used throughout this PR is maybe worth understanding before reviewing individual files:
This pattern appears in:
InputSystem.cs,InputManager.cs,NativeInputRuntime.cs,InputActionSetupExtensions.cs,InputActionAsset.cs,InputActionReference.cs,EnhancedTouchSupport.cs,RemoteInputPlayerConnection.cs,OnScreenStick.cs,TrackedPoseDriver.cs,InputSystemUIInputModule.cs.Review Guide Suggestions
1.
InputSystem/Runtime/InputSystem.csusing UnityEditor;is gone.s_OnSettingsChanged,s_OnActionsChanging,s_ShouldEnableActions,s_OnPlayModeChangeCallback) are used consistently and null-safely.#if UNITY_EDITORblock forOnPlayModeChangeis intentional — it is a forwarding shim for tests that call it directly.2.
InputSystem/Runtime/NativeInputRuntime.csEditorApplication.isPlaying,EditorApplication.isPaused,InternalEditorUtility.isApplicationActive, andEditorAnalyticscalls.internal boolfields (m_IsInPlayMode,m_IsEditorPaused,m_IsEditorActive) and callbacks (m_SendEditorAnalytic,m_SetUnityRemoteMessageHandler, etc.).UpdateEditorState()method inInputSystemEditorInitializeris pumping those fields onEditorApplication.update.3.
InputSystem/Runtime/InputManager.csEditorApplication.isPaused→m_Runtime.isEditorPaused(fromIInputRuntime).InputSystem.s_SystemObjectreferences replaced with localm_ExitEditModeTime/m_EnterPlayModeTimefields.ProjectWideActionsBuildProvider.actionsToIncludeInPlayerBuildreplaced withs_GetProjectWideActionscallback.4.
InputSystem/Runtime/IInputRuntime.csonPlayModeChangedchanged fromAction<PlayModeStateChange>toAction<int>— removes theUnityEditorenum from the interface.bool isEditorPaused { get; }.5.
InputSystem/Editor/InputSystemEditorInitializer.cs(new file)OnEditorPlayModeStateChanged→ setsm_IsInPlayMode,m_IsEditorPaused, dispatchesDispatchPlayModeChange, etc.InputSystemStateis loaded/restored here.6. Extracted
*Editorclasses (Editor/Actions/Interactions/,Editor/Actions/Composites/,Editor/Controls/Processors/,Editor/Plugins/OnScreen/)7.
InputSystem/Runtime/Actions/InputActionReference.cs,InputActionAsset.cs,EnhancedTouchSupport.csAssetDatabase/AssemblyReloadEventscalls replaced with callback fields.#if UNITY_EDITOR-guarded.8. Test fixtures —
Tests/TestFixture/InputTestRuntime.cs:onPlayModeChangedis nowAction<int>— make sure test call sites cast correctly.InputTestStateManager.cs: there were some naming changes, so something to be aware.9. Assembly /
.asmdeffilesUnity.InputSystem.asmdefshould still reference a single assembly (no Editor split).Unity.InputSystemonly.AssemblyInfo.cs:InternalsVisibleToentries cover test assemblies — verify no entry was accidentally dropped.InputSystemForUI.Editor.asmdef, since the previous folder containingInputSystemForUI.asmdefwas split into Editor and Runtime. This is part of internal API so I don't see any breakages in this area when upgrading.Testing status & QA
Built and run Samples on both Windows and macOS Play-mode and Standalone builds.
I recommend validating all the Samples work and do kind of the sames tests as we would do for a Release since this PR touches a lot of things.
Overall Product Risks
Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.
Comments to reviewers
Some sanity check guides:
using UnityEditor;in any file underInputSystem/Runtime/InputSystemEditorInitializerregisters every callback that has a field on the runtime sideInputTestRuntimecompiles and tests pass with the updatedAction<int>signatureUnity.InputSystemwas accidentally wired to a now-deletedUnity.InputSystem.Editorassembly. Hopefully there are not left overs of this initial work.One thing is not in place is a mechanism to guarantee we don't leak UnityEditor references in the Runtime folder. I'll follow up on that in another PR.
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.