feat: simplifying reconnection (#804)

LPLafontaineB · web-flow · commit db908139f96c · 2023-03-06T10:45:31.000-05:00
* Simplifying reconnection

* Adding possibility of early failure when reconnecting if lobby no longer exists
diff --git a/.yamato/mobile-build-and-run.yml b/.yamato/mobile-build-and-run.yml
@@ -13,7 +13,7 @@ Build_Player_With_Tests_iOS_{{ project.name }}_{{ editor }}:
         flavor: b1.large
 
     commands:
-        - pip install unity-downloader-cli --index-url https://artifactory.prd.it.unity3d.com/artifactory/api/pypi/pypi/simple --upgrade
+        - pip install unity-downloader-cli==1.2.0 --index-url https://artifactory.prd.it.unity3d.com/artifactory/api/pypi/pypi/simple --upgrade
         - unity-downloader-cli -c Editor -c iOS -u {{ editor }} --fast --wait
         - curl -s https://artifactory.prd.it.unity3d.com/artifactory/unity-tools-local/utr-standalone/utr --output utr
         - chmod +x ./utr
@@ -42,7 +42,7 @@ Build_Player_With_Tests_Android_{{ project.name }}_{{ editor }}:
 
     commands:
         # Download unity-downloader-cli
-        - pip install unity-downloader-cli --index-url https://artifactory.prd.it.unity3d.com/artifactory/api/pypi/pypi/simple --upgrade
+        - pip install unity-downloader-cli==1.2.0 --index-url https://artifactory.prd.it.unity3d.com/artifactory/api/pypi/pypi/simple --upgrade
         - curl -s https://artifactory.prd.it.unity3d.com/artifactory/unity-tools/utr-standalone/utr.bat --output utr.bat
         - python .yamato/disable-burst-if-requested.py --project-path {{ project.path }} --platform Android
         - unity-downloader-cli -c Editor -c Android -u {{ editor }} --fast --wait
diff --git a/Assets/Scripts/ConnectionManagement/ConnectionManager.cs b/Assets/Scripts/ConnectionManagement/ConnectionManager.cs
@@ -1,7 +1,6 @@
 using System;
 using System.Collections.Generic;
 using Unity.BossRoom.Utils;
-using Unity.Collections;
 using Unity.Netcode;
 using UnityEngine;
 using UUnity.BossRoom.ConnectionManagement;
diff --git a/Assets/Scripts/ConnectionManagement/ConnectionMethod.cs b/Assets/Scripts/ConnectionManagement/ConnectionMethod.cs
@@ -23,10 +23,28 @@ public abstract class ConnectionMethodBase
         protected readonly string m_PlayerName;
         protected const string k_DtlsConnType = "dtls";
 
+        /// <summary>
+        /// Setup the host connection prior to starting the NetworkManager
+        /// </summary>
+        /// <returns></returns>
         public abstract Task SetupHostConnectionAsync();
 
+
+        /// <summary>
+        /// Setup the client connection prior to starting the NetworkManager
+        /// </summary>
+        /// <returns></returns>
         public abstract Task SetupClientConnectionAsync();
 
+        /// <summary>
+        /// Setup the client for reconnection prior to reconnecting
+        /// </summary>
+        /// <returns>
+        /// success = true if succeeded in setting up reconnection, false if failed.
+        /// shouldTryAgain = true if we should try again after failing, false if not.
+        /// </returns>
+        public abstract Task<(bool success, bool shouldTryAgain)> SetupClientReconnectionAsync();
+
         public ConnectionMethodBase(ConnectionManager connectionManager, ProfileManager profileManager, string playerName)
         {
             m_ConnectionManager = connectionManager;
@@ -82,6 +100,12 @@ public override async Task SetupClientConnectionAsync()
             utp.SetConnectionData(m_Ipaddress, m_Port);
         }
 
+        public override async Task<(bool success, bool shouldTryAgain)> SetupClientReconnectionAsync()
+        {
+            // Nothing to do here
+            return (true, true);
+        }
+
         public override async Task SetupHostConnectionAsync()
         {
             SetConnectionPayload(GetPlayerId(), m_PlayerName); // Need to set connection payload for host as well, as host is a client too
@@ -91,7 +115,7 @@ public override async Task SetupHostConnectionAsync()
     }
 
     /// <summary>
-    /// UTP's Relay connection setup
+    /// UTP's Relay connection setup using the Lobby integration
     /// </summary>
     class ConnectionMethodRelay : ConnectionMethodBase
     {
@@ -132,6 +156,32 @@ public override async Task SetupClientConnectionAsync()
             utp.SetRelayServerData(new RelayServerData(joinedAllocation, k_DtlsConnType));
         }
 
+        public override async Task<(bool success, bool shouldTryAgain)> SetupClientReconnectionAsync()
+        {
+            if (m_LobbyServiceFacade.CurrentUnityLobby == null)
+            {
+                Debug.Log("Lobby does not exist anymore, stopping reconnection attempts.");
+                return (false, false);
+            }
+
+            // When using Lobby with Relay, if a user is disconnected from the Relay server, the server will notify the
+            // Lobby service and mark the user as disconnected, but will not remove them from the lobby. They then have
+            // some time to attempt to reconnect (defined by the "Disconnect removal time" parameter on the dashboard),
+            // after which they will be removed from the lobby completely.
+            // See https://docs.unity.com/lobby/reconnect-to-lobby.html
+            var lobby = await m_LobbyServiceFacade.ReconnectToLobbyAsync(m_LocalLobby.LobbyID);
+            var success = lobby != null;
+            if (success)
+            {
+                Debug.Log("Successfully reconnected to Lobby.");
+            }
+            else
+            {
+                Debug.Log("Failed to reconnect to Lobby.");
+            }
+            return (success, true); // return a success if reconnecting to lobby returns a lobby
+        }
+
         public override async Task SetupHostConnectionAsync()
         {
             Debug.Log("Setting up Unity Relay host");
diff --git a/Assets/Scripts/ConnectionManagement/ConnectionState/ClientConnectingState.cs b/Assets/Scripts/ConnectionManagement/ConnectionState/ClientConnectingState.cs
@@ -1,9 +1,7 @@
 using System;
 using System.Threading.Tasks;
-using Unity.BossRoom.UnityServices.Lobbies;
 using Unity.Multiplayer.Samples.Utilities;
 using UnityEngine;
-using VContainer;
 
 namespace Unity.BossRoom.ConnectionManagement
 {
@@ -13,11 +11,7 @@ namespace Unity.BossRoom.ConnectionManagement
     /// </summary>
     class ClientConnectingState : OnlineState
     {
-        [Inject]
-        protected LobbyServiceFacade m_LobbyServiceFacade;
-        [Inject]
-        protected LocalLobby m_LocalLobby;
-        ConnectionMethodBase m_ConnectionMethod;
+        protected ConnectionMethodBase m_ConnectionMethod;
 
         public ClientConnectingState Configure(ConnectionMethodBase baseConnectionMethod)
         {
diff --git a/Assets/Scripts/ConnectionManagement/ConnectionState/ClientReconnectingState.cs b/Assets/Scripts/ConnectionManagement/ConnectionState/ClientReconnectingState.cs
@@ -19,15 +19,13 @@ class ClientReconnectingState : ClientConnectingState
         IPublisher<ReconnectMessage> m_ReconnectMessagePublisher;
 
         Coroutine m_ReconnectCoroutine;
-        string m_LobbyCode = "";
         int m_NbAttempts;
 
         const float k_TimeBetweenAttempts = 5;
 
         public override void Enter()
         {
             m_NbAttempts = 0;
-            m_LobbyCode = m_LobbyServiceFacade.CurrentUnityLobby != null ? m_LobbyServiceFacade.CurrentUnityLobby.LobbyCode : "";
             m_ReconnectCoroutine = m_ConnectionManager.StartCoroutine(ReconnectCoroutine());
         }
 
@@ -108,36 +106,25 @@ IEnumerator ReconnectCoroutine()
             Debug.Log($"Reconnecting attempt {m_NbAttempts + 1}/{m_ConnectionManager.NbReconnectAttempts}...");
             m_ReconnectMessagePublisher.Publish(new ReconnectMessage(m_NbAttempts, m_ConnectionManager.NbReconnectAttempts));
             m_NbAttempts++;
-            if (!string.IsNullOrEmpty(m_LobbyCode)) // Attempting to reconnect to lobby.
-            {
-                // When using Lobby with Relay, if a user is disconnected from the Relay server, the server will notify
-                // the Lobby service and mark the user as disconnected, but will not remove them from the lobby. They
-                // then have some time to attempt to reconnect (defined by the "Disconnect removal time" parameter on
-                // the dashboard), after which they will be removed from the lobby completely.
-                // See https://docs.unity.com/lobby/reconnect-to-lobby.html
-                var reconnectingToLobby = m_LobbyServiceFacade.ReconnectToLobbyAsync(m_LocalLobby?.LobbyID);
-                yield return new WaitUntil(() => reconnectingToLobby.IsCompleted);
+            var reconnectingSetupTask = m_ConnectionMethod.SetupClientReconnectionAsync();
+            yield return new WaitUntil(() => reconnectingSetupTask.IsCompleted);
 
-                // If succeeded, attempt to connect to Relay
-                if (!reconnectingToLobby.IsFaulted && reconnectingToLobby.Result != null)
-                {
-                    // If this fails, the OnClientDisconnect callback will be invoked by Netcode
-                    var connectingToRelay = ConnectClientAsync();
-                    yield return new WaitUntil(() => connectingToRelay.IsCompleted);
-                }
-                else
-                {
-                    Debug.Log("Failed reconnecting to lobby.");
-                    // Calling OnClientDisconnect to mark this attempt as failed and either start a new one or give up
-                    // and return to the Offline state
-                    OnClientDisconnect(0);
-                }
-            }
-            else // If not using Lobby, simply try to reconnect to the server directly
+            if (!reconnectingSetupTask.IsFaulted && reconnectingSetupTask.Result.success)
             {
                 // If this fails, the OnClientDisconnect callback will be invoked by Netcode
-                var connectingClient = ConnectClientAsync();
-                yield return new WaitUntil(() => connectingClient.IsCompleted);
+                var connectingToRelay = ConnectClientAsync();
+                yield return new WaitUntil(() => connectingToRelay.IsCompleted);
+            }
+            else
+            {
+                if (!reconnectingSetupTask.Result.shouldTryAgain)
+                {
+                    // setting number of attempts to max so no new attempts are made
+                    m_NbAttempts = m_ConnectionManager.NbReconnectAttempts;
+                }
+                // Calling OnClientDisconnect to mark this attempt as failed and either start a new one or give up
+                // and return to the Offline state
+                OnClientDisconnect(0);
             }
         }
     }
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,9 @@ Additional documentation and release notes are available at [Multiplayer Documen
 * Clarified a TODO comment inside ClientCharacter, detailing how anticipation should only be executed on owning client players (#786)
 * Removed now unnecessary cached NetworkBehaviour status on some components, since they now do not allocate memory (#799)
 
+### Changed
+* Simplified reconnection flow by offloading responsibility to ConnectionMethod (#804). Now the ClientReconnectingState uses the ConnectionMethod it is configured with to handle setting up reconnection (i.e. reconnecting to the Lobby before trying to reconnect to the Relay server if it is using Relay and Lobby). It can now also fail early and stop retrying if the lobby doesn't exist anymore.
+
 ### Fixed
 * EnemyPortals' VFX get disabled and re-enabled once the breakable crystals are broken (#784)
 * Elements inside the Tank's and Rogue's AnimatorTriggeredSpecialFX list have been revised to not loop AudioSource clips, ending the logging of multiple warnings to the console (#785)
