-
Notifications
You must be signed in to change notification settings - Fork 327
NEW: MouseEvents for InputSystem - Sending data from package & Samples and Tests #2207
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2207 +/- ##
===========================================
- Coverage 68.15% 68.15% -0.01%
===========================================
Files 367 367
Lines 53662 53663 +1
===========================================
- Hits 36576 36574 -2
- Misses 17086 17089 +3 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @ritamerkl ! I like the approach and the sample is of reasonable size to test the main functionality. Also really nice to have plenty of tests.
However:
- This functionality needs to be guarded with defines in asmdef, as this will only be available from 6.3 forward AFAIK. So if I try this PR without the trunk PR, I'll get compilation errors.
- I would like to see at least 1 test with Touch as well, particularly with more than 1 touch. Mouse and Pen are pretty similar as they are a single pointer.
Also there's some CI failures that will need to be addressed. Not sure if it's only because of the lack of defines or if there's any impact on pointer devices functionality. |
WIP: working on adding touch samples (multi-touch) & parallel pointers (eg. touch&pen). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great!
I'm happy to see us improving our feature parity! Also thanks for investing the time on the tests!
I've left a few comments, but I'll leave it up to you to decide which ones to address
[Category("MouseEvents")] | ||
public IEnumerator PenEvents_CanReceiveOnMouseDown() | ||
{ | ||
var pen = InputSystem.AddDevice<Pen>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we are duplicating the exact same tests for both mouse and pen, I'm wondering if we can parameterize that and just have a single test...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree to previous comment, this isn't necessarily the case any longer either right? "That we have to queue into NativeInputRuntime"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: 937d7b6
untitled folder/Il2CppOutputProject/Source/il2cppOutput/il2cpp-compile.traceevents
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see 3 files under "untitled folder", the folder including those files should be removed from this PR right?
@@ -3646,6 +3647,9 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev | |||
//// mess in the event buffer | |||
//// same goes for events that someone may queue from a change monitor callback | |||
InvokeAfterUpdateCallback(updateType); | |||
//send pointer data to backend for OnMouseEvents | |||
if (Pointer.current != null && gameIsPlaying) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ritamerkl Have you checked if Input Manager supports OnMouseXXX events from a script that is marked with [ExecuteInEditorMode]? That could be relevant here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, since the InputManager MouseEvents were only sent from the InputModuleRegistration player loop callback when ::IsWorldPlayingThisFrame() was true, which basically is only true if in playmode, not editmode. Unless I miss the way [ExecuteInEditorMode] is working now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, then I believe we might be safe, would to good to make sure we haven't overlooked something like that though.
@@ -3646,6 +3647,9 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev | |||
//// mess in the event buffer | |||
//// same goes for events that someone may queue from a change monitor callback | |||
InvokeAfterUpdateCallback(updateType); | |||
//send pointer data to backend for OnMouseEvents | |||
if (Pointer.current != null && gameIsPlaying) | |||
NativeInputSystem.SetMouseEventsData(Pointer.current.press.isPressed, Pointer.current.press.wasPressedThisFrame, Pointer.current.position.x.value, Pointer.current.position.y.value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be routed via INativeInputRuntime interface and its NativeInputRuntime and TestInputRuntime implementations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NativeInputRuntime is the name of the Bindings class in the module, I fail to understand what exactly you are referring to.
I placed it here to have all updated data after the inputsystem Update, this happens for test as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I referred to the fact that this is calling NativeInputSystem directly. If you look at NativeInputRuntime.cs you will see it implements INativeInputRuntime as an abstraction of the module to allow stubbing it away in e.g. tests. Hence InputTestRuntime also exists stubbing these calls. It's basically a mocking strategy that has historically been used. If this is possible to auto-test without mocking, it should right? Then I see no reason why this would be just fine, but just wanted to let you know in case you had not noticed since it seems SetMouseEventsData is part of Input module based on where its placed/mapped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And NativeInputRuntime is not the bindings class, it's just an abstraction within the package. The bindings class (P/Invoke) is called NativeInputSystem which is the one you are calling here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was adding this same comment. We should call this as m_Runtime.SetMouseEventsData(...);
. This avoids making other classes (like this one) dependent on the bindings as well. This way we only have NativeInputRuntime
class as a single dependent of the Module bindings (NativeInputSystem
) and make it consistent across the codebase as is.
And by making it part of IInputRuntime
interface we leverage the mocking strategy @ekcoh mentioned as well. So I think this should be improved.
…echnologies/InputSystem into mouseevents-samles-tests
using UnityEngine.InputSystem.EnhancedTouch; | ||
using Touch = UnityEngine.InputSystem.EnhancedTouch.Touch; | ||
|
||
public class OnMouseEventSample : MonoBehaviour |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the intent here is to serve as a sample (and maybe also our own way to manually test or debug this), I would suggest that we have all the supported implicit script mouse event callback functions present in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. missing OnMouseOver, OnMouseEnter, OnMouseExit, OnMouseUp, OnMouseUpAsButton
@@ -0,0 +1,199 @@ | |||
#if UNITY_6000_4_OR_NEWER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great you added automated tests for all the events! This way we protect ourselves from regressions in this area. Similarly I think there is a lot to gain from having them covered in the sample as well.
using UnityEngine.InputSystem; | ||
using UnityEngine.TestTools; | ||
using Is = UnityEngine.TestTools.Constraints.Is; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Redundant empty row
|
||
private static HashSet<Type> _testDevices = new HashSet<Type> { typeof(Mouse), typeof(Pen), typeof(Touchscreen) }; | ||
// OnMouseOver/Exit and Hover events are not supported for touch | ||
private static HashSet<Type> _testDevicesNoTouch = new HashSet<Type> { typeof(Mouse), typeof(Pen) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't a HashSet overkill here (dynamic allocation etc) when function with a switch statement with 2 cases would be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see (or am I missing something) that we use lookup in the tests? If not, then I would recommend a plain static readonly array for these. (Less allocation, leaner, simpler) and also likely much faster. Also, when using HashSet, the iteration order is "random"
[Category("MouseEvents")] | ||
public IEnumerator MouseEvents_CanReceiveOnMouseDown([ValueSource(nameof(_testDevices))] Type pointerType) | ||
{ | ||
var pointer = (Pointer)InputSystem.s_Manager.AddDevice(pointerType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: It looks like this
var pointer = (Pointer)InputSystem.s_Manager.AddDevice(pointerType);
InputSystem.AddDevice(pointer);
Is repeated for all the tests. Can't we just embed it into SetUpScene() and have it as an out parameter from that function to reduce redundant code?
gameObject.AddComponent<OnMouseEventsTest>(); | ||
var vec = Camera.main.WorldToScreenPoint(gameObject.transform.position); | ||
|
||
Set(pointer, new Vector2(vec.x, vec.y), true, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throughout all of these tests I see we have the same pattern, e.g. Set(...); yield return null; (repeat)
I would be reassuring we also verified this works correctly without the yields, e.g. when events are not in separate frames. Maybe it's possible to parameterise the test with another variable e.g. bool yieldBetweenSet
so that we could test both scenario by having the yields not happening when that is false. Maybe in that case we only need to yield at the end to trigger script callbacks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on how this have been done in InputManager.cs I wonder if my comment above doesn't make sense. It seems we only call the script once per frame which I wonder if its valid. E.g. that way there wouldn't be any events for e.g. two events in the same frame. Is this really how it works with InputManager? This is the main thing to sort out before I approve. Maybe its useful to test InputManager vs Input System with forced 1 FPS frame rate to sort this out?
|
||
void Set(Pointer pointer, Vector2 point, bool pressed, bool released) | ||
{ | ||
if (pointer is Touchscreen touchscreen) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Pen/Stylus falling under the same behaviour as mouse? Or is it excluded from this functionality somehow?
@@ -16,6 +16,7 @@ however, it has to be formatted properly to pass verification tests. | |||
|
|||
### Added | |||
- Exposed MediaPlayPause, MediaRewind, MediaForward keys on Keyboard. | |||
- Added OnMouse events for the InputSystem from editor version 6.3, including samples and tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest saying "Added support for (MonoBehavior
OnMouse-events) [https://docs.unity3d.com/ScriptReference/MonoBehaviour.html] when running the Input System on Unity 6.3 or newer."
Then I would make a separate entry for the sample linking to it. It makes more sense to say Unity 6.3 IMO since editor might indicate it would only be supported in editor but this is for players as well right?
//send pointer data to backend for OnMouseEvents | ||
#if UNITY_6000_4_OR_NEWER | ||
if (Pointer.current != null && gameIsPlaying) | ||
NativeInputSystem.SetMouseEventsData(Pointer.current.press.isPressed, Pointer.current.press.wasPressedThisFrame, Pointer.current.position.x.value, Pointer.current.position.y.value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together @ritamerkl, I added a few suggestions and my only main concern is still whether this will behave similar to Input Manager when we only call into the SetMouseEventData(...) function once every frame - this looks incorrect to me. I would have expected that this would not be based on a polling-based evaluation like this here but rather sit inside the event processing loop above or from within the device state updates after event compression. Does this work for e.g. 1 FPS?
Maybe I am overlooking something.... but was Input Manager only calling this once every frame?
I see CI is failing with "Packages\com.unity.inputsystem\InputSystem\InputManager.cs(3710,35): error CS0117: 'NativeInputSystem' does not contain a definition for 'SetMouseEventsData'" but this would be expected until the feature is available in Unity. |
It is weird since this is wrapped in defines for unity 600.4 or newer, but still fails on 600.3 🤔 |
@@ -3703,6 +3704,11 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev | |||
//// mess in the event buffer | |||
//// same goes for events that someone may queue from a change monitor callback | |||
InvokeAfterUpdateCallback(updateType); | |||
//send pointer data to backend for OnMouseEvents | |||
#if UNITY_6000_4_OR_NEWER | |||
if (Pointer.current != null && gameIsPlaying) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means we will be calling this every frame, regardless of having received an event from a Pointer device or not, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to look at how its implemented under the hood, there might even be timeout logic in there which would require it to be once a frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there might even be timeout logic in there which would require it to be once a frame.
I'm not sure I understand what you mean with this. Could you specify it a bit?
var pointer = (Pointer)InputSystem.s_Manager.AddDevice(pointerType); | ||
InputSystem.AddDevice(pointer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's great we have tests with the Pointer abstraction. But could we have also some real device tests, for Touch, Pen and Mouse devices to make sure the events are generated for these devices?
You can take advantage of TestCase
attribute pattern already used in other tests in our codebase. Something like:
[TestCase("Pointer")]
[TestCase("Pen")]
[TestCase("Touchscreen")]
[TestCase("Mouse")]
MouseEvents_CanReceiveOnMouseUp(string deviceLayout)
{
var device = InputSystem.AddDevice(layoutName);
....
}
This removes the need for private static devices such as HashSet _testDevices
@@ -3703,6 +3704,11 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev | |||
//// mess in the event buffer | |||
//// same goes for events that someone may queue from a change monitor callback | |||
InvokeAfterUpdateCallback(updateType); | |||
//send pointer data to backend for OnMouseEvents | |||
#if UNITY_6000_4_OR_NEWER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would advise against this. Instead, create an asmdef symbol that defines the feature/capability, e.g. UNITY_INPUTSYSTEM_HAS_MOUSE_SCRIPT_EVENTS_SUPPORTED, then let that symbol be conditionally compiled based on the specific version that contains the native changes, e.g. 6000.0.4.a7, that is more specific and also removes versions from the source code which only cares about a certain API being a certain way instead. And it centralises the version number mess. At least this is what we agreed upon in #2176, and the same technique was used in #2200 to handle the API difference in module API and behaviour .
Description
This PR introduces sending pointer data (e.g. mouse, pen, touch...) to native for the OnMouse events driven by the InputSystem. It also adds tests and samples for the OnMouse events working with the InputSystem. The behaviour on native side is introduced with this PR and both PR's should land together, or one (shortly) after the other.
Testing status & QA
Only manual testing for the sample.
Overall Product Risks
Comments to reviewers
Please review together with https://github.cds.internal.unity3d.com/unity/unity/pull/73198.
Checklist
Before review:
Changed
,Fixed
,Added
sections.Area_CanDoX
,Area_CanDoX_EvenIfYIsTheCase
,Area_WhenIDoX_AndYHappens_ThisIsTheResult
.During merge:
NEW: ___
.FIX: ___
.DOCS: ___
.CHANGE: ___
.RELEASE: 1.1.0-preview.3
.After merge: