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

Add factory method to create a Message by from tag and IMessageBuffer #187

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tedbarth
Copy link

Currently the only way to get pre-serialized Data (byte[]) into a Message is by either using DarkRift.DarkRiftWriter.WriteRaw and passing the Writer to Message factory method or by passing an object implementing IDarkRiftSerializable.

Both ways will cause an unnecessary memory copy of the data.

Luckily IMessageBuffer is public, so we could write this:

public class ByteArrayMappingMessageBuffer : IMessageBuffer {
  public ByteArrayMappingMessageBuffer(byte[] buffer) {
    Buffer = buffer;
  }

  public void Dispose() {
    throw new System.NotImplementedException();
  }

  public IMessageBuffer Clone() {
    throw new System.NotImplementedException();
  }

  public byte[] Buffer { get; }

  public int Count {
    get => Buffer.Length;
    set => throw new System.NotImplementedException();
  }

  public int Offset {
    get => 0;
    set => throw new System.NotImplementedException();
  }
}

The PR adds a new method to Message allowing to not only pass a DarkRiftWriter but an IMessageBuffer directly.
Passing the one above will allow us to create and send a Message using pre-serialized data and also helps separating Serialization from Networking.

byte[] serializedData = ...;
var buffer = new ByteArrayMappingMessageBuffer(serializedData);
Message message = Message.Create(message.Tag, buffer);
client.SendMessage(message, sendMode);

@JamJar00
Copy link
Member

Thanks for the PR! I really like the idea of separating out the serialization from the messages. It will help facilitate using other serializers a lot and will make some usecases a lot simpler without needing writers!

I'm not totally convinced exposing an interface for an IMessageBuffer is the right option here though. DarkRift's message buffers are a core part of its memory management and there are some nuances that go with that, for example you can't just throw an exception on Clone, that has very specific and important meaning to DarkRift's memory management!

Secondly I think we need a corresponding read method to get the data out the other end again and that can definitely not be as an IMessageBuffer (for many reasons)!

I think there's still value in the prinicple here though. So I wonder if creating void Create(ushort, byte[]) and byte[] Deserialize() would be useful. DarkRift actually has a class very similar to your ByteArrayMappingMemoryBuffer already which could be used behind the scenes of Create, however Deserialize will almost always need a memory copy still because the byte array in the message buffer will be managed by DarkRift so shouldn't be exposed to user code.

@tedbarth
Copy link
Author

Thanks for the fast reaction!
I totally understand your points and have replaced the new method (now private) with:

public static Message Create(ushort tag, byte[] data)

I currently can't see, why Deserialize() would be necessary, though. There already is this direct way, which I think is enough, unless you want it as helper method:

Message message = e.GetMessage();
DarkRiftReader darkRiftReader = message.GetReader();
byte[] data = darkRiftReader.ReadRaw(darkRiftReader.Length);

@tedbarth
Copy link
Author

tedbarth commented Nov 19, 2023

Offtopic: I often get confused by method semantic and now understood why: Your Get Methods often create new objects with separate state. For instance Message.GetReader() does create a new reader with a position set to 0. Calling it again will give you another with its own position. What confuses me here is that due to the Get prefix of the method I would expect an idempotent behavior, meaning every time I either get the same reader or a new one that is immutable or a new one that shares the same state with other readers. Instead the returned reader right now is mutable with its own state. I would expect this behavior from a method with name Message.CreateReader().

@tedbarth tedbarth closed this Nov 19, 2023
@tedbarth
Copy link
Author

tedbarth commented Nov 19, 2023

Oh damn, I somehow closed this issue. :D

@tedbarth tedbarth reopened this Nov 19, 2023
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.

3 participants