refactor!: NetworkBehaviour and NetworkObject NetworkManager instance can now be overriden #762
Conversation
… can now be overriden BREAKING CHANGE: Static lifecycle properties in NetworkBehaviour are no longer static
| public NetworkManager NetworkManager => NetworkManagerTestOverride != null ? NetworkManagerTestOverride : NetworkManager.Singleton; | ||
|
|
||
| internal NetworkManager NetworkManagerTestOverride; |
There was a problem hiding this comment.
we should have #if UNITY_EDITOR || DEVELOPMENT_BUILD surrounding this.
(we wouldn't have NetworkManagerTestOverride in non-Editor, non-Development builds)
There was a problem hiding this comment.
Not too sure I agree tbh. This is internal behaviour which we might want exposed eventually. I generally prefer not to conditional compile things if we don't have to.
Perhaps renaming it and removing the word "Test" from the field name?
There was a problem hiding this comment.
we're not there yet.
let's keep this more focused on testing for now.
There was a problem hiding this comment.
We're not there yet, but we want to move there as quickly as possible. Part of this epic is to prepare for that future when we are there. IFDefing is going against of the goal outlined in the brief.
There was a problem hiding this comment.
we're not there yet, today.
we're adding a very small overhead and extra member into release builds but in reality, it doesn't actually have to be there.
when the use-case and implementations in other places started to become more generic, then we can roll it out to other places as well.
| public NetworkManager NetworkManager => NetworkManagerTestOverride != null ? NetworkManagerTestOverride : NetworkManager.Singleton; | |
| internal NetworkManager NetworkManagerTestOverride; | |
| #if UNITY_EDITOR || DEVELOPMENT_BUILD | |
| public NetworkManager NetworkManager => NetworkManagerTestOverride != null ? NetworkManagerTestOverride : NetworkManager.Singleton; | |
| internal NetworkManager NetworkManagerTestOverride; | |
| #else | |
| public NetworkManager NetworkManager => NetworkManager.Singleton; | |
| #endif |
There was a problem hiding this comment.
I'm looking at the internal gdoc proposal, where did you outline upcoming changes you're referring to? are they somewhere else?
This part:
This partial step is also a clear path forward to remove Singleton dependency fully from the library and allow users to also run multiple NetworkManagers at once.
is what this is doing. Preparing the library for that stage.
There was a problem hiding this comment.
yeah, I see. let's have a chat next week about this.
There was a problem hiding this comment.
I've been bitten by #ifdef blocked code enough times to err on the side of not #ifdef gating things when not necessary.
And I would wonder, can we count on DEVELOPMENT_BUILD being set when we're doing testing? What if we're testing a fully production build?
But I would stipulate we decorate this variable however we name it with a comment, something to the effect of "in normal use, NetworkObject's all share the one-and-only-one networking manager. For testing and some anticipated future uses, we allow this networking manager to be overridden so that objects can belong to multiple different NetworkManagers"
There was a problem hiding this comment.
I'm not sure why this conversation marked as resolved. I'm still not entirely happy about rolling something out over runtime without proper planning, especially if something was targeting testing/debugging use-cases only somehow started to effect runtime code.
please organize an internal meeting and let's try to align on our future plans together.
There was a problem hiding this comment.
Resolving after internal alignment!
| } | ||
|
|
||
| if (clientId == NetworkManager.Singleton.ServerClientId) | ||
| NetworkManager networkManager = networkObjects[0].NetworkManager; |
There was a problem hiding this comment.
Same comment as above, can this not be (down in the loop):
| NetworkManager networkManager = networkObjects[0].NetworkManager; | |
| NetworkManager networkManager = networkObjects[i].NetworkManager; |
There was a problem hiding this comment.
Nope, because of how things gets packed. What we do is:
- Get the first NetworkManager
- Iterate all NetworkObjects in a safety-check loop to ensure they are all spawned and such. We also check that they all belong to the NetworManager we previouly grabbed.
- Send off the packet
| // Set override | ||
| networkObject.NetworkManagerTestOverride = networkManager; | ||
|
|
||
| Debug.Assert(networkObject.NetworkManager == networkManager); |
There was a problem hiding this comment.
Especially if you agree with my above comment, a good test would be to create a list of objects that belong to different NM's and then call NetworkHide / NetworkShow and make sure it works.
There was a problem hiding this comment.
It's not meant to work :)
If you call the static versions of NetworkShow and NetworkHide on objects with different NetworkManagers it should throw an exception.
Comment added to NetworkManagerOwner. |
TwoTenPvP
force-pushed
the
singleton-removal-network-object
branch
from
730fc4d to
d77f930
Compare
TwoTenPvP
force-pushed
the
singleton-removal-network-object
branch
from
d77f930 to
84cf1c1
Compare
BREAKING CHANGE: Static lifecycle properties in NetworkBehaviour are no longer static