Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Fixed multiple OnScreenStick Components that does not work together when using them simultaneously in isolation mode (ISXB-813) #2090

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 64 additions & 14 deletions Assets/Tests/InputSystem/Plugins/OnScreenTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ public void Devices_DisablingLastOnScreenControlDoesReportActiveControl()
// https://fogbugz.unity3d.com/f/cases/1271942
[UnityTest]
[Category("Devices")]
public IEnumerator Devices_CanHaveOnScreenJoystickControls()
public IEnumerator Devices_CanHaveOnScreenJoystickControls([Values(false, true)] bool useInIsolation)
{
foreach (var c in Camera.allCameras)
Object.Destroy(c.gameObject);
Expand All @@ -422,18 +422,33 @@ public IEnumerator Devices_CanHaveOnScreenJoystickControls()
canvasGO.AddComponent<GraphicRaycaster>();
canvas.renderMode = RenderMode.ScreenSpaceOverlay;

var stickGO = new GameObject("Stick");
stickGO.SetActive(false);
var stickTransform = stickGO.AddComponent<RectTransform>();
var stick = stickGO.AddComponent<OnScreenStick>();
stickGO.AddComponent<Image>();
stickTransform.SetParent(canvasTransform);
stickTransform.anchorMin = new Vector2(0, 0);
stickTransform.anchorMax = new Vector2(0, 0);
stickTransform.anchoredPosition = new Vector2(100, 100);
stickTransform.sizeDelta = new Vector2(100, 100);
stick.controlPath = "<Gamepad>/leftStick";
stickGO.SetActive(true);
var stickLeftGO = new GameObject("StickLeft");
stickLeftGO.SetActive(false);
var stickLeftTransform = stickLeftGO.AddComponent<RectTransform>();
var stickLeft = stickLeftGO.AddComponent<OnScreenStick>();
stickLeft.useIsolatedInputActions = useInIsolation;
stickLeftGO.AddComponent<Image>();
stickLeftTransform.SetParent(canvasTransform);
stickLeftTransform.anchorMin = new Vector2(0, 0);
stickLeftTransform.anchorMax = new Vector2(0, 0);
stickLeftTransform.anchoredPosition = new Vector2(100, 100);
stickLeftTransform.sizeDelta = new Vector2(100, 100);
stickLeft.controlPath = "<Gamepad>/leftStick";
stickLeftGO.SetActive(true);

var stickRightGO = new GameObject("StickRight");
stickRightGO.SetActive(false);
var stickRightTransform = stickRightGO.AddComponent<RectTransform>();
var stickRight = stickRightGO.AddComponent<OnScreenStick>();
stickRight.useIsolatedInputActions = useInIsolation;
stickRightGO.AddComponent<Image>();
stickRightTransform.SetParent(canvasTransform);
stickRightTransform.anchorMin = new Vector2(0, 0);
stickRightTransform.anchorMax = new Vector2(0, 0);
stickRightTransform.anchoredPosition = new Vector2(500, 100);
stickRightTransform.sizeDelta = new Vector2(100, 100);
stickRight.controlPath = "<Gamepad>/rightStick";
stickRightGO.SetActive(true);

var buttonGO = new GameObject("Button");
buttonGO.SetActive(false);
Expand Down Expand Up @@ -464,7 +479,7 @@ public IEnumerator Devices_CanHaveOnScreenJoystickControls()

Assert.That(player.devices, Is.EquivalentTo(new[] { Gamepad.all[0] }));

// Touch the stick and drag it upwards.
// Touch the Left stick and drag it upwards.
BeginTouch(1, new Vector2(150, 150));
yield return null;
eventSystem.Update();
Expand All @@ -491,6 +506,38 @@ public IEnumerator Devices_CanHaveOnScreenJoystickControls()
InputSystem.Update(); // Button is feeding events when responding to UI events.

Assert.That(Gamepad.all[0].buttonSouth.isPressed, Is.False);

// Touch the right stick and drag it downwards
BeginTouch(2, new Vector2(550, 150));
yield return null;
eventSystem.Update();
Assert.That(eventSystem.IsPointerOverGameObject(), Is.True);
MoveTouch(2, new Vector2(550, 50));
yield return null;
eventSystem.Update();
InputSystem.Update(); // Stick is feeding events when responding to UI events.

Assert.That(Gamepad.all[0].leftStick.ReadValue(), Is.EqualTo(new Vector2(0, 1)).Using(Vector2EqualityComparer.Instance));
Assert.That(Gamepad.all[0].rightStick.ReadValue(), Is.EqualTo(new Vector2(0, -1)).Using(Vector2EqualityComparer.Instance));

// Release finger one and move second and ensure that it still works
EndTouch(1, new Vector2(550, 200));
MoveTouch(2, new Vector2(600, 150));
yield return null;
eventSystem.Update();
InputSystem.Update(); // Stick is feeding events when responding to UI events.

Assert.That(Gamepad.all[0].leftStick.ReadValue(), Is.EqualTo(new Vector2(0, 0)).Using(Vector2EqualityComparer.Instance));
Assert.That(Gamepad.all[0].rightStick.ReadValue(), Is.EqualTo(new Vector2(1, 0)).Using(Vector2EqualityComparer.Instance));

// Release finger two
EndTouch(2, new Vector2(600, 150));
yield return null;
eventSystem.Update();
InputSystem.Update(); // Stick is feeding events when responding to UI events.

Assert.That(Gamepad.all[0].leftStick.ReadValue(), Is.EqualTo(new Vector2(0, 0)).Using(Vector2EqualityComparer.Instance));
Assert.That(Gamepad.all[0].rightStick.ReadValue(), Is.EqualTo(new Vector2(0, 0)).Using(Vector2EqualityComparer.Instance));
}

[UnityTest]
Expand Down Expand Up @@ -519,6 +566,9 @@ public IEnumerator Devices_OnScreenStickDoesNotReceivePointerUpEventsInIsolatedM
uiTestScene.uiInputModule.actionsAsset.actionMaps[0].LazyResolveBindings(true);
};

// Ensure that the OnScreenStick component has been started
yield return null;

yield return uiTestScene.PressAndDrag(image, new Vector2(50, 50));

// The OnScreenStick when being driven from the UI (non-isolated mode) queues the events into the next
Expand Down
1 change: 1 addition & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ however, it has to be formatted properly to pass verification tests.
- Fixed an issue with default device selection when adding new Control Scheme.
- Fixed an issue where action map delegates were not updated when the asset already assigned to the PlayerInput component were changed [ISXB-711](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-711).
- Fixed Action properties edition in the UI Toolkit version of the Input Actions Asset editor. [ISXB-1277](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1277)
- Fixed multiple `OnScreenStick` Components that does not work together when using them simultaneously in isolation mode. [ISXB-813](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-813)

### Changed
- Added back the InputManager to InputSystem project-wide asset migration code with performance improvement (ISX-2086).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using UnityEngine.InputSystem.Layouts;
using UnityEngine.InputSystem.Utilities;
using UnityEngine.UI;
using UnityEngine.InputSystem.Controls;

#if UNITY_EDITOR
using UnityEditor;
Expand Down Expand Up @@ -91,7 +92,10 @@ private void Start()
if (m_PointerDownAction == null || m_PointerDownAction.bindings.Count == 0)
{
if (m_PointerDownAction == null)
m_PointerDownAction = new InputAction();
m_PointerDownAction = new InputAction(type: InputActionType.PassThrough);
// ensure PassThrough mode
else if (m_PointerDownAction.m_Type != InputActionType.PassThrough)
m_PointerDownAction.m_Type = InputActionType.PassThrough;

#if UNITY_EDITOR
InputExitPlayModeAnalytic.suppress = true;
Expand Down Expand Up @@ -121,8 +125,7 @@ private void Start()
#endif
}

m_PointerDownAction.started += OnPointerDown;
m_PointerDownAction.canceled += OnPointerUp;
m_PointerDownAction.performed += OnPointerChanged;
m_PointerDownAction.Enable();
m_PointerMoveAction.Enable();
}
Expand Down Expand Up @@ -154,8 +157,7 @@ private void OnDestroy()
{
if (m_UseIsolatedInputActions)
{
m_PointerDownAction.started -= OnPointerDown;
m_PointerDownAction.canceled -= OnPointerUp;
m_PointerDownAction.performed -= OnPointerChanged;
}
}

Expand Down Expand Up @@ -227,11 +229,20 @@ private void EndInteraction()

private void OnPointerDown(InputAction.CallbackContext ctx)
{
if (m_IsIsolationActive) { return; }
Debug.Assert(EventSystem.current != null);

var screenPosition = Vector2.zero;
if (ctx.control?.device is Pointer pointer)
TouchControl touchControl = null;
if (ctx.control?.parent is TouchControl touch)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is the design, generally this is a perfect fit for a device-agnostic pointer interface (just a side note)

{
touchControl = touch;
screenPosition = touch.position.ReadValue();
}
else if (ctx.control?.device is Pointer pointer)
{
screenPosition = pointer.position.ReadValue();
}

m_PointerEventData.position = screenPosition;
EventSystem.current.RaycastAll(m_PointerEventData, m_RaycastResults);
Expand All @@ -251,23 +262,63 @@ private void OnPointerDown(InputAction.CallbackContext ctx)
return;

BeginInteraction(screenPosition, GetCameraFromCanvas());
if (touchControl != null)
{
m_TouchControl = touchControl;
m_PointerMoveAction.ApplyBindingOverride($"{touchControl.path}/position", path: "<Touchscreen>/touch*/position");
}

m_PointerMoveAction.performed += OnPointerMove;
m_IsIsolationActive = true;
}

private void OnPointerChanged(InputAction.CallbackContext ctx)
{
if (ctx.control.IsPressed())
OnPointerDown(ctx);
else
OnPointerUp(ctx);
}

private void OnPointerMove(InputAction.CallbackContext ctx)
{
// only pointer devices are allowed
Debug.Assert(ctx.control?.device is Pointer);
Vector2 screenPosition;

var screenPosition = ((Pointer)ctx.control.device).position.ReadValue();
// If it's a finger take the value from the finger that initiated the change
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More a general conceptual thought around the scalability of this than the particular fix, what if the on screen control is interacted with a pen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question I don't have a pen/touch setup to test.
But we can fix the pen case after if needed. Here it only fix when it comes from a touch device

if (m_TouchControl != null)
{
// if the finger is up ignore the move
if (m_TouchControl.isInProgress == false)
{
return;
}
screenPosition = m_TouchControl.position.ReadValue();
}
else
{
screenPosition = ((Pointer)ctx.control.device).position.ReadValue();
}

MoveStick(screenPosition, GetCameraFromCanvas());
}

private void OnPointerUp(InputAction.CallbackContext ctx)
{
if (!m_IsIsolationActive) return;

// if it's a finger ensure that is the one that get released
if (m_TouchControl != null)
{
if (m_TouchControl.isInProgress) return;
m_PointerMoveAction.ApplyBindingOverride(null, path: "<Touchscreen>/touch*/position");
m_TouchControl = null;
}

EndInteraction();
m_PointerMoveAction.performed -= OnPointerMove;
m_IsIsolationActive = false;
}

private Camera GetCameraFromCanvas()
Expand Down Expand Up @@ -430,6 +481,10 @@ public bool useIsolatedInputActions
private List<RaycastResult> m_RaycastResults;
[NonSerialized]
private PointerEventData m_PointerEventData;
[NonSerialized]
private TouchControl m_TouchControl;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really related to this fix, but more to the original design, this implies that a member would need to be added for any kind of pointer device right? Or is this particularly due to TouchControl not being a derivative of pointer in your opinion @bmalrat ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we don't need it for each type, only for touch to identify which one was touched in the /touch*/press. I'm not aware of an other type that could aggregate multiple source into one binding.

[NonSerialized]
private bool m_IsIsolationActive;

protected override string controlPathInternal
{
Expand Down
Loading