Network Refactor Pt. 3: Introduce a packet Codec and Connection type #105
Conversation
StripedMonkey
force-pushed
the
connection_refactor
branch
from
40dd918
to
8d801fb
Compare
|
So what benefits this gives us? |
|
This makes connections composable, and encapsulates connections as frameable streams of any type. Encryption is handled transparently. This is a part of my continued efforts to make the networking stack better through more encapsulation and rigid API boundaries. All the code here does is handle taking an incoming TCP (or any |
|
This PR is not designed to actually add features that did not exist before, but make it easier to maintain and extend the existing code. I'm trying to get to the point where working with network connections is completely abstracted out of the core game logic like it currently is. |
StripedMonkey
force-pushed
the
connection_refactor
branch
from
8d801fb
to
de306de
Compare
|
That crate paste has the same functionality as unstable concat_idents 👍 |
|
It saves like 10 lines of repetitive code, but honestly I don't think it's worth the extra dependency. It's nice to have but not necessary here. |
| #[derive(Deserialize)] | ||
| pub struct SHandShake { | ||
| pub protocol_version: VarInt, | ||
| pub server_address: String, // 255 | ||
| pub server_port: u16, | ||
| pub next_state: ConnectionState, | ||
| } |
There was a problem hiding this comment.
Hey, The only reason i used manuel Deserialization here is because the server address has a max length of 255. There where plans of implementing maybe some kinda struct called BoundedString, Not sure tough how
There was a problem hiding this comment.
I've been playing with a solution to this problem for fixed length bitsets or similar, but not addressed it in depth.
In theory, it's possible to make a newtype wrapper like
struct BoundedLengthString<const LEN: usize>(String);and implement a string visitor that restricts the size, implementing From/Into would allow you to use
#[serde(try_from = BoundedLengthString<255>)]
pub server_address: String
but there are some considerations to make regarding strings in general. The current implementation is actually incorrect in that it assumes string length is in bytes. However, according to the wiki Strings are actually prefixed with their length in characters, making the implementation faulty on Non-ASCII/english characters.
There was a problem hiding this comment.
I like the solution valence took, See https://github.com/valence-rs/valence/blob/f526a175bb7fee13d59c6987d2bf4e9b532866e5/crates/valence_protocol/src/packets/handshaking/handshake_c2s.rs#L7
There was a problem hiding this comment.
@Snowiiii Imo @StripedMonkey 's solution is cleaner, since we dont need to cast to a different type when using the packet.
but there are some considerations to make regarding strings in general. The current implementation is actually incorrect in that it assumes string length is in bytes. However, according to the wiki Strings are actually prefixed with their length in characters, making the implementation faulty on Non-ASCII/english characters.
^^ regarding this, i dont think thats true tho, on wiki.vg it says that a string is a UTF-8 string prefixed with its size in bytes as a VarInt., the size here does not refer to amount of characters, but amount of bytes. This can be easily checked, by simply sending a chat message in pumpkin with the current implementation, with characters outside of the single byte range of UTF-8
There was a problem hiding this comment.
Sorry, I missread, Max string length in characters is what I was actually reading. So the limits provided in the wki aren't in bytes.
StripedMonkey
force-pushed
the
connection_refactor
branch
from
de306de
to
19c6eb5
Compare
This introduces a series of Codecs for encoding and decoding packets, as well as a Connection<T> type which frames streams with encryption and protocol framing.
This introduces a `format` module which (currently) contains the (de)serialization for the module.
StripedMonkey
force-pushed
the
connection_refactor
branch
from
19c6eb5
to
885a2b8
Compare
|
Hey @StripedMonkey, I would love to merge or close this PR soon. Do you have any plans finishing the PR soon ? |
|
Life has been busy for the last bit, so I've not dedicated much time to this. The largest outstanding issue here is that I depend on things implementing It should be relatively straightforward to do, but it's still time consuming. If you would like to take my code and work on it, maintainers should have commit access to the pr. Otherwise feel free to copy what you like, and discard the rest. I have no timeline at this point. Not this weekend for sure. |
This introduces a series of Codecs for encoding and decoding packets, as well as a Connection type which frames streams with encryption and protocol framing.
I have withheld hooking this up to anything, in favor of some tests to show that it works due to the large burden of refactoring code to use it. Partial refactors containing code hooking this up can be found in a dump commit on my fork of Pumpkin.