Singleton Removal #11
Conversation
|
|
| - If you have multiple `NetworkingManager`s in one scene (for example, to talk to two different servers at once), then you should take care that all placed `NetworkedObject`s be wired to the `NetworkingManager` they are associated with via a serialized field. | ||
| - `NetworkedObject`s spawned by a `NetworkingManager` will always be bound to the `NetworkingManager` that created them, and be placed in the same scene as their parent `NetworkingManager`. | ||
| - If launching an in-process server, be sure to only call `StartClient` after the `OnServerStarted` callback has fired. | ||
| - Currently only the ENet Transport is supported. UNet shares a common static at the Unity level (`NetworkTransport`), which prevents it from being fully separated. |
There was a problem hiding this comment.
This may be a showstopper; we can't live without UNet at least initially (and it's included in the SDK). ENet support is actually not in the current scope, and actually is only in the core SDK right now due to an oversight (though we certainly want Enet to work)
There was a problem hiding this comment.
Also, how about the UTP impl?
| - `NetworkedObject`s spawned by a `NetworkingManager` will always be bound to the `NetworkingManager` that created them, and be placed in the same scene as their parent `NetworkingManager`. | ||
| - If launching an in-process server, be sure to only call `StartClient` after the `OnServerStarted` callback has fired. | ||
| - Currently only the ENet Transport is supported. UNet shares a common static at the Unity level (`NetworkTransport`), which prevents it from being fully separated. | ||
| - We have removed the old `NetworkingManager.Singleton` method. For migrators, if you are writing code that in a component sitting on a `NetworkedObject`, you can simply get the `NetworkedObject`s `NetManager` instead. For other code, you will need to adopt your own strategy (for example, finding the `NetworkingManager` in the same scene as yourself via [`GameObject.FindWithTag`](https://docs.unity3d.com/ScriptReference/GameObject.FindWithTag.html)). |
There was a problem hiding this comment.
For consistenct, NetManager probably should be NetworkingManager
There was a problem hiding this comment.
+1 (at least for now). We have plans to rename/refactor API a little bit very soon, we would change some names but at least for now, let's keep it consistent with existing namings.
| - If launching an in-process server, be sure to only call `StartClient` after the `OnServerStarted` callback has fired. | ||
| - Currently only the ENet Transport is supported. UNet shares a common static at the Unity level (`NetworkTransport`), which prevents it from being fully separated. | ||
| - We have removed the old `NetworkingManager.Singleton` method. For migrators, if you are writing code that in a component sitting on a `NetworkedObject`, you can simply get the `NetworkedObject`s `NetManager` instead. For other code, you will need to adopt your own strategy (for example, finding the `NetworkingManager` in the same scene as yourself via [`GameObject.FindWithTag`](https://docs.unity3d.com/ScriptReference/GameObject.FindWithTag.html)). | ||
| - Note that the MLAPI Profiler doesn't support profiling multiple `NetworkingManager`s simultaneously. If run when multiple `NetworkingManager`s are active, it will profile the first `NetworkingManager` it finds. |
There was a problem hiding this comment.
Long term we're migrating to the Unity profiler, this should be ok, but good to call it out. I suppose the profiler could pop a warning in the log if > 1 NetworkingManagers are in play?
There was a problem hiding this comment.
Update, we have committed to eliminating the MLAPI profiler for March
| # Summary | ||
| [summary]: #summary | ||
|
|
||
| This change removes the `NetworkingManager.Singleton` static, as well as the majority of other static state in MLAPI. |
There was a problem hiding this comment.
So usability-wise, one still creates an empty object, then adds the NetworkingManager component to it, yes?
Looking at scale demo, most of the singleton accesses are on IsClient / IsServer. So would is code...
public class ExitButtonScript : MonoBehaviour
{
public void OnExitScene() {
if(NetworkingManager.Singleton.IsClient) {
NetworkingManager.Singleton.StopClient();
}
}
}
....turn into...
public class ExitButtonScript : MonoBehaviour
{
public void OnExitScene() {
if(NetManager.IsClient) {
NetManager.StopClient();
}
}
}
There was a problem hiding this comment.
that's right--you'd have the NetworkingManager you wanted to interact with dragged in as serialized field.
| [guide-level-explanation]: #guide-level-explanation | ||
|
|
||
| MLAPI supports having multiple `NetworkingManager`s running concurrently. There are a few things to keep in mind when doing so: | ||
| - The simplest case is to only have one `NetworkingManager` per scene. This is what you do to support an in-process server: load a "server" version of your scene, and then call `StartServer` on the `NetworkingManager` in it. All "server" physics calculations must then be performed via the `PhysicsScene` associated with the server scene. |
There was a problem hiding this comment.
Also, does the scene teardown automatically tear down its associated NetworkingManager?
There was a problem hiding this comment.
I think if you haven't marked the NetworkingManager to be DontDestroyOnLoad, then the NetworkingManager gets destroyed when its scene goes away. I believe the NetworkingManager does sensibly handle this, but if it doesn't, it wouldn't be hard to make it clean itself up in its OnDestroy.
| MLAPI supports having multiple `NetworkingManager`s running concurrently. There are a few things to keep in mind when doing so: | ||
| - The simplest case is to only have one `NetworkingManager` per scene. This is what you do to support an in-process server: load a "server" version of your scene, and then call `StartServer` on the `NetworkingManager` in it. All "server" physics calculations must then be performed via the `PhysicsScene` associated with the server scene. | ||
| - If you have multiple `NetworkingManager`s in one scene (for example, to talk to two different servers at once), then you should take care that all placed `NetworkedObject`s be wired to the `NetworkingManager` they are associated with via a serialized field. | ||
| - `NetworkedObject`s spawned by a `NetworkingManager` will always be bound to the `NetworkingManager` that created them, and be placed in the same scene as their parent `NetworkingManager`. |
There was a problem hiding this comment.
I'm a little bit curious about inter-NetworkManager transfer of NetworkObject/NetworkBehaviour — also sharing the same NetworkObject across multiple NetworkManagers. I'm not asking for a solution here but wanted to call out to spike some interest.
Having said that, more persistent/resilient/reliable network ID generation would greatly help in this area as well.
There was a problem hiding this comment.
Hmm, the only use-case I can think of for two NetworkManagers sharing the same NetworkObject is pretty advanced: the "seamless world" problem of having two adjacent blocks, managed by two different servers, and supporting a networked object crossing from server A to server B. I agree that would be nice to support at the MLAPI level, and probably would require a globally unique networkID.
| - `NetworkedObject`s spawned by a `NetworkingManager` will always be bound to the `NetworkingManager` that created them, and be placed in the same scene as their parent `NetworkingManager`. | ||
| - If launching an in-process server, be sure to only call `StartClient` after the `OnServerStarted` callback has fired. | ||
| - Currently only the ENet Transport is supported. UNet shares a common static at the Unity level (`NetworkTransport`), which prevents it from being fully separated. | ||
| - We have removed the old `NetworkingManager.Singleton` method. For migrators, if you are writing code that in a component sitting on a `NetworkedObject`, you can simply get the `NetworkedObject`s `NetManager` instead. For other code, you will need to adopt your own strategy (for example, finding the `NetworkingManager` in the same scene as yourself via [`GameObject.FindWithTag`](https://docs.unity3d.com/ScriptReference/GameObject.FindWithTag.html)). |
There was a problem hiding this comment.
Rather than finding NetworkManager with GameObject.FindWithTag, it might be useful to have a static NetworkManager[] array globally where we could quickly iterate over and find the one we're looking for (just for eye-candy and ease-of-use :) )
|
|
||
| `NetworkingManager.Singleton` is removed. `SpawnManager`, `InternalMessageSender`, `InternalMessageHandler`, `NetworkSceneManager`, and `BitReaderPool` have all been made non-static, and hang off the `NetworkingManager` that owns them. | ||
|
|
||
| `NetworkedObject`s have a `NetManager` field that is stamped on them at their moment of creation (when spawned by MLAPI), or should already be set in editor (for placed objects). If still unset by the time `NetworkedObject.Start` runs, the `NetworkedObject` takes the first `NetworkingManager` it finds in the same scene as itself. |
There was a problem hiding this comment.
I think this implicit behavior is somewhat shaky IMHO. I wouldn't expect NetworkManager or NetworkObject to find each other automatically unless set explicitly. There might be a good chance to create another NetworkLinker or something similar that has multiple linking options which could link NetworkObject/NetworkBehaviour to NetworkManager. In one of the modes, NetworkLinker could find all unregistered NetworkObjects and register them with the first NetworkManager instance in the scene. Or maybe NetworkLinker could be further extended in the future where is routes registration of NetworkObject based on some sort of condition and maps it to a specific NetworkManager instance. To put more simply, I think outsourcing this particular need to another 3rd NetworkLinker component would be a better approach except linking them by hand explicitly. What do you think? :)
There was a problem hiding this comment.
I think I could live with this. I was tempted to suggest it assert loud and proud rather than magically fixing the NetworkingManager (NM), but then I think "when won't I want an object I instantiated in the scene to not be managed by that scene".
I am curious though how you think one would set the NM of an object in the editor? Would the NMs be enumerable to where you could see a pickable list in the editor? Or would the setting of the NM need to be in code?
There was a problem hiding this comment.
Currently, NetworkManager is a MonoBehaviour which means it's being serialized in Editor window therefore one could set a NetworkObject's NetworkManager field in the Editor via inspector window. We could even script NetworkObject editor window entirely to give it a completely customized look in the inspector. There are lots of ways to achieve that in the Editor.
But having said that, this work is not required nor essential so we could address object-manager linking in the future and keep this auto-register idea around for now. I do not want to extend the scope here.
There was a problem hiding this comment.
My approach changed a little bit when I actually came to implement this. Now it's the NetworkManager that stamps itself on the NetworkObject (via NetworkSpawnManager.ServerSpawnSceneObjectsOnStartSweep and the equivalent path on the client).
Importantly, the NetworkManager won't stamp itself on a NetworkObject that is already claimed by a NetworkManager. Right now, this creates a worrying order-dependency for startup. If you have a "Chat" NetworkManager and a "Game" NetworkManager, you better hope the "Game" one starts up first!
I think a good enhancement would be a NetworkConfig.IgnoreSceneObjects option that you could set to true. That way you could enforce that your Chat NetworkManager never steals your scene objects.
|
|
||
| `NetworkedObject`s have a `NetManager` field that is stamped on them at their moment of creation (when spawned by MLAPI), or should already be set in editor (for placed objects). If still unset by the time `NetworkedObject.Start` runs, the `NetworkedObject` takes the first `NetworkingManager` it finds in the same scene as itself. | ||
|
|
||
| A few things have been left static. `BitWriterPool` doesn't make use of `NetworkingManager` at all, and has consequently been left shared. The `LogLevel` has also been left static, as it is a bother to thread through into some of the places where it is used (however, I can see the argument for biting the bullet and doing so, as it would be nice to see whether log lines were generated from the server or client, respectively). |
There was a problem hiding this comment.
Sounds like Server/Client/Host origin could be an (optional?) parameter in the log method signatures?
There was a problem hiding this comment.
Yeah, I really was missing that in my initial test. I have implemented it in this round. I maintained static "LogInfo" etc methods to be used when there was no other choice, but preferred non-static versions everywhere possible--these add the tag you mention, which makes the logging a lot more legible.
|
|
||
| A few things have been left static. `BitWriterPool` doesn't make use of `NetworkingManager` at all, and has consequently been left shared. The `LogLevel` has also been left static, as it is a bother to thread through into some of the places where it is used (however, I can see the argument for biting the bullet and doing so, as it would be nice to see whether log lines were generated from the server or client, respectively). | ||
|
|
||
| For editor code, there is a new static method: `NetworkingManagerEditor.GetAnyNetworkingManager` that just wraps a [`FindObjectOfType`](https://docs.unity3d.com/ScriptReference/Object.FindObjectOfType.html) call. This is the replacement for editor code that needed to operate on "the NetworkingManager" (back when there was only one). |
There was a problem hiding this comment.
Similar to what I said above, we could store NetworkManager instances in a static NetworkManager[] container to avoid Object.FindObjectOfType call.
There was a problem hiding this comment.
Say we have that static array...if it's just a numeric array, does referencing it work? Like would one specify NetworkManager[2] vs. NetworkManager[1]?
OTOH, you could have the key be a string and then you could get to the NM via NetworkManager["chat"] or NetworkManager[""] for the default.
@dwoodruffsf thoughts on this central static array?
There was a problem hiding this comment.
oh, static array or hashmap, the main idea being avoiding gameobject hierarchy traversal with Object.FindObjectOfType because seems like it could be unnecessary with a faster centralized static container lookup.
| # Unresolved questions | ||
| [unresolved-questions]: #unresolved-questions | ||
|
|
||
| - Should we maintain a `NetworkingManager.Singleton` for the convenience of people not using this feature? |
There was a problem hiding this comment.
I'd highly argue against having 2 ways of doing things — my vote is NO :P
There was a problem hiding this comment.
Yeah, agree @MFatihMAR though we could argue if we have the central singleton thing that they might be tempted to replace NetworkingManager.Singleton with NetworkingManager.NetworkManager[0]
| [unresolved-questions]: #unresolved-questions | ||
|
|
||
| - Should we maintain a `NetworkingManager.Singleton` for the convenience of people not using this feature? | ||
| - Should we force `NetworkLog` to need a `NetworkingManager` context? This makes it less convenient, but it means the logs could append `[SERVER]` or `[CLIENT]` tags. |
There was a problem hiding this comment.
As I commented above, I'd suggest making Server/Client/Host tag a parameter and call-sites would specify that.
|
|
||
| - Should we maintain a `NetworkingManager.Singleton` for the convenience of people not using this feature? | ||
| - Should we force `NetworkLog` to need a `NetworkingManager` context? This makes it less convenient, but it means the logs could append `[SERVER]` or `[CLIENT]` tags. | ||
| - Are there any "must-do" changes to how I've handled MLAPIProfiler in particular, or is the current compromise of profiling the first-found `NetworkingManager` acceptable? |
|
I just wanted to add a flag here that this has the potential to impact the docs in a massive way. We have a lot of code samples and callouts that use singleton which would need to be rewritten so we will need to take this into account. |
|
some work is already done by @TwoTenPvP targeting testing infrastructure. |
LukeStampfli
added
rejected
See linked RFC document →