refactor(NetworkManagerGUI): prevent potential NRE spam if reference is lost and tidy up #1130
Conversation
This file contains 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
Somehow NMGUI can lose references for some reason, so short-circuit to prevent log spam.
"Add parentheses for clarity"
If auto configure is set but NetworkManager field is not null, then don't do anything.
SoftwareGuy
changed the title
|
Example of what it looks like now:
|
Remove regions as per James request
James-Frowen
reviewed
Feb 2, 2023
| // Is this STILL null? Then we throw in the towel and go home. | ||
| if (NetworkManager == null) | ||
| { | ||
| throw new ArgumentNullException("NetworkManager", $"You requested automatic configuration for the NetworkManagerGUI component on '{gameObject.name}'" + |
There was a problem hiding this comment.
this should be InvalidOperationException instead of Argument
James-Frowen
reviewed
Feb 2, 2023
| // Coburn, 2023-01-29: | ||
| // If automatic configuration of NetworkManager is enabled, then attempt to grab the | ||
| // NetworkManager that this script is attached to. | ||
| if (AutoConfigureNetworkManager) |
There was a problem hiding this comment.
if it hard to follow this chain of if/else,, would be better to do early returns instead
James-Frowen
reviewed
Feb 2, 2023
| { | ||
| if (logger.logEnabled) | ||
| { | ||
| logger.LogWarning($"You have enabled Auto Configure for this NetworkManagerGUI component on '{gameObject.name}' but the NetworkManager field is already set. " + |
There was a problem hiding this comment.
I dont think we need this warning.
|
🎉 This PR is included in version 129.6.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.
While chasing a ghost error due to Unity adding two NetworkManagerGUIs to one object and I forgot to check that, I discovered that the component and spam NREs in its OnGUI loop.
So, what this PR does is quite a fair bit actually...
ArgumentNullExceptionand tell them to fix it. However, I have implemented it so if you do specify a NetworkManager reference and leave it ticked to automatically configure itself that it will not override your manual reference.As an added bonus, it also addresses Visual Studio complaining about IDE0048: "Add parentheses for clarity" by... you name it, adding parentheses around some Rect math.
Mirage. Makin' NetworkManageGUI great again.