From 75255273ae217acf86b54e8717701899cac15477 Mon Sep 17 00:00:00 2001 From: Tim Keosababian <36088732+timkeo@users.noreply.github.com> Date: Thu, 18 Apr 2024 14:36:51 -0700 Subject: [PATCH] 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() --- Assets/Tests/InputSystem/CoreTests_Actions.cs | 86 +++++++++++-------- Assets/Tests/InputSystem/CoreTests_Editor.cs | 2 + .../Controls/InputControlLayout.cs | 2 +- .../InputSystem/Devices/Touchscreen.cs | 30 +++++-- .../InputSystem/InputManager.cs | 38 ++++++-- .../InputSystem/InputSystemTestHooks.cs | 5 ++ .../InputSystem/NativeInputRuntime.cs | 21 ++++- .../Plugins/EnhancedTouch/TouchSimulation.cs | 4 +- .../Plugins/UI/InputSystemUIInputModule.cs | 3 +- 9 files changed, 137 insertions(+), 54 deletions(-) diff --git a/Assets/Tests/InputSystem/CoreTests_Actions.cs b/Assets/Tests/InputSystem/CoreTests_Actions.cs index 8fb9d0256b..af9674deb6 100644 --- a/Assets/Tests/InputSystem/CoreTests_Actions.cs +++ b/Assets/Tests/InputSystem/CoreTests_Actions.cs @@ -5223,56 +5223,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(() => 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(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[] { () => new DefaultInputActions().asset, CreateMap, CreateSingletonAction }) { + var singletonMap = new Func(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); } } } @@ -5303,14 +5315,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 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(); if (modification == Modification.AddDevice || modification == Modification.RemoveDevice) diff --git a/Assets/Tests/InputSystem/CoreTests_Editor.cs b/Assets/Tests/InputSystem/CoreTests_Editor.cs index 88ba90641c..c08b9cb650 100644 --- a/Assets/Tests/InputSystem/CoreTests_Editor.cs +++ b/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()); } +#endif // !ENABLE_CORECLR [Test] [Category("Editor")] diff --git a/Packages/com.unity.inputsystem/InputSystem/Controls/InputControlLayout.cs b/Packages/com.unity.inputsystem/InputSystem/Controls/InputControlLayout.cs index 501378afdc..ad8923044f 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Controls/InputControlLayout.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Controls/InputControlLayout.cs @@ -109,7 +109,7 @@ public delegate string InputDeviceFindControlLayoutDelegate(ref InputDeviceDescr /// 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 = ";"; diff --git a/Packages/com.unity.inputsystem/InputSystem/Devices/Touchscreen.cs b/Packages/com.unity.inputsystem/InputSystem/Devices/Touchscreen.cs index c119cb05c6..748198f11b 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Devices/Touchscreen.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Devices/Touchscreen.cs @@ -534,6 +534,14 @@ protected TouchControl[] touchControlArray /// Current touch screen. public new static Touchscreen current { get; internal set; } + /// + /// The current global settings for Touchscreen devices. + /// + /// + /// These are cached values taken from . + /// + internal static TouchscreenSettings settings { get; set; } + /// 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; + } + + /// + /// Cached settings retrieved from . + /// + internal struct TouchscreenSettings + { + public float tapTime; + public float tapDelayTime; + public float tapRadiusSquared; } } diff --git a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs index c3a67bcddc..a467b438d8 100644 --- a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs +++ b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs @@ -2519,7 +2519,7 @@ private void InstallBeforeUpdateHookIfNecessary() private void RestoreDevicesAfterDomainReloadIfNecessary() { - #if UNITY_EDITOR + #if UNITY_EDITOR && !ENABLE_CORECLAR if (m_SavedDeviceStates != null) RestoreDevicesAfterDomainReload(); #endif @@ -2713,10 +2713,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; @@ -3957,6 +3961,7 @@ internal void RestoreStateWithoutDevices(SerializedState state) internal DeviceState[] m_SavedDeviceStates; internal AvailableDevice[] m_SavedAvailableDevices; +#if !ENABLE_CORECLR /// /// Recreate devices based on the devices we had before a domain reload. /// @@ -4040,6 +4045,29 @@ internal void RestoreDevicesAfterDomainReload() Profiler.EndSample(); } + /// + /// Notifies all devices of removal to better cleanup data when using SimulateDomainReload test hook + /// + /// + /// 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. + /// + 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: // diff --git a/Packages/com.unity.inputsystem/InputSystem/InputSystemTestHooks.cs b/Packages/com.unity.inputsystem/InputSystem/InputSystemTestHooks.cs index 1ac0be8b62..3f2ed56f46 100644 --- a/Packages/com.unity.inputsystem/InputSystem/InputSystemTestHooks.cs +++ b/Packages/com.unity.inputsystem/InputSystem/InputSystemTestHooks.cs @@ -34,6 +34,7 @@ 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 @@ -41,12 +42,16 @@ internal static void TestHook_SimulateDomainReload(IInputRuntime runtime) // 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 /// diff --git a/Packages/com.unity.inputsystem/InputSystem/NativeInputRuntime.cs b/Packages/com.unity.inputsystem/InputSystem/NativeInputRuntime.cs index 6bba685046..217df9bf88 100644 --- a/Packages/com.unity.inputsystem/InputSystem/NativeInputRuntime.cs +++ b/Packages/com.unity.inputsystem/InputSystem/NativeInputRuntime.cs @@ -20,7 +20,26 @@ namespace UnityEngine.InputSystem.LowLevel /// 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() { } + + /// + /// Employ the Singleton pattern for this class and initialize a new instance on first use. + /// + /// + /// This property is typically used to initialize InputManager and isn't used afterwards, i.e. there's + /// no perf impact to the null check. + /// + public static NativeInputRuntime instance + { + get + { + s_Instance ??= new NativeInputRuntime(); + return s_Instance; + } + } public int AllocateDeviceId() { diff --git a/Packages/com.unity.inputsystem/InputSystem/Plugins/EnhancedTouch/TouchSimulation.cs b/Packages/com.unity.inputsystem/InputSystem/Plugins/EnhancedTouch/TouchSimulation.cs index f8de763e49..d25132fc43 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Plugins/EnhancedTouch/TouchSimulation.cs +++ b/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; } diff --git a/Packages/com.unity.inputsystem/InputSystem/Plugins/UI/InputSystemUIInputModule.cs b/Packages/com.unity.inputsystem/InputSystem/Plugins/UI/InputSystemUIInputModule.cs index 66c4f95bb1..c19026e397 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Plugins/UI/InputSystemUIInputModule.cs +++ b/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(); }