-
Notifications
You must be signed in to change notification settings - Fork 328
FIX: Narrowing scope of ENABLE_CLOUD_SERVICES_ANALYTICS #1991
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
Conversation
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.
Needs testing to verify as I'm not convinced this is correct
Packages/com.unity.inputsystem/InputSystem/NativeInputRuntime.cs
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 don't see the errors that I was getting before the original fix and our Input Automated test suites pass.
However, I would like Consoles QA to have a proper test of this before it is merged as they have better test projects than me.
#endif // UNITY_INPUT_SYSTEM_ENABLE_ANALYTICS || UNITY_2023_1_OR_NEWER | ||
#endif // UNITY_2023_2_OR_NEWER | ||
#elif (UNITY_ANALYTICS) // Implicitly: !UNITY_EDITOR | ||
#elif (ENABLE_CLOUD_SERVICES_ANALYTICS) // Implicitly: !UNITY_EDITOR && UNITY_ANALYTICS |
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.
Follow-up, logged into a console VM and built a basically empty project for target hardware.
Interestingly enough I got these results in editor (build profile set to console profile):
UNITY_EDITOR:True, UNITY_ANALYTICS:False, ENABLE_CLOUD_SERVICES_ANALYTICS:False
UnityEngine.Debug:Log (object)
Symbols:Start () (at Assets/Symbols.cs:26)
And on target device (player):
UNITY_EDITOR:False, UNITY_ANALYTICS:False, ENABLE_CLOUD_SERVICES_ANALYTICS:False
UnityEngine.Debug:Log (object)
Symbols:Start () (at Assets/Symbols.cs:26)
Additionally, I removed the ENABLE_CLOUD_SERVICES_ANALYTICS check completely from NativeInputRuntime.cs and I could still build for the console without issues, both in editor and player based on how it's currently defined. This confused me since the original fix states this was not possible?
I then removed all checks/code but the Analytics.XXX part and that fails to build in editor in this case. Hence, guarding this with ENABLE_CLOUD_SERVICES_ANALYTICS makes sense.
Note: Previously guarded with UNITY_ANALYTICS which also happen to be false and hence why observation in paragraph above worked to compile as-is.
I then removed all but the EditorAnalytics part. This compiles/runs fine in editor and is excluded in player since guarded with UNITY_EDITOR.
Hence it boils down to ENABLE_CLOUD_SERVICES_ANALYTICS should be used for access to Analytics, i.e. the analytics module. However, whether UNITY_ANALYTICS should be checked or not remains an enigma to me. Probably it should since we had it there previously?
#endif // UNITY_EDITOR | ||
|
||
#if UNITY_ANALYTICS || UNITY_EDITOR | ||
|
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.
Question remains whether UNITY_ANALYTICS should be a precondition 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.
On line #392
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 git history I would just assume it should, e.g. before I started to edit this it looked like this (dates back to 2018 when it was introduced) 6df6e11:
public void RegisterAnalyticsEvent(string name, int maxPerHour, int maxPropertiesPerEvent)
{
#if UNITY_ANALYTICS
const string vendorKey = "unity.input";
#if UNITY_EDITOR
EditorAnalytics.RegisterEventWithLimit(name, maxPerHour, maxPropertiesPerEvent, vendorKey);
#else
Analytics.Analytics.RegisterEvent(name, maxPerHour, maxPropertiesPerEvent, vendorKey);
#endif // UNITY_EDITOR
#endif // UNITY_ANALYTICS
}
public void SendAnalyticsEvent(string name, object data)
{
#if UNITY_ANALYTICS
#if UNITY_EDITOR
EditorAnalytics.SendEventWithLimit(name, data);
#else
Analytics.Analytics.SendEvent(name, data);
#endif // UNITY_EDITOR
#endif // UNITY_ANALYTICS
}
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.
Looks simpler and tidier and what I would expect.
Not sure the CHANGELOG should be fix in the same commit but I don't have a problem with it either.
@lewish-unity do you have some recommendations for console QA to verify the updated fix? I am pretty confident this is how it should be now. |
Hi @cristian-murarescu could you help us with the playstation platform testing? Just need to see if there's any issues building/running its player |
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.
Tested PS4 & PS5 player builds & runs successfully.
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.
LGTM, here's some things I checked:
- tried deploying to Xbox VM without issues
- checked that Analytics debugger shows same events as before when interacting with input related things (although I have to note that they seem kinda sparse in both versions, I don't know if I needed to enable anything extra but there wasn't anything for setting bindings, changing control schemes, etc. Things that were logged were context menu actions, interaction/processor adding and file saving)
- checked basic actions window functionality and testing scenes on Windows
Hmmm that sounds odd, I will do a final check myself on desktop before merging then. |
@Pauliusd01 I checked this branch with Analytics debugger but couldn't see anything that looked odd to me. |
Description
An attempt to revisit #1971 narrowing the introduced conditional compile guard to allow player analytics to pass through (like previous to 1.9.0) but gate editor analytics to when ENABLE_CLOUD_SERVICES_ANALYTICS is defined. See internal ticket ISX-2092 for reasoning. Still confused why this would make a difference for the target platform in the linked issue though, so still uncertain whether this is a correct fix.
Changes made
Narrowed scope of define introduced to not attempt to send editor analytics when platform not having it enabled.
Testing
Would need assistance, @lewish-unity if this tweak still resolves the build issue on consoles or if it reintroduces it.
Risk
Please describe the potential risks of your changes for the reviewers.
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: