Misc refactoring to clean up static fields and other small improvements (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()

Author: timkeo

Assets/Tests/InputSystem/CoreTests_Actions.cs

@@ -5263,56 +5263,68 @@ public class ModificationCases : IEnumerable
         [Preserve]
         public ModificationCases() {}
 
+        private static readonly Modification[] ModificationAppliesToSingleActionMap =
+        {
+            Modification.AddBinding,
+            Modification.RemoveBinding,
+            Modification.ModifyBinding,
+            Modification.ApplyBindingOverride,
+            Modification.AddAction,
+            Modification.RemoveAction,
+            Modification.ChangeBindingMask,
+            Modification.AddDevice,
+            Modification.RemoveDevice,
+            Modification.AddDeviceGlobally,
+            Modification.RemoveDeviceGlobally,
+            // Excludes: AddMap, RemoveMap
+        };
+
+        private static readonly Modification[] ModificationAppliesToSingletonAction =
+        {
+            Modification.AddBinding,
+            Modification.RemoveBinding,
+            Modification.ModifyBinding,
+            Modification.ApplyBindingOverride,
+            Modification.AddDeviceGlobally,
+            Modification.RemoveDeviceGlobally,
+        };
+
         public IEnumerator GetEnumerator()
         {
-            bool ModificationAppliesToSingletonAction(Modification modification)
+            // NOTE: This executes *outside* of our test fixture during test discovery.
+
+            // We cannot directly create the InputAction objects within GetEnumerator() because the underlying
+            // asset object might be invalid by the time the tests are actually run.
+            //
+            // That is, NUnit TestCases are generated once when the Assembly is loaded and will persist until it's unloaded,
+            // meaning they'll never be recreated without a Domain Reload. However, since InputActionAsset is a ScriptableObject,
+            // it could be deleted or otherwise invalidated between test case creation and actual test execution.
+            //
+            // So, instead we'll create a delegate to create the Actions object as the parameter for each test case, allowing
+            // the test case to create an Actions object itself when it actually runs.
             {
-                switch (modification)
+                var actionsFromAsset = new Func<IInputActionCollection2>(() => new DefaultInputActions().asset);
+                foreach (var value in Enum.GetValues(typeof(Modification)))
                 {
-                    case Modification.AddBinding:
-                    case Modification.RemoveBinding:
-                    case Modification.ModifyBinding:
-                    case Modification.ApplyBindingOverride:
-                    case Modification.AddDeviceGlobally:
-                    case Modification.RemoveDeviceGlobally:
-                        return true;
+                    yield return new TestCaseData(value, actionsFromAsset);
                 }
-                return false;
             }
 
-            bool ModificationAppliesToSingleActionMap(Modification modification)
             {
-                switch (modification)
+                var actionMap = new Func<IInputActionCollection2>(CreateMap);
+                foreach (var value in Enum.GetValues(typeof(Modification)))
                 {
-                    case Modification.AddMap:
-                    case Modification.RemoveMap:
-                        return false;
+                    if (ModificationAppliesToSingleActionMap.Contains((Modification)value))
+                        yield return new TestCaseData(value, actionMap);
                 }
-                return true;
             }
 
-            // NOTE: This executes *outside* of our test fixture during test discovery.
-
-            // Creates a matrix of all permutations of Modifications combined with assets, maps, and singleton actions.
-            foreach (var func in new Func<IInputActionCollection2>[] { () => new DefaultInputActions().asset, CreateMap, CreateSingletonAction })
             {
+                var singletonMap = new Func<IInputActionCollection2>(CreateSingletonAction);
                 foreach (var value in Enum.GetValues(typeof(Modification)))
                 {
-                    var actions = func();
-                    if (actions is InputActionMap map)
-                    {
-                        if (map.m_SingletonAction != null)
-                        {
-                            if (!ModificationAppliesToSingletonAction((Modification)value))
-                                continue;
-                        }
-                        else if (!ModificationAppliesToSingleActionMap((Modification)value))
-                        {
-                            continue;
-                        }
-                    }
-
-                    yield return new TestCaseData(value, actions);
+                    if (ModificationAppliesToSingletonAction.Contains((Modification)value))
+                        yield return new TestCaseData(value, singletonMap);
                 }
             }
         }
@@ -5343,14 +5355,14 @@ private InputActionMap CreateSingletonAction()
     [Test]
     [Category("Actions")]
     [TestCaseSource(typeof(ModificationCases))]
-    public void Actions_CanHandleModification(Modification modification, IInputActionCollection2 actions)
+    public void Actions_CanHandleModification(Modification modification, Func<IInputActionCollection2> getActions)
     {
 #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
         // Exclude project-wide actions from this test
         InputSystem.actions?.Disable();
         InputActionState.DestroyAllActionMapStates(); // Required for `onActionChange` to report correct number of changes
 #endif
-
+        var actions = getActions();
         var gamepad = InputSystem.AddDevice<Gamepad>();
 
         if (modification == Modification.AddDevice || modification == Modification.RemoveDevice)

Assets/Tests/InputSystem/CoreTests_Editor.cs

@@ -165,6 +165,7 @@ public void Editor_CanSaveAndRestoreState()
         Assert.That(unsupportedDevices[0].interfaceName, Is.EqualTo("Test"));
     }
 
+#if !ENABLE_CORECLR
     // onFindLayoutForDevice allows dynamically injecting new layouts into the system that
     // are custom-tailored at runtime for the discovered device. Make sure that our domain
     // reload can restore these.
@@ -345,6 +346,7 @@ public void Editor_DomainReload_CanRemoveDevicesDuringDomainReload()
         Assert.That(InputSystem.devices, Has.Count.EqualTo(1));
         Assert.That(InputSystem.devices[0], Is.AssignableTo<Keyboard>());
     }
+#endif // !ENABLE_CORECLR
 
     [Test]
     [Category("Editor")]

Packages/com.unity.inputsystem/InputSystem/Controls/InputControlLayout.cs

@@ -109,7 +109,7 @@ public delegate string InputDeviceFindControlLayoutDelegate(ref InputDeviceDescr
     /// </remarks>
     public class InputControlLayout
     {
-        private static InternedString s_DefaultVariant = new InternedString("Default");
+        private static readonly InternedString s_DefaultVariant = new InternedString("Default");
         public static InternedString DefaultVariant => s_DefaultVariant;
 
         public const string VariantSeparator = ";";

Packages/com.unity.inputsystem/InputSystem/Devices/Touchscreen.cs

@@ -534,6 +534,14 @@ protected TouchControl[] touchControlArray
         /// <value>Current touch screen.</value>
         public new static Touchscreen current { get; internal set; }
 
+        /// <summary>
+        /// The current global settings for Touchscreen devices.
+        /// </summary>
+        /// <remarks>
+        /// These are cached values taken from <see cref="InputSettings"/>.
+        /// </remarks>
+        internal static TouchscreenSettings settings { get; set; }
+
         /// <inheritdoc />
         public override void MakeCurrent()
         {
@@ -624,14 +632,14 @@ protected override void FinishSetup()
                 //       that to do so we would have to add another record to keep track of timestamps for each touch. And
                 //       since we know the maximum time that a tap can take, we have a reasonable estimate for when a prior
                 //       tap must have ended.
-                if (touchStatePtr->tapCount > 0 && InputState.currentTime >= touchStatePtr->startTime + s_TapTime + s_TapDelayTime)
+                if (touchStatePtr->tapCount > 0 && InputState.currentTime >= touchStatePtr->startTime + settings.tapTime + settings.tapDelayTime)
                     InputState.Change(touches[i].tapCount, (byte)0);
             }
 
             var primaryTouchState = (TouchState*)((byte*)statePtr + stateBlock.byteOffset);
             if (primaryTouchState->delta != default)
                 InputState.Change(primaryTouch.delta, Vector2.zero);
-            if (primaryTouchState->tapCount > 0 && InputState.currentTime >= primaryTouchState->startTime + s_TapTime + s_TapDelayTime)
+            if (primaryTouchState->tapCount > 0 && InputState.currentTime >= primaryTouchState->startTime + settings.tapTime + settings.tapDelayTime)
                 InputState.Change(primaryTouch.tapCount, (byte)0);
 
             Profiler.EndSample();
@@ -720,11 +728,11 @@ protected override void FinishSetup()
 
                         // Detect taps.
                         var isTap = newTouchState.isNoneEndedOrCanceled &&
-                            (eventPtr.time - newTouchState.startTime) <= s_TapTime &&
+                            (eventPtr.time - newTouchState.startTime) <= settings.tapTime &&
                             ////REVIEW: this only takes the final delta to start position into account, not the delta over the lifetime of the
                             ////        touch; is this robust enough or do we need to make sure that we never move more than the tap radius
                             ////        over the entire lifetime of the touch?
-                            (newTouchState.position - newTouchState.startPosition).sqrMagnitude <= s_TapRadiusSquared;
+                            (newTouchState.position - newTouchState.startPosition).sqrMagnitude <= settings.tapRadiusSquared;
                         if (isTap)
                             newTouchState.tapCount = (byte)(currentTouchState[i].tapCount + 1);
                         else
@@ -1044,8 +1052,16 @@ private static void TriggerTap(TouchControl control, ref TouchState state, Input
             state.isTapRelease = false;
         }
 
-        internal static float s_TapTime;
-        internal static float s_TapDelayTime;
-        internal static float s_TapRadiusSquared;
+        private static TouchscreenSettings s_Settings;
+    }
+
+    /// <summary>
+    /// Cached settings retrieved from <see cref="InputSettings"/>.
+    /// </summary>
+    internal struct TouchscreenSettings
+    {
+        public float tapTime;
+        public float tapDelayTime;
+        public float tapRadiusSquared;
     }
 }

Packages/com.unity.inputsystem/InputSystem/InputManager.cs

@@ -2520,7 +2520,7 @@ private void InstallBeforeUpdateHookIfNecessary()
 
         private void RestoreDevicesAfterDomainReloadIfNecessary()
         {
-            #if UNITY_EDITOR
+            #if UNITY_EDITOR && !ENABLE_CORECLR
             if (m_SavedDeviceStates != null)
                 RestoreDevicesAfterDomainReload();
             #endif
@@ -2714,10 +2714,14 @@ internal void ApplySettings()
                 }
             }
 
-            // Cache some values.
-            Touchscreen.s_TapTime = settings.defaultTapTime;
-            Touchscreen.s_TapDelayTime = settings.multiTapDelayTime;
-            Touchscreen.s_TapRadiusSquared = settings.tapRadius * settings.tapRadius;
+            // Cache Touch specific settings to Touchscreen
+            Touchscreen.settings = new TouchscreenSettings
+            {
+                tapTime = settings.defaultTapTime,
+                tapDelayTime = settings.multiTapDelayTime,
+                tapRadiusSquared = settings.tapRadius * settings.tapRadius
+            };
+
             // Extra clamp here as we can't tell what we're getting from serialized data.
             ButtonControl.s_GlobalDefaultButtonPressPoint = Mathf.Clamp(settings.defaultButtonPressPoint, ButtonControl.kMinButtonPressPoint, float.MaxValue);
             ButtonControl.s_GlobalDefaultButtonReleaseThreshold = settings.buttonReleaseThreshold;
@@ -4009,6 +4013,7 @@ internal void RestoreStateWithoutDevices(SerializedState state)
         internal DeviceState[] m_SavedDeviceStates;
         internal AvailableDevice[] m_SavedAvailableDevices;
 
+#if !ENABLE_CORECLR
         /// <summary>
         /// Recreate devices based on the devices we had before a domain reload.
         /// </summary>
@@ -4092,6 +4097,29 @@ internal void RestoreDevicesAfterDomainReload()
             Profiler.EndSample();
         }
 
+        /// <summary>
+        /// Notifies all devices of removal to better cleanup data when using SimulateDomainReload test hook
+        /// </summary>
+        /// <remarks>
+        /// Devices maintain their own list of Devices within static fields, updated via NotifyAdded and NotifyRemoved overrides.
+        /// These fields are reset during a real DR, but not so when we "simulate" causing them to report incorrect values when
+        /// queried via direct APIs, e.g. Gamepad.all. So, to mitigate this we'll call NotifyRemove during this scenario.
+        /// </remarks>
+        internal void TestHook_RemoveDevicesForSimulatedDomainReload()
+        {
+            if (m_Devices == null)
+                return;
+
+            foreach (var device in m_Devices)
+            {
+                if (device == null)
+                    break;
+
+                device.NotifyRemoved();
+            }
+        }
+#endif // !ENABLE_CORECLR
+
         // We have two general types of devices we need to care about when recreating devices
         // after domain reloads:
         //

Packages/com.unity.inputsystem/InputSystem/InputSystemTestHooks.cs

@@ -34,19 +34,24 @@ internal static void TestHook_InitializeForPlayModeTests(bool enableRemoting, II
 #endif
         }
 
+#if !ENABLE_CORECLR
         internal static void TestHook_SimulateDomainReload(IInputRuntime runtime)
         {
             // This quite invasive goes into InputSystem internals. Unfortunately, we
             // have no proper way of simulating domain reloads ATM. So we directly call various
             // internal methods here in a sequence similar to what we'd get during a domain reload.
             // Since we're faking it, pass 'true' for calledFromCtor param.
 
+            // Need to notify devices of removal so their static fields are cleaned up
+            InputSystem.s_Manager.TestHook_RemoveDevicesForSimulatedDomainReload();
+
             InputSystem.s_DomainStateManager.OnBeforeSerialize();
             InputSystem.s_DomainStateManager = null;
             InputSystem.s_Manager = null; // Do NOT Dispose()! The native memory cannot be freed as it's reference by saved state
             InputSystem.s_PluginsInitialized = false;
             InputSystem.InitializeInEditor(true, runtime);
         }
+#endif
 #endif // UNITY_EDITOR
 
         /// <summary>

Packages/com.unity.inputsystem/InputSystem/NativeInputRuntime.cs

@@ -20,7 +20,26 @@ namespace UnityEngine.InputSystem.LowLevel
     /// </summary>
     internal class NativeInputRuntime : IInputRuntime
     {
-        public static readonly NativeInputRuntime instance = new NativeInputRuntime();
+        private static NativeInputRuntime s_Instance;
+
+        // Private ctor exists to enforce Singleton pattern
+        private NativeInputRuntime() { }
+
+        /// <summary>
+        /// Employ the Singleton pattern for this class and initialize a new instance on first use.
+        /// </summary>
+        /// <remarks>
+        /// This property is typically used to initialize InputManager and isn't used afterwards, i.e. there's
+        /// no perf impact to the null check.
+        /// </remarks>
+        public static NativeInputRuntime instance
+        {
+            get
+            {
+                s_Instance ??= new NativeInputRuntime();
+                return s_Instance;
+            }
+        }
 
         public int AllocateDeviceId()
         {

Packages/com.unity.inputsystem/InputSystem/Plugins/EnhancedTouch/TouchSimulation.cs

@@ -325,8 +325,8 @@ private unsafe void UpdateTouch(int touchIndex, int pointerIndex, TouchPhase pha
 
                 if (phase == TouchPhase.Ended)
                 {
-                    touch.isTap = time - oldTouchState->startTime <= Touchscreen.s_TapTime &&
-                        (position - oldTouchState->startPosition).sqrMagnitude <= Touchscreen.s_TapRadiusSquared;
+                    touch.isTap = time - oldTouchState->startTime <= Touchscreen.settings.tapTime &&
+                        (position - oldTouchState->startPosition).sqrMagnitude <= Touchscreen.settings.tapRadiusSquared;
                     if (touch.isTap)
                         ++touch.tapCount;
                 }

Packages/com.unity.inputsystem/InputSystem/Plugins/UI/InputSystemUIInputModule.cs

@@ -1378,7 +1378,8 @@ public InputActionReference trackedDevicePosition
 
         public void AssignDefaultActions()
         {
-            if (defaultActions == null)
+            // Without Domain Reloads, the InputActionAsset could be "null" even if defaultActions is valid
+            if (defaultActions == null || defaultActions.asset == null)
             {
                 defaultActions = new DefaultInputActions();
             }