NullReferenceException in Editor after a Hot-Swap when Graphy is disabled

[scscgit]

While in Editor, if Graphy is disabled (e.g. by default via Ctrl+H) and a Hot-Swap occurs (alt+tabbing into Unity after any script was modified), the OnApplicationFocus() calls UpdateParameters() on both FpsGraph and AudioGraph while their m_shaderGraph == null, throwing a NullReferenceException.

My solution is to simply return those functions if m_shaderGraph == null, so there should be some code review in order to make sure this doesn't cause any new issues. I noticed that in RamGraph, the UpdateParameters() calls Init() if any variable with a m_shaderGraph* prefix is null, so this may be possibly a better solution. Why are those classes even implemented differently, was it just an oversight?

My question is though, why are the Update* methods even getting called? If the Graphy is completely disabled, why does it even do anything, and why does it require any initialization? I've checked some of your README materials, and I don't see any "zero performance cost if disabled" stated anywhere. Does that mean this extension is unstable and shouldn't be used in production? Please make sure this isn't the case :)

Tested in Unity 2019.2.8f1

[scscgit]

Fixed in PR https://github.com/Tayx94/graphy/pull/69, but first please respond to my question about performance cost of this extension :)

[SuperPenguin]

Related to this https://github.com/Tayx94/graphy/issues/67, probably won't fix. When you toggle active to disable, it's the modules scripts that is disabled, the GraphyManager still enabled, so it's not zero performance when disabled either, Graphy still need GraphyManager to listen to any hotkey and re-enable the modules.

I took profiler sampling from my low-end 2013 laptop, when graphy toggled to disable, GraphyManager peak at 0.04 ms, 0.00-0.02 ms on average. New laptop/PC probably gonna peak even lower than that.

[scscgit]

Oh I didn't notice that #67 actually had the same issue, they didn't mention the critical detail that it only happens when Graphy is disabled, which is the case based on my observation. I noticed the response that it's "not worth fixing as recompiling while Unity is running is not supported and will create strange behaviours.". What the hell? Hot-swapping is a completely normal part of development, and the fix only takes a couple of seconds. If you don't understand how the lifecycle of functions like OnEnable work, then just let the contributors do it. I don't see any "strange behaviours" with hot-swaps, and it's not like there is any excuse to throw exceptions when the extension is completely disabled. (By the way, noone cares about minor visual glitches around Hot-Swaps.) Sure, if you want to take care of optional features like "Keep Alive", maybe there are some static fields you would have to check, It's not like we can't disable a feature or two if they are truly hard to support with Hot-Swap.

More importantly, I understand that it can't literally be "zero-performance cost", but the hotkey support is definitely acceptable compared to initialization and update costs :) I mean if it's only initialization, 0.04ms is acceptable I guess (exceptions aren't), but it should be only a matter of a single condition. Also a memory usage could be an other metric :)

[scscgit]

Actually now that I think more about the OnApplicationFocus() part of the lifecycle - this occurs every time a user clicks to the Game view (e.g. after interacting with a Scene or an Inspector), or when he clicks in any external window and returns back, and it completely wipes out all the histographs. Isn't that also a huge issue, as the wipe unnecessarily occurs even if everything is already initialized? I believe developers would prefer to work in editor without having the history reset every few seconds (as the main use-case is to aid in debugging). // EDIT: This also seems to get triggered e.g. when the game plays on Android, and the Google Play updates any app on the background. I wrongly thought this part could be removed entirely: if (m_initialized && isFocused){ RefreshAllParameters(); } But it looks the refresh is needed, so I found a better fix: if(m_fpsArray == null) could be added before m_fpsArray = new int[m_resolution]; in CreatePoints() of G_FpsGraph.cs. This seems to have no bad sice effects, and a similar fix could most likely be used even for graphs other than FPS. And by the way, the following m_shaderGraph.Array initialization loop seems to be completely redundant, as it's always overwritten :)