ShotgunNinja / Kerbalism

Hundreds of Kerbals were killed in the making of this mod.
The Unlicense
43 stars 19 forks source link

Enable/disable internal profiler at compile time #108

Closed PiezPiedPy closed 7 years ago

PiezPiedPy commented 7 years ago

Moved some initialization code out of the constructor and into an Awake() method. Now uses a compiler conditional called KERBALISM_PROFILER to enable conditional building of Profiler code.

ShotgunNinja commented 7 years ago

There is no reason to move the initialization code from ctor to awake. We can even argue than that break the RAII concept, but that's for another time.

The KERBALISM_PROFILER config 'macro' is also not necessary. The rationale of your change was: to allow switching profiler on/off by switching a config macro.

But when you use the profiler you will put begin/end calls (and scope objects) wherever you want to measure. So you are changing code, and at that point your rationale doesn't apply anymore. Same for when you want to stop profiling code.

Now we can argue that being/end methods of profiler could get empty bodies when KERBALISM_PROFILER is false, so that we can leave all the profiling calls around and just enable/disable the profiler with a macro switch.

But that doesn't work because:

So lets just keep it the way it is. When you want to profile the frame budget, 'enable' the profiler code and put some begin/end (or profile scope) calls wherever. When you are done profiling, 'disable' the profiler code and remove the profiling calls.

PiezPiedPy commented 7 years ago

The idea was to also to wrap any begin/end calls you use with a

 #if KERBALISM_PROFILER
 begin/end call
 #endif

The initialization code was moved to Awake because the changes made to KSP cause errors when you initialize in the constructor and so the initialization code has to be done in the Awake method.

PiezPiedPy commented 7 years ago

Note that the new calls are done in the constructor and so there should be no issues regarding RAII.

ShotgunNinja commented 7 years ago

The idea was to also to wrap any begin/end calls you use

Ah ok. That make sense actually.

The initialization code was moved to Awake because the changes made to KSP cause errors when you initialize in the constructor and so the initialization code has to be done in the Awake method.

Can you clarify this?

PiezPiedPy commented 7 years ago

Here's a snip from the KSP.log (while running in Development Build)

[LOG 05:29:49.972] [HighLogic]: =========================== Scene Change : From LOADING to MAINMENU =====================
[LOG 05:29:50.634] [AddonLoader]: Instantiating addon 'Profiler' from assembly 'Kerbalism'
[ERR 05:29:50.647] DontDestroyOnLoad is not allowed to be called from a MonoBehaviour constructor (or instance field initializer), call it in Awake or Start instead. Called from MonoBehaviour 'Profiler' on game object 'Profiler'.
See "Script Serialization" page in the Unity Manual for further details.
    UnityEngine.Object:DontDestroyOnLoad(Object)
    KERBALISM.Profiler:.ctor() (at D:/Private Shit/Visual Studio/Lambda Aerospace/Kerbalism/src/Utility/Profiler.cs:71)
    UnityEngine.GameObject:AddComponent(Type)
    AddonLoader:StartAddon(LoadedAssembly, Type, KSPAddon, Startup)
    AddonLoader:StartAddons(Startup)
    AddonLoader:OnLevelLoaded(Int32)
    AddonLoader:OnSceneLoaded(Scene, LoadSceneMode)
    UnityEngine.SceneManagement.SceneManager:Internal_SceneLoaded(Scene, LoadSceneMode)
ShotgunNinja commented 7 years ago

That is weird. I've been calling DontDestroyOnLoad(this) in constructors since the beginning. Ok then, lets move it to awake().