Skip to content

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Sep 23, 2024
Merged

FIX: Narrowing scope of ENABLE_CLOUD_SERVICES_ANALYTICS #1991

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8fc32a8
FIX: Attempt to narrow scope to only editor analytics for previous bu…
ekcoh Aug 29, 2024
8c0cbd0
Added CHANGELOG entry
ekcoh Aug 29, 2024
90dfc1e
Merge branch 'develop' into isx-2092
ekcoh Sep 4, 2024
418c6cb
Moved services check to the correct conditional compilation check
ekcoh Sep 11, 2024
1b697ee
Merge branch 'develop' into isx-2092
ekcoh Sep 11, 2024
b6ab42e
Merge branch 'develop' into isx-2092
ekcoh Sep 13, 2024
aa54349
Merge branch 'develop' into isx-2092
ekcoh Sep 16, 2024
1c12e65
Merge branch 'develop' into isx-2092
ekcoh Sep 16, 2024
69cc757
Merge branch 'develop' into isx-2092
lyndon-unity Sep 20, 2024
43a5775
Merge branch 'develop' into isx-2092
ekcoh Sep 23, 2024
81fbd7c
Merge branch 'develop' into isx-2092
ekcoh Sep 23, 2024
4be0f1c
FIX: formatting error in CHANGELOG
ekcoh Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ however, it has to be formatted properly to pass verification tests.
- Fixed 'OnDrop' event not called when 'IPointerDownHandler' is also listened. [ISXB-1014](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1014)
- Fixed InputSystemUIInputModule calling pointer events on parent objects even when the "Send Pointer Hover To Parent" is off. [ISXB-586](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-586)
- Improved performance of disconnected device activation (ISX-1450)
- Fixed conditional compilation for non-editor analytics on platforms not enabling analytics.

### Added
- Added Hinge Angle sensor support for foldable devices.
- Added InputDeviceMatcher.WithManufacturerContains(string noRegexMatch) API to improve DualShockSupport.Initialize performance (ISX-1411)
- Added an IME Input sample scene.

### Changed
- Use `ProfilerMarker` instead of `Profiler.BeginSample` and `Profiler.EndSample` when appropriate to enable recording of profiling data.
Expand Down
4 changes: 1 addition & 3 deletions Packages/com.unity.inputsystem/InputSystem/NativeInputRuntime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,6 @@ public Action onProjectChange

Copy link
Collaborator Author

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...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On line #392

Copy link
Collaborator Author

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
        }

public void SendAnalytic(InputAnalytics.IInputAnalytic analytic)
{
#if ENABLE_CLOUD_SERVICES_ANALYTICS
#if (UNITY_EDITOR)
#if (UNITY_2023_2_OR_NEWER)
EditorAnalytics.SendAnalytic(analytic);
Expand All @@ -405,15 +404,14 @@ public void SendAnalytic(InputAnalytics.IInputAnalytic analytic)
EditorAnalytics.SendEventWithLimit(info.Name, analytic);
#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
Copy link
Collaborator Author

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?

var info = analytic.info;
Analytics.Analytics.RegisterEvent(info.Name, info.MaxEventsPerHour, info.MaxNumberOfElements, InputAnalytics.kVendorKey);
if (analytic.TryGatherData(out var data, out var error))
Analytics.Analytics.SendEvent(info.Name, data);
else
Debug.Log(error); // Non fatal
#endif //UNITY_EDITOR
#endif //ENABLE_CLOUD_SERVICES_ANALYTICS
}

#endif // UNITY_ANALYTICS || UNITY_EDITOR
Expand Down