fix: prevent OnPerformanceTickData from sending data on null#586
fix: prevent OnPerformanceTickData from sending data on null#586mattwalsh-unity merged 5 commits intodevelopfrom
Conversation
…er never allocates data. in these cases there is no point in rendering.
com.unity.multiplayer.mlapi/Runtime/Profiling/ProfilerCountersInfo.cs
Outdated
Show resolved
Hide resolved
1d42ee0 to
58693eb
Compare
| if (NetworkConfig.NetworkTransport is ITransportProfilerData profileTransport) | ||
| var data = PerformanceDataManager.GetData(); | ||
| var eventHandler = OnPerformanceDataEvent; | ||
| if (eventHandler != null && data!= null) |
There was a problem hiding this comment.
I guess you could warn and say "Did you forget to call RegisterMLAPIPerformanceEvent() first?"
| } | ||
|
|
||
| OnPerformanceDataEvent?.Invoke(PerformanceDataManager.GetData()); | ||
| eventHandler?.Invoke(data); |
There was a problem hiding this comment.
| eventHandler?.Invoke(data); | |
| eventHandler.Invoke(data); |
unnecessary null check.
0xFA11
left a comment
There was a problem hiding this comment.
since things changed a lot after I approved, I'll take that back and re-review again.
also please update the PR title again matching the changes accordingly.
| } | ||
| else if (data == null) | ||
| { | ||
| NetworkLog.LogWarning("No data available. Did you forget to call PerformanceDataManager.BeginNewTick() first?"); |
There was a problem hiding this comment.
Some suggestions for this log, in case it makes its way to users:
- "No data available" should be more specific. "No performance data available." perhaps
- "Did you forget to call..." this will confuse users because it's not something they are supposed to call. At least i think it should be made sure that this is an internal error, and not something they can necessarily fix
There was a problem hiding this comment.
minor: I'd also suggest string interpolation syntax and nameof operator here.
NetworkLog.LogWarning($"No data available. Did you forget to call {nameof(PerformanceDataManager)}.{nameof(PerformanceDataManager.BeginNewTick)}() first?");
There was a problem hiding this comment.
Agreed, that's worth a quick fix to use the future-proofed naming system. We've started to adopt that make make it our standard.
0xFA11
left a comment
There was a problem hiding this comment.
I couldn't remove myself from the reviewers list, so here I'm approving it 😛
If something causes an exception it's possible that the profiler never allocates data. in these cases, there is no point in sending data outside of MLAPI