refactor: Remove unnecessary exceptions and refactor logging on NetworkSpawnManager and NetworkObject - #3933
Conversation
Sorry, something went wrong.
NoelStephensUnity
left a comment
There was a problem hiding this comment.
The only suggestion I have is adding a changelog entry for this PR before merging it.
The rest looks great to me!
👍
Sorry, something went wrong.
| // In distributed authority mode, we send a single message that is broadcasted to all clients | ||
| // that will be shown the object (i.e. 1 message to service that then broadcasts that to the | ||
| // targeted clients). When using a DAHost, we skip this and send like we do in client-server | ||
| // In distributed authority mode, we send a single message to the service, |
There was a problem hiding this comment.
Nice clean up of this comment!
😻
Sorry, something went wrong.
| return; | ||
| } | ||
|
|
||
| if (networkManager == null || !networkManager.IsListening) |
There was a problem hiding this comment.
Can networkManager be null here? If so, wouldn’t that already cause an exception on line 2299 (networkManager.ShutdownInProgres) and again on line 2312 (networkManager.DistributedAuthorityMode)?
I’d suggest either removing this null check, or checking for null at the start of the function if it can actually be empty.
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah...that is a good call-out.
Moving this just under line 2298 make much more sense (especially since we do a check against it (currently) on line 2299.
Sorry, something went wrong.
There was a problem hiding this comment.
I hadn’t seen your message before syncing with Emma, let me know if this fix works.
Sorry, something went wrong.
NoelStephensUnity
left a comment
There was a problem hiding this comment.
Looks good @noellie-velez !
Added a suggested change back to the networkManager vs NetworkManagerOwner.
This was originally added to handle the issue with running multiple NetworkManager instances within the same application domain but after looking around a bit it looks like it is starting to be used a bit more than expected.
Really, the comment above this field (screenshot below) is a bit vague and only hints that it is specific to integration testing (i.e. "Multi NetworkManagers").
Sorry, something went wrong.
| return; | ||
| } | ||
|
|
||
| if (networkManager == null || !networkManager.IsListening) |
There was a problem hiding this comment.
Yeah...that is a good call-out.
Moving this just under line 2298 make much more sense (especially since we do a check against it (currently) on line 2299.
Sorry, something went wrong.
Purpose of this PR
Remove exceptions from Network Spawn Manager
Refactor logging; fix tests and cleanup
Refactor logging:
Tests:
Cleanup:
Remove unreachable code, redundant branches, and unused comments
General formatting and code cleanup
Update changelog
Address review feedback
Jira ticket
MTT-14791
Changelog
Documentation
Testing & QA (How your changes can be verified during release Playtest)
Functional Testing
Manual testing :
Automated tests:
Does the change require QA team to:
If any boxes above are checked the QA team will be automatically added as a PR reviewer.
Backports