NEW: Add InputSystemProvider for InputForUI #1679

jamesmcgill · 2023-05-19T11:44:47Z

Description

This adds the InputSystemProvider implementation for the InputForUI module.
The InputForUI module was added to newer Unity Engine versions (2023.2 and above) to better abstract UI components (i.e. UI Toolkit) from the Input implementation.
This is purely an internal api artifact and should not result in any behaviour changes for users.

InputSystemProvider is responsible for providing input to UI frameworks when InputSystem is installed and enables.

Changes made

Added the InputSystemProvider class which sits in it's own Assembly.

Checklist

Before review:

Changelog entry added.
- Explains the change in Changed, Fixed, Added sections.
- For API change contains an example snippet and/or migration example.
- FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
- FogBugz is marked as "Resolved" with next release version correctly set.
Tests added/changed, if applicable.
- Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
- Performance tests.
- Integration tests.
Docs for new/changed API's.
- Xmldoc cross references are set correctly.
- Added explanation how the API works.
- Usage code examples added.
- The manual is updated, if needed.

During merge:

Commit message for squash-merge is prefixed with one of the list:
- NEW: ___.
- FIX: ___.
- DOCS: ___.
- CHANGE: ___.
- RELEASE: 1.1.0-preview.3.

* Add dispatch of navigation events * Fix formatting * Unifies GetEventSource() for callback context and devices * Use DispatchFromCallback and clean todos

* Update renaming of Event.CompareType * Add Next/Previous navigation events repetition Currently it doesn't deal with keys other than tab and shift+tab. Discussion needs to happen to allow configuration for this with the Input System.

… events (#1674)

…ISX-1386)

…r (ISX-1397)

jfreire-unity · 2023-05-19T15:19:25Z

Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs

+        InputEventPartialProvider m_InputEventPartialProvider;

-        private Configuration _cfg;
+        Configuration m_Cfg;

-        private InputActionAsset _inputActionAsset;
-        private InputActionReference _pointAction;
-        private InputActionReference _moveAction;
-        private InputActionReference _submitAction;
-        private InputActionReference _cancelAction;
-        private InputActionReference _leftClickAction;
-        private InputActionReference _middleClickAction;
-        private InputActionReference _rightClickAction;
-        private InputActionReference _scrollWheelAction;
+        InputActionAsset m_InputActionAsset;
+        InputActionReference m_PointAction;
+        InputActionReference m_MoveAction;
+        InputActionReference m_SubmitAction;
+        InputActionReference m_CancelAction;
+        InputActionReference m_LeftClickAction;
+        InputActionReference m_MiddleClickAction;
+        InputActionReference m_RightClickAction;
+        InputActionReference m_ScrollWheelAction;

-        InputAction _nextPreviousAction;
+        InputAction m_NextPreviousAction;

-        List<Event> _events = new List<Event>();
+        List<Event> m_Events = new List<Event>();

-        private PointerState _mouseState;
+        PointerState m_MouseState;

-        private PointerState _penState;
-        private bool _seenPenEvents;
+        PointerState m_PenState;
+        bool m_SeenPenEvents;

-        private PointerState _touchState;
-        private bool _seenTouchEvents;
+        PointerState m_TouchState;
+        bool m_SeenTouchEvents;

-        private const float kSmallestReportedMovementSqrDist = 0.01f;
+        const float k_SmallestReportedMovementSqrDist = 0.01f;


Thanks a lot for these changes 🙏 !

jfreire-unity

I'm approving this because it works. Even though I think there's opportunity to refactor this into a more readable state in a following PR. Since we're trying to fix some CI issues, I don't want to delay the merging of the code into develop.
IMO we can discuss improvements next week @jamesmcgill

lyndon-unity · 2023-05-19T15:37:47Z

...s/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/InputActionsTreeViewItem.cs


            FocusOnRenameTextField();
-            e.PreventDefault();
+            e.StopPropagation();


Good to see this warning fix :)

lyndon-unity

Seems like a reasonable initial implemention - some feedback comments but nothing critical to update in this PR.

lyndon-unity · 2023-05-19T15:39:48Z

Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs

+        {
+            // Only if InputSystem is enabled in the PlayerSettings do we set it as the provider.
+            // This includes situations where both InputManager and InputSystem are enabled.
+#if ENABLE_INPUT_SYSTEM


Should this ifdef be wider to exclude more of the code.

lyndon-unity · 2023-05-19T16:03:04Z

Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs

+
+        EventModifiers m_EventModifiers => m_InputEventPartialProvider._eventModifiers;
+
+        DiscreteTime m_CurrentTime => (DiscreteTime)Time.timeAsRational;


Slightly nervous about when this timer wraps but not something to hold up this first PR

lyndon-unity · 2023-05-19T16:05:51Z

Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs

+            m_TouchState.Reset();
+            m_SeenTouchEvents = false;
+
+            // TODO should UITK somehow override this?


Yes. Could revise to say in future UITK will be able to override this

But find here since we agreed with UI team this is ok for first pass.

lyndon-unity · 2023-05-19T16:13:54Z

Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs

+                        direction = direction,
+                        timestamp = currentTime,
+                        eventSource = GetEventSource(GetActiveDeviceFromDirection(direction)),
+                        playerId = k_DefaultPlayerId,


I see this is a fixed value at the moment so will need updating later but fine for this PR

lyndon-unity · 2023-05-19T16:15:45Z

Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs

+            return (move, axisWasPressed);
+        }
+
+        NavigationEvent.Direction ReadNextPreviousDirection()


Have you considered this returning a tuple like the prior function to avoid the use of m_NextPreviousAction directly in the DirectionNavigation function above
axesButtonWerePressed = m_NextPreviousAction.WasPressedThisFrame();

Not critical for this PR

lyndon-unity · 2023-05-19T16:34:20Z

Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs

+            }));
+        }
+
+        void OnLeftClickPerformed(InputAction.CallbackContext ctx) => OnClickPerformed(ctx, GetEventSource(ctx), PointerEvent.Button.MouseLeft);


Could do GetEventSource call inside the above OnClickPerformed function and avoid passing as parameter?

lyndon-unity · 2023-05-19T16:36:03Z

Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs

+
+        void RegisterNextPreviousAction()
+        {
+            m_NextPreviousAction = new InputAction(name: "nextPreviousAction", type: InputActionType.Button);


Why is this separate from the actions setup in register actions ?

Hmm I think I see as its a special case not in the default setup

lyndon-unity · 2023-05-19T16:36:45Z

Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs

+            // The Next/Previous action is not part of the input actions asset
+            UnregisterNextPreviousAction();
+
+            UnityEngine.Object.Destroy(m_InputActionAsset); // TODO check if this is ok


Did we check this is ok ?

Packages/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs

lyndon-unity

Seems like a reasonable initial implemention - some feedback comments but nothing critical to update in this PR.

lyndon-unity · 2023-05-19T16:44:06Z

I'm unclear why the test is failing though. Seems ISX backend should be registering and it seemed to work locally for me. As James said this needs investigating

jfreire-unity · 2023-05-23T10:52:18Z

cc @jamesmcgill Since all the required tests are passing, I'm going to merge this into develop even though trunk tests currently fail as discussed with @lyndon-unity today to speed up the release process.

Dmytro Ivanov and others added 30 commits April 12, 2023 16:48

Adding dependency to a provider package

2f1498d

wip

c6da50d

wip

9ee7492

wip

622d6de

wip

96ce804

wip

0bf9bc2

wip

09295ea

sorting of events and filtering of pointer events

ee1978a

pointer indices

19e79e2

more robust scroll events

404840c

forgot to reset state flags

dde508e

trying out to use touchscreen for finger index

cebfb54

removing leftovers

a949f6b

Add dispatch of navigation events for InputForUI (#1667)

d26e458

* Add dispatch of navigation events * Fix formatting * Unifies GetEventSource() for callback context and devices * Use DispatchFromCallback and clean todos

Set seenTouchEvents and seenTouchEvents for missing control paths and…

32cbfc8

… events (#1674)

fix compilation errors when opening InputSystem project

1ab3c35

fix compilation errors when opening project in older editors

df24be7

Bootstrap InputForUI provider only if enabled in the PlayerSettings (…

d7dd239

…ISX-1386)

Fix null reference errors by not initializing InputForUI in the edito…

e7e81f0

…r (ISX-1397)

Fix to avoid PointerEvents from Mouse when using Touch

1441c5f

Reverse bool logic of resetSeenEventsOnUpdate

3f57bc3

Remove debug logs

73d6dd1

Fix PointerEvents with same pointerIndex for all fingers

f29e0a9

fix InputForUI not being bootstrapped in IL2CPP builds (ISX-1424)

04323d7

fix for deprecated warning

21e3fff

fix for unused variable warning

a5a7230

run formatting tool

9f4e996

modify naming to conform to coding convention

386d16f

Merge branch 'develop' into dmytro/common-input-api-for-ui

4d59404

jfreire-unity mentioned this pull request May 19, 2023

CHANGE: Add automated testing setup for InputForUI #1680

Merged

3 tasks

jfreire-unity reviewed May 19, 2023

View reviewed changes

jfreire-unity approved these changes May 19, 2023

View reviewed changes

lyndon-unity reviewed May 19, 2023

View reviewed changes

lyndon-unity approved these changes May 19, 2023

View reviewed changes

jamesmcgill and others added 2 commits May 22, 2023 18:39

fix for running tests in standalone builds

e6c532b

Remove commented code and add missing comment

0ff50f6

jfreire-unity merged commit 32aee68 into develop May 23, 2023

jfreire-unity deleted the dmytro/common-input-api-for-ui branch May 23, 2023 10:53


		EventModifiers m_EventModifiers => m_InputEventPartialProvider._eventModifiers;

		DiscreteTime m_CurrentTime => (DiscreteTime)Time.timeAsRational;

NEW: Add InputSystemProvider for InputForUI #1679

NEW: Add InputSystemProvider for InputForUI #1679

Uh oh!

Conversation

jamesmcgill commented May 19, 2023

Description

Changes made

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfreire-unity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyndon-unity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lyndon-unity left a comment

Choose a reason for hiding this comment

Uh oh!

lyndon-unity commented May 19, 2023

Uh oh!

jfreire-unity commented May 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants