fix: prevent OnPerformanceTickData from sending data on null #586
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…er never allocates data. in these cases there is no point in rendering.
kvassall-unity
requested review from
Rosme,
0xFA11,
LukeStampfli and
becksebenius-unity
com.unity.multiplayer.mlapi/Runtime/Profiling/ProfilerCountersInfo.cs
Outdated
Show resolved
Hide resolved
0xFA11
changed the title
kvassall-unity
force-pushed
the
feature/profiler_null_check
branch
from
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.
Suggested change
| eventHandler?.Invoke(data); | |
| eventHandler.Invoke(data); |
unnecessary null check.
0xFA11
suggested changes
Mar 11, 2021
kvassall-unity
changed the title
| } | ||
| 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.
becksebenius-unity
approved these changes
Mar 11, 2021
0xFA11
approved these changes
Mar 12, 2021
mattwalsh-unity
approved these changes
Mar 12, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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