Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

breaking: no need to override Serialize/Deserialize in messages #2317

Merged
merged 12 commits into from Oct 6, 2020

Conversation

paulpach
Copy link
Contributor

@paulpach paulpach commented Oct 5, 2020

Messages no longer serialize themselves. This has been decoupled. Serializing a message is now done
via readers and writers, which can be either generated or user provided.

This lifts some restrictions,

  • you no longer need to have a default constructor in messages
  • Messages types can be recursive
  • struct Messages don't need to provide an empty Serialize and Deserialize method

Before:

public struct ReadyMessage : IMessageBase
{
    public void Deserialize(NetworkReader reader) { }

    public void Serialize(NetworkWriter writer) { }
}

After:

public struct ReadyMessage : NetworkMessage
{
}

For custom overrides for Serialize/Deserialize instead use custom reader and writer instead

Before (Click to expand)
public struct ProjectileState
{
  public uint projectileId;
  public Vector3 position;
  public Quaternion rotation;
  public Vector3 velocity;
}
public struct SyncProjectilesMessage : IMessageBase
{
  public static ProjectileState[] buffer = new ProjectileState[1000];
  public int count;

  public void Deserialize(NetworkReader reader)
  {
      count = reader.ReadInt32();
      for (int i = 0; i < count; i++)
      {
          buffer[i].projectileId = reader.ReadUInt32();
          buffer[i].position = reader.ReadVector3();
          buffer[i].rotation = reader.ReadQuaternion();
          buffer[i].velocity = reader.ReadVector3();
      }
  }

  public void Serialize(NetworkWriter writer)
  {
      writer.WriteInt32(count);
      for (int i = 0; i < count; i++)
      {
          writer.WriteUInt32(buffer[i].projectileId);
          writer.WriteVector3(buffer[i].position);
          writer.WriteQuaternion(buffer[i].rotation);
          writer.WriteVector3(buffer[i].velocity);
      }
  }
}
After (Click to expand)
public struct ProjectileState
{
  public uint projectileId;
  public Vector3 position;
  public Quaternion rotation;
  public Vector3 velocity;
}
public struct SyncProjectilesMessage : NetworkMessage
{
  public static ProjectileState[] buffer = new ProjectileState[1000];
  public int count;
}
public static class SyncProjectilesMessageFunctions
{
  public static void Serialize(this NetworkWriter writer, SyncProjectilesMessage value)
  {
      writer.WriteInt32(value.count);

      for (int i = 0; i < value.count; i++)
      {
          writer.WriteUInt32(SyncProjectilesMessage.buffer[i].projectileId);
          writer.WriteVector3(SyncProjectilesMessage.buffer[i].position);
          writer.WriteQuaternion(SyncProjectilesMessage.buffer[i].rotation);
          writer.WriteVector3(SyncProjectilesMessage.buffer[i].velocity);
      }
  }

  public static SyncProjectilesMessage Deserialize(this NetworkReader reader)
  {
      SyncProjectilesMessage value = new SyncProjectilesMessage
      {
          count = reader.ReadInt32(),
      };

      for (int i = 0; i < value.count; i++)
      {
          SyncProjectilesMessage.buffer[i].projectileId = reader.ReadUInt32();
          SyncProjectilesMessage.buffer[i].position = reader.ReadVector3();
          SyncProjectilesMessage.buffer[i].rotation = reader.ReadQuaternion();
          SyncProjectilesMessage.buffer[i].velocity = reader.ReadVector3();
      }

      return value;
  }
}

BREAKING CHANGE: Messages must be public
BREAKING CHANGE: Use custom reader and writer instead of Serialize/Deserialize methods

Messages no longer serilize themselves.  This has been decoupled.  Serializing a message is now done
via readers and writers, which can be either generated or user provided.

This lifts some restrictions,
* you no longer need to have a default constructor in messages
* Messages types can be recursive
* struct Messages don't need to provide an empty Serialize and Deserialize method

Before:
```cs
public struct ReadyMessage : IMessageBase
{
    public void Deserialize(NetworkReader reader) { }

    public void Serialize(NetworkWriter writer) { }
}
```

After:
```cs
public struct ReadyMessage : IMessageBase
{
}
```

BREAKING CHANGE: Messages must be public
BREAKING CHANGE: Use custom reader and writer instead of Serialize/Deserialize methods
@@ -1,23 +0,0 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this being removed?

@James-Frowen
Copy link
Contributor

This breaks a lot of messages that use the Serialize/Deserialize methods.

The way in which is breaks the message is the worst part.

Some types that were handled manually before are no longer handled causing errors like this when updating:
image

The hint or message to say that a now broken message should have custom writers.

Once users figure out (probably with support from discord) then the result will look like this, which writing from scratch seems fine, but going from before->after seems pointless.
Before: https://hatebin.com/otnlbfnoow
After: https://hatebin.com/iqbdqwoydw

The worst part is even after fixing the weaver errors the whole game break because custom Serialize/Deserialize methods are no longer being used such has: https://hatebin.com/tufaepvksq.

I dont think breaking most messages that take advantage of the Serialize/Deserialize methods is worth 500 LOC.

@sonarcloud
Copy link

sonarcloud bot commented Oct 5, 2020

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

78.3% 78.3% Coverage
0.0% 0.0% Duplication

@miwarnec miwarnec merged commit 31b07ae into master Oct 6, 2020
@miwarnec miwarnec deleted the noserialize branch October 6, 2020 07:31
miwarnec pushed a commit that referenced this pull request Oct 6, 2020
* breaking: no need to override Serialize/Deserialize in messages

Messages no longer serilize themselves.  This has been decoupled.  Serializing a message is now done
via readers and writers, which can be either generated or user provided.

This lifts some restrictions,
* you no longer need to have a default constructor in messages
* Messages types can be recursive
* struct Messages don't need to provide an empty Serialize and Deserialize method

Before:
```cs
public struct ReadyMessage : IMessageBase
{
    public void Deserialize(NetworkReader reader) { }

    public void Serialize(NetworkWriter writer) { }
}
```

After:
```cs
public struct ReadyMessage : IMessageBase
{
}
```

BREAKING CHANGE: Messages must be public
BREAKING CHANGE: Use custom reader and writer instead of Serialize/Deserialize methods

* Remove unused method

* remove unused methods

* remove unused methods

* make all messages struct

* Fix test code generator

* Get rid of MessageBase

* Rename IMessageBase -> NetworkMessage

* add MessageBase as obsolete

* Use a default request

* Empty file to make asset store happy

* Apply suggestions from code review

Co-authored-by: James Frowen <jamesfrowendev@gmail.com>

Co-authored-by: James Frowen <jamesfrowendev@gmail.com>
@miwarnec miwarnec mentioned this pull request Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants