-
Notifications
You must be signed in to change notification settings - Fork 326
NEW: Added achievable frequency and latency (processing delay) to Input Debugger (ISX-2254) #2142
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
NEW: Added achievable frequency and latency (processing delay) to Input Debugger (ISX-2254) #2142
Conversation
…thub.com:Unity-Technologies/InputSystem into isx-2254-frequency-and-latency-in-input-debugger
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's a great to have, thanks for adding this!
I only did a quick run through but it looks good from my POV. I need to take another closer look and try it myself.
Packages/com.unity.inputsystem/InputSystem/Editor/Debugger/InputDeviceDebuggerWindow.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Editor/Debugger/InputDeviceDebuggerWindow.cs
Outdated
Show resolved
Hide resolved
private double m_LastUpdateTime; | ||
private int m_SampleCount; | ||
|
||
public SampleFrequencyCalculator(float targetFrequency, double realtimeSinceStartup) |
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.
Target frequency is currently only a "convient storage" here. Hower I would suggest keeping it inside this type since an evolved implementation will likely use it to compute error compared to target and create a CDF or PDF of it to illustrate the applications or architectures ability to promptly react to stimuli.
I think this is great step toward transparency of processing of input events. I think it should be made clear that this doesn't measure the latency between the creation of an input event to when it shows up on the screen, but from the creation of the event to when it's processed by the input system. |
Agreed, that's a much simpler/better way to say what I am trying to say in that tool tip. Also why I renamed the label to "Processing Delay". Will try to improve the tooltip based your description here. Thanks! :) |
Perhaps the manual could be updated with the new screenshot, and a little blurb about what value this adds could be helpful as well. |
…Debugger screenshot.
You probably found another issue with remote devices having some unusable timestamps. This is probably a root of other bad things to happen. Great find. Did you use Unity remote to get this happening? I decided to cap the "age" at 1000.0 ms and this is what you see. However this is only based on current time - event timestamp. If event timestamp comes from another clock (remote) device, this doesn't make sense, so it probably means remote devices are not re-timestamped when coming in to the host or the remote protocol do not allow sending the remote device time to allow computing the true latency (including network latency). I think its unrelated to the PR, but we might to either filter remote ones out for now until we have figured that part out or do something similar.... I didn't try remote when doing this. |
The steps to repro are: Connect an android device and build with the development build type -> In the Input Debugger click on the Remote Devices dropdown and pick your android device -> in the android devices list double click any device -> Processing Delay is always above 1k ms |
Not really, on Windows which was the platform you tested on, DualSense is processed over HID. The current HID implementation do not have any rate-adoption (apart from max read requests in flight I believe) so it will run as fast as the underlying HID driver allows it to do. It's a good observation since this shows processing differences in platform-dependent code that could be up for discussion. However, you are correct that the noise is part of it as well since the noise will allow events/samples to be forwarded since the state "changes". If there was no noise or if signal was pre filtered it would reduce the frequency while the device is idle. |
E.g. on macOS where I am currently working with another DualSense bug, the achieved frequency is around 65 Hz. So 1/8 compared to Win at the moment. |
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.
Checked debugger window for these devices: touch, keyboard, mouse, PS and Xbox controllers and commonly used android sensors.
Thanks for the rigorous testing of this debug feature @Pauliusd01 |
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.
Approving to unblock this, will add more thoughts next week !
I tested the newly added Input Debugger features myself now, and think it is priceless. The only wish I would have is to name target/ Polling Frequency with a bit more clarity or adding an explanation with a small text underneath, or a hover textbox? For me the meaning wasn't clear initially (your live demo on friday was very helpful) - also for the restrictions eg when n/a shows up and what this means. |
I agree, the UI layout isn't very helpful, maybe I should try revisit the IMGUI design here to at least make it a bit better (I think the debugger window is overpopulated to begin with, contains a mix of static and dynamic data and should likely be split into multiple windows and/or perspectives). Regarding the N/A and Target polling rate I would propose the following:
Taking both above into account maybe we should do this before landing this:
What do you think? Adding the IOCTL could be done on this branch in managed and separately in native. It just needs a good fallback when we cannot tell. When we cannot tell, maybe we should just hide Target since it doesn't add any info anyways. |
Sounds good to me! |
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.
Reviewing on behalf of docs. Approving so as not to block, but left some suggestions which should be applied before merging.
@@ -42,7 +42,11 @@ In the Input Debugger window, navigate to the __Devices__ list and double-click | |||
|
|||
 | |||
|
|||
The top of the Device window displays general information about the specific Device, such as name, manufacturer, and serial number. | |||
The top of the Device window displays general information about the specific Device, such as name, manufacturer, associated layout, device flags, device ID and serial number. In addition, this section also display the current __sample frequency__ and __processing delay__ of the deivce. |
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.
The top of the Device window displays general information about the specific Device, such as name, manufacturer, associated layout, device flags, device ID and serial number. In addition, this section also display the current __sample frequency__ and __processing delay__ of the deivce. | |
The top of the Device window displays general information about the specific Device, such as name, manufacturer, associated layout, device flags, device ID and serial number. This section also displays the current __sample frequency__ and __processing delay__ of the device. |
|
||
__Sample frequency__ indicates the current frequency of samples or events the Input System is currently processing at given in Hertz (Hz). For devices receiving events this reflects the current flow of events received by the system, and for device receiving periodic readings this reflects the achievable sample rate of the system. The latter may be compared to the globally configured target sampling frequency, while achievable event frequency is uncorrelated to the sample polling frequency setting. | ||
|
||
__Processing delay__ indicates the average, minimum and maximum latency contribution from creating an input event or reading until the Input System has processed the same input event. Note that this excludes any additional input delay caused by OS, drivers or device communication. Also note that this excludes any additional output latency that may be caused by additional processing, rendering, GPU swap-chains or display refresh rate. |
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.
__Processing delay__ indicates the average, minimum and maximum latency contribution from creating an input event or reading until the Input System has processed the same input event. Note that this excludes any additional input delay caused by OS, drivers or device communication. Also note that this excludes any additional output latency that may be caused by additional processing, rendering, GPU swap-chains or display refresh rate. | |
__Processing delay__ indicates the average, minimum, and maximum time delay from when an input event or reading is created until the Input System has processed it. Note that this excludes any additional input delay caused by OS, drivers, or device communication. Also note that this excludes any additional output latency that may be caused by additional processing, rendering, GPU swap-chains, or display refresh rate. |
EditorGUILayout.LabelField(new GUIContent("Sample Frequency", sampleFrequencyTooltip), new GUIContent(m_DeviceFrequencyString), EditorStyles.label); | ||
const string processingDelayTooltip = | ||
"Displays the average, minimum and maximum observed input processing delay. This shows the time from " + | ||
"when an input event is first created within Unity until its processed by the Input System. " + |
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.
"when an input event is first created within Unity until its processed by the Input System. " + | |
"when an input event is first created within Unity until it's processed by the Input System. " + |
Packages/com.unity.inputsystem/InputSystem/Editor/Debugger/InputDeviceDebuggerWindow.cs
Outdated
Show resolved
Hide resolved
Improved documentation Co-authored-by: chrstphrsxtn <74899224+chrstphrsxtn@users.noreply.github.com>
…utDeviceDebuggerWindow.cs Co-authored-by: chrstphrsxtn <74899224+chrstphrsxtn@users.noreply.github.com>
Description
Adds achievable sample/event frequency (as well as sensor frequency and global polling frequency) to Input Debugger device window. These are just basic calculations over periods of one second at the moment similar to FPS counter.
Adds input processing delay (average, min, max) to Input Debugger device window.
The diagnostic values are similar to FPS updated once a second to show primarily the average. This is intended to help with diagnosing the behaviour of the system and devices and help spot problems.
Added 11 new test cases to cover the calculators as part of editor test assembly which required modification to accept unsafe code in the test assembly which I believe should be ok to change.
Target frequency shown is based on IOCTL sample frequency query response which I believe is only supported by sensors currently. Polling frequency is as we know global at the moment, but showing it since it relates to devices with polling backends which we currently cannot determine from the system. If both of these could be capability queried in the future we can make a better message here.
Testing status & QA
Tested manually with mouse, keyboard and gamepad on macOS during development.
Added unit tests to verify that calculations are working as expected.
Overall Product Risks
Very small. Has no impact on production or player code.
Comments to reviewers
If possible to cover sensors in review/QA it would be great.
Note that both label fields have tooltip that also needs review.
Checklist
Before review:
Changed
,Fixed
,Added
sections.InputLatencyCalculatorTests
andSampleFrequencyCalculatorTests
.During merge:
NEW: ___
.FIX: ___
.DOCS: ___
.CHANGE: ___
.RELEASE: 1.1.0-preview.3
.After merge: