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

[ShadauxCat]

This is implemented by putting NetworkVariable's read and write functionality inside a delegate and using ILPP to override that delegate at startup for INetworkMessage types.

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.

MTT-1444

Changelog

com.unity.netcode.gameobjects

• Fixed: Fixed NetworkVariable not calling NetworkSerialize on INetworkSerializable types

Testing and Documentation

• Includes integration tests.
• No documentation changes or additions were necessary.

[andrews-unity]

Can we move this up into the unreleased section as pre.3 is going to go out today and this will likely land in pre.4 which will include this and a UTP update to pre.8 for the UTP Adapter.

[ShadauxCat]

Done.

[0xFA11]

... runtime reflection and boxing ... need to do a var is INetworkSerializable check ...

do you mean var is INetworkSerializable when you say reflection?

also I'm not entirely sure if all this complexity would be justified by just "avoiding boxing allocation".
the trade-off there might not be worth at least until it actually becomes a problem and show up in performance metrics.

[ShadauxCat]

@MFatihMAR

do you mean var is INetworkSerializable when you say reflection?

No - I mean that one way to avoid boxing is to look up the correct method using reflection and basically do exactly what this solution does. The difference being in how it finds the delegate.

Boxing would actually be done twice - apparently var is INetworkSerializable is itself a boxing operation, and then the cast would be another boxing operation. I'm not sure if the compiler is smart enough to reuse the same boxing allocation for both statements or not.

[0xFA11]

alright, I got you now.

well, you can have only 1 object boxing with something like this:

if (myNetVar is INetworkSerializable serializableNetVar) // <-- check type and define the variable with type
{
    // do something with `serializableNetVar` which has `INetworkSerializable` type
}

so, personally, I'm still not entirely convinced that the code complexity would worth it.
however, I'm happy to defer to @andrews-unity and/or @mattwalsh-unity if you'd like.

[andrews-unity]

So the is check will always cause a box in our case but the compiler is smart enough using the syntax above to only do one box.

public interface IThing {
}

public class ABC<T> where T : unmanaged {
    public T x;
    
    public void Rawr() {
        if (x is IThing s) {
          Console.WriteLine(s.ToString());
        }    
    }
    
}

public struct Thing : IThing 
{
}

.method public hidebysig 
        instance void Rawr () cil managed 
    {
        // Method begins at RVA 0x2058
        // Code size 32 (0x20)
        .maxstack 1
        .locals init (
            [0] class IThing s
        )

        IL_0000: ldarg.0
        IL_0001: ldfld !0 class ABC`1<!T>::x
        IL_0006: box !T
        IL_000b: isinst IThing
        IL_0010: stloc.0
        IL_0011: ldloc.0
        IL_0012: brfalse.s IL_001f

        IL_0014: ldloc.0
        IL_0015: callvirt instance string [System.Private.CoreLib]System.Object::ToString()
        IL_001a: call void [System.Console]System.Console::WriteLine(string)

        IL_001f: ret
    } // end of method ABC`1::Rawr

[ShadauxCat]

well, what I will say is this implementation was the result of a couple of hours of conversation with @andrews-unity and @mattwalsh-unity . They're entitled to changing their minds, of course, but this was the solution we landed on after discussing several options, one of which was just accepting the box.

Personally I think this would be one of the worst possible places to have a box because this would cause

One box
Per frame
Per user network variable that was changed
Per network variable write OR read

If user code has 100 objects, each of which is writing to 10 network variables every frame, we'd be saddling our users with 1,000 boxing allocations per frame.... and the number of allocations scales with user usage patterns... I think avoiding that is worth the complexity, but that's just my opinion.

[0xFA11]

if we're really trying to avoid it, we could synthesize serialize/deserialize method delegates in the constructor with reflection (as your other reflection-based suggestion). we could use reader.ReadValueSafe/writer.WriteValueSafe for non-INetworkSerialize and INetworkSerializable.NetworkSerialize for INetworkSerialize-based NetworkVariable<T>s.

that'd avoid ILPP at a reasonable (one-off) runtime cost and get us a non-boxed solution.

[ShadauxCat]

@MFatihMAR That was actually one of my initial suggestions, but I was told Unity style forbids using reflection at all. Plus, based on what we're seeing with tests, xbox one, xbox scarlet, and switch may not even work with that approach.... they seem to be aggressively removing functions that we only access via reflection.

[andrews-unity]

To be clear I do not think we should be using reflection within Runtime code and we generally avoid doing so at all costs with most Unity code. (not saying we don't do it at times but you won't find many cases within the core engine or many packages where we are doing it)

[0xFA11]

given the constraints and problems we have, I guess I'll bite the bullet here