colyseus / colyseus-unity-sdk

⚔ Colyseus Multiplayer SDK for Unity
https://docs.colyseus.io/getting-started/unity-sdk/
MIT License
371 stars 100 forks source link

Reflection and serialization issues between classes with the same number and type of fields #226

Open JDWon opened 4 months ago

JDWon commented 4 months ago

Hi, There is an issue related to Reflection and I would like to report it.

export class State2 extends Schema {
  @type('string')
  fieldString: string;

  @type('number') // varint
  fieldNumber: number;

  @type(Player)
  player: Player;

  @type([ Player ])
  arrayOfPlayers: ArraySchema<Player>;

  @type({ map: Player })
  mapOfPlayers: MapSchema<Player>;
}

export class State extends Schema {
  @type('string')
  fieldString: string;

  @type('number') // varint
  fieldNumber: number;

  @type(Player)
  player: Player;

  @type([ Player ])
  arrayOfPlayers: ArraySchema<Player>;

  @type({ map: Player })
  mapOfPlayers: MapSchema<Player>;
}
/// <inheritdoc />
public void Handshake(byte[] bytes, int offset)
{
    System.Type targetType = typeof(T);

    System.Type[] allTypes = targetType.Assembly.GetTypes();
    System.Type[] namespaceSchemaTypes = Array.FindAll(allTypes, t => t.Namespace == targetType.Namespace &&
                                                                      typeof(Schema.Schema).IsAssignableFrom(
                                                                          targetType));

    ColyseusReflection reflection = new ColyseusReflection();
    Iterator it = new Iterator {Offset = offset};

    reflection.Decode(bytes, it);

    for (int i = 0; i < reflection.types.Count; i++)
    {
        System.Type schemaType = Array.Find(namespaceSchemaTypes, t => CompareTypes(t, reflection.types[i]));

        if (schemaType != null)
        {
            ColyseusContext.GetInstance().SetTypeId(schemaType, reflection.types[i].id);   
        } 
        else 
        {
            UnityEngine.Debug.LogWarning(
                "Local schema mismatch from server. Use \"schema-codegen\" to generate up-to-date local definitions.");
        }
    }
}

In the following case, after reflecting the State, I expect the State to be designated as the Type id in the Serialize that occurs during the handshake, but State2 is designated, so I think I need to check.

Is it possible to check whether this is a restriction or whether separate exception handling is required?

endel commented 4 months ago

Hi @JDWon, thanks for reporting. Curious if this is related with the "global context" described here? https://github.com/colyseus/colyseus-unity-sdk/issues/131

If you define the properties of each State with their own "context" does this issue goes away? e.g.:

const type = Context.create(); // this is your @type() decorator bound to a context

class State2 extends Schema {
  @type(...) my_prop
}

(This limitation will be fixed on https://github.com/colyseus/colyseus/issues/709 with no need to manually create contexts.)

JDWon commented 4 months ago

Hi @endel , thanks for your reply. Unfortunately, even with the solution you provided, this problem occurs in Colyseus Unity sdk (Client schema).

// ex) State type id is 3, global context
export class State1 extends Schema {
  @type('string')
  fieldString: string;

  @type('number') // varint
  fieldNumber: number;
}

C# code State schema

[System.Serializable]
public class State2 : Schema
{
    //public string updateHash;

    [Type(0, "string")]
    public string id = default(string);

    [Type(1, "string")]
    public string ownerId = default(string);
}

[System.Serializable]
public class State1 : Schema
{
    //public string updateHash;

    [Type(0, "string")]
    public string id = default(string);

    [Type(1, "string")]
    public string ownerId = default(string);
}

From what I understand, the HandShake step of ColyseusSchemaSerializer in C# code performs reflection based on the field type, name, and field index. ( Assets/Colyseus/Runtime/Colyseus/Serializer/SchemaSerializer.cs => public void Handshake(byte[] bytes, int offset))

At this time, it appears that a class with the same configuration (type, name, index...) is found in the assembly and the typeID is set. (Assets/Colyseus/Runtime/Colyseus/Serializer/Schema/Context.cs => ColyseusContext.GetInstance().SetTypeId(..))

        /// <inheritdoc />
        public void Handshake(byte[] bytes, int offset)
        {
            System.Type targetType = typeof(T);

            System.Type[] allTypes = targetType.Assembly.GetTypes();
            // namespaceSchemaTypes = State2, State1
            System.Type[] namespaceSchemaTypes = Array.FindAll(allTypes, t => t.Namespace == targetType.Namespace &&
                                                                              typeof(Schema.Schema).IsAssignableFrom(
                                                                                  targetType));

            ColyseusReflection reflection = new ColyseusReflection();
            Iterator it = new Iterator {Offset = offset};

            reflection.Decode(bytes, it);

            for (int i = 0; i < reflection.types.Count; i++)
            {
                // Find State2, but I excepted find State1
                System.Type schemaType = Array.Find(namespaceSchemaTypes, t => CompareTypes(t, reflection.types[i]));

                if (schemaType != null)
                {
                   // Set type id 3, State2
                    ColyseusContext.GetInstance().SetTypeId(schemaType, reflection.types[i].id);   
                } 
                else 
                {
                    UnityEngine.Debug.LogWarning(
                        "Local schema mismatch from server. Use \"schema-codegen\" to generate up-to-date local definitions.");
                }
            }
        }

It is expected to read State1 during the reflection stage, but it reads State2 first, causing an unintended operation. This appears to be because State2 is declared first on an assembly basis and has the same field configuration.

I wonder if there is a plan to deal with this or if it is placed as a restriction.

endel commented 4 months ago

Hi @JDWon, you're right, thanks for the detailed report! This is considered a bug and is intended to be fixed in the next version.

On the incoming v3 of schemas we have a TypeContext for the encoder and decoder. I'm not yet sure how to approach this in C#, will need to experiment!

JDWon commented 4 months ago

Thanks for your reply. I will wait next version. Thanks!