Skip to content

Conversation

@reitermarkus
Copy link

  • Derive Debug for every struct.
  • Add missing favicon field for ServerStatus.
  • Add Packet struct to decode full packets.

use crate::error::{DecodeError, EncodeError};

#[derive(Debug)]
pub struct Packet {
Copy link
Member

@vaIgarashi vaIgarashi Dec 9, 2021

Choose a reason for hiding this comment

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

When you will intergrate protocol in a network library like tokio this structure would not be necessary. There are more steps in packet codec. Like encryption and compression.

Copy link
Author

Choose a reason for hiding this comment

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

When you will intergrate protocol in a network library like tokio this structure would not be necessary.

Do you have an example for how this works?

I am currently converting a tokio::net::TcpStream into std::net::TcpStream so I can use Packet::decode with it, so this would definitely be an improvement over what I have now.

@timvisee
Copy link
Contributor

timvisee commented Dec 15, 2021

Do you have an example for how this works?

I've bodged together packet reading/writing with compression support here:

https://github.com/timvisee/lazymc/blob/0c69752fecdc4ad95d2c5c96c3fd37f72ea11d70/src/proto/packet.rs#L18-L235

My implementation handles compression, but no encryption though.

In the login stage, the server may send a SetCompression(n) packet after which compression should be enabled if the packet payload is >= n bytes.

Read more about compression here: https://wiki.vg/Protocol#Packet_format

@reitermarkus
Copy link
Author

reitermarkus commented Dec 16, 2021

Thanks @timvisee, I will have a look, your “Lobby” feature looks very interesting.

For my use case (a Kubernetes game server proxy for auto-scaling, pretty similar to your lazymc project actually) I didn't need compression/encryption support since the only thing I need to know is whether a login packet has been received, after that everything else is forwarded as is to the upstream server.

@reitermarkus
Copy link
Author

reitermarkus commented Dec 17, 2021

Okay, looking at @timvisee's implementation, this Packet struct with its Decoder implementation could replace the read_packet_id_data function. Maybe calling it UncompressedPacket makes more sense then?

@reitermarkus
Copy link
Author

Okay, I renamed the Packet to RawPacket and also added structs for CompressedRawPacket and UncompressedRawPacket to support both compressed and uncompressed packets.

@reitermarkus
Copy link
Author

@vaIgarashi, can you have another look?

Err(DecodeError::IoError { io_error })
if io_error.kind() == io::ErrorKind::UnexpectedEof =>
{
return Err(DecodeError::Incomplete { bytes_needed: 1 })
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct because read var int may vary from 1 to 4 bytes

Copy link
Author

Choose a reason for hiding this comment

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

This is used only as an indication, so it should be read as “at least 1”. If more are needed, it will happen in the next iteration.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe it should be an Option so it works similar to https://docs.rs/nom/latest/nom/enum.Needed.html.

let mut buf = Vec::new();
let packet = RawPacket {
id: self.id,
data: self.data.clone(),
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid data cloning?

Copy link
Author

Choose a reason for hiding this comment

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

Changed the method to take an owned self.

id: self.id,
data: self.data.clone(),
};
if let Some(threshold) = compression_threshold {
Copy link
Member

Choose a reason for hiding this comment

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

nth: match are more preferable when you use else branch

encoder.write_all(&packet_buf)?;
encoder.finish()?;
} else {
writer.write_var_i32(0)?;
Copy link
Member

Choose a reason for hiding this comment

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

Why zero?

Copy link
Author

Choose a reason for hiding this comment

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

https://wiki.vg/Protocol#With_compression

If the size of the buffer containing the packet data and ID (as a VarInt) is smaller than the threshold specified in the packet Set Compression. It will be sent as uncompressed. This is done by setting the data length as 0. (Comparable to sending a non-compressed format with an extra 0 between the length, and packet data).

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