NEW: Insights analytics support for the Input System#2272
NEW: Insights analytics support for the Input System#2272
Conversation
70a4618 to
c93bcdb
Compare
ekcoh
left a comment
There was a problem hiding this comment.
Looks good, but would suggest to use interface similar to other module facing calls given the current test fixture design.
Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs
Outdated
Show resolved
Hide resolved
| }, | ||
| { | ||
| "name": "Unity", | ||
| "expression": "6000.5.0a5", |
There was a problem hiding this comment.
This will have to be changed to whatever is the actual Unity version we happen to release Input insights in. We will know that once the native PR lands. Then we will change this for every backport
|
|
||
| #endif // UNITY_ANALYTICS || UNITY_EDITOR | ||
| // We don't want to populate Insights from within tests, even when running them in players | ||
| public void LogDeviceConnectedInsight(InputDeviceDescription description) |
There was a problem hiding this comment.
Might want to store these in harness in case you want to test it in the package
|
Looks like you need to (re)run formatter on this one |
| m_Runtime.LogDeviceConnectedInsight(description); | ||
|
|
There was a problem hiding this comment.
I see that logging the input action is bracketed by the new define UNITY_INPUT_SYSTEM_SUPPORTS_INSIGHTS. Should this also be conditional on it?
| m_Runtime.LogDeviceDisconnectedInsight(device.description); | ||
|
|
There was a problem hiding this comment.
Similar: should this be #if UNITY_INPUT_SYSTEM_SUPPORTS_INSIGHTS?
| public void LogInputActionInsight(InputAction action) | ||
| { | ||
| #if UNITY_INPUT_SYSTEM_SUPPORTS_INSIGHTS && !UNITY_EDITOR | ||
| NativeInputSystem.LogInputActionInsight(action.name, action.type.ToString()); |
There was a problem hiding this comment.
Not too familiar with input actions, I'm worried that enabling this will add a string allocation to every input action, resulting in two potential issues: performance degradation of all inputs (allocation perf) and frequent / large garbage collection (after many disparate allocations). Can this be cached? Do we have the action type string already stored somewhere? Is there another reason that invalidates the concern?
| #if UNITY_INPUT_SYSTEM_SUPPORTS_INSIGHTS | ||
| // also report all actions to insights | ||
| foreach (var inputAction in map.actions) | ||
| { | ||
| InputSystem.s_Manager.m_Runtime.LogInputActionInsight( inputAction ); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Was looking at this again and am a little confused. Is LogInputActionInsight for when the InputAction happens, or for when it's enabled?
I think as it is right now, we're sending insights for all the actions, but only once (when they're all enabled). We're not tracking when the actions happen. Is that the intent?
Description
This PR brings support for the Insights analytics module to Input System.
Requires native side PR to land first: https://github.cds.internal.unity3d.com/unity/unity/pull/83832
Currently, we send these kinds of data to Insights:
Testing status & QA
Local testing
Overall Product Risks
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: