From 5c4af8ac8875ab78af88372bfa000ea0a07bedd6 Mon Sep 17 00:00:00 2001 From: Rene Damm Date: Wed, 29 Jan 2020 13:24:51 +0100 Subject: [PATCH] FIX: GC heap garbage from mouse input in UI. --- Assets/Tests/InputSystem/Plugins/UITests.cs | 75 +++++++-- Packages/com.unity.inputsystem/CHANGELOG.md | 6 + .../Plugins/UI/InputSystemUIInputModule.cs | 158 +++++++++--------- Packages/com.unity.inputsystem/package.json | 2 +- 4 files changed, 145 insertions(+), 96 deletions(-) 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 bdee44163f..33096c89d5 100755 --- a/Packages/com.unity.inputsystem/CHANGELOG.md +++ b/Packages/com.unity.inputsystem/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. Due to package verification, the latest version below is the unpublished version and the date is meaningless. however, it has to be formatted properly to pass verification tests. +## [1.0.0-preview.5] - 2020-12-12 + +### Fixed + +- `InputSystemUIInputModule` no longer generates GC heap garbage every time mouse events are processed. + ## [1.0.0-preview.4] - 2020-01-24 This release includes a number of Quality-of-Life improvements for a range of common problems that users have reported. 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; } } diff --git a/Packages/com.unity.inputsystem/package.json b/Packages/com.unity.inputsystem/package.json index 8335778db1..62de2af413 100755 --- a/Packages/com.unity.inputsystem/package.json +++ b/Packages/com.unity.inputsystem/package.json @@ -1,7 +1,7 @@ { "name": "com.unity.inputsystem", "displayName": "Input System", - "version": "1.0.0-preview.4", + "version": "1.0.0-preview.5", "unity": "2019.1", "repository": { "type": "git",