Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 495af61

Browse filesBrowse files
timkeotimkeo
committed
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()
1 parent 1bcabff commit 495af61
Copy full SHA for 495af61

File tree

Expand file treeCollapse file tree

9 files changed

+137
-54
lines changed
Filter options
Expand file treeCollapse file tree

9 files changed

+137
-54
lines changed

‎Assets/Tests/InputSystem/CoreTests_Actions.cs

Copy file name to clipboardExpand all lines: Assets/Tests/InputSystem/CoreTests_Actions.cs
+49-37Lines changed: 49 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -5223,56 +5223,68 @@ public class ModificationCases : IEnumerable
52235223
[Preserve]
52245224
public ModificationCases() {}
52255225

5226+
private static readonly Modification[] ModificationAppliesToSingleActionMap =
5227+
{
5228+
Modification.AddBinding,
5229+
Modification.RemoveBinding,
5230+
Modification.ModifyBinding,
5231+
Modification.ApplyBindingOverride,
5232+
Modification.AddAction,
5233+
Modification.RemoveAction,
5234+
Modification.ChangeBindingMask,
5235+
Modification.AddDevice,
5236+
Modification.RemoveDevice,
5237+
Modification.AddDeviceGlobally,
5238+
Modification.RemoveDeviceGlobally,
5239+
// Excludes: AddMap, RemoveMap
5240+
};
5241+
5242+
private static readonly Modification[] ModificationAppliesToSingletonAction =
5243+
{
5244+
Modification.AddBinding,
5245+
Modification.RemoveBinding,
5246+
Modification.ModifyBinding,
5247+
Modification.ApplyBindingOverride,
5248+
Modification.AddDeviceGlobally,
5249+
Modification.RemoveDeviceGlobally,
5250+
};
5251+
52265252
public IEnumerator GetEnumerator()
52275253
{
5228-
bool ModificationAppliesToSingletonAction(Modification modification)
5254+
// NOTE: This executes *outside* of our test fixture during test discovery.
5255+
5256+
// We cannot directly create the InputAction objects within GetEnumerator() because the underlying
5257+
// asset object might be invalid by the time the tests are actually run.
5258+
//
5259+
// That is, NUnit TestCases are generated once when the Assembly is loaded and will persist until it's unloaded,
5260+
// meaning they'll never be recreated without a Domain Reload. However, since InputActionAsset is a ScriptableObject,
5261+
// it could be deleted or otherwise invalidated between test case creation and actual test execution.
5262+
//
5263+
// So, instead we'll create a delegate to create the Actions object as the parameter for each test case, allowing
5264+
// the test case to create an Actions object itself when it actually runs.
52295265
{
5230-
switch (modification)
5266+
var actionsFromAsset = new Func<IInputActionCollection2>(() => new DefaultInputActions().asset);
5267+
foreach (var value in Enum.GetValues(typeof(Modification)))
52315268
{
5232-
case Modification.AddBinding:
5233-
case Modification.RemoveBinding:
5234-
case Modification.ModifyBinding:
5235-
case Modification.ApplyBindingOverride:
5236-
case Modification.AddDeviceGlobally:
5237-
case Modification.RemoveDeviceGlobally:
5238-
return true;
5269+
yield return new TestCaseData(value, actionsFromAsset);
52395270
}
5240-
return false;
52415271
}
52425272

5243-
bool ModificationAppliesToSingleActionMap(Modification modification)
52445273
{
5245-
switch (modification)
5274+
var actionMap = new Func<IInputActionCollection2>(CreateMap);
5275+
foreach (var value in Enum.GetValues(typeof(Modification)))
52465276
{
5247-
case Modification.AddMap:
5248-
case Modification.RemoveMap:
5249-
return false;
5277+
if (ModificationAppliesToSingleActionMap.Contains((Modification)value))
5278+
yield return new TestCaseData(value, actionMap);
52505279
}
5251-
return true;
52525280
}
52535281

5254-
// NOTE: This executes *outside* of our test fixture during test discovery.
5255-
5256-
// Creates a matrix of all permutations of Modifications combined with assets, maps, and singleton actions.
5257-
foreach (var func in new Func<IInputActionCollection2>[] { () => new DefaultInputActions().asset, CreateMap, CreateSingletonAction })
52585282
{
5283+
var singletonMap = new Func<IInputActionCollection2>(CreateSingletonAction);
52595284
foreach (var value in Enum.GetValues(typeof(Modification)))
52605285
{
5261-
var actions = func();
5262-
if (actions is InputActionMap map)
5263-
{
5264-
if (map.m_SingletonAction != null)
5265-
{
5266-
if (!ModificationAppliesToSingletonAction((Modification)value))
5267-
continue;
5268-
}
5269-
else if (!ModificationAppliesToSingleActionMap((Modification)value))
5270-
{
5271-
continue;
5272-
}
5273-
}
5274-
5275-
yield return new TestCaseData(value, actions);
5286+
if (ModificationAppliesToSingletonAction.Contains((Modification)value))
5287+
yield return new TestCaseData(value, singletonMap);
52765288
}
52775289
}
52785290
}
@@ -5303,14 +5315,14 @@ private InputActionMap CreateSingletonAction()
53035315
[Test]
53045316
[Category("Actions")]
53055317
[TestCaseSource(typeof(ModificationCases))]
5306-
public void Actions_CanHandleModification(Modification modification, IInputActionCollection2 actions)
5318+
public void Actions_CanHandleModification(Modification modification, Func<IInputActionCollection2> getActions)
53075319
{
53085320
#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
53095321
// Exclude project-wide actions from this test
53105322
InputSystem.actions?.Disable();
53115323
InputActionState.DestroyAllActionMapStates(); // Required for `onActionChange` to report correct number of changes
53125324
#endif
5313-
5325+
var actions = getActions();
53145326
var gamepad = InputSystem.AddDevice<Gamepad>();
53155327

53165328
if (modification == Modification.AddDevice || modification == Modification.RemoveDevice)

‎Assets/Tests/InputSystem/CoreTests_Editor.cs

Copy file name to clipboardExpand all lines: Assets/Tests/InputSystem/CoreTests_Editor.cs
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,7 @@ public void Editor_CanSaveAndRestoreState()
165165
Assert.That(unsupportedDevices[0].interfaceName, Is.EqualTo("Test"));
166166
}
167167

168+
#if !ENABLE_CORECLR
168169
// onFindLayoutForDevice allows dynamically injecting new layouts into the system that
169170
// are custom-tailored at runtime for the discovered device. Make sure that our domain
170171
// reload can restore these.
@@ -345,6 +346,7 @@ public void Editor_DomainReload_CanRemoveDevicesDuringDomainReload()
345346
Assert.That(InputSystem.devices, Has.Count.EqualTo(1));
346347
Assert.That(InputSystem.devices[0], Is.AssignableTo<Keyboard>());
347348
}
349+
#endif // !ENABLE_CORECLR
348350

349351
[Test]
350352
[Category("Editor")]

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

Copy file name to clipboardExpand all lines: Packages/com.unity.inputsystem/InputSystem/Controls/InputControlLayout.cs
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ public delegate string InputDeviceFindControlLayoutDelegate(ref InputDeviceDescr
109109
/// </remarks>
110110
public class InputControlLayout
111111
{
112-
private static InternedString s_DefaultVariant = new InternedString("Default");
112+
private static readonly InternedString s_DefaultVariant = new InternedString("Default");
113113
public static InternedString DefaultVariant => s_DefaultVariant;
114114

115115
public const string VariantSeparator = ";";

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

Copy file name to clipboardExpand all lines: Packages/com.unity.inputsystem/InputSystem/Devices/Touchscreen.cs
+23-7Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -534,6 +534,14 @@ protected TouchControl[] touchControlArray
534534
/// <value>Current touch screen.</value>
535535
public new static Touchscreen current { get; internal set; }
536536

537+
/// <summary>
538+
/// The current global settings for Touchscreen devices.
539+
/// </summary>
540+
/// <remarks>
541+
/// These are cached values taken from <see cref="InputSettings"/>.
542+
/// </remarks>
543+
internal static TouchscreenSettings settings { get; set; }
544+
537545
/// <inheritdoc />
538546
public override void MakeCurrent()
539547
{
@@ -624,14 +632,14 @@ protected override void FinishSetup()
624632
// that to do so we would have to add another record to keep track of timestamps for each touch. And
625633
// since we know the maximum time that a tap can take, we have a reasonable estimate for when a prior
626634
// tap must have ended.
627-
if (touchStatePtr->tapCount > 0 && InputState.currentTime >= touchStatePtr->startTime + s_TapTime + s_TapDelayTime)
635+
if (touchStatePtr->tapCount > 0 && InputState.currentTime >= touchStatePtr->startTime + settings.tapTime + settings.tapDelayTime)
628636
InputState.Change(touches[i].tapCount, (byte)0);
629637
}
630638

631639
var primaryTouchState = (TouchState*)((byte*)statePtr + stateBlock.byteOffset);
632640
if (primaryTouchState->delta != default)
633641
InputState.Change(primaryTouch.delta, Vector2.zero);
634-
if (primaryTouchState->tapCount > 0 && InputState.currentTime >= primaryTouchState->startTime + s_TapTime + s_TapDelayTime)
642+
if (primaryTouchState->tapCount > 0 && InputState.currentTime >= primaryTouchState->startTime + settings.tapTime + settings.tapDelayTime)
635643
InputState.Change(primaryTouch.tapCount, (byte)0);
636644

637645
Profiler.EndSample();
@@ -720,11 +728,11 @@ protected override void FinishSetup()
720728

721729
// Detect taps.
722730
var isTap = newTouchState.isNoneEndedOrCanceled &&
723-
(eventPtr.time - newTouchState.startTime) <= s_TapTime &&
731+
(eventPtr.time - newTouchState.startTime) <= settings.tapTime &&
724732
////REVIEW: this only takes the final delta to start position into account, not the delta over the lifetime of the
725733
//// touch; is this robust enough or do we need to make sure that we never move more than the tap radius
726734
//// over the entire lifetime of the touch?
727-
(newTouchState.position - newTouchState.startPosition).sqrMagnitude <= s_TapRadiusSquared;
735+
(newTouchState.position - newTouchState.startPosition).sqrMagnitude <= settings.tapRadiusSquared;
728736
if (isTap)
729737
newTouchState.tapCount = (byte)(currentTouchState[i].tapCount + 1);
730738
else
@@ -1044,8 +1052,16 @@ private static void TriggerTap(TouchControl control, ref TouchState state, Input
10441052
state.isTapRelease = false;
10451053
}
10461054

1047-
internal static float s_TapTime;
1048-
internal static float s_TapDelayTime;
1049-
internal static float s_TapRadiusSquared;
1055+
private static TouchscreenSettings s_Settings;
1056+
}
1057+
1058+
/// <summary>
1059+
/// Cached settings retrieved from <see cref="InputSettings"/>.
1060+
/// </summary>
1061+
internal struct TouchscreenSettings
1062+
{
1063+
public float tapTime;
1064+
public float tapDelayTime;
1065+
public float tapRadiusSquared;
10501066
}
10511067
}

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

Copy file name to clipboardExpand all lines: Packages/com.unity.inputsystem/InputSystem/InputManager.cs
+33-5Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2519,7 +2519,7 @@ private void InstallBeforeUpdateHookIfNecessary()
25192519

25202520
private void RestoreDevicesAfterDomainReloadIfNecessary()
25212521
{
2522-
#if UNITY_EDITOR
2522+
#if UNITY_EDITOR && !ENABLE_CORECLR
25232523
if (m_SavedDeviceStates != null)
25242524
RestoreDevicesAfterDomainReload();
25252525
#endif
@@ -2713,10 +2713,14 @@ internal void ApplySettings()
27132713
}
27142714
}
27152715

2716-
// Cache some values.
2717-
Touchscreen.s_TapTime = settings.defaultTapTime;
2718-
Touchscreen.s_TapDelayTime = settings.multiTapDelayTime;
2719-
Touchscreen.s_TapRadiusSquared = settings.tapRadius * settings.tapRadius;
2716+
// Cache Touch specific settings to Touchscreen
2717+
Touchscreen.settings = new TouchscreenSettings
2718+
{
2719+
tapTime = settings.defaultTapTime,
2720+
tapDelayTime = settings.multiTapDelayTime,
2721+
tapRadiusSquared = settings.tapRadius * settings.tapRadius
2722+
};
2723+
27202724
// Extra clamp here as we can't tell what we're getting from serialized data.
27212725
ButtonControl.s_GlobalDefaultButtonPressPoint = Mathf.Clamp(settings.defaultButtonPressPoint, ButtonControl.kMinButtonPressPoint, float.MaxValue);
27222726
ButtonControl.s_GlobalDefaultButtonReleaseThreshold = settings.buttonReleaseThreshold;
@@ -3957,6 +3961,7 @@ internal void RestoreStateWithoutDevices(SerializedState state)
39573961
internal DeviceState[] m_SavedDeviceStates;
39583962
internal AvailableDevice[] m_SavedAvailableDevices;
39593963

3964+
#if !ENABLE_CORECLR
39603965
/// <summary>
39613966
/// Recreate devices based on the devices we had before a domain reload.
39623967
/// </summary>
@@ -4040,6 +4045,29 @@ internal void RestoreDevicesAfterDomainReload()
40404045
Profiler.EndSample();
40414046
}
40424047

4048+
/// <summary>
4049+
/// Notifies all devices of removal to better cleanup data when using SimulateDomainReload test hook
4050+
/// </summary>
4051+
/// <remarks>
4052+
/// Devices maintain their own list of Devices within static fields, updated via NotifyAdded and NotifyRemoved overrides.
4053+
/// These fields are reset during a real DR, but not so when we "simulate" causing them to report incorrect values when
4054+
/// queried via direct APIs, e.g. Gamepad.all. So, to mitigate this we'll call NotifyRemove during this scenario.
4055+
/// </remarks>
4056+
internal void TestHook_RemoveDevicesForSimulatedDomainReload()
4057+
{
4058+
if (m_Devices == null)
4059+
return;
4060+
4061+
foreach (var device in m_Devices)
4062+
{
4063+
if (device == null)
4064+
break;
4065+
4066+
device.NotifyRemoved();
4067+
}
4068+
}
4069+
#endif // !ENABLE_CORECLR
4070+
40434071
// We have two general types of devices we need to care about when recreating devices
40444072
// after domain reloads:
40454073
//

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

Copy file name to clipboardExpand all lines: Packages/com.unity.inputsystem/InputSystem/InputSystemTestHooks.cs
+5Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,19 +34,24 @@ internal static void TestHook_InitializeForPlayModeTests(bool enableRemoting, II
3434
#endif
3535
}
3636

37+
#if !ENABLE_CORECLR
3738
internal static void TestHook_SimulateDomainReload(IInputRuntime runtime)
3839
{
3940
// This quite invasive goes into InputSystem internals. Unfortunately, we
4041
// have no proper way of simulating domain reloads ATM. So we directly call various
4142
// internal methods here in a sequence similar to what we'd get during a domain reload.
4243
// Since we're faking it, pass 'true' for calledFromCtor param.
4344

45+
// Need to notify devices of removal so their static fields are cleaned up
46+
InputSystem.s_Manager.TestHook_RemoveDevicesForSimulatedDomainReload();
47+
4448
InputSystem.s_DomainStateManager.OnBeforeSerialize();
4549
InputSystem.s_DomainStateManager = null;
4650
InputSystem.s_Manager = null; // Do NOT Dispose()! The native memory cannot be freed as it's reference by saved state
4751
InputSystem.s_PluginsInitialized = false;
4852
InputSystem.InitializeInEditor(true, runtime);
4953
}
54+
#endif
5055
#endif // UNITY_EDITOR
5156

5257
/// <summary>

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

Copy file name to clipboardExpand all lines: Packages/com.unity.inputsystem/InputSystem/NativeInputRuntime.cs
+20-1Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,26 @@ namespace UnityEngine.InputSystem.LowLevel
2020
/// </summary>
2121
internal class NativeInputRuntime : IInputRuntime
2222
{
23-
public static readonly NativeInputRuntime instance = new NativeInputRuntime();
23+
private static NativeInputRuntime s_Instance;
24+
25+
// Private ctor exists to enforce Singleton pattern
26+
private NativeInputRuntime() { }
27+
28+
/// <summary>
29+
/// Employ the Singleton pattern for this class and initialize a new instance on first use.
30+
/// </summary>
31+
/// <remarks>
32+
/// This property is typically used to initialize InputManager and isn't used afterwards, i.e. there's
33+
/// no perf impact to the null check.
34+
/// </remarks>
35+
public static NativeInputRuntime instance
36+
{
37+
get
38+
{
39+
s_Instance ??= new NativeInputRuntime();
40+
return s_Instance;
41+
}
42+
}
2443

2544
public int AllocateDeviceId()
2645
{

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

Copy file name to clipboardExpand all lines: Packages/com.unity.inputsystem/InputSystem/Plugins/EnhancedTouch/TouchSimulation.cs
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -325,8 +325,8 @@ private unsafe void UpdateTouch(int touchIndex, int pointerIndex, TouchPhase pha
325325

326326
if (phase == TouchPhase.Ended)
327327
{
328-
touch.isTap = time - oldTouchState->startTime <= Touchscreen.s_TapTime &&
329-
(position - oldTouchState->startPosition).sqrMagnitude <= Touchscreen.s_TapRadiusSquared;
328+
touch.isTap = time - oldTouchState->startTime <= Touchscreen.settings.tapTime &&
329+
(position - oldTouchState->startPosition).sqrMagnitude <= Touchscreen.settings.tapRadiusSquared;
330330
if (touch.isTap)
331331
++touch.tapCount;
332332
}

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

Copy file name to clipboardExpand all lines: Packages/com.unity.inputsystem/InputSystem/Plugins/UI/InputSystemUIInputModule.cs
+2-1Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1378,7 +1378,8 @@ public InputActionReference trackedDevicePosition
13781378

13791379
public void AssignDefaultActions()
13801380
{
1381-
if (defaultActions == null)
1381+
// Without Domain Reloads, the InputActionAsset could be "null" even if defaultActions is valid
1382+
if (defaultActions == null || defaultActions.asset == null)
13821383
{
13831384
defaultActions = new DefaultInputActions();
13841385
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.