refactor: Minor cleanups in Unity Transport (#3377)

Just a few minor cleanups in Unity Transport:

- Remove the internal `NetworkDriver` accessor. There's a public
`GetNetworkDriver` nowadays that can be used for the same purpose.
- Add a `using` directive for transport errors which avoids using the
full (and long) type name.
- Remove the internal "state" member. The same information can be
gleaned by looking at the driver's state and the value of
`m_ServerClientId`. Plus, the `Listening` state did not have the same
meaning as it does in NGO itself, which could lead to confusion.
- Simplify `ErrorUtilities` to avoid the useless constants and only
providing strings for errors users can actually do something about. Also
moved it to the bottom of the file. The top is too prime real estate for
such a minor utility. :P

Author: simon-lemay-unity

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs

@@ -16,8 +16,10 @@
 using Unity.Networking.Transport.TLS;
 using Unity.Networking.Transport.Utilities;
 using UnityEngine;
-using NetcodeNetworkEvent = Unity.Netcode.NetworkEvent;
-using TransportNetworkEvent = Unity.Networking.Transport.NetworkEvent;
+
+using NetcodeEvent = Unity.Netcode.NetworkEvent;
+using TransportError = Unity.Networking.Transport.Error.StatusCode;
+using TransportEvent = Unity.Networking.Transport.NetworkEvent.Type;
 
 namespace Unity.Netcode.Transports.UTP
 {
@@ -42,56 +44,6 @@ void CreateDriver(
             out NetworkPipeline reliableSequencedPipeline);
     }
 
-    /// <summary>
-    /// Helper utility class to convert <see cref="Networking.Transport"/> error codes to human readable error messages.
-    /// </summary>
-    public static class ErrorUtilities
-    {
-        private static readonly FixedString128Bytes k_NetworkSuccess = "Success";
-        private static readonly FixedString128Bytes k_NetworkIdMismatch = "Invalid connection ID {0}.";
-        private static readonly FixedString128Bytes k_NetworkVersionMismatch = "Connection ID is invalid. Likely caused by sending on stale connection {0}.";
-        private static readonly FixedString128Bytes k_NetworkStateMismatch = "Connection state is invalid. Likely caused by sending on connection {0} which is stale or still connecting.";
-        private static readonly FixedString128Bytes k_NetworkPacketOverflow = "Packet is too large to be allocated by the transport.";
-        private static readonly FixedString128Bytes k_NetworkSendQueueFull = "Unable to queue packet in the transport. Likely caused by send queue size ('Max Send Queue Size') being too small.";
-
-        /// <summary>
-        /// Convert a UTP error code to human-readable error message.
-        /// </summary>
-        /// <param name="error">UTP error code.</param>
-        /// <param name="connectionId">ID of the connection on which the error occurred.</param>
-        /// <returns>Human-readable error message.</returns>
-        public static string ErrorToString(Networking.Transport.Error.StatusCode error, ulong connectionId)
-        {
-            return ErrorToString((int)error, connectionId);
-        }
-
-        internal static string ErrorToString(int error, ulong connectionId)
-        {
-            return ErrorToFixedString(error, connectionId).ToString();
-        }
-
-        internal static FixedString128Bytes ErrorToFixedString(int error, ulong connectionId)
-        {
-            switch ((Networking.Transport.Error.StatusCode)error)
-            {
-                case Networking.Transport.Error.StatusCode.Success:
-                    return k_NetworkSuccess;
-                case Networking.Transport.Error.StatusCode.NetworkIdMismatch:
-                    return FixedString.Format(k_NetworkIdMismatch, connectionId);
-                case Networking.Transport.Error.StatusCode.NetworkVersionMismatch:
-                    return FixedString.Format(k_NetworkVersionMismatch, connectionId);
-                case Networking.Transport.Error.StatusCode.NetworkStateMismatch:
-                    return FixedString.Format(k_NetworkStateMismatch, connectionId);
-                case Networking.Transport.Error.StatusCode.NetworkPacketOverflow:
-                    return k_NetworkPacketOverflow;
-                case Networking.Transport.Error.StatusCode.NetworkSendQueueFull:
-                    return k_NetworkSendQueueFull;
-                default:
-                    return FixedString.Format("Unknown error code {0}.", error);
-            }
-        }
-    }
-
     /// <summary>
     /// The Netcode for GameObjects NetworkTransport for UnityTransport.
     /// Note: This is highly recommended to use over UNet.
@@ -114,13 +66,6 @@ public enum ProtocolType
             RelayUnityTransport,
         }
 
-        private enum State
-        {
-            Disconnected,
-            Listening,
-            Connected,
-        }
-
         /// <summary>
         /// The default maximum (receive) packet queue size
         /// </summary>
@@ -421,17 +366,16 @@ private struct PacketLossCache
 
         internal static event Action<int, NetworkDriver> TransportInitialized;
         internal static event Action<int> TransportDisposed;
-        internal NetworkDriver NetworkDriver => m_Driver;
 
         /// <summary>
-        /// Provides access to the <see cref="Networking.Transport.NetworkDriver"/> for this <see cref="UnityTransport"/> instance.
+        /// Provides access to the <see cref="NetworkDriver"/> for this instance.
         /// </summary>
         protected NetworkDriver m_Driver;
 
         /// <summary>
-        /// Gets a reference to the <see cref="Networking.Transport.NetworkDriver"/>.
+        /// Gets a reference to the <see cref="NetworkDriver"/>.
         /// </summary>
-        /// <returns>ref <see cref="Networking.Transport.NetworkDriver"/></returns>
+        /// <returns>ref <see cref="NetworkDriver"/></returns>
         public ref NetworkDriver GetNetworkDriver()
         {
             return ref m_Driver;
@@ -455,7 +399,6 @@ public NetworkEndpoint GetLocalEndpoint()
 
         private PacketLossCache m_PacketLossCache = new PacketLossCache();
 
-        private State m_State = State.Disconnected;
         private NetworkSettings m_NetworkSettings;
         private ulong m_ServerClientId;
 
@@ -501,7 +444,7 @@ private void InitDriver()
                 out m_UnreliableSequencedFragmentedPipeline,
                 out m_ReliableSequencedPipeline);
 
-            TransportInitialized?.Invoke(GetInstanceID(), NetworkDriver);
+            TransportInitialized?.Invoke(GetInstanceID(), m_Driver);
         }
 
         private void DisposeInternals()
@@ -583,8 +526,7 @@ private bool ClientBindAndConnect()
                 return false;
             }
 
-            var serverConnection = Connect(serverEndpoint);
-            m_ServerClientId = ParseClientId(serverConnection);
+            Connect(serverEndpoint);
 
             return true;
         }
@@ -624,7 +566,6 @@ private bool ServerBindAndListen(NetworkEndpoint endPoint)
                 return false;
             }
 
-            m_State = State.Listening;
             return true;
         }
 
@@ -776,9 +717,9 @@ public void Execute()
                 while (!Queue.IsEmpty)
                 {
                     var result = Driver.BeginSend(pipeline, connection, out var writer);
-                    if (result != (int)Networking.Transport.Error.StatusCode.Success)
+                    if (result != (int)TransportError.Success)
                     {
-                        Debug.LogError($"Error sending message: {ErrorUtilities.ErrorToFixedString(result, clientId)}");
+                        Debug.LogError($"Send error on connection {clientId}: {ErrorUtilities.ErrorToFixedString(result)}");
                         return;
                     }
 
@@ -803,9 +744,9 @@ public void Execute()
                         // and we'll retry sending them later). Otherwise log the error and remove the
                         // message from the queue (we don't want to resend it again since we'll likely
                         // just get the same error again).
-                        if (result != (int)Networking.Transport.Error.StatusCode.NetworkSendQueueFull)
+                        if (result != (int)TransportError.NetworkSendQueueFull)
                         {
-                            Debug.LogError($"Error sending the message: {ErrorUtilities.ErrorToFixedString(result, clientId)}");
+                            Debug.LogError($"Send error on connection {clientId}: {ErrorUtilities.ErrorToFixedString(result)}");
                             Queue.Consume(written);
                         }
 
@@ -849,7 +790,7 @@ private bool AcceptConnection()
                 return false;
             }
 
-            InvokeOnTransportEvent(NetcodeNetworkEvent.Connect,
+            InvokeOnTransportEvent(NetcodeEvent.Connect,
                 ParseClientId(connection),
                 default,
                 m_RealTimeProvider.RealTimeSinceStartup);
@@ -887,7 +828,7 @@ private void ReceiveMessages(ulong clientId, NetworkPipeline pipeline, DataStrea
                     break;
                 }
 
-                InvokeOnTransportEvent(NetcodeNetworkEvent.Data, clientId, message, m_RealTimeProvider.RealTimeSinceStartup);
+                InvokeOnTransportEvent(NetcodeEvent.Data, clientId, message, m_RealTimeProvider.RealTimeSinceStartup);
             }
         }
 
@@ -898,44 +839,38 @@ private bool ProcessEvent()
 
             switch (eventType)
             {
-                case TransportNetworkEvent.Type.Connect:
+                case TransportEvent.Connect:
                     {
-                        InvokeOnTransportEvent(NetcodeNetworkEvent.Connect,
+                        InvokeOnTransportEvent(NetcodeEvent.Connect,
                             clientId,
                             default,
                             m_RealTimeProvider.RealTimeSinceStartup);
 
-                        m_State = State.Connected;
+                        m_ServerClientId = clientId;
                         return true;
                     }
-                case TransportNetworkEvent.Type.Disconnect:
+                case TransportEvent.Disconnect:
                     {
-                        // Handle cases where we're a client receiving a Disconnect event. The
-                        // meaning of the event depends on our current state. If we were connected
-                        // then it means we got disconnected. If we were disconnected means that our
-                        // connection attempt has failed.
-                        if (m_State == State.Connected)
-                        {
-                            m_State = State.Disconnected;
-                            m_ServerClientId = default;
-                        }
-                        else if (m_State == State.Disconnected)
+                        // If we're a client and had not yet set the server client ID, it means
+                        // our connection to the server failed to be established. Any other case
+                        // means a clean disconnect that doesn't require logging.
+                        if (!m_Driver.Listening && m_ServerClientId == default)
                         {
                             Debug.LogError("Failed to connect to server.");
-                            m_ServerClientId = default;
                         }
 
+                        m_ServerClientId = default;
                         m_ReliableReceiveQueues.Remove(clientId);
                         ClearSendQueuesForClientId(clientId);
 
-                        InvokeOnTransportEvent(NetcodeNetworkEvent.Disconnect,
+                        InvokeOnTransportEvent(NetcodeEvent.Disconnect,
                             clientId,
                             default,
                             m_RealTimeProvider.RealTimeSinceStartup);
 
                         return true;
                     }
-                case TransportNetworkEvent.Type.Data:
+                case TransportEvent.Data:
                     {
                         ReceiveMessages(clientId, pipeline, reader);
                         return true;
@@ -957,7 +892,7 @@ protected override void OnEarlyUpdate()
                     Debug.LogError("Transport failure! Relay allocation needs to be recreated, and NetworkManager restarted. " +
                         "Use NetworkManager.OnTransportFailure to be notified of such events programmatically.");
 
-                    InvokeOnTransportEvent(NetcodeNetworkEvent.TransportFailure, 0, default, m_RealTimeProvider.RealTimeSinceStartup);
+                    InvokeOnTransportEvent(NetcodeEvent.TransportFailure, 0, default, m_RealTimeProvider.RealTimeSinceStartup);
                     return;
                 }
 
@@ -1180,21 +1115,21 @@ private void FlushSendQueuesForClientId(ulong clientId)
         /// </summary>
         public override void DisconnectLocalClient()
         {
-            if (m_State == State.Connected)
+            if (m_ServerClientId != default)
             {
                 FlushSendQueuesForClientId(m_ServerClientId);
 
                 if (m_Driver.Disconnect(ParseClientId(m_ServerClientId)) == 0)
                 {
-                    m_State = State.Disconnected;
+                    m_ServerClientId = default;
 
                     m_ReliableReceiveQueues.Remove(m_ServerClientId);
                     ClearSendQueuesForClientId(m_ServerClientId);
 
                     // If we successfully disconnect we dispatch a local disconnect message
                     // this how uNET and other transports worked and so this is just keeping with the old behavior
                     // should be also noted on the client this will call shutdown on the NetworkManager and the Transport
-                    InvokeOnTransportEvent(NetcodeNetworkEvent.Disconnect,
+                    InvokeOnTransportEvent(NetcodeEvent.Disconnect,
                         m_ServerClientId,
                         default,
                         m_RealTimeProvider.RealTimeSinceStartup);
@@ -1209,14 +1144,14 @@ public override void DisconnectLocalClient()
         public override void DisconnectRemoteClient(ulong clientId)
         {
 #if DEBUG
-            if (m_State != State.Listening)
+            if (!m_Driver.IsCreated)
             {
                 Debug.LogWarning($"{nameof(DisconnectRemoteClient)} should only be called on a listening server!");
                 return;
             }
 #endif
 
-            if (m_State == State.Listening)
+            if (m_Driver.IsCreated)
             {
                 FlushSendQueuesForClientId(clientId);
 
@@ -1331,12 +1266,12 @@ public override void Initialize(NetworkManager networkManager = null)
         /// <param name="payload">The incoming data payload</param>
         /// <param name="receiveTime">The time the event was received, as reported by m_RealTimeProvider.RealTimeSinceStartup.</param>
         /// <returns>Returns the event type</returns>
-        public override NetcodeNetworkEvent PollEvent(out ulong clientId, out ArraySegment<byte> payload, out float receiveTime)
+        public override NetcodeEvent PollEvent(out ulong clientId, out ArraySegment<byte> payload, out float receiveTime)
         {
             clientId = default;
             payload = default;
             receiveTime = default;
-            return NetcodeNetworkEvent.Nothing;
+            return NetcodeEvent.Nothing;
         }
 
         /// <summary>
@@ -1404,7 +1339,7 @@ public override void Send(ulong clientId, ArraySegment<byte> payload, NetworkDel
                         DisconnectRemoteClient(clientId);
 
                         // DisconnectRemoteClient doesn't notify SDK of disconnection.
-                        InvokeOnTransportEvent(NetcodeNetworkEvent.Disconnect,
+                        InvokeOnTransportEvent(NetcodeEvent.Disconnect,
                             clientId,
                             default(ArraySegment<byte>),
                             m_RealTimeProvider.RealTimeSinceStartup);
@@ -1520,10 +1455,9 @@ public override void Shutdown()
             DisposeInternals();
 
             m_ReliableReceiveQueues.Clear();
-            m_State = State.Disconnected;
 
             // We must reset this to zero because UTP actually re-uses clientIds if there is a clean disconnect
-            m_ServerClientId = 0;
+            m_ServerClientId = default;
         }
 
         private void ConfigureSimulator()
@@ -1786,4 +1720,37 @@ public override int GetHashCode()
             }
         }
     }
+
+    /// <summary>
+    /// Utility class to convert Unity Transport error codes to human-readable error messages.
+    /// </summary>
+    public static class ErrorUtilities
+    {
+        /// <summary>
+        /// Convert a Unity Transport error code to human-readable error message.
+        /// </summary>
+        /// <param name="error">Unity Transport error code.</param>
+        /// <param name="connectionId">ID of connection on which error occurred (unused).</param>
+        /// <returns>Human-readable error message.</returns>
+        public static string ErrorToString(TransportError error, ulong connectionId)
+        {
+            return ErrorToFixedString((int)error).ToString();
+        }
+
+        internal static FixedString128Bytes ErrorToFixedString(int error)
+        {
+            switch ((TransportError)error)
+            {
+                case TransportError.NetworkVersionMismatch:
+                case TransportError.NetworkStateMismatch:
+                    return "invalid connection state (likely stale/closed connection)";
+                case TransportError.NetworkPacketOverflow:
+                    return "packet is too large for the transport (likely need to increase MTU)";
+                case TransportError.NetworkSendQueueFull:
+                    return "send queue full (need to increase 'Max Send Queue Size' parameter)";
+                default:
+                    return FixedString.Format("unexpected error code {0}", error);
+            }
+        }
+    }
 }

com.unity.netcode.gameobjects/Tests/Editor/Transports/UnityTransportTests.cs

@@ -189,7 +189,7 @@ public void UnityTransport_EmptySecurityStringsShouldThrow([Values("", null)] st
                     networkManager.StartServer();
                 });
                 // Make sure StartServer failed
-                Assert.False(transport.NetworkDriver.IsCreated);
+                Assert.False(transport.GetNetworkDriver().IsCreated);
                 Assert.False(networkManager.IsServer);
                 Assert.False(networkManager.IsListening);
             }

com.unity.netcode.gameobjects/Tests/Runtime/Metrics/PacketLossMetricsTests.cs

@@ -64,9 +64,9 @@ public IEnumerator TrackPacketLossAsClient()
             var clientNetworkManager = m_ClientNetworkManagers[0];
 
             var clientTransport = (UnityTransport)clientNetworkManager.NetworkConfig.NetworkTransport;
-            clientTransport.NetworkDriver.CurrentSettings.TryGet<SimulatorUtility.Parameters>(out var parameters);
+            clientTransport.GetNetworkDriver().CurrentSettings.TryGet<SimulatorUtility.Parameters>(out var parameters);
             parameters.PacketDropPercentage = m_PacketLossRate;
-            clientTransport.NetworkDriver.ModifySimulatorStageParameters(parameters);
+            clientTransport.GetNetworkDriver().ModifySimulatorStageParameters(parameters);
 
             var waitForPacketLossMetric = new WaitForGaugeMetricValues((clientNetworkManager.NetworkMetrics as NetworkMetrics).Dispatcher,
                 NetworkMetricTypes.PacketLoss,