Custom class serialization without default constructor does not work

[Muchaszewski]

Describe the bug Custom class serialization without default constructor does not work

To Reproduce Steps to reproduce the behavior:

    [ServerRpc]
    public void TestServerRCP(Foo foo)
    {
    // foo is null
    }

    public class Foo : INetworkSerializable
    {
        public string Boo;
        public string Poo;

        public Foo(string boo, string poo)
        {
            Boo = boo;
            Poo = poo;
        }

        /// <inheritdoc />
        public void NetworkSerialize(NetworkSerializer serializer)
        {
            serializer.Serialize(ref Boo);
            serializer.Serialize(ref Poo);
        }
    }

Actual outcome Foo is null

Expected outcome

• Provided constructor should be used instead, (if possible) or exception should be returned
• Exception should be returned that the default constructor is missing.
• Allow for custom constructors in serializer API

Environment (please complete the following information):

• OS: Windows 10
• Unity Version: 2020.3.5f1
• MLAPI Version: 1.0.0

[LukeStampfli]

Can you please confirm whether Foo is a struct? Structs in c# are value types they cannot be null. Most likely Foo was a class instead?

[Muchaszewski]

You are right. This container was initially a struct but I had changed it to class and forgot about it. I modified the issue to reflect the real problem with class and not struct.

[LukeStampfli]

And just to confirm. The above will result in a null ref exception but if we add the following constructor (default constructor) to it, it should work?

        public Foo()
        {
        }

[Muchaszewski]

Thats correct

[LukeStampfli]

@MFatihMAR 👀

[0xFA11]

yeah, deserialization code can't construct/instantiate types with non-default constructors. that's a limitation and I think we should mention this on the docs side too and ILPP should spit out an error indicating this limitation.

[ArgentumVir]

In case its useful to anyone else, in this thread (and the current documentation) the term default constructor is being used to refer to a parameterless constructor (this messed me up when googling a bit). To elaborate unnecessarily, this constructor only exists if you have explicitly defined it or you have defined no constructors (and so the default constructor given to you is a parameterless one)

tldr; In short, your receiver will receive null instead of the value you expect if no parameterless constructor is provided in the class you are trying to serialize.

Gratuitous examples for those who only read code and are allergic to text:

✔️ The following will serialize properly (C# gives you a default constructor since you didn't declare any constructors at all and this default constructor is parameterless)

using MLAPI.Serialization;

public class Example : INetworkSerializable
{
    public int SomeVariable;

    public void NetworkSerialize(NetworkSerializer serializer)
    {
        serializer.Serialize(ref SomeVariable);
    }
}

❌ The following will NOT serialize properly, the receiver will just get null because you have defined a constructor with parameters, so C# will not give you a default one (which would have been parameterless)

using MLAPI.Serialization;

public class Example : INetworkSerializable
{
    public int SomeVariable;

    public void NetworkSerialize(NetworkSerializer serializer)
    {
        serializer.Serialize(ref SomeVariable);
    }

    public Example(int someVariable)
    {
        SomeVariable = someVariable;
    }
}

✔️ The following will serialize properly because a parameterless constructor was explicitly declared on the class

using MLAPI.Serialization;

public class Example : INetworkSerializable
{
    public int SomeVariable;

    public void NetworkSerialize(NetworkSerializer serializer)
    {
        serializer.Serialize(ref SomeVariable);
    }

    // You need this parameterless constructor for serialization to not return null
    public Example()
    {
    }

    public Example(int someVariable)
    {
        SomeVariable = someVariable;
    }
}

[0xFA11]

what would you think about this @ShadauxCat?

[ShadauxCat]

@MFatihMAR this is working as expected and as designed. We can't deserialize without knowing how to construct the type, and the only way we can reasonably construct it is with a parameterless constructor since we have no parameters to pass in. What @ArgentumVir describes is how the C# language works, and we can't change it, and even if we could... If a class or struct has defined a constructor, then any default parameterless constructor we could provide would almost certainly be incorrect.

The answer here is that users simply need to make sure a parameterless constructor exists, in this case by defining it manually since they have provided another constructor. The new() constraint on those methods is there intentionally... I'd say this is not ”will not fix”, but ”cannot fix” and probably also ”should not fix” even if we could since there's no solution I can imagine that we could reasonably be certain is correct.

[0xFA11]

I mean what do you think about minor docs update + erroring when no default ctor found? :)

[ShadauxCat]

Oh, sorry. Yes, taking another look at it, I realize this is actually a little different than what I thought it was.

This seems to be specific to RPC parameters, and the issue is that the ILPP code that's generating RPC instructions isn't properly checking the new() constraint on the method it's emitting code for. As a result, the method's getting called, but it's unable to operate (invalid IL instructions), so the result ends up being null.

That's something we can (and should and will) fix.

[0xFA11]

@ShadauxCat, not urgent and also not triaged yet but I think it'd worth checking if we implemented RPC params new() (default parameterless constructor) constraint check on the ILPP side and closing this one off. There might be a minor update on the docs side (as we mentioned before above) but I actually want to just leave all this on your judgement.