googleanalytics / google-analytics-plugin-for-unity

Google Analytics plugin for the Unity game creation system
Apache License 2.0
386 stars 140 forks source link

The plugin breaks Unity hotswapping #69

Open cobbpg opened 9 years ago

cobbpg commented 9 years ago

When the plugin is active and I recompile while playing the game, I get an endless explosion of errors. The solution is simple: on the line https://github.com/googleanalytics/google-analytics-plugin-for-unity/blob/1ecf8923879c3b002db5edcdaa5d83d321495e47/source/Plugins/GoogleAnalyticsV3/GoogleAnalyticsV3.cs#L133 the check for null should be replaced with !string.IsNullOrEmpty(uncaughtExceptionStackTrace), since Unity replaces nulls with empty strings when deserialising.

sebastienalcamo commented 8 years ago

since Unity replaces nulls with empty strings when deserialising.

It seems not true, string is a Serializable type.

In my opinion, the root cause of this problem is one step further. The complex static variables (like GoogleAnalyticsV3.instance) are not serialized and are reset to null after a successful hot swap. If you have set GA "UncaughtExceptionReporting" to true, the plugin will try to log any exceptions thrown. After a hot swap, on the next SendGaHitWithMeasurementProtocol, there will an exception thrown that will lead to a new SendGaHitWithMeasurementProtocol that will lead to another exception, etc... I guess that stracktraces returned by Unity are inconsistent after a hot swap, which explains that uncaughtExceptionStackTrace is an empty string.

This kind of scenario only happens when running the application in Unity editor. Unity's Hotswapping can't work since we use non-serializable data types.

cobbpg commented 8 years ago

Part of the problem is that the plugin attempts to report errors after a hotswap even if UncaughtExceptionReporting is set to false. And yes, string is serialisable, but due to the way Unity's system works, anything that doesn't inherit from UnityEngine.Object is treated as a value type by the serialiser, so nulls are replaced by default values. This is very easy to verify: just add a private string member to any MonoBehaviour, never set it, log whether it's null, then perform a hotswap.

By making the change I proposed, the annoying deluge of errors can be prevented, which is "good enough" for most purposes.

sebastienalcamo commented 8 years ago

I just checked and you were right about the string variable set to an Empty string after a hotswap. This is not the behaviour i was expecting. Thank you.

I agree about the !string.IsNullOrEmpty(uncaughtExceptionStackTrace) update. Do you always work with hot swapping ?

cobbpg commented 8 years ago

Based on past experience where we ignored hotswapping, we have decided a while ago to make a conscious effort to support it as much as possible, because it’s a huge productivity win especially as your project grows and your recompile-execute-test cycle becomes longer.

As it turns out, there’s a solution for almost everything and so far we didn’t need to compromise much in terms of code organisation. For instance, static variables can be replaced with static getters that recreate instances on demand. The only thing that doesn’t seem to have an easy workaround is fields of interface types; those you just have to avoid for the time being. Also, you need to be able to recreate your callbacks from the serialised state, which can get tricky but in the end it is a matter of designing for hotswapping from the ground up.