Unity-Technologies / com.unity.netcode.gameobjects

Netcode for GameObjects is a high-level netcode SDK that provides networking capabilities to GameObject/MonoBehaviour workflows within Unity and sits on top of underlying transport layer.
MIT License
2.15k stars 437 forks source link

Custom Serialization can't be used because of compile-time error in RPCs. #665

Closed Dziii closed 2 years ago

Dziii commented 3 years ago

Describe the bug RPCs with parameters that are not built in types trigger a compile time error. This prevents us from using the custom serialization described here: https://mp-docs.dl.it.unity3d.com/docs/advanced-topics/custom-serialization

To Reproduce 1) Create an RPC with a parameter you plan to have custom serialization for. IE: public void ExampleServerRPC(Vector2Int v2i) { }

2) Observe that the Unity Editor returns "error - RPC method parameter does not support serialization: UnityEngine.Vector2Int" as a compile-time error

Expected behavior No compile-time error. Instead, unity returns a run-time error if a non built in type isn't registered with custom serialization.

Environment (please complete the following information):

0xFA11 commented 3 years ago

created MTT-576 for tracking in the backlog.

ArcadeRenegade commented 3 years ago

Is there a workaround for this?

0xFA11 commented 3 years ago

hey @Dziii — do you mind sharing more code snippets? potentially the Vector2Int implementation and the class that implements ExampleServerRPC method? a custom type that implements INetworkSerializable should just be fine, I wonder what you were trying to achieve there.

ArcadeRenegade commented 3 years ago

If it helps, my use case is using Protobuf to serialize complex classes. I got it working with NetworkVariable but I can't get it to work with RPCs because of the compiler error.

using System.IO;
using UnityEngine;
using MLAPI.Serialization;
using MLAPI.Serialization.Pooled;
using Models;
using Google.Protobuf;

[ExecuteInEditMode]
public class CustomSerialization : MonoBehaviour
{
    void Start()
    {
        SerializationManager.RegisterSerializationHandlers<InventoryState>(
            SerializeProtobufMessage,
            DeserializeInventoryState
        );

        SerializationManager.RegisterSerializationHandlers<EquipmentState>(
            SerializeProtobufMessage,
            DeserializeEquipmentState
        );
    }

    private void SerializeProtobufMessage<T>(Stream stream, T instance) where T : IMessage<T>
    {
        using (PooledNetworkWriter writer = PooledNetworkWriter.Get(stream))
        {
            writer.WriteByteArray(
                instance.ToByteArray()
            );
        }
    }

    private InventoryState DeserializeInventoryState(Stream stream)
    {
        return DeserializeProtobufMessage<InventoryState>(stream, InventoryState.Parser);
    }

    private EquipmentState DeserializeEquipmentState(Stream stream)
    {
        return DeserializeProtobufMessage<EquipmentState>(stream, EquipmentState.Parser);
    }

    private T DeserializeProtobufMessage<T>(Stream stream, MessageParser<T> parser) where T : IMessage<T>
    {
        using (PooledNetworkReader reader = PooledNetworkReader.Get(stream))
        {
            return parser.ParseFrom(
                reader.ReadByteArray()
            );
        }
    }
}
TwoTenPvP commented 3 years ago

hey @Dziii — do you mind sharing more code snippets?

potentially the Vector2Int implementation and the class that implements ExampleServerRPC method?

a custom type that implements INetworkSerializable should just be fine, I wonder what you were trying to achieve there.

https://docs.unity3d.com/ScriptReference/Vector2Int.html

My understanding is that he wants to use custom serialization handlers for the RPC.

Dziii commented 3 years ago

hey @Dziii — do you mind sharing more code snippets? potentially the Vector2Int implementation and the class that implements ExampleServerRPC method? a custom type that implements INetworkSerializable should just be fine, I wonder what you were trying to achieve there.

https://docs.unity3d.com/ScriptReference/Vector2Int.html

My understanding is that he wants to use custom serialization handlers for the RPC.

Yup, exactly. It used to work before the version 0.1.0 merge. Based on the logging in this thread, it sounds like it has been fixed and we're just waiting for the next release? (When could we expect it?)

For what it's worth though, this is basically the code:

public class NetworkController : NetworkBehaviour
{
    public void Start()
    {
        SerializationManager.RegisterSerializationHandlers(NetworkSerializations.SerializeVector2Int, NetworkSerializations.DeserializeVector2Int);
    }

    [ServerRpc]
    public void ExampleServerRPC(Vector2Int v2i)
    {
    }
}
0xFA11 commented 3 years ago

Currently, custom serialization handlers are not supported by RPC API. We have some plans around serialization and custom serialization, I believe this would also be captured under there as well.

We have built-in types serialization (which doesn't cover Vector2Int but covers others suchs as Vector2, int etc.) We have custom type serialization with INetworkSerializable interface.

A workaround might be to wrap Vector2Int into a custom type that implements INetworkSerializable and it'll be just fine. (see: Serialization / INetworkSerializable in docs)

Example:

struct MyVector2Int : INetworkSerializable
{
    Vector2Int Value;

    void NetworkSerialize(NetworkSerializer serializer)
    {
        // serialization code here
    }
}
Marc-Ducret commented 3 years ago

I am running into the same issue while trying to upgrade to MLAPI 0.1.0.

The custom serialization handlers were very useful to me as some of my serializers depend on runtime data that is not available statically (in ScriptableObjects). I had my serialization handlers defined in MonoBehaviors which I could link to those ScriptableObjects.

The migration is therefore going to be quite hard for me, and this was not documented at all so this is slightly frustrating.

As a side note, I read through https://github.com/Unity-Technologies/com.unity.multiplayer.rfcs/blob/master/text/0001-std-rpc-api.md and I think the following part confuses "exponential" with "quadratic":

Previously, UNET tried to replace all RPC call-sites via ILPP which meant that it had to iterate over every single method to check potential RPC call sites, traversing the entire assembly from top to bottom. That's not an optimal and clever way of doing it, also it doesn't scale well. 1000 RPCs meant 1000 iterations over the entire assembly, which as you might expect, doesn't scale up well, it scales exponentially.

ArcadeRenegade commented 3 years ago

Hi @MFatihMAR

Currently, custom serialization handlers are not supported by RPC API. We have some plans around serialization and custom serialization, I believe this would also be captured under there as well.

https://docs-multiplayer.unity3d.com/docs/advanced-topics/custom-serialization

The below is in the Custom Serialization docs. It specifically mentions RPCs and that we can override serialization for all types.

When using RPC's, NetworkVariable's or any other MLAPI related task that requires serialization. The MLAPI uses a default serialization pipeline that looks like this:

With this flow, you can override ALL serialization for ALL types, even built in types, and with the API provided, it can even be done with types that you have not defined yourself, those who are behind a 3rd party wall, such as .NET types.

I know my custom sterilization works because it does work for NetworkVariables. The compiler just will not let me use it for RPCs. Unfortunately, having my Protobuf models inherit INetworkSerializable is not a viable workaround as INetworkSerializable overrides any custom serialization handlers.

Would it be possible for Unity to return a run-time error instead like @Dziii mentioned?

0xFA11 commented 3 years ago

custom serialization handlers feature is not ripped out from the codebase (works for NetworkVariables) but we didn't roll them over to RPCs yet.

we will rework custom serialization handlers and also roll them over to RPCs as well. build-time error is intended, runtime is too late for an error in the current RPC design.

Marc-Ducret commented 3 years ago

Actually serializing the parameters of my RPCs is now strictly impossible in 0.1.0.

The INetworkSerializable interface makes the assumption that the deserialized value is a new object.

I have a type T which has a few complex yet constant instances that I store in a array of type T[]. Then I want to serialize this type T by assuming the array contains all possible values of T and therefore using the index in the array.

This pattern is impossible to implement using the INetworkSerializable but is fairly simple using the custom serialization handlers.

0xFA11 commented 3 years ago

hey @Marc-Ducret — do you mind sharing a barebones sample code snippet?

Marc-Ducret commented 3 years ago

Sure, here is such a code snippet:

public class BuildingSerialization : MonoBehaviour {
    // Scriptable object linked in editor
    public BuildingTypes buildingTypes;

    private Dictionary<BuildingType, int> indexes;
    private BuildingType[] orderedBuildingTypes;

    private void Awake() {
        orderedBuildingTypes = new[] {
            buildingTypes.house,
            buildingTypes.farm
        };
        indexes = new Dictionary<BuildingType, int>();
        for (int i = 0; i < orderedBuildingTypes.Length; i++) indexes[orderedBuildingTypes[i]] = i;
    }

    private void OnEnable() {
        SerializationManager.RegisterSerializationHandlers(
            (stream, instance) => {
                using PooledNetworkWriter writer = PooledNetworkWriter.Get(stream);
                WriteBuildingType(writer, instance);
            },
            stream => {
                using PooledNetworkReader reader = PooledNetworkReader.Get(stream);
                return ReadBuildingType(reader);
            });
    }

    private void OnDisable() {
        SerializationManager.RemoveSerializationHandlers<BuildingType>();
    }

    public void WriteBuildingType(NetworkWriter writer, BuildingType buildingType) {
        writer.WriteInt32Packed(indexes[buildingType]);
    }

    public BuildingType ReadBuildingType(NetworkReader reader) {
        return orderedBuildingTypes[reader.ReadInt32Packed()];
    }
}
0xFA11 commented 3 years ago

if I'm getting this right, you're not even serializing BuildingType at all, you're just serializing an index. why serializing an int index is not enough to cover you in this case?

and again, we will have custom serialization handlers in the future on RPC API as well (even also with some refactorings) but I believe you could still achieve similar results with a few code reorganizations/workarounds atm.

Marc-Ducret commented 3 years ago

Serializing as an int is not a great solution in terms of type safety.

I was able to fix this issue by wrapping all custom types in the following hacky struct:

    public struct Serializable<T> : INetworkSerializable {
        public T value;

        public void NetworkSerialize(NetworkSerializer serializer) {
            if (serializer.IsReading) value = (T) serializer.Reader.ReadObjectPacked(typeof(T));
            else serializer.Writer.WriteObjectPacked(value);
        }
    }

    public static class SerializableExtension {
        public static Serializable<T> Serializable<T>(this T value) =>
            new Serializable<T> {value = value};
    }

Previous RPC:

public void SendSync() {
    InvokeClientRpcOnEveryone(Sync, Data);
}

[ClientRPC]
private void Sync(T data) {
    ApplySync(data);
}

New RPC:

public void SendSync() {
    SyncClientRpc(Data.Serializable());
}

[ClientRpc]
private void SyncClientRpc(Serializable<T> data) {
    ApplySync(data.value);
}

But then since many of my RPCs are defined in classes with generic types I ran into https://github.com/Unity-Technologies/com.unity.multiplayer.mlapi/issues/714 (and the suggested fix would require a huge refactoring and break a lot of encapsulation). So I will stick with v12.1.7 for this project since it, so far, flawlessly solves my needs.

shortenda commented 3 years ago

Hi, any update on this work or a time it can be expected to land?

0xFA11 commented 2 years ago

I see MTT-576 marked as "Done" — would you like to give us a summary here and over at the JIRA side too? @ShadauxCat

ShadauxCat commented 2 years ago

This entire system was rewritten as part of the serialization updates. #1082 and #1187. The current way of handling custom type serialization is: