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

Support reconnecting dummy by redirecting to same port #9482

Merged
merged 1 commit into from
Jan 5, 2025

Conversation

Robyt3
Copy link
Member

@Robyt3 Robyt3 commented Jan 5, 2025

Previously, the redirect message NETMSG_REDIRECT would always cause the main connection to be redirected and the dummy to be disconnected no matter on which connection it is received. Now, if the redirect message is received on the dummy connection with the destination port being idential to the current port then the dummy only will be reconnected. Redirecting the dummy connection to a different port is not allowed, as the dummy should not be connected to a different server than the main, as the client assumes that main and dummy use the same map.

Closes #9480.

Checklist

  • Tested the change ingame
  • Provided screenshots if it is a visual change
  • Tested in combination with possibly related configuration options
  • Written a unit test (especially base/) or added coverage to integration test
  • Considered possible null pointers and out of bounds array indexing
  • Changed no physics that affect existing maps
  • Tested the change with ASan+UBSan or valgrind's memcheck (optional)

Previously, the redirect message `NETMSG_REDIRECT` would always cause the main connection to be redirected and the dummy to be disconnected no matter on which connection it is received. Now, if the redirect message is received on the dummy connection with the destination port being idential to the current port then the dummy only will be reconnected. Redirecting the dummy connection to a different port is not allowed, as the dummy should not be connected to a different server than the main, as the client assumes that main and dummy use the same map.

Closes ddnet#9480.
@Robyt3 Robyt3 added this to the DDNet 18.9 milestone Jan 5, 2025
return;
}
DummyConnect();
}
Copy link
Member

Choose a reason for hiding this comment

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

I like this fix. It accurately represents the dummy as a secondary player that still disconnects on a redirect packet but does not cause a desync.

Could you maybe add a message that simply reconnects without taking a port so we can move away from this "hack" of using the redirect function in some time?

Copy link
Member

@heinrich5991 heinrich5991 left a comment

Choose a reason for hiding this comment

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

Even with my comment, I think this PR strictly improves the situation.

@heinrich5991 heinrich5991 added this pull request to the merge queue Jan 5, 2025
Merged via the queue into ddnet:master with commit 5ba3b1d Jan 5, 2025
17 checks passed
@Robyt3 Robyt3 deleted the Client-Dummy-Redirect-Reconnect branch January 5, 2025 22:40
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.

TOKEN ignored in the redirect packet
2 participants