From 9d8b65d2ce4387e53278bf2758e9e23dbc95d4c4 Mon Sep 17 00:00:00 2001
From: Fernando Cortez <fernando.cortez@unity3d.com>
Date: Tue, 13 Dec 2022 15:46:59 -0500
Subject: [PATCH] fix: ClientDriven initial position sync fix on owning clients
 (#85)

* removing clientrpc for setting position on spawn, disabling charactercontroller on clients

* adding more detailed comments

* changelog addition
---
 .../Prefabs/PlayerArmature_Networked.prefab   |  4 ++++
 .../Assets/Scripts/ClientPlayerMove.cs        | 14 +++++---------
 .../Assets/Scripts/ServerPlayerMove.cs        | 19 ++++++++++++-------
 CHANGELOG.md                                  |  1 +
 4 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/Basic/ClientDriven/Assets/Prefabs/PlayerArmature_Networked.prefab b/Basic/ClientDriven/Assets/Prefabs/PlayerArmature_Networked.prefab
index e1c8d36b..bdfab96b 100644
--- a/Basic/ClientDriven/Assets/Prefabs/PlayerArmature_Networked.prefab
+++ b/Basic/ClientDriven/Assets/Prefabs/PlayerArmature_Networked.prefab
@@ -126,6 +126,10 @@ PrefabInstance:
       propertyPath: GroundLayers.m_Bits
       value: 640
       objectReference: {fileID: 0}
+    - target: {fileID: 4416926081852918491, guid: 64dce48905ffd9b4293e595fa6941544, type: 3}
+      propertyPath: m_Enabled
+      value: 0
+      objectReference: {fileID: 0}
     - target: {fileID: 4416926081852918493, guid: 64dce48905ffd9b4293e595fa6941544, type: 3}
       propertyPath: m_Actions
       value: 
diff --git a/Basic/ClientDriven/Assets/Scripts/ClientPlayerMove.cs b/Basic/ClientDriven/Assets/Scripts/ClientPlayerMove.cs
index 516ba644..e8c9706e 100644
--- a/Basic/ClientDriven/Assets/Scripts/ClientPlayerMove.cs
+++ b/Basic/ClientDriven/Assets/Scripts/ClientPlayerMove.cs
@@ -45,6 +45,7 @@ public class ClientPlayerMove : NetworkBehaviour
         // ghost clients.
         m_ThirdPersonController.enabled = false;
         m_CapsuleCollider.enabled = false;
+        m_CharacterController.enabled = false;
     }
 
     public override void OnNetworkSpawn()
@@ -63,6 +64,10 @@ public class ClientPlayerMove : NetworkBehaviour
         // player input is only enabled on owning players
         m_PlayerInput.enabled = true;
         m_ThirdPersonController.enabled = true;
+        
+        // see the note inside ServerPlayerMove why this step is also necessary for synchronizing initial player
+        // position on owning clients
+        m_CharacterController.enabled = true;
 
         var cinemachineVirtualCamera = FindObjectOfType<CinemachineVirtualCamera>();
         cinemachineVirtualCamera.Follow = m_CameraFollow;
@@ -99,13 +104,4 @@ public class ClientPlayerMove : NetworkBehaviour
             }
         }
     }
-
-    [ClientRpc]
-    public void SetSpawnClientRpc(Vector3 position, ClientRpcParams clientRpcParams = default)
-    {
-        m_CharacterController.enabled = false;
-        transform.position = position;
-        m_CharacterController.enabled = true;
-        gameObject.SetActive(true);
-    }
 }
diff --git a/Basic/ClientDriven/Assets/Scripts/ServerPlayerMove.cs b/Basic/ClientDriven/Assets/Scripts/ServerPlayerMove.cs
index 04853763..94d68aba 100644
--- a/Basic/ClientDriven/Assets/Scripts/ServerPlayerMove.cs
+++ b/Basic/ClientDriven/Assets/Scripts/ServerPlayerMove.cs
@@ -10,9 +10,6 @@ using UnityEngine;
 [DefaultExecutionOrder(0)] // before client component
 public class ServerPlayerMove : NetworkBehaviour
 {
-    [SerializeField]
-    ClientPlayerMove m_ClientPlayerMove;
-
     public NetworkVariable<bool> isObjectPickedUp = new NetworkVariable<bool>();
     
     NetworkObject m_PickedUpObject;
@@ -23,7 +20,6 @@ public class ServerPlayerMove : NetworkBehaviour
     // DOC START HERE
     public override void OnNetworkSpawn()
     {
-        base.OnNetworkSpawn();
         if (!IsServer)
         {
             enabled = false;
@@ -31,6 +27,8 @@ public class ServerPlayerMove : NetworkBehaviour
         }
 
         OnServerSpawnPlayer();
+        
+        base.OnNetworkSpawn();
     }
 
     void OnServerSpawnPlayer()
@@ -38,9 +36,16 @@ public class ServerPlayerMove : NetworkBehaviour
         // this is done server side, so we have a single source of truth for our spawn point list
         var spawnPoint = ServerPlayerSpawnPoints.Instance.ConsumeNextSpawnPoint();
         var spawnPosition = spawnPoint ? spawnPoint.transform.position : Vector3.zero;
-        // using client RPC since ClientNetworkTransform can only be modified by owner (which is client side)
-        m_ClientPlayerMove.SetSpawnClientRpc(spawnPosition, 
-            new ClientRpcParams() { Send = new ClientRpcSendParams() { TargetClientIds = new []{OwnerClientId}}});
+        transform.position = spawnPosition;
+        
+        // A note specific to owner authority:
+        // Side Note:  Specific to Owner Authoritative
+        // Setting the position works as and can be set in OnNetworkSpawn server-side unless there is a
+        // CharacterController that is enabled by default on the authoritative side. With CharacterController, it
+        // needs to be disabled by default (i.e. in Awake), the server applies the position (OnNetworkSpawn), and then
+        // the owner of the NetworkObject should enable CharacterController during OnNetworkSpawn. Otherwise,
+        // CharacterController will initialize itself with the initial position (before synchronization) and updates the
+        // transform after synchronization with the initial position, thus overwriting the synchronized position.
     }
 
     [ServerRpc]
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 16271623..ff8d30ff 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -23,6 +23,7 @@
 - Upgrade to Netcode for GameObjects v1.2.0 & cleaned up in-scene NetworkVariables (#78)
 - Ingredient spawn position offset (#81)
 - In-game UI backgrounds (#82)
+- Initial position sync fix on owning clients (#85)
 
 ### 2DSpaceShooter