diff --git a/Assets/Tests/InputSystem/Plugins/UITests.cs b/Assets/Tests/InputSystem/Plugins/UITests.cs index c770570b15..870fe1e018 100644 --- a/Assets/Tests/InputSystem/Plugins/UITests.cs +++ b/Assets/Tests/InputSystem/Plugins/UITests.cs @@ -11,9 +11,12 @@ using UnityEngine.InputSystem.LowLevel; using UnityEngine.InputSystem.UI; using UnityEngine.InputSystem.Utilities; +using UnityEngine.Profiling; using UnityEngine.TestTools; +using UnityEngine.TestTools.Constraints; using UnityEngine.UI; using TouchPhase = UnityEngine.InputSystem.TouchPhase; +using Is = UnityEngine.TestTools.Constraints.Is; #pragma warning disable CS0649 ////TODO: app focus handling @@ -113,8 +116,8 @@ private static TestObjects CreateScene(int minY = 0 , int maxY = 480) } [UnityTest] - [Category("Actions")] - public IEnumerator Actions_CanDriveUIFromMouse() + [Category("UI")] + public IEnumerator UI_CanDriveUIFromMouse() { // Create devices. var mouse = InputSystem.AddDevice(); @@ -254,8 +257,8 @@ public IEnumerator Actions_CanDriveUIFromMouse() } [UnityTest] - [Category("Actions")] - public IEnumerator Actions_TouchActionsCanDriveUIAndDistinguishMultipleTouches() + [Category("UI")] + public IEnumerator UI_TouchActionsCanDriveUIAndDistinguishMultipleTouches() { // Create devices. var touchScreen = InputSystem.AddDevice(); @@ -441,10 +444,10 @@ public IEnumerator Actions_TouchActionsCanDriveUIAndDistinguishMultipleTouches() } [UnityTest] - [Category("Actions")] + [Category("UI")] // Check that two players can have separate UI, and that both selections will stay active when // clicking on UI with the mouse, using MultiPlayerEventSystem.playerRoot to match UI to the players. - public IEnumerator Actions_CanOperateMultiplayerUIGloballyUsingMouse() + public IEnumerator UI_CanOperateMultiplayerUIGloballyUsingMouse() { var mouse = InputSystem.AddDevice(); @@ -523,10 +526,10 @@ public IEnumerator Actions_CanOperateMultiplayerUIGloballyUsingMouse() } [UnityTest] - [Category("Actions")] + [Category("UI")] // Check that two players can have separate UI and control it using separate gamepads, using // MultiplayerEventSystem. - public IEnumerator Actions_CanOperateMultiplayerUILocallyUsingGamepads() + public IEnumerator UI_CanOperateMultiplayerUILocallyUsingGamepads() { // Create devices. var gamepads = new[] { InputSystem.AddDevice(), InputSystem.AddDevice() }; @@ -638,8 +641,8 @@ public IEnumerator Actions_CanOperateMultiplayerUILocallyUsingGamepads() } [UnityTest] - [Category("Actions")] - public IEnumerator Actions_CanDriveUIFromGamepad() + [Category("UI")] + public IEnumerator UI_CanDriveUIFromGamepad() { // Create devices. var gamepad = InputSystem.AddDevice(); @@ -741,8 +744,8 @@ public IEnumerator Actions_CanDriveUIFromGamepad() } [Test] - [Category("Actions")] - public void Actions_CanReassignUIActions() + [Category("UI")] + public void UI_CanReassignUIActions() { var go = new GameObject(); go.AddComponent(); @@ -789,6 +792,46 @@ public void Actions_CanReassignUIActions() Assert.That(uiModule.point?.action, Is.Null); } + [Test] + [Category("UI")] + [Retry(2)] // Warm up JIT + public void UI_MovingAndClickingMouseDoesNotAllocateGCMemory() + { + var mouse = InputSystem.AddDevice(); + + var actions = ScriptableObject.CreateInstance(); + var uiActions = actions.AddActionMap("UI"); + var pointAction = uiActions.AddAction("Point", type: InputActionType.PassThrough, binding: "/position"); + var clickAction = uiActions.AddAction("Click", type: InputActionType.PassThrough, binding: "/leftButton"); + + actions.Enable(); + + var eventSystemGO = new GameObject(); + eventSystemGO.AddComponent(); + var uiModule = eventSystemGO.AddComponent(); + uiModule.actionsAsset = actions; + uiModule.point = InputActionReference.Create(pointAction); + uiModule.leftClick = InputActionReference.Create(clickAction); + + // We allow the first hit on the UI module to set up internal data structures + // and thus allocate something. So go and run one event with data on the mouse. + // Also gets rid of GC noise from the initial input system update. + InputSystem.QueueStateEvent(mouse, new MouseState { position = new Vector2(1, 2) }); + InputSystem.Update(); + + // Make sure we don't get an allocation from the string literal. + var kProfilerRegion = "UI_MovingAndClickingMouseDoesNotAllocateMemory"; + + Assert.That(() => + { + Profiler.BeginSample(kProfilerRegion); + Set(mouse.position, new Vector2(123, 234)); + Set(mouse.position, new Vector2(234, 345)); + Press(mouse.leftButton); + Profiler.EndSample(); + }, Is.Not.AllocatingGCMemory()); + } + // The tracked device tests fail with NullReferenceException in the windows editor on yamato. I cannot reproduce this locally, so will disable them on windows for now. #if !UNITY_EDITOR_WIN @@ -824,8 +867,8 @@ protected override void FinishSetup() } [UnityTest] - [Category("Actions")] - public IEnumerator Actions_CanDriveUIFromTrackedDevice() + [Category("UI")] + public IEnumerator UI_CanDriveUIFromTrackedDevice() { // Create device. InputSystem.RegisterLayout(); @@ -958,8 +1001,8 @@ public IEnumerator Actions_CanDriveUIFromTrackedDevice() } [UnityTest] - [Category("Actions")] - public IEnumerator Actions_CanDriveUIFromMultipleTrackedDevices() + [Category("UI")] + public IEnumerator UI_CanDriveUIFromMultipleTrackedDevices() { // Create device. InputSystem.RegisterLayout(); diff --git a/Packages/com.unity.inputsystem/CHANGELOG.md b/Packages/com.unity.inputsystem/CHANGELOG.md index 03d3ba3a95..c1825eff2b 100755 --- a/Packages/com.unity.inputsystem/CHANGELOG.md +++ b/Packages/com.unity.inputsystem/CHANGELOG.md @@ -12,6 +12,7 @@ however, it has to be formatted properly to pass verification tests. ### Fixed - XR controllers and HMDs have proper display names in the UI again. This regressed in preview.4 such that all XR controllers were displayed as just "XR Controller" in the UI and all HMDs were displayed as "XR HMD". +- `InputSystemUIInputModule` no longer generates GC heap garbage every time mouse events are processed. #### Actions diff --git a/Packages/com.unity.inputsystem/InputSystem/Plugins/UI/InputSystemUIInputModule.cs b/Packages/com.unity.inputsystem/InputSystem/Plugins/UI/InputSystemUIInputModule.cs index 715c1d5320..9c430e5e4c 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Plugins/UI/InputSystemUIInputModule.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Plugins/UI/InputSystemUIInputModule.cs @@ -1,10 +1,12 @@ using System; -using System.Collections.Generic; using UnityEngine.EventSystems; using UnityEngine.InputSystem.Controls; +using UnityEngine.InputSystem.Utilities; ////REVIEW: should each of the actions be *lists* of actions? +////FIXME: doesn't handle devices getting removed; will just keep device states forever + ////TODO: add ability to query which device was last used with any of the actions ////TODO: come up with an action response system that doesn't require hooking and unhooking all those delegates @@ -36,7 +38,7 @@ public override void ActivateModule() public override bool IsPointerOverGameObject(int pointerId) { - foreach (var state in mouseStates) + foreach (var state in m_MouseStates) { if (state.touchId == pointerId) return state.pointerTarget != null; @@ -72,7 +74,7 @@ internal void ProcessMouse(ref MouseModel mouseState) eventData.pointerCurrentRaycast = PerformRaycast(eventData); - /// Left Mouse Button + // Left Mouse Button // The left mouse button is 'dominant' and we want to also process hover and scroll events as if the occurred during the left click. var buttonState = mouseState.leftButton; buttonState.CopyTo(eventData); @@ -90,7 +92,7 @@ internal void ProcessMouse(ref MouseModel mouseState) buttonState.CopyFrom(eventData); mouseState.leftButton = buttonState; - /// Right Mouse Button + // Right Mouse Button buttonState = mouseState.rightButton; buttonState.CopyTo(eventData); eventData.button = PointerEventData.InputButton.Right; @@ -101,7 +103,7 @@ internal void ProcessMouse(ref MouseModel mouseState) buttonState.CopyFrom(eventData); mouseState.rightButton = buttonState; - /// Middle Mouse Button + // Middle Mouse Button buttonState = mouseState.middleButton; buttonState.CopyTo(eventData); eventData.button = PointerEventData.InputButton.Middle; @@ -119,13 +121,10 @@ internal void ProcessMouse(ref MouseModel mouseState) // not under the current MultiplayerEventSystem's root. private bool PointerShouldIgnoreTransform(Transform t) { - if (eventSystem is MultiplayerEventSystem) + if (eventSystem is MultiplayerEventSystem multiplayerEventSystem && multiplayerEventSystem.playerRoot != null) { - var mes = eventSystem as MultiplayerEventSystem; - - if (mes.playerRoot != null) - if (!t.IsChildOf(mes.playerRoot.transform)) - return true; + if (!t.IsChildOf(multiplayerEventSystem.playerRoot.transform)) + return true; } return false; } @@ -474,24 +473,27 @@ private AxisEventData GetOrCreateCachedAxisEvent() public float repeatDelay { - get { return m_RepeatDelay; } - set { m_RepeatDelay = value; } + get => m_RepeatDelay; + set => m_RepeatDelay = value; } public float repeatRate { - get { return m_RepeatRate; } - set { m_RepeatRate = value; } + get => m_RepeatRate; + set => m_RepeatRate = value; } public float trackedDeviceDragThresholdMultiplier { - get { return m_TrackedDeviceDragThresholdMultiplier; } - set { m_TrackedDeviceDragThresholdMultiplier = value; } + get => m_TrackedDeviceDragThresholdMultiplier; + set => m_TrackedDeviceDragThresholdMultiplier = value; } - private static void SwapAction(ref InputActionReference property, InputActionReference newValue, bool actionsHooked, Action actionCallback) + private void SwapAction(ref InputActionReference property, InputActionReference newValue, bool actionsHooked, Action actionCallback) { + if (property == newValue || (property != null && newValue != null && property.action == newValue.action)) + return; + if (property != null && actionsHooked) { property.action.performed -= actionCallback; @@ -500,6 +502,14 @@ private static void SwapAction(ref InputActionReference property, InputActionRef property = newValue; + #if DEBUG + if (newValue != null && newValue.action != null && newValue.action.type != InputActionType.PassThrough) + { + Debug.LogWarning("Actions used with the UI input module should generally be set to Pass-Through type so that the module can properly distinguish between " + + $"input from multiple devices (action {newValue.action} is set to {newValue.action.type})", this); + } + #endif + if (newValue != null && actionsHooked) { property.action.performed += actionCallback; @@ -637,7 +647,7 @@ protected override void OnDisable() UnhookActions(); } - bool IsAnyActionEnabled() + private bool IsAnyActionEnabled() { return (m_PointAction?.action?.enabled ?? true) && (m_LeftClickAction?.action?.enabled ?? true) && @@ -652,7 +662,6 @@ bool IsAnyActionEnabled() (m_TrackedDeviceSelectAction?.action?.enabled ?? true); } - bool m_OwnsEnabledState; /// /// This is a quick accessor for enabling all actions. Currently, action ownership is ambiguous, /// and we need a way to enable/disable inspector-set actions. @@ -699,86 +708,85 @@ public void DisableAllActions() } } - int GetTrackedDeviceIndexForCallbackContext(InputAction.CallbackContext context) + private int GetTrackedDeviceIndexForCallbackContext(InputAction.CallbackContext context) { - Debug.Assert(context.action.type == InputActionType.PassThrough, $"XR actions should be pass-through actions, so the UI can properly distinguish multiple tracked devices. Please set the action type of '{context.action.name}' to 'Pass-Through'."); - for (var i = 0; i < trackedDeviceStates.Count; i++) + for (var i = 0; i < m_TrackedDeviceStatesCount; i++) { - if (trackedDeviceStates[i].device == context.control.device) + if (m_TrackedDeviceStates[i].device == context.control.device) return i; } - trackedDeviceStates.Add(new TrackedDeviceModel(m_RollingPointerId++, context.control.device)); - return trackedDeviceStates.Count - 1; + + return ArrayHelpers.AppendWithCapacity(ref m_TrackedDeviceStates, ref m_TrackedDeviceStatesCount, + new TrackedDeviceModel(m_RollingPointerId++, context.control.device)); } - int GetMouseDeviceIndexForCallbackContext(InputAction.CallbackContext context) + private int GetMouseDeviceIndexForCallbackContext(InputAction.CallbackContext context) { - Debug.Assert(context.action.type == InputActionType.PassThrough, $"Pointer actions should be pass-through actions, so the UI can properly distinguish multiple pointing devices/fingers. Please set the action type of '{context.action.name}' to 'Pass-Through'."); var touchId = PointerInputModule.kMouseLeftId; - if (context.control.parent is TouchControl) - touchId = ((TouchControl)context.control.parent).touchId.ReadValue(); + if (context.control.parent is TouchControl touchControl) + touchId = touchControl.touchId.ReadValue(); - for (var i = 0; i < mouseStates.Count; i++) + for (var i = 0; i < m_MouseStates.length; i++) { - if (mouseStates[i].device == context.control.device && mouseStates[i].touchId == touchId) + if (m_MouseStates[i].device == context.control.device && m_MouseStates[i].touchId == touchId) return i; } - mouseStates.Add(new MouseModel(m_RollingPointerId++, context.control.device, touchId)); - return mouseStates.Count - 1; + + return m_MouseStates.AppendWithCapacity(new MouseModel(m_RollingPointerId++, context.control.device, touchId)); } - void OnAction(InputAction.CallbackContext context) + private void OnAction(InputAction.CallbackContext context) { var action = context.action; if (action == m_PointAction?.action) { var index = GetMouseDeviceIndexForCallbackContext(context); - var state = mouseStates[index]; + var state = m_MouseStates[index]; state.position = context.ReadValue(); - mouseStates[index] = state; + m_MouseStates[index] = state; } else if (action == m_ScrollWheelAction?.action) { var index = GetMouseDeviceIndexForCallbackContext(context); - var state = mouseStates[index]; + var state = m_MouseStates[index]; // The old input system reported scroll deltas in lines, we report pixels. // Need to scale as the UI system expects lines. const float kPixelPerLine = 20; state.scrollDelta = context.ReadValue() * (1.0f / kPixelPerLine); - mouseStates[index] = state; + m_MouseStates[index] = state; } else if (action == m_LeftClickAction?.action) { var index = GetMouseDeviceIndexForCallbackContext(context); - var state = mouseStates[index]; + var state = m_MouseStates[index]; var buttonState = state.leftButton; buttonState.isDown = context.ReadValue() > 0; buttonState.clickCount = (context.control.device as Mouse)?.clickCount.ReadValue() ?? 0; state.leftButton = buttonState; - mouseStates[index] = state; + m_MouseStates[index] = state; } else if (action == m_RightClickAction?.action) { var index = GetMouseDeviceIndexForCallbackContext(context); - var state = mouseStates[index]; + var state = m_MouseStates[index]; var buttonState = state.rightButton; buttonState.isDown = context.ReadValue() > 0; buttonState.clickCount = (context.control.device as Mouse)?.clickCount.ReadValue() ?? 0; state.rightButton = buttonState; - mouseStates[index] = state; + m_MouseStates[index] = state; } else if (action == m_MiddleClickAction?.action) { var index = GetMouseDeviceIndexForCallbackContext(context); - var state = mouseStates[index]; + var state = m_MouseStates[index]; var buttonState = state.middleButton; buttonState.isDown = context.ReadValue() > 0; buttonState.clickCount = (context.control.device as Mouse)?.clickCount.ReadValue() ?? 0; state.middleButton = buttonState; - mouseStates[index] = state; + m_MouseStates[index] = state; } else if (action == m_MoveAction?.action) { @@ -795,23 +803,23 @@ void OnAction(InputAction.CallbackContext context) else if (action == m_TrackedDeviceOrientationAction?.action) { var index = GetTrackedDeviceIndexForCallbackContext(context); - var state = trackedDeviceStates[index]; + var state = m_TrackedDeviceStates[index]; state.orientation = context.ReadValue(); - trackedDeviceStates[index] = state; + m_TrackedDeviceStates[index] = state; } else if (action == m_TrackedDevicePositionAction?.action) { var index = GetTrackedDeviceIndexForCallbackContext(context); - var state = trackedDeviceStates[index]; + var state = m_TrackedDeviceStates[index]; state.position = context.ReadValue(); - trackedDeviceStates[index] = state; + m_TrackedDeviceStates[index] = state; } else if (action == m_TrackedDeviceSelectAction?.action) { var index = GetTrackedDeviceIndexForCallbackContext(context); - var state = trackedDeviceStates[index]; + var state = m_TrackedDeviceStates[index]; state.select = context.ReadValue() > 0; - trackedDeviceStates[index] = state; + m_TrackedDeviceStates[index] = state; } } @@ -826,27 +834,27 @@ private void DoProcess() if (!eventSystem.isFocused) { joystickState.OnFrameFinished(); - foreach (var mouseState in mouseStates) - mouseState.OnFrameFinished(); - foreach (var trackedDeviceState in trackedDeviceStates) - trackedDeviceState.OnFrameFinished(); + for (var i = 0; i < m_MouseStates.length; ++i) + m_MouseStates[i].OnFrameFinished(); + for (var i = 0; i < m_TrackedDeviceStatesCount; ++i) + m_TrackedDeviceStates[i].OnFrameFinished(); } else { ProcessJoystick(ref joystickState); - for (var i = 0; i < mouseStates.Count; i++) + for (var i = 0; i < m_MouseStates.length; i++) { - var state = mouseStates[i]; + var state = m_MouseStates[i]; ProcessMouse(ref state); - mouseStates[i] = state; + m_MouseStates[i] = state; } - for (var i = 0; i < trackedDeviceStates.Count; i++) + for (var i = 0; i < m_TrackedDeviceStatesCount; i++) { - var state = trackedDeviceStates[i]; + var state = m_TrackedDeviceStates[i]; ProcessTrackedDevice(ref state); - trackedDeviceStates[i] = state; + m_TrackedDeviceStates[i] = state; } } } @@ -934,8 +942,6 @@ private InputActionReference UpdateReferenceForNewAsset(InputActionReference act return InputActionReference.Create(newAction); } - [SerializeField, HideInInspector] private InputActionAsset m_ActionsAsset; - public InputActionAsset actionsAsset { get => m_ActionsAsset; @@ -961,17 +967,7 @@ public InputActionAsset actionsAsset } } - /// - /// An delivering a 2D screen position - /// used as a cursor for pointing at UI elements. - /// - [SerializeField, HideInInspector] private InputActionReference m_PointAction; - - /// - /// An delivering a 2D motion vector - /// used for sending events. - /// [SerializeField, HideInInspector] private InputActionReference m_MoveAction; [SerializeField, HideInInspector] private InputActionReference m_SubmitAction; [SerializeField, HideInInspector] private InputActionReference m_CancelAction; @@ -984,12 +980,16 @@ public InputActionAsset actionsAsset [SerializeField, HideInInspector] private InputActionReference m_TrackedDeviceOrientationAction; [SerializeField, HideInInspector] private InputActionReference m_TrackedDeviceSelectAction; - [NonSerialized] private int m_RollingPointerId; - [NonSerialized] private bool m_ActionsHooked; - [NonSerialized] private Action m_OnActionDelegate; + [SerializeField, HideInInspector] private InputActionAsset m_ActionsAsset; + + private int m_RollingPointerId; + private bool m_OwnsEnabledState; + private bool m_ActionsHooked; + private Action m_OnActionDelegate; - [NonSerialized] private JoystickModel joystickState; - [NonSerialized] private List trackedDeviceStates = new List(); - [NonSerialized] private List mouseStates = new List(); + private JoystickModel joystickState; + private int m_TrackedDeviceStatesCount; + private TrackedDeviceModel[] m_TrackedDeviceStates; + private InlinedArray m_MouseStates; } }