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

Change default Unity compilation symbols for json serializer #187

Closed voidtuxic closed 2 years ago

voidtuxic commented 2 years ago

The JSON serializer configures the default serializers for unity object but uses quite old version related compilation symbols. These unfortunately don't work in newer Unity versions, which means that sending a message with a unity vector will stack overflow in the ObjectSerializer.

https://github.com/colyseus/colyseus-unity3d/blob/0e003af9e558544268e352d38441c1e93139de5d/Assets/Colyseus/Runtime/GameDevWare.Serialization/Json.cs#L98

As much as it is easy to define new symbols in Unity, I am not very comfortable with the idea of defining a symbol that represents an old Unity version, because of the obvious conflicts that might occur with parallel code from my scripts or other vendors that support legacy versions of the editor. Moreover the fact that using these serializers on newer Unity versions works fine. I am thinking of making a PR to at least change the define symbols in code if that's okay with the team.

Ideally, I'd suggest to the team to add a custom define to the mix, as their vendor define, which many plugins like Odin do, and auto add it to the player config when the package is imported in the project. This mostly serves as making sure project setup is good, but also allow people to disable it if it's useless. Of course that's maybe something for the future.

endel commented 2 years ago

HI @voidtuxic, thanks for reporting this!

The MsgPack implementation is a copy from deniszykov/msgpack-unity3d

I noticed they've updated their implementation slightly here: https://github.com/deniszykov/msgpack-unity3d/blob/faa19c250837fc1b175809a70cf1e12252ff7b02/src/GameDevWare.Serialization.Unity/Assets/Plugins/GameDevWare.Serialization/Json.cs#L98

I'm not a heavy Unity user, do you think that'd be enough? Thanks!

voidtuxic commented 2 years ago

yep that was exactly the fix I was thinking of making a PR for, this works great!