Unity-Technologies / com.unity.netcode.gameobjects

Netcode for GameObjects is a high-level netcode SDK that provides networking capabilities to GameObject/MonoBehaviour workflows within Unity and sits on top of underlying transport layer.
MIT License
2.09k stars 429 forks source link

NetworkTransform sends wrong initial position to remote clients, and this error can persist #2913

Open zachstronaut opened 1 month ago

zachstronaut commented 1 month ago

Bug report for v1.9.1, and I believe this problem did not exist in 1.7.1.

A host and a client join a game, and a new scene is additively loaded.

I have a NetworkObject with a NetworkTransform, let's call it Andy. It is placed in the additively loaded scene as a child of a child of another NetworkObject. So that means Andy's direct parent is not a NetworkObject. Andy and its parents are not at 0,0,0 world positions.

In NetworkTransform, the first time ApplyTransformToNetworkStateWithInfo runs on the host, WorldPositionStays() is false because m_CachedWorldPositionStays was set to false in NetworkObject.ApplyNetworkParenting -- because var parentNetworkObject = transform.parent.GetComponent<NetworkObject>(); is null. This means the host always sends a LOCAL position over the wire!

When ApplyTeleportingState runs for the first time on a remote client (via OnSynchronize), it receives that local position however it will apply the local Vec3 data as a world position because InLocalSpace is not true for the NetworkTransform.

This problem tends to be HIDDEN by the fact that much of the time, but not always, ApplyTeleportingState runs a second time on the remote client (via ApplyUpdatedState) and this second time the position data over the wire is in world space.

I don't know why ApplyTeleportingState doesn't always run for the remote client when the scene loads. When it doesn't run, the NetworkObject with the NetworkTransform -- Andy -- can remain in the wrong world position!

NoelStephensUnity commented 1 month ago

To make sure I understand:

Is this the hierarchy you are describing?

zachstronaut commented 3 weeks ago

yeah, that's basically correct although Andy is also not at 0,0,0 relative to its immediate parent.

The exact scenario is:

Although I think the extra nesting of children doesn't matter and a single non-network object immediate parent that's not at world 0,0,0 would be enough to reproduce.

NoelStephensUnity commented 3 weeks ago

@zachstronaut I think I can solve this by doing a recursive parent crawl:

        private NetworkObject GetNetworkObjectParent(Transform transform)
        {
            if (transform.parent != null)
            {
                var networkObject = transform.parent.GetComponent<NetworkObject>();
                if (networkObject == null)
                {
                    return GetNetworkObjectParent(transform.parent);
                }
                return networkObject;
            }
            return null;
        }

Then replace this: var parentNetworkObject = transform.parent.GetComponent<NetworkObject>();

with this: var parentNetworkObject = GetNetworkObjectParent(transform);

Which should assure world position stays will not be false and the position sent will be sent world space relative.

NoelStephensUnity commented 3 weeks ago

@zachstronaut So, after writing some more tests and looking further into this... I am wondering if there isn't something else you might be doing that could be impacting things?

Server-side synchronization:

When the server first spawns an in-scene placed NetworkObject after it additively loads a scene, it will invoke NetworkManager.SpawnManager.SpawnNetworkObjectLocally which invokes NetworkSpawnManager.SpawnNetworkObjectLocallyCommon. Within NetworkSpawnManager.SpawnNetworkObjectLocallyCommon, it will invoke NetworkObject.ApplyNetworkParenting which, as you pointed out, views a NetworkObject nested under a GameObject as being parenting with WorldPositionStays set to false (i.e. use local space values).

However, the more important thing to note here is that it sets m_CachedWorldPositionStays to false ==and== it sets the HasParent flag to notify the client it has a parent.

So far (based on the hierarchy presented) the server will serialize the following values:

Then, for normal NetworkObject transform synchronization, the server determines whether it should use world or local space here :

var syncRotationPositionLocalSpaceRelative = obj.HasParent && !m_CachedWorldPositionStays;

Which based on the above values (HasParent & !WorldPositionStays), it should set syncRotationPositionLocalSpaceRelative to true.

Next it determines local vs worldspace values to use within the NetworkObject.GetMessageSceneObject method here:

Position = syncRotationPositionLocalSpaceRelative ? transform.localPosition : transform.position,

So, the final values of interest for Andy's NetworkObject should be:

Now, during the server-side NetworkTransform.OnSynchronize method it eventually makes its way down to determine which worldspace value to use here:

                if (isSynchronization)
                {
                    if (NetworkObject.WorldPositionStays())
                    {
                        position = transformToUse.position;
                    }
                    else
                    {
                        position = transformToUse.localPosition;
                    }
                }

Which the position variable is used to set all axis enabled for synchronization on the state to send for the initial NetworkTransform synchronization. Since WorldPositionStays == false, it will use the local space position values.

Client-side synchronization:

During the initial synchronization of an in-scene placed NetworkObject, the client will invoke NetworkSpawnManager.CreateLocalNetworkObject which this eventually applies the NetworkObject's m_CachecWorldPositionStays value via NetworkObject.SetNetworkParenting.

So, we know the In-Scene placed NetworkObject's m_CachecWorldPositionStays value will be the same as the servers (i.e. false). Then the client will apply the NetworkObject's transform values here:

                    if (worldPositionStays || !networkObject.AutoObjectParentSync)
                    {
                        networkObject.transform.position = position;
                        networkObject.transform.rotation = rotation;
                    }
                    else
                    {
                        networkObject.transform.localPosition = position;
                        networkObject.transform.localRotation = rotation;
                    }

Since WorldPositionStays is false and I am assuming you have not disabled AutoObjectParentSync, it should apply the serialized NetworkObject's transform values (if transform sync is enabled) as local space relative. Then during NetworkTransform.OnSynchronize it applies position based on its world position stays setting:

                if (isSynchronization)
                {
                    if (NetworkObject.WorldPositionStays())
                    {
                        position = transformToUse.position;
                    }
                    else
                    {
                        position = transformToUse.localPosition;
                    }
                }

Final States of Interest Serialized:

So, under the scenario you have outlined... unless you are changing any of the children transform values (other than the one with the NetworkTransform), parenting things on fly (i.e. dynamically creating a scene to be loaded or adding things to the in-scene placed hierarchy prior to invoking OnSynchronize), or have a derived version of NetworkTransform that overrides the OnSynchronize or OnNetworkSpawn methods and makes changes to values after OnSynchronize is invoked or prior to OnNetworkSpawn being invoked....

I am having a hard time replicating the issue with v1.9.1... as at first pass glance it seemed like what you were saying made sense....but when I wrote tests for the fix...my test scene that gets loaded (arranged exactly as you outlined) passes without that fix... which made me do a double take on the process...and I came to the realization that what you are seeing shouldn't be happening unless there is something else you might have left out about your configuration (i.e. any additional scripts or the like that could be changing things during serialization)?

zachstronaut commented 3 weeks ago

I’m starting a paternity leave now so unfortunately I’m not gonna be much help investigating this with you for about 4 weeks! 🥰On May 8, 2024, at 19:18, Noel Stephens @.***> wrote: @zachstronaut So, after writing some more tests and looking further into this... I am wondering if there isn't something else you might be doing that could be impacting things? Server-side synchronization: When the server first spawns an in-scene placed NetworkObject after it additively loads a scene, it will invoke NetworkManager.SpawnManager.SpawnNetworkObjectLocally which invokes NetworkSpawnManager.SpawnNetworkObjectLocallyCommon which invokes NetworkSpawnManager.SpawnNetworkObjectLocallyCommon. Within NetworkSpawnManager.SpawnNetworkObjectLocallyCommon, it will invoke NetworkObject.ApplyNetworkParenting which, as you pointed out, views a NetworkObject nested under a GameObject as being parenting with WorldPositionStays set to false (i.e. use local space values). However, the more important thing to note here is that it sets m_CachedWorldPositionStays to false ==and== it sets the HasParent flag to notify the client it has a parent. So far (based on the hierarchy presented) the server will serialize the following values:

WorldPositionStays = false HasParent = true

Then, for normal NetworkObject transform synchronization, the server determines whether it should use world or local space here : var syncRotationPositionLocalSpaceRelative = obj.HasParent && !m_CachedWorldPositionStays; Which based on the above values (HasParent & !WorldPositionStays), it should set syncRotationPositionLocalSpaceRelative to true. Then finally, it determines local vs worldspace values to use within the NetworkObject.GetMessageSceneObject method here: Position = syncRotationPositionLocalSpaceRelative ? transform.localPosition : transform.position, So, the final values of interest for Andy's NetworkObject should be:

WorldPositionStays = false HasParent = true Position = (the local space position values)

Now, during the server-side NetworkTransform.OnSynchronize method it eventually makes its way down to which worldspace value to use here: if (isSynchronization) { if (NetworkObject.WorldPositionStays()) { position = transformToUse.position; } else { position = transformToUse.localPosition; } } Which the position variable is used to set all axis on the state to send for the initial NetworkTransform synchronization. Since WorldPositionStays == false, it will use the local space position value. Client-side synchronization: During the initial synchronization, it applies position based on its world position stays setting: if (isSynchronization) { if (NetworkObject.WorldPositionStays()) { position = transformToUse.position; } else { position = transformToUse.localPosition; } } Which this is applied when a client is deserializing the in-scene object and it invokes NetworkSpawnManager.CreateLocalNetworkObject which this eventually applies the NetworkObject's m_CachecWorldPositionStays value via NetworkObject.SetNetworkParenting. Final States of Interest Serialized:

Server-Side:

NetworkObject

WorldPositionStays = false HasParent = true Position = (the local space position values)

NetworkTransform

Position = (the local space position values)

Client-Side:

NetworkObject

Applies WorldPositionStays value when "creating" the NetworkObject.

For in-scene placed, it gets the instance created when the scene is loaded.

Applies the Transform's local space position

NetworkTransform.OnSynchronize:

It applies the position of the NetworkTransformState to the transform's local space value because WorldPositionStays is false.

So, under the scenario you have outlined... unless you are changing any of the children transform values (other than the one with the NetworkTransform), parenting things on fly (i.e. dynamically creating a scene to be loaded or adding things to the in-scene placed hierarchy prior to invoking OnSynchronize), or have a derived version of NetworkTransform that overrides the OnSynchronize or OnNetworkSpawn methods and makes changes to values after OnSynchronize is invoked or prior to OnNetworkSpawn being invoked.... I am having a hard time replicating the issue with v1.9.1... as at first pass glance it seemed like what you were saying made sense....but when I wrote tests for the fix...my test scene that gets loaded (arranged exactly as you outlined) passes with or without that fix... which made me do a double take on the process...and I came to the realization that what you are seeing shouldn't be happening unless there is something else you might have left out about your configuration (i.e. any additional scripts or the like that could be changing things during serialization?).

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

NoelStephensUnity commented 3 weeks ago

@zachstronaut

When ApplyTeleportingState runs for the first time on a remote client (via OnSynchronize), it receives that local position however it will apply the local Vec3 data as a world position because InLocalSpace is not true for the NetworkTransform.

That shouldn't be happening during synchronization.

This problem tends to be HIDDEN by the fact that much of the time, but not always, ApplyTeleportingState runs a second time on the remote client (via ApplyUpdatedState) and this second time the position data over the wire is in world space. I don't know why ApplyTeleportingState doesn't always run for the remote client when the scene loads. When it doesn't run, the NetworkObject with the NetworkTransform -- Andy -- can remain in the wrong world position!

It could be sending the state update from the server immediately after the synchronization message and a client could potentially receive and process both messages back to back... but even then it should always send and process those messages in the same order... but I will look over the SetState being sent when a NetworkTransform spawns to see if that could be causing the issue.

NoelStephensUnity commented 3 weeks ago

I’m starting a paternity leave now so unfortunately I’m not gonna be much help investigating this with you for about 4 weeks! 🥰On May 8, 2024, at 19:18, Noel Stephens @.***> wrote:

Congrats!!!!!!! 😻

(No worries... I will continue digging into this and if I can replicate it then will push out a fix for it...)

AsmPrgmC3 commented 3 weeks ago

It sounds to me like you're running into #2888 (with an additional network object ancestor).

@NoelStephensUnity As for the synchronization overview: The code snippet for Client-Side Network Transform synchronization is from ApplyTransformToNetworkStateWithInfo, which is executed server-side. The client-side executes ApplyTeleportingState which doesn't query WorldPositionStays.

NoelStephensUnity commented 3 weeks ago

@AsmPrgmC3 (It was late and I see the disconnect... for some reason I was still looking at the authority side. Thank you for catching that!)

As for the synchronization overview: The code snippet for Client-Side Network Transform synchronization is from ApplyTransformToNetworkStateWithInfo, which is executed server-side. The client-side executes ApplyTeleportingState which doesn't query WorldPositionStays.

💯

What needs to happen is that it needs to perform the same parenting and world position stays check during OnSynchronize on the non-authority side when ApplyTeleportingState is invoked. 👍