fix: Fix NetworkVariable<INetworkSerializable> so that it will properly call NetworkSerialize instead of using the default memcpy operation. (#1383)

ShadauxCat · web-flow · commit b53926ce5042 · 2021-11-10T19:43:32.000Z
* fix: Fix NetworkVariable&lt;INetworkSerializable&gt; so that it will properly call NetworkSerialize instead of using the default memcpy operation.

* Changelog entry

* standards fix

* Whoops, syntax error.

* More standards fixes

* Fixed typo

* Moved changelog entry to Unreleased
diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md
@@ -22,6 +22,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 - Fixed invalid IL code being generated when using `this` instead of `this ref` for the FastBufferReader/FastBufferWriter parameter of an extension method. (#1393)
 - Fixed an issue where if you are running as a server (not host) the LoadEventCompleted and UnloadEventCompleted events would fire early by the NetworkSceneManager (#1379)
 - Fixed a runtime error when sending an array of an INetworkSerializable type that's implemented as a struct (#1402)
+- Fixed NetworkVariable not calling NetworkSerialize on INetworkSerializable types (#1383)
 
 ### Changed
 
diff --git a/com.unity.netcode.gameobjects/Editor/CodeGen/CodeGenHelpers.cs b/com.unity.netcode.gameobjects/Editor/CodeGen/CodeGenHelpers.cs
@@ -264,6 +264,28 @@ public static void AddError(this List<DiagnosticMessage> diagnostics, SequencePo
             });
         }
 
+        public static void AddWarning(this List<DiagnosticMessage> diagnostics, string message)
+        {
+            diagnostics.AddWarning((SequencePoint)null, message);
+        }
+
+        public static void AddWarning(this List<DiagnosticMessage> diagnostics, MethodDefinition methodDefinition, string message)
+        {
+            diagnostics.AddWarning(methodDefinition.DebugInformation.SequencePoints.FirstOrDefault(), message);
+        }
+
+        public static void AddWarning(this List<DiagnosticMessage> diagnostics, SequencePoint sequencePoint, string message)
+        {
+            diagnostics.Add(new DiagnosticMessage
+            {
+                DiagnosticType = DiagnosticType.Warning,
+                File = sequencePoint?.Document.Url.Replace($"{Environment.CurrentDirectory}{Path.DirectorySeparatorChar}", ""),
+                Line = sequencePoint?.StartLine ?? 0,
+                Column = sequencePoint?.StartColumn ?? 0,
+                MessageData = $" - {message}"
+            });
+        }
+
         public static void RemoveRecursiveReferences(this ModuleDefinition moduleDefinition)
         {
             // Weird behavior from Cecil: When importing a reference to a specific implementation of a generic
diff --git a/com.unity.netcode.gameobjects/Editor/CodeGen/INetworkSerializableILPP.cs b/com.unity.netcode.gameobjects/Editor/CodeGen/INetworkSerializableILPP.cs
@@ -0,0 +1,167 @@
+using System;
+using System.IO;
+using System.Linq;
+using System.Collections.Generic;
+using System.Reflection;
+using Mono.Cecil;
+using Mono.Cecil.Cil;
+using Mono.Cecil.Rocks;
+using Unity.CompilationPipeline.Common.Diagnostics;
+using Unity.CompilationPipeline.Common.ILPostProcessing;
+using ILPPInterface = Unity.CompilationPipeline.Common.ILPostProcessing.ILPostProcessor;
+using MethodAttributes = Mono.Cecil.MethodAttributes;
+
+namespace Unity.Netcode.Editor.CodeGen
+{
+
+    internal sealed class INetworkSerializableILPP : ILPPInterface
+    {
+        public override ILPPInterface GetInstance() => this;
+
+        public override bool WillProcess(ICompiledAssembly compiledAssembly) =>
+            compiledAssembly.Name == CodeGenHelpers.RuntimeAssemblyName ||
+            compiledAssembly.References.Any(filePath => Path.GetFileNameWithoutExtension(filePath) == CodeGenHelpers.RuntimeAssemblyName);
+
+        private readonly List<DiagnosticMessage> m_Diagnostics = new List<DiagnosticMessage>();
+
+        public override ILPostProcessResult Process(ICompiledAssembly compiledAssembly)
+        {
+            if (!WillProcess(compiledAssembly))
+            {
+                return null;
+            }
+
+
+            m_Diagnostics.Clear();
+
+            // read
+            var assemblyDefinition = CodeGenHelpers.AssemblyDefinitionFor(compiledAssembly, out var resolver);
+            if (assemblyDefinition == null)
+            {
+                m_Diagnostics.AddError($"Cannot read assembly definition: {compiledAssembly.Name}");
+                return null;
+            }
+
+            // process
+            var mainModule = assemblyDefinition.MainModule;
+            if (mainModule != null)
+            {
+                try
+                {
+                    if (ImportReferences(mainModule))
+                    {
+                        var types = mainModule.GetTypes()
+                            .Where(t => t.Resolve().HasInterface(CodeGenHelpers.INetworkSerializable_FullName) && !t.Resolve().IsAbstract && t.Resolve().IsValueType)
+                            .ToList();
+                        // process `INetworkMessage` types
+                        if (types.Count == 0)
+                        {
+                            return null;
+                        }
+
+                        CreateModuleInitializer(assemblyDefinition, types);
+                    }
+                    else
+                    {
+                        m_Diagnostics.AddError($"Cannot import references into main module: {mainModule.Name}");
+                    }
+                }
+                catch (Exception e)
+                {
+                    m_Diagnostics.AddError((e.ToString() + e.StackTrace.ToString()).Replace("\n", "|").Replace("\r", "|"));
+                }
+            }
+            else
+            {
+                m_Diagnostics.AddError($"Cannot get main module from assembly definition: {compiledAssembly.Name}");
+            }
+
+            mainModule.RemoveRecursiveReferences();
+
+            // write
+            var pe = new MemoryStream();
+            var pdb = new MemoryStream();
+
+            var writerParameters = new WriterParameters
+            {
+                SymbolWriterProvider = new PortablePdbWriterProvider(),
+                SymbolStream = pdb,
+                WriteSymbols = true
+            };
+
+            assemblyDefinition.Write(pe, writerParameters);
+
+            return new ILPostProcessResult(new InMemoryAssembly(pe.ToArray(), pdb.ToArray()), m_Diagnostics);
+        }
+
+        private MethodReference m_InitializeDelegates_MethodRef;
+
+        private const string k_InitializeMethodName = nameof(NetworkVariableHelper.InitializeDelegates);
+
+        private bool ImportReferences(ModuleDefinition moduleDefinition)
+        {
+
+            var helperType = typeof(NetworkVariableHelper);
+            foreach (var methodInfo in helperType.GetMethods(BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public))
+            {
+                switch (methodInfo.Name)
+                {
+                    case k_InitializeMethodName:
+                        m_InitializeDelegates_MethodRef = moduleDefinition.ImportReference(methodInfo);
+                        break;
+                }
+            }
+            return true;
+        }
+
+        private MethodDefinition GetOrCreateStaticConstructor(TypeDefinition typeDefinition)
+        {
+            var staticCtorMethodDef = typeDefinition.GetStaticConstructor();
+            if (staticCtorMethodDef == null)
+            {
+                staticCtorMethodDef = new MethodDefinition(
+                    ".cctor", // Static Constructor (constant-constructor)
+                    MethodAttributes.HideBySig |
+                    MethodAttributes.SpecialName |
+                    MethodAttributes.RTSpecialName |
+                    MethodAttributes.Static,
+                    typeDefinition.Module.TypeSystem.Void);
+                staticCtorMethodDef.Body.Instructions.Add(Instruction.Create(OpCodes.Ret));
+                typeDefinition.Methods.Add(staticCtorMethodDef);
+            }
+
+            return staticCtorMethodDef;
+        }
+
+        // Creates a static module constructor (which is executed when the module is loaded) that registers all the
+        // message types in the assembly with MessagingSystem.
+        // This is the same behavior as annotating a static method with [ModuleInitializer] in standardized
+        // C# (that attribute doesn't exist in Unity, but the static module constructor still works)
+        // https://docs.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.moduleinitializerattribute?view=net-5.0
+        // https://web.archive.org/web/20100212140402/http://blogs.msdn.com/junfeng/archive/2005/11/19/494914.aspx
+        private void CreateModuleInitializer(AssemblyDefinition assembly, List<TypeDefinition> networkSerializableTypes)
+        {
+            foreach (var typeDefinition in assembly.MainModule.Types)
+            {
+                if (typeDefinition.FullName == "<Module>")
+                {
+                    var staticCtorMethodDef = GetOrCreateStaticConstructor(typeDefinition);
+
+                    var processor = staticCtorMethodDef.Body.GetILProcessor();
+
+                    var instructions = new List<Instruction>();
+
+                    foreach (var type in networkSerializableTypes)
+                    {
+                        var method = new GenericInstanceMethod(m_InitializeDelegates_MethodRef);
+                        method.GenericArguments.Add(type);
+                        instructions.Add(processor.Create(OpCodes.Call, method));
+                    }
+
+                    instructions.ForEach(instruction => processor.Body.Instructions.Insert(processor.Body.Instructions.Count - 1, instruction));
+                    break;
+                }
+            }
+        }
+    }
+}
diff --git a/com.unity.netcode.gameobjects/Editor/CodeGen/INetworkSerializableILPP.cs.meta b/com.unity.netcode.gameobjects/Editor/CodeGen/INetworkSerializableILPP.cs.meta
diff --git a/com.unity.netcode.gameobjects/Editor/CodeGen/RuntimeAccessModifiersILPP.cs b/com.unity.netcode.gameobjects/Editor/CodeGen/RuntimeAccessModifiersILPP.cs
@@ -52,6 +52,9 @@ public override ILPostProcessResult Process(ICompiledAssembly compiledAssembly)
                         case nameof(NetworkBehaviour):
                             ProcessNetworkBehaviour(typeDefinition);
                             break;
+                        case nameof(NetworkVariableHelper):
+                            ProcessNetworkVariableHelper(typeDefinition);
+                            break;
                         case nameof(__RpcParams):
                             typeDefinition.IsPublic = true;
                             break;
@@ -100,6 +103,17 @@ private void ProcessNetworkManager(TypeDefinition typeDefinition, string[] assem
             }
         }
 
+        private void ProcessNetworkVariableHelper(TypeDefinition typeDefinition)
+        {
+            foreach (var methodDefinition in typeDefinition.Methods)
+            {
+                if (methodDefinition.Name == nameof(NetworkVariableHelper.InitializeDelegates))
+                {
+                    methodDefinition.IsPublic = true;
+                }
+            }
+        }
+
         private void ProcessNetworkBehaviour(TypeDefinition typeDefinition)
         {
             foreach (var nestedType in typeDefinition.NestedTypes)
diff --git a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariable.cs
@@ -9,6 +9,55 @@ namespace Unity.Netcode
     [Serializable]
     public class NetworkVariable<T> : NetworkVariableBase where T : unmanaged
     {
+        // Functions that know how to serialize INetworkSerializable
+        internal static void WriteNetworkSerializable<TForMethod>(FastBufferWriter writer, ref TForMethod value)
+            where TForMethod : INetworkSerializable, new()
+        {
+            writer.WriteNetworkSerializable(value);
+        }
+        internal static void ReadNetworkSerializable<TForMethod>(FastBufferReader reader, out TForMethod value)
+            where TForMethod : INetworkSerializable, new()
+        {
+            reader.ReadNetworkSerializable(out value);
+        }
+
+        // Functions that serialize other types
+        private static void WriteValue<TForMethod>(FastBufferWriter writer, ref TForMethod value) where TForMethod : unmanaged
+        {
+            writer.WriteValueSafe(value);
+        }
+
+        private static void ReadValue<TForMethod>(FastBufferReader reader, out TForMethod value)
+            where TForMethod : unmanaged
+        {
+            reader.ReadValueSafe(out value);
+        }
+
+        internal delegate void WriteDelegate<TForMethod>(FastBufferWriter writer, ref TForMethod value);
+
+        internal delegate void ReadDelegate<TForMethod>(FastBufferReader reader, out TForMethod value);
+
+        // These static delegates provide the right implementation for writing and reading a particular network variable
+        // type.
+        //
+        // For most types, these default to WriteValue() and ReadValue(), which perform simple memcpy operations.
+        //
+        // INetworkSerializableILPP will generate startup code that will set it to WriteNetworkSerializable()
+        // and ReadNetworkSerializable() for INetworkSerializable types, which will call NetworkSerialize().
+        //
+        // In the future we may be able to use this to provide packing implementations for floats and integers to
+        // optimize bandwidth usage.
+        //
+        // The reason this is done is to avoid runtime reflection and boxing in NetworkVariable - without this,
+        // NetworkVariable would need to do a `var is INetworkSerializable` check, and then cast to INetworkSerializable,
+        // *both* of which would cause a boxing allocation. Alternatively, NetworkVariable could have been split into
+        // NetworkVariable and NetworkSerializableVariable or something like that, which would have caused a poor
+        // user experience and an API that's easier to get wrong than right. This is a bit ugly on the implementation
+        // side, but it gets the best achievable user experience and performance.
+        internal static WriteDelegate<T> Write = WriteValue;
+        internal static ReadDelegate<T> Read = ReadValue;
+
+
         /// <summary>
         /// Delegate type for value changed event
         /// </summary>
@@ -106,7 +155,7 @@ public override void WriteDelta(FastBufferWriter writer)
         public override void ReadDelta(FastBufferReader reader, bool keepDirtyDelta)
         {
             T previousValue = m_InternalValue;
-            reader.ReadValueSafe(out m_InternalValue);
+            Read(reader, out m_InternalValue);
 
             if (keepDirtyDelta)
             {
@@ -119,13 +168,13 @@ public override void ReadDelta(FastBufferReader reader, bool keepDirtyDelta)
         /// <inheritdoc />
         public override void ReadField(FastBufferReader reader)
         {
-            reader.ReadValueSafe(out m_InternalValue);
+            Read(reader, out m_InternalValue);
         }
 
         /// <inheritdoc />
         public override void WriteField(FastBufferWriter writer)
         {
-            writer.WriteValueSafe(m_InternalValue);
+            Write(writer, ref m_InternalValue);
         }
     }
 }
diff --git a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableHelper.cs b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableHelper.cs
@@ -0,0 +1,22 @@
+namespace Unity.Netcode
+{
+    public class NetworkVariableHelper
+    {
+        // This is called by ILPP during module initialization for all unmanaged INetworkSerializable types
+        // This sets up NetworkVariable so that it properly calls NetworkSerialize() when wrapping an INetworkSerializable value
+        //
+        // The reason this is done is to avoid runtime reflection and boxing in NetworkVariable - without this,
+        // NetworkVariable would need to do a `var is INetworkSerializable` check, and then cast to INetworkSerializable,
+        // *both* of which would cause a boxing allocation. Alternatively, NetworkVariable could have been split into
+        // NetworkVariable and NetworkSerializableVariable or something like that, which would have caused a poor
+        // user experience and an API that's easier to get wrong than right. This is a bit ugly on the implementation
+        // side, but it gets the best achievable user experience and performance.
+        //
+        // RuntimeAccessModifiersILPP will make this `public`
+        internal static void InitializeDelegates<T>() where T : unmanaged, INetworkSerializable
+        {
+            NetworkVariable<T>.Write = NetworkVariable<T>.WriteNetworkSerializable;
+            NetworkVariable<T>.Read = NetworkVariable<T>.ReadNetworkSerializable;
+        }
+    }
+}
diff --git a/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableHelper.cs.meta b/com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableHelper.cs.meta
diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/MultiInstanceHelpers.cs b/com.unity.netcode.gameobjects/Tests/Runtime/MultiInstanceHelpers.cs
@@ -450,7 +450,7 @@ public static IEnumerator RunAndWaitForCondition(Action workload, Func<bool> pre
 
             if (!waitResult.Result)
             {
-                throw new Exception();
+                Assert.Fail("Predicate condition failed");
             }
         }
 
diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs

Original file line number	Diff line number	Diff line change
`@@ -450,7 +450,7 @@ public static IEnumerator RunAndWaitForCondition(Action workload, Func<bool> pre`
`450`	`450`
`451`	`451`	`if (!waitResult.Result)`
`452`	`452`	`{`
`453`		`- throw new Exception();`
	`453`	`+ Assert.Fail("Predicate condition failed");`
`454`	`454`	`}`
`455`	`455`	`}`
`456`	`456`