MirrorNetworking / Mirror

#1 Open Source Unity Networking Library
https://mirror-networking.com
MIT License
5.22k stars 772 forks source link

[Documentation & Suggestions] NetworkServer.Spawn() causing parent to be lost on clients, and clients modifying SyncVar #1853

Closed scscgit closed 3 years ago

scscgit commented 4 years ago

The documentation of NetworkServer.Spawn() is lacking what I view as a major use-case, as it doesn't really explain that the gameObject's parent is lost, which applies even if the parent has an identity (e.g. the Player prefab), and it even removes the effect of parent's transform, causing the spawned position to differ between a server (with the parent) and a client. This should be documented using <summary> in the source code of the Spawn function itself, and I'd also suggest that some check could be made, displaying a warning by default, unless the user explicitly chooses that the parent on server is supposed to differ, e.g. via an overload (the check could be disabled at runtime via #if UNITY_EDITOR).

As a further suggestion, maybe there could be an overload that supports this explicitly under supported conditions (e.g. if both client and server share a reference to some object, or if the parent is a NetworkIdentity), as it'd make game architecture arguably simpler if developers didn't have to introduce boilerplate to explicitly initialize and protect some parts of the code until the parent is set, or use RPC to spawn/initialize the object. In a worse scenario, developers may actually choose to abuse Update() for that, checking for a null field and assigning a parent every single frame for every single object. People shouldn't be forced to follow bad practices this early on.

Here is an example of a workaround that one may require in a Player class in order to parent a car to it:

[SyncVar] private GameObject _car;
private NetworkIdentity _identity;

// Using extension method defined in a static class
public static void ExecuteWithoutParent(this GameObject on, Action<GameObject> action)
{
    // Temporary unparenting, similar to adding parent's transform values and later subtracting them
    var onTransform = on.transform;
    var lastParent = onTransform.parent;
    onTransform.parent = null;
    action(on);
    onTransform.parent = lastParent;
}

public void Start()
{
    _identity = GetComponent<NetworkIdentity>();
    // The car was already spawned by other player before connecting
    if (!ReferenceEquals(_car, null))
    {
        _car.transform.parent = transform;
    }
}

// Starting point of player choosing to spawn, or a car being re-spawned by the server
public void SelectedCar(int carIndex)
{
    if (isServer)
    {
        var car = Instantiate(cars[carIndex], transform);
        // Spawning without parent is necessary, otherwise stacked cars collide on client at the
        // moment of re-spawn at position (0, 0, 0), causing rigidbody of client to fly away and
        // keep its velocity in spite of being properly teleported once parented using the RPC.
        car.ExecuteWithoutParent(o => NetworkServer.Spawn(o, _identity.connectionToClient));
        _car = car;
        RpcOnSpawnedCar(car);
    }
    else
    {
        CmdSelectedCar(carIndex);
    }
}

[Command]
public void CmdSelectedCar(int carIndex)
{
    var car = Instantiate(cars[carIndex], transform);
    // Spawning without parent is necessary, otherwise the parent position is ignored
    car.ExecuteWithoutParent(o => NetworkServer.Spawn(o, _identity.connectionToClient));
    _car = car;
    RpcOnSpawnedCar(car);
}

[ClientRpc]
public void RpcOnSpawnedCar(GameObject car)
{
    // The _car will be synced in the following cycle, so we have to use param car instead
    car.transform.parent = transform;
}

Note that to solve this simple issue, I've already had to learn how to use the [SyncVar], including the fact that only objects with NetworkIdentity are supported. That made me spend several days constantly debugging new issues before I finally understood few things, including the significance of the documented phrase such that the values are synchronized from the server to clients:

It's also mentioned that the variables are synced after OnStartClient(), but it doesn't say how that relates to Unity's Start(). Either way, if synced variables aren't meant to ever be modified on the client, not only should it be documented explicitly, but a perfect solution could be if it were possible to display a warning. I'm not sure what are the C# limitations, but if it's possible to implement a client-side setter hook, I think Mirror could at least mark the variable as dirty. Or is there any situation where client should intentionally de-sync the variable?


By the way, the other overload public static void Spawn(GameObject obj, GameObject player), which uses performance-heavy player.GetComponent<NetworkIdentity>(), even has a comment of // Deprecated 11/23/2019, so why doesn't it use [Obsolete]? At a first glance this overload feels to be analogous to the Instantiate, which has the parent as a second parameter, so it's easy to misunderstand how you fixed an issue, for example (this happened in my team) by wrongly deducing that "client's object needs to be spawned with their Player as a parent to sync correctly". I think the other Spawn should also document within its comment an explanation of the NetworkConnection parameter, so that one can easily see the quickest solution to obtain the NetworkConnection without googling around, even if the GameObject player overload gets removed.

To illustrate the severity of this issue, I've had a discussion on Mirror's Discord where another user had been fighting a problem that an object, which was as a child of a canvas, has only spawned on the server, not on the clients. Not only is an UI object without a canvas object invisible, its position will also randomly move far away. It's not even mentioned which position is used by default if it was parented. There are dozens of similar examples as to why this introduces a very steep learning curve.

I've also collected further feedback regarding the SpawnObject documentation on Discord, along with my ideas:

Of course, this is only supposed to be a constructive criticism, intended to also prevent similar issues in the future, as Mirror is still an amazing free library, yet we should address the UX questions of learnability along with major use-case patterns. It's preferable to have this addressed by someone who can see a bigger picture, but feel free to choose which documentation issues you'd rather have us as a community fix by ourselves via PR.

TwoCoderDevs commented 4 years ago

I believe if there is a network assign parent it will be nice....

MrGadget1024 commented 4 years ago
1. There's one specific typo: `The Network Manager before trying to register it with the Network Manager.`

Fixed.

MrGadget1024 commented 4 years ago
  1. The difference between NetworkServer.Destroy and Unity's Destroy isn't really explained, as Destroy seems to work just fine.

There is no actual difference, because if users invoke Destroy by accident on the server for a networked object, we cover for them in an OnDestroy callback that invokes NetworkServer.Destroy.

MrGadget1024 commented 4 years ago
  1. The use-case of nested spawnable objects isn't covered.
  2. No comments are made regarding workarounds if one needs to add or remove network components at a runtime

Neither of these are supported and cannot be given the current architecture.

scscgit commented 4 years ago

Points 8 and 10 (there's currently a GitHub Issues bug that displays 8. and 9. whenever you start lines by 8. and 10.)

Is there a way you could specify some of the technical limitations in the docs for other contributors, so that we know what makes it impossible before we try to implement it on our own, and before we make false assumptions? Personally I think that nested prefabs could support this if we were allowed to "disable a child's NetworkIdentity", so that it'd be simply ignored when we spawn a parent. Then we'd be free to spawn the child by ourselves (currently using the parent workaround I presented). At the very least, we could despawn the nested child ourselves (by tracking a [SyncVar]), but the native option of tracking its despawn state seems like a useful feature for the the future. Or can you think of any better project structure for games that'd support complex scenarios using some workarounds?

And regarding the runtime component modification (of network components), is there no way to at least register a client-sided function (like RPC) to modify the components right after the object gets spawned? That could prevent any mismatch, because it'd deterministically modify an object before it even gets started/enabled, while internally having the initialization parameters be just like [SyncVar]. (In theory, this would even address the previous point if one were okay with having the object re-spawn in its entirety.) Thanks for your time.

James-Frowen commented 4 years ago

A few notes from your First post

if (!ReferenceEquals(_car, null))

Try not to use ReferenceEquals in Unity as their Object class has a custom equals which needs to check if the native object is null or not.

example of a workaround

A better working around would be using a custom spawnhandler, See ClientScene.RegisterPrefab/RegisterHandler. The docs have an example of a custom spawnHandler for pooling objects here.

Using a spawn handler you could set the parent before the local position from the spawn message is applied.

public static void Spawn(GameObject obj, GameObject player)

Both these methods do the same thing:

Spawn(GameObject obj, GameObject player)
Spawn(GameObject obj, NetworkConnection ownerConnection = null)

The player/ownerConnection parameter can be set to give a client authority over the object, setting hasAuthority to true on the owning client.

yet nothing is synced until [SyncVar] or components like NetworkTransform do it.

When the spawn message is sent, it serialize all the NetworkBehaviours on the object (which be default includes SycnVars). Without double checking the docs, I think most of the time it mentions state it is talking about the state of the NetworkBehaviours attached to the GameObject, rather than the GameObject itself.

James-Frowen commented 4 years ago

On the network parenting feature, We have to find a solution that doesn't add too much complexity.

The outcome might just be to create examples of ways to do it with the current system.

scscgit commented 4 years ago

Try not to use ReferenceEquals in Unity as their Object class has a custom equals which needs to check if the native object is null or not.

Quite the opposite, I'm specifically trying to check if it was synchronized as null, due to the expensive nature of Unity's custom == operator, which I don't want to use. While it may be true that Unity can initialize public fields automatically, this is a private field, which is otherwise ignored by Unity's serialization process. I also don't want to rely on checking if the car was destroyed on the client-side ("== null" would return true), because such side-effects would go against defensive programming by hiding the errors from a developer. Thus I believe this to be the simplest example, having also the least performance impact.

A better working around would be using a custom spawnhandler Using a spawn handler you could set the parent before the local position from the spawn message is applied.

It may be a better option, though it doesn't seem to be any less verbose until I create some generic abstraction, but that's exactly the issue, because all new Mirror users will have to come up with their own workarounds, reinventing the wheel over and over (just making my workaround example took me several weeks of debugging before I understood these Mirror's limitations). It would be nice if some other developers shared their solutions that work properly in large-scale projects, so that we could get inspired by them.

public static void Spawn(GameObject obj, GameObject ownerPlayer)

I see it's no longer deprecated, so this is probably supposed to be the go-to overload for new users, executing GetComponent<NetworkIdentity>(); twice to make it more comfortable to use. Still, the documentation only mentions the other overload, which doesn't even explain how to get <param name="ownerConnection"> using obj.GetComponent<NetworkIdentity>().connectionToClient, and it assumes we know how the parents behave.

Docs: "it has the same Transform, movement state"

  • it serialize all the NetworkBehaviours on the object

I think it could be at least hinted how the Transform can differ on the server, or that it's local (ignoring parents), and that the movement state (I don't understand if this means rigidbody or not) not only doesn't get synced, but that there's no way to sync it. Plus the NetworkTransform keeps Rigidbody's velocity around zero, which I also had to find out by experimenting, but that's unrelated to spawning.

examples of ways to do it with the current system

Yeah, I hope it'll be easy to utilize features like [SyncVar] to make it easy to maintain the code while making it easier for people to rapidly prototype without getting stuck in ambiguities. For example, I'm very curious about the feasibility of my nested prefab example :)

James-Frowen commented 4 years ago
if (!ReferenceEquals(_car, null))
    {
        _car.transform.parent = transform;
    }

This will cause MissingReferenceException if the object has been destroyed.

ReferenceEquals makes the code overly complicated and using it can make it easier for someone to make a mistake and cause errors.

You might understand an ok use of ReferenceEquals now, but will someone else working on your code, or even you remember that in 6 months, remember that and avoid making future mistakes.

scscgit commented 4 years ago

!ReferenceEquals(_car, null)

As I said, my intention is to prevent hiding the errors from a developer, thus getting a MissingReferenceException is exactly what I want, so that I'll see the mistake instead of creating random bugs and spending several days debugging why there's a random hazard caused by latency or some other lifecycle inconsistency, along with never being able to see the exception in a server log or client analytics. I don't see how being ambiguous can ever help to prevent mistakes, and I'm sure you know very well how difficult it is to debug asynchronous code, so we can't just randomly corrupt some state in the middle of a process. This serves as an assert that if the server ever configures the instance not to be null, it mustn't ever destroy the car and sync that before initialization. For example, if Mirror couldn't provide a similar guarantee of atomicity, it would make it one order of magnitude harder to design any similar architecture :)

In this specific scenario I believe that Start() (Player's OnStartClient() called on a new client right after connecting to an existing match, or after spawning a new Player, before any interaction) is probably as safe as it can be, so I can't imagine why a general-purpose solution should do something more. For example, I was doing the same check in a method that de-spawns the car before Destroying it, but I've had to make a CmdDestroyCar() in order to never let the server keep a _car reference after destroying the object, so that we prevent letting a player spawn multiple cars. By the way, back when I mistakenly expected that [SyncVar] always syncs the value from server (and so I set that value in a client to prevent having to wait for a next update), I managed to get _car stuck as a missing reference instead of being set by server to null. If left everything up to chance, hoping that Unity's magic operator will take care of it, developers probably wouldn't even notice some of the new hazards they introduce.

This is getting way more off-topic than I intended, but let me just ask you: if Unity Object didn't implement its operator overload (which people generally criticize), would you also consider it to be "simpler" to use some !ObjectUtil.IsNullOrInvalid(_car) rather than a C# native _car != null? I don't see how it's any different from this just because they abuse the syntax that looks more nice. That's why I have to argue that people who work on the code, or me in 6 months, literally have the responsibility to understand this operator; but paradoxically, it's quite the opposite. People who read this code don't need to know anything, unlike how they would have to remember the semantics of Unity's operator if I were to use it :)

James-Frowen commented 4 years ago

In order to do calls on unity objects, likeobj.transform you need to make sure that both the c# and the c++ object are not null, otherwise you might get an Exception.

Throwing an Exception is a critical fail because anything else in the call will not run. Where as with an error log the rest of the code will still run. This might not be important in cases like your example because you are only doing a single thing in the start call.

When using ReferenceEquals(A, B) instead A==B you might be gaining small amounts of performance but the cost of maintenance. And there are likely other changes you could make to have a much bigger impact on performance.

scscgit commented 4 years ago

It seems as if you didn't understand a single thing I said. Why would anyone ever want a game to continue in an unstable state, even more so if it's related to a server-sided logic, which can affect something else, like your database? If I need to assume that the object isn't destroyed, why'd I try so hard to make the code produce unknown side-effects just so that the corrupted game state can continue without being informed about it? Why'd I actively harm my debugging process in a team project, along with the code quality, just so that I can "avoid an exception"? Isn't the point of an exception that the fatal state, which should be irrecoverable, can bubble up and interrupt the faulty process before it causes further harm? You are supposed to use the "finally" block if you need to ensure something executes. Once again, ReferenceEquals is the original == operator, so it produces the same logic as you'd naturally expect. If I am setting a field to null, then it means I expect it to stay null, and I don't ever want to get a false positive of a variable becoming null as a side-effect. We use the Unity's overloaded operator when we want to ask IsNativeObjectAlive(Object o), not when we want to use a reference comparison, because those are two separate use-cases. Thus I'd still compare the references even it had a higher performance cost. You still haven't answered me the question at the end of my previous post :)

James-Frowen commented 4 years ago

Isn't the point of an exception that the fatal state, which should be irrecoverable

Which is why for non-fatal problems they should just be logged as errors instead of throwing exception.

The simplest thing would be to use the unity framework in a unity game. Unity's == will work for every case you need it to.

If you call Destroy(targetBehaviour.gameObject); unity does not destroy the c# object meaning it is not truly null. If you are only checking ReferenceEquals then you might be calling functions on Destroyed objects.

scscgit commented 4 years ago

Yes, the simplest thing will work for the simplest games, and I don't have any need to convince you to do this in your games, so it's up to developers to decide if their game should work in a corrupted state, which is a fine choice for most single-player games and small non-tested game prototypes. Once again, you say that ReferenceEquals doesn't check if the object is alive as if it were self-evident that this is the behavior everyone's heart desires, yet after 2 replies you still haven't replied to my question related to a fictional !ObjectUtil.IsNullOrInvalid(_car) :) But it's getting very off-topic, so if you want to collect feedback from others regarding example code fragments to be used in the documentation, a new issue is probably a better choice to incentivize others to join the discussion.

James-Frowen commented 4 years ago

Ye this is getting off topic, and you seem convinced your way is the "correct" way.

It is probably a good idea to make one Issue per topic you listed in your first post. (Some can be grouped, for example how to spawn prefabs)

scscgit commented 4 years ago

There's not really any universally "correct" way, it's only a question of which approach will you prefer in your internal implementation if this feature were to become a separate Mirror component, or if this use-case gets mentioned in the docs :)

Sadly, I can't think of any efficient way to separate this issue, as many of the presented options depend on which specific decisions get made, and they all seem to require deep knowledge of the reasons behind spawn limitations. There are basically the following concerns (only the first two are the topic of this issue, but I didn't want to make new issue for every detail missing in docs):

  1. Parent cannot be synchronized (or maybe generally: initialization function cannot be specified)
  2. SyncVar is hard to learn and requires workarounds to share variables during RPC
  3. SpawnObject documentation misses critical information & Spawn doesn't support some use-cases
  4. Specifically, nested prefabs are impossible to spawn (but I'd like to hear suggestions first)

All of them can addressed either by solving the underlying root cause and implementing a nice overload parameter, or by specifying within documentation that Mirror doesn't care about the issue and instead providing workarounds that community can further develop and discuss. I can only see the surface of what's missing in the docs, but while some of them may be solved together, some may require being split into several issues each if they have the potential to require discussion and collaboration in your team. Feel free to choose specific issues feasible for direct implementation, I'm available if you want to brainstorm any options.

I hope we can generalize some of them to prevent unnecessary complexity, for example a "spawn initialization function" could theoretically address the following, along with a "runtime (re-)spawn modification RPC", possibly using SpawnDelegate wrapped around a serializable Dictionary parameter of a Spawn overload, which would be internally persisted as a [SyncVar]:

  1. Assigning a parent via lambda/function that's available on a client
  2. Configuring prefab variants by adding/removing network components via script before it's enabled
  3. Attaching, detaching or removing children of nested network prefabs
  4. Synchronizing a state of a field with a spawned instance (e.g. Player's field _car in my scenario)

Imagine if all of these issues could be solved using a single Spawn parameter and one method ✨ 😌

James-Frowen commented 4 years ago

One thing on the docs: there is a lot more notes about some of the classes and how to use them in the api reference, for example ClientScene.

Can you try re-wording this? I'm having a hard time understanding what you mean.

I hope we can generalize some of them to prevent unnecessary complexity, for example a "spawn initialization function" could theoretically address the following, along with a "runtime (re-)spawn modification RPC", possibly using SpawnDelegate wrapped around a serializable Dictionary parameter of a Spawn overload, which would be internally persisted as a [SyncVar]:

  1. Assigning a parent via lambda/function that's available on a client
  2. Configuring prefab variants by adding/removing network components via script before it's enabled
  3. Attaching, detaching or removing children of nested network prefabs
  4. Synchronizing a state of a field with a spawned instance (e.g. Player's field _car in my scenario)

Imagine if all of these issues could be solved using a single Spawn parameter and one method ✨ 😌

scscgit commented 4 years ago

Well, that's why I mentioned lacking <summary> as the second sentence of my original issue, because the API reference (which I always consult for ambiguities as it's directly visible from code in IDE) still doesn't cover anything beyond the simplest examples, e.g. Spawn doesn't address any of the hidden limitations, and SpawnDelegate doesn't address, well, anything, but that's understandable. Most users will begin learning from the official guide or similar tutorials anyway.

Regarding the example, I haven't come up with any user-friendly syntax to illustrate a solution yet. If the issues only depend on consistent synchronization, then the initialization function can simply modify the instance during Start.

Maybe there's a simpler alternative to better cover requirements such as component modification or parent attaching/detaching including tracking parent-child references. Also I'm not sure if there's effectively a need to a make a respawn function (as opposed to despawning and spawning every object, which may have some negative consequences), but if it's preferable in some use-cases (e.g. to avoid losing interpolation state of NetworkTransform), then this would be the place where components could be swapped around, which is otherwise forbidden due to breaking serialization. It may be also required to specify some consistency requirement for users to follow while respawning (modifying) an object, or an additional serialized parameter may be exposed to keep track of the change.

This is just an attempt at turning some boilerplate (without any examples documented yet) into some one-liners, so I'm sure someone can come up with even better examples. Instead of implementing the abstraction, we may actually find out that it's better to devise our own multiplayer design patterns and just consider how users can better encapsulate them. Nevertheless, we gotta make sure the elementary building blocks cover all relevant use-cases, and I think my original ExecuteWithoutParent workaround illustrates that it's definitely still lacking :)


Here's one random sync idea: the parent could assign [SyncChild] public Car car before/after the Spawn, and the child could have [SyncParent] public Player player, which gets assigned automatically in every related NetworkBehaviour (the type could be GameObject unless a strong type is feasible to be synced).

Also, some SpawnNested function could hook all of these references along with being able to spawn nested NetworkIdentity instances, which may require listing all of the children using a parameter of that function if this cannot be done efficiently using any "prefab reflection". Due to the end result being equivalent to spawning all objects individually and assigning parents using verbose hacks, this would effectively become a one-liner. All that's now required to cover the other use-cases is an option to dynamically modify network components, e.g. using a [SyncOptionalComponent] public MyComponent myComponent, but most of these scenarios could be replaced by spawning the component as an individual child anyway. Plus, I imagine that this architecture could support some dependency injection from parents to their children and back, considering that we must often set up bi-directional references inside prefabs using editor. Do you agree about this being an important use-case, or can you see a simpler alternative via explicit initialization event functions?

beardhero commented 4 years ago

Yeah I still have no clue how to use the NetworkManager after reading the doc, the api ref, this thread, and everything else I could find searching. All I want to do is a spawn some objects and it just doesn't work. Pls send help.

James-Frowen commented 4 years ago

https://mirror-networking.com/docs/Articles/Guides/GameObjects/SpawnObject.html

var clone = Instantiate(prefab)
NetworkServer.Spawn(clone);

prefab must have a NetworkIdentity and must be registered on the client, either by adding it to the list in NetworkManager or calling ClientScene.RegisterPrefab at runtime

beardhero commented 4 years ago

Thanks, that's what I'm doing. The problem is that the client isn't connected when I'm trying to spawn in Start(). I tried using the code from the "Spawning without NetworkManager" but the functions there are not getting run. Never get into ServerListen() or any of those other functions, am I supposed to run them manually? When? And how do I do that properly on client/server?

beardhero commented 4 years ago

I guess I did get those to run automatically, but I still just get "NetworkServer is not active. Cannot spawn objects without an active server."

beardhero commented 4 years ago

Also tried following the example exactly. Doesn't do anything, never runs the functions.

James-Frowen commented 4 years ago

you have to spawn an object on the server then mirror will send a message to the client and it will also spawn there.

beardhero commented 4 years ago

Well thanks for all your help.. I'm getting the prefabs to spawn on the client now, but I'm doing some dynamic editing of them after spawning them on server, creating a mesh m and setting

myCollider.sharedMesh = m; myFilter.sharedMesh = m;

which doesn't seem to propogate.

I think i'm going to have to create each one as a new prefab, register, and then spawn it.

MrGadget1024 commented 3 years ago

Closing this. The docs have all been moved to the new website and several people will be going through them all and redoing a lot of content over the summer. Additionally, docs have been changed and Mirror has been evolving over the months since this report, so whatever hasn't been addressed will be during the review process.