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 delay for ack packets #156

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add delay for ack packets #156

wants to merge 1 commit into from

Conversation

lucaspoffo
Copy link
Owner

@lucaspoffo lucaspoffo commented Mar 31, 2024

Currently, renet sends acks every update frame, this increases the bandwidth for connections.

This PR adds a delay for sending ACK packets, when receiving a reliable message this delay is ignored and the ACK is sent immediatly (so we don't delay acks for reliable messages).

This reduces the bandwidth of connections. For idle connections this seems to reduce from 2.15 kbps to 0.5 kbps (at 60 hz and 200ms max delay). If the connection is receiving reliable messages every frame this PR will not change the bandwidth since it will still send ACKs every frame.

This change will affect a bit the stats for the connection , since the values can be delaye, but at small MAX_ACK_DELAY ( 200-300ms), this shouldn't impact much.

TODO: make sure the RTT calculations is working correctly and check that the new delay field in the ACK packet cannot be wrongly used with malicious packets.

@UkoeHB
Copy link
Contributor

UkoeHB commented Apr 1, 2024

Why put the ack delay in the ack packet, and not as a config? This makes ack packets bigger and adds risk of malicious effects. It also breaks from the netcode standard.

@lucaspoffo
Copy link
Owner Author

lucaspoffo commented Oct 26, 2024

Why put the ack delay in the ack packet, and not as a config? This makes ack packets bigger and adds risk of malicious effects. It also breaks from the netcode standard.

Just a late reply, this changes the custom renet protocol, shouldn't interfere with the netcode protocol.

The delay is added to the ack packet so that we can use it to calculate the RTT, if the ack delay is 0, we know that the ack was sent instantly. Otherwise, if the packet had some delay, we sent how long it waited, so when it is received the delay can be used to calculate the correctly the RTT.

So why do this at all. If we don't send any reliable messages, we would never send acks instantly and without the delay specified in the ack packet, we could not calculate the RTT correctly.

@UkoeHB
Copy link
Contributor

UkoeHB commented Oct 27, 2024

What about adding the config to ConnectionConfig? Users are already expected to use the same channels, so including ack delay wouldn't be too unusual. You can make it Option<Duration> and then default to 60Hz if not set (or use Some(60Hz) as the default when constructing config, and allow acks to be sent every tick when None).

@lucaspoffo
Copy link
Owner Author

What about adding the config to ConnectionConfig? Users are already expected to use the same channels, so including ack delay wouldn't be too unusual. You can make it Option<Duration> and then default to 60Hz if not set (or use Some(60Hz) as the default when constructing config, and allow acks to be sent every tick when None).

In my mind, this is not something a user should need to configure or know/worry about. Most of the times this value won't impact much because if any reliable packet is received it will always send an ack immediatly. A value between 250 - 500ms should be good enough. Also, this would not be an intuitive config for some users.

@UkoeHB
Copy link
Contributor

UkoeHB commented Dec 22, 2024

How about instead of Duration you store a u16 for the number of milliseconds delayed? EDIT: Oh I see you're doing a u32 for us. Is that level of precision useful?

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.

2 participants