RiptideNetworking / Riptide

Lightweight C# networking solution for multiplayer games.
https://riptide.tomweiland.net
MIT License
1.14k stars 144 forks source link

Compact message data #160

Closed Per6 closed 1 month ago

Per6 commented 1 month ago

This allows for more compact messages to be made, by defining a maximum and minimum for whole numbers and bits of accuracy for decimal ones. Also allows for strings to be more compact. I also removed a bunch of Converter stuff as well as the Get/SetBits methods, since they don't fit into riptide anymore at all.

Per6 commented 1 month ago

I forgot, that i still have queued send mode in there as well, but that can be removed if it's not wanted.

tom-weiland commented 1 month ago

This allows for more compact messages to be made,

What do you mean by "more compact"? Using less bits/bytes?

by defining a maximum and minimum for whole numbers and bits of accuracy for decimal ones.

This was already doable with AddBits (and some extension methods if you wanted it to be more convenient).

I also removed a bunch of Converter stuff as well as the Get/SetBits methods, since they don't fit into riptide anymore at all.

Why...? And how do they not "fit into Riptide anymore"??

From skimming through the changes in this PR for a mere 5 minutes (which is the first problem with merging it—there are way too many different changes all in one PR), I already have a number of questions/concerns:

There are SO many other fundamental changes in the Message class alone—the vast majority of which are breaking changes—that I really see no benefit to merging this. You seem to have changed how much of the library works and I think it's better for this to just remain a fork.

Also, regarding these comments you added in the Converter class:

// this is probably better but dont wanna break anything
// byte[] bytes = BitConverter.GetBytes(value);
// Buffer.BlockCopy(bytes, 0, array, 0, 4);

It is in fact not "better". Using these BitConverter methods is slower and allocates a whole new array every time—there is a reason I did the conversion "manually" instead.

Per6 commented 1 month ago

This was already doable with AddBits (and some extension methods if you wanted it to be more convenient).

no, AddBits allows for compacting to Ceil(log2(possibleStates)) bits however i allow for actually compacting to log2(possibleStates) bits

Why...? And how do they not "fit into Riptide anymore"??

there is no method, that uses the deleted functions anymore and the peek/setbits functions didn't even fit in in the first place.

  • The newly added files say // Copyright (c) not Tom Weiland but me at the top which is a problem because who "me" is is not specified.

alright i can change that

  • Why have you shrunk the message size to 64 bytes (new public static int InitialMessageSize = 64;)?

this is only the initial message size, the message can still grow in size and contain maxSize bytes

  • You seem to have removed message pooling. Why??

due to the previous it is no longer necessary and because i'd have to reset the state of each message anyway due to the new system it's not benificial at all anymore

  • Why is data a FastBigInt now?

to conceptualize it better

  • Why did you remove the parameterless Message.Create() overload?

idk why you'd have that in the first place, you can't send that message anyway

  • There are SO many other fundamental changes in the Message class alone—the vast majority of which are breaking changes—that I really see no benefit to merging this. You seem to have changed how much of the library works and I think it's better for this to just remain a fork.

The only fundamental changes should be, that i removed some stuff, added some stuff and made reading take away the part of the message that was read. So the only change in behavior should be resending messages, that have been read. If there are any other breaking changes, then i'll need to fix that