Skip to content

Singleton Removal #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Singleton Removal #11

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93fc03c
singleton removal RFC
dwoodruffsf Feb 11, 2021
f8bb44e
minor visual/styling polish
0xFA11 Feb 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions text/0000-singleton-removal.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
- Feature Name: `singleton-remove`
- Start Date: 2021-02-11
- RFC PR: [RFC#11](https://github.com/Unity-Technologies/com.unity.multiplayer.rfcs/pull/11)
- Issue: [Unity-Technologies/com.unity.multiplayer#0000](https://github.com/Unity-Technologies/com.unity.multiplayer/issues/0000)

# Summary
[summary]: #summary

This change removes the `NetworkingManager.Singleton` static, as well as the majority of other static state in MLAPI.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
        }
     }
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's right--you'd have the NetworkingManager you wanted to interact with dragged in as serialized field.


# Motivation
[motivation]: #motivation

The purpose of the change is to allow multiple instances of the MLAPI Networking Stack to co-exist in the same process. This enables:
- in-process servers. This allows developers to architect single-player and host-mode versions of their multiplayer game identically to the dedicated-server version of the same game.
- integration testing. Both games that use MLAPI and MLAPI itself can write full server/client tests that run entirely in the scope of a single process.
- clients talking to multiple servers simultaneously. This is expected to be a requirement for MMO support.

# Guide-level explanation
[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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, does the scene teardown automatically tear down its associated NetworkingManager?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- 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`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, how about the UTP impl?

- 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)).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistenct, NetManager probably should be NetworkingManager

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :) )

- 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.
Copy link
Contributor

@mattwalsh-unity mattwalsh-unity Feb 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update, we have committed to eliminating the MLAPI profiler for March


# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

`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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like Server/Client/Host origin could be an (optional?) parameter in the log method signatures?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to what I said above, we could store NetworkManager instances in a static NetworkManager[] container to avoid Object.FindObjectOfType call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Drawbacks
[drawbacks]: #drawbacks

Singletons are convenient. For code that operates in the context of a `NetworkedObject`, this isn't a loss, but for any other code, it requires more thought. For games that have multiple `NetworkingManager`s (particularly in the same scene), there is no recourse to making users think harder about _which_ `NetworkingManager` they want to interact with. However, for games that don't make use of multiple `NetworkingManager`s, it creates a pain point with no upside. I've considered if we should just leave the `NetworkingManager.Singleton` in place (with the first `NetworkingManager` to claim it "winning"), just to maintain continuity for users that don't need this feature. However, there is a risk of this creating bugs and confusion for users who do start down the path of multiple `NetworkingManager`s.

This also makes the editor facilities feel a bit more halfbaked. It would be nice if the MLAPIProfiler could provide feedback for all the `NetworkingManager` instances that are running, but this will entail more work.

# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives

There is no alternative design that covers all use cases of removing MLAPI singleton dependence. An architecturally similar approach to supporting host-based games is to have the client start up a separate, out-of-process server, but this will increase the minspec of the Host machine, because it effectively doubles its memory requirements. There is no good solution to talking to multiple servers simultaneously via MLAPI. It may be possible to live without this feature when writing MMOs, as a common pattern is to have the client only talk to a single gateway server in production; however, forcing this pattern onto development is undesirable, as configuring a local gateway server can be nontrivial.

# Prior art
[prior-art]: #prior-art

There is no specific prior art to cite, but avoiding static global state is a common software development recommendation, as it reduces system isolation and flexibility.

# Unresolved questions
[unresolved-questions]: #unresolved-questions

- Should we maintain a `NetworkingManager.Singleton` for the convenience of people not using this feature?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd highly argue against having 2 ways of doing things — my vote is NO :P

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

- 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I commented above, I'd suggest making Server/Client/Host tag a parameter and call-sites would specify that.

- 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?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@becksebenius-unity 👀


# Future possibilities
[future-possibilities]: #future-possibilities

This is a fairly self-contained feature (really somewhere between a feature and a code refactor). While it enables new use patterns mentioned above, I don't have anything specific to bring up here.