colyseus / schema

An incremental binary state serializer with delta encoding for games.
https://docs.colyseus.io/state/schema/
MIT License
130 stars 39 forks source link

Client decoder (C#) should support decoding newer version of schema correctly. #40

Open ishnu47 opened 5 years ago

ishnu47 commented 5 years ago

Hello, As I see that the current client decoder (C#) does not support decoding with data encoded by a new schema version (adding new properties/types), could we update the client lib to support this?

With online mobile games, we usually update our schemas when adding/updating features. Whenever we submit a new client version for store review, we also need to deploy the new server version. The live client version (old schema) which users playing should also communicate with the new server without any problems.

Thank you.

endel commented 5 years ago

Hi @ishnu47, we had a discussion about this in the Discord server after you posted this. @seiyria @Wenish and @Antriel were on this discussion.

Here are some thoughts:

The clients for interpreted languages do not suffer from this since the entire schema structure is built dynamically in the client from the HANDSHAKE message sent by the server (https://github.com/colyseus/colyseus.js/blob/ca951eef1c4cf0458783c65c23790278bcd3457d/src/serializer/SchemaSerializer.ts#L32-L33)

The perfect solution would be to allow the C# client to build the actual schema structures in the client-side as well. The schema-codegen tool is currently used for that - which is great because you get strict type checking and auto-completion for the state values. I'm not sure if it's possible to use just reflection without using the schema-codegen in C# because of compilation restrictions.

As always, suggestions are welcome.

Cheers!

ishnu47 commented 5 years ago

Hello, Thanks for your response :)

I think we misunderstood about the deployment strategy.

the server-side code should not be updated until the app is not available for iOS/Android users

How can the app store review team review our new/update features in this case? We usually deploy the new version of server to serve both old client (which is played by users) and new client (which will be reviewed by the review team), and I think this is a typical deployment strategy.

Actually, I did some updates on your decoder to support this feature, but I think that might be a temporary approach. The idea is that I use the schema info (which is received by handshake response from server) to skip decoding data that belong to new fields/types. It works at this time but I'm not sure how it will be in the future.

One more suggestion on this, we're currently using field_index to sync data to client, that leads to another drawback: Removing properties in schema will break the schema update also (with old client). Can we change this to something like field_id?

Thank you :)

endel commented 4 years ago

Thanks for your input @ishnu47. Do you mind sharing the changes you've done in the C# handshake?

I was reading how flatbuffers solves this and found this article: https://google.github.io/flatbuffers/md__schemas.html

FlatBuffers relies on new field declarations being added at the end, and earlier declarations to not be removed, but be marked deprecated when needed. We think this is an improvement over the manual number assignment that happens in Protocol Buffers (and which is still an option using the id attribute mentioned above).

This feature is referred to as forwards/backwards compatibility.

Thanks! Cheers!

endel commented 4 years ago

I've had some progress with this today, the C# and JavaScript decoders are now backwards/forwards compatible by declaring new fields at the end of existing structures, and earlier declarations to not be removed, but be marked @deprecated() when needed.

The next step is fixing the C# handshake. The handshake currently iterates through the local namespace's types - trying to match fields compatible with the one coming from the server. This match is necessary to allow the decoder to instantiate the right instance in case you're using inheritance (e.g. allow Entity / Player / Bot inside arrays and/or maps).

I'm not sure how to solve this considering backwards/forwards compatibility, since the field count between old/new version of the schema is certainly going to mismatch. Ideally, we should keep this as simple as possible for all client implementations.

ishnu47 commented 4 years ago

Hello, I have added new classes for my implementation:

public class IndexedReflectionType
{
    public uint id;
    public Dictionary<int, ReflectionField> fieldsByIndex = new Dictionary<int, ReflectionField>();
    public Dictionary<string, ReflectionField> fieldsByName = new Dictionary<string, ReflectionField>();

    public IndexedReflectionType(ReflectionType type)
    {
        id = type.id;
        for (var i = 0; i < type.fields.Count; i++)
        {
            fieldsByIndex.Add(i, type.fields[i]);
            fieldsByName.Add(type.fields[i].name, type.fields[i]);
        }
    }
}

public class IndexedReflection
{
    public uint rootType;
    public Dictionary<uint, IndexedReflectionType> types = new Dictionary<uint, IndexedReflectionType>();

    public IndexedReflection(Reflection reflection)
    {
        rootType = reflection.rootType;
        for (var i = 0; i < reflection.types.Count; i++)
        {
            var type = reflection.types[i];
            types.Add(type.id, new IndexedReflectionType(type));
        }
    }
}

The idea is storing server schemas, instead of relying on C# reflection of client classes.

Inside SerializerSchema.cs:

public void Handshake (byte[] bytes, int offset)
{
        var reflection = new Reflection();
        reflection.DecodeReflection(bytes);
        indexedReflection = new IndexedReflection(reflection);
}

public void SetState(byte[] data)
{
        (state as Schema.Schema).Decode(data, indexedReflection, indexedReflection.rootType);
}

public void Patch(byte[] data)
{
        (state as Schema.Schema).Decode(data, indexedReflection, indexedReflection.rootType);
}

The method DecodeReflection is the original Decode method of Schema. I also implement new method for decoding that support skipping new added fields/types:

void Decode(byte[] bytes, IndexedReflection reflection, uint typeId, Iterator it = null);

The idea is that, with every field index received from server, I query its info from the indexedReflection (fulfilled by the handshake process, to make sure we use the latest info from server).

static string ExtractChildPrimitiveType(string collectionType)
{
      var parts = collectionType.Split(':');
      if (parts.Length > 1) return parts[1];
      return null;
}

void Decode(byte[] bytes, IndexedReflection reflection, uint typeId, Iterator it = null)
{
       ....
       // type from typeId 
       var rootType = reflection.types[typeId];
       while (it.Offset < totalBytes) {
            var index = bytes[it.Offset++];
            if (index == (byte)SPEC.END_OF_STRUCTURE)
            {
                break;
            }
            // field info from server
            var reflectionField = rootType.fieldsByIndex[index];
            var field = reflectionField.name;
            var fieldType = reflectionField.type;
            var refType = reflectionField.referencedType;

            // type at client
            System.Type childType;
            fieldChildTypes.TryGetValue(field, out childType);

            string childPrimitiveType = ExtractChildPrimitiveType(fieldType);

            if (fieldType == "ref") {
                    if (childType == null) 
                            SkipDecode(bytes, reflection, reflectionField.referencedType, it);
                    ...
            }
            // Array type
            else if (fieldType.Contains("array")) {
                    if (!fieldTypes.ContainsKey(field))
                            SkipDecodeArray(bytes, reflection, reflectionField, it);
                    ...
            }
            else if (fieldType.Contains("map")) {
                    if (!fieldTypes.ContainsKey(field)) 
                            SkipDecodeMap(bytes, reflection, reflectionField, it);
                    ...
            }
            ...
       }
}

The skip decoding logic is very similar to the original logic, to make sure we update the iterator appropriately. Hope it might help.

I love your idea that supports backward/forward compatibility :)