refactor: Remove exceptions from Network Spawn Manager#3933
refactor: Remove exceptions from Network Spawn Manager#3933noellie-velez wants to merge 16 commits intodevelop-2.0.0from
Conversation
NoelStephensUnity
left a comment
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!
👍
| // 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!
😻
| 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.
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.
There was a problem hiding this comment.
I hadn’t seen your message before syncing with Emma, let me know if this fix works.
NoelStephensUnity
left a comment
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").
| 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.
2 participants
Purpose of this PR
Remove exceptions from Network Spawn Manager
Jira ticket
MTT-14791
Changelog
Documentation
Testing & QA (How your changes can be verified during release Playtest)
Functional Testing
Manual testing :
Manual testing doneAutomated tests:
Covered by existing automated testsCovered by new automated testsDoes the change require QA team to:
Review automated tests?Execute manual tests?Provide feedback about the PR?If any boxes above are checked the QA team will be automatically added as a PR reviewer.
Backports