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.

Sign up for GitHub

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

Tunnel autochooser #179

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Tunnel autochooser #179

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7fed700
Automatic tunnel selection will now make better use of tunnel resources.
FunkyFr3sh May 19, 2020
be6430f
Fixes rating selection for -1 pings. And fixes checking for the best …
dkeetonx Sep 12, 2020
dc6b52a
Adds definitions for opaque constants in the tunnel selection algorit…
dkeetonx Sep 14, 2020
e408b20
Log what tunnel server is being used by the host.
dkeetonx Sep 14, 2020
67718cf
Fix TunnelListBox refresh logic
Metadorius Oct 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 28 additions & 30 deletions DXMainClient/DXGUI/Multiplayer/CnCNet/TunnelListBox.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ private void TunnelHandler_TunnelsRefreshed(object sender, EventArgs e)
{
ClearItems();

lowestTunnelRating = int.MaxValue;
int tunnelIndex = 0;

foreach (CnCNetTunnel tunnel in tunnelHandler.Tunnels)
Expand All @@ -83,14 +84,11 @@ private void TunnelHandler_TunnelsRefreshed(object sender, EventArgs e)

AddItem(info, true);

if ((tunnel.Official || tunnel.Recommended) && tunnel.PingInMs > -1)
int rating = tunnel.Rating;
if (rating < lowestTunnelRating)
{
int rating = GetTunnelRating(tunnel);
if (rating < lowestTunnelRating)
{
bestTunnelIndex = tunnelIndex;
lowestTunnelRating = rating;
}
bestTunnelIndex = tunnelIndex;
lowestTunnelRating = rating;
}

tunnelIndex++;
Expand All @@ -113,7 +111,19 @@ private void TunnelHandler_TunnelsRefreshed(object sender, EventArgs e)
isManuallySelectedTunnel = false;
}
else
SelectedIndex = manuallySelectedIndex;
{
CnCNetTunnel tunnel = tunnelHandler.Tunnels[manuallySelectedIndex];

if (tunnel.Clients < tunnel.MaxClients)
{
SelectedIndex = manuallySelectedIndex;
}
else
{
SelectedIndex = bestTunnelIndex;
isManuallySelectedTunnel = false;
}
}
}
}

Expand All @@ -128,32 +138,20 @@ private void TunnelHandler_TunnelPinged(int tunnelIndex)
if (tunnel.PingInMs == -1)
lbItem.Text = "Unknown";
else
{
lbItem.Text = tunnel.PingInMs + " ms";
int rating = GetTunnelRating(tunnel);

if (isManuallySelectedTunnel)
return;

if ((tunnel.Recommended || tunnel.Official) && rating < lowestTunnelRating)
{
bestTunnelIndex = tunnelIndex;
lowestTunnelRating = rating;
SelectedIndex = tunnelIndex;
}
}
}

private int GetTunnelRating(CnCNetTunnel tunnel)
{
double usageRatio = (double)tunnel.Clients / tunnel.MaxClients;

if (usageRatio == 0)
usageRatio = 0.1;
int rating = tunnel.Rating;

usageRatio *= 100.0;
if (isManuallySelectedTunnel)
return;

return Convert.ToInt32(Math.Pow(tunnel.PingInMs, 2.0) * usageRatio);
if (rating < lowestTunnelRating)
{
bestTunnelIndex = tunnelIndex;
lowestTunnelRating = rating;
SelectedIndex = tunnelIndex;
isManuallySelectedTunnel = false;
}
}

private void TunnelListBox_SelectedIndexChanged(object sender, EventArgs e)
Expand Down
35 changes: 22 additions & 13 deletions DXMainClient/DXGUI/Multiplayer/GameLobby/CnCNetGameLobby.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public class CnCNetGameLobby : MultiplayerGameLobby

public CnCNetGameLobby(WindowManager windowManager, string iniName,
TopBar topBar, List<GameMode> GameModes, CnCNetManager connectionManager,
TunnelHandler tunnelHandler, GameCollection gameCollection, CnCNetUserData cncnetUserData, MapLoader mapLoader, DiscordHandler discordHandler) :
TunnelHandler tunnelHandler, GameCollection gameCollection, CnCNetUserData cncnetUserData, MapLoader mapLoader, DiscordHandler discordHandler) :
base(windowManager, iniName, topBar, GameModes, mapLoader, discordHandler)
{
this.connectionManager = connectionManager;
Expand Down Expand Up @@ -175,7 +175,7 @@ public override void Initialize()

private void GameBroadcastTimer_TimeElapsed(object sender, EventArgs e) => BroadcastGame();

public void SetUp(Channel channel, bool isHost, int playerLimit,
public void SetUp(Channel channel, bool isHost, int playerLimit,
CnCNetTunnel tunnel, string hostName, bool isCustomPassword)
{
this.channel = channel;
Expand Down Expand Up @@ -581,10 +581,19 @@ protected override void HostLaunchGame()

List<int> playerPorts = tunnelHandler.CurrentTunnel.GetPlayerPortInfo(Players.Count);

if (playerPorts.Count < Players.Count && tunnelHandler.Tunnels.Any())
{
Logger.Log($"Picking a new tunnel server because '{tunnelHandler.CurrentTunnel.Name}' was full");
tunnelHandler.CurrentTunnel = tunnelHandler.Tunnels.Aggregate((i1, i2) => i1.Rating < i2.Rating ? i1 : i2);
playerPorts = tunnelHandler.CurrentTunnel.GetPlayerPortInfo(Players.Count);
Copy link
Member

Choose a reason for hiding this comment

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

I'm against hidden changes of tunnel server, and this stuff also makes the whole following codeblock obsolete.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added logging here so that the change is no longer hidden.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps I didn't phrase it clear enough, my bad. I meant that the user should be aware of the tunnel being chosen at least. Also the next codeblock after that still duplicates this one's functionally and won't be used in 99% cases.

The best possible solution I see is to just select the suggested tunnel server in the opened window if the previous tunnel was manually selected, and in case the previous tunnel was automatically selected add a notice to chatbox that the tunnel was changed to TunnelName instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

This bit of code is designed to reduce frustration when a player wants to start a game and the tunnel server is out of resources. Manual tunnel selection is an advanced option now but it will be exclusively a power user option in the future when v3 tunnels are implemented.

The manual selection code isn't really ready to handle corner cases like this. It's a bit hacky, it should have it's own PR to handle it correctly. I don't think it's a good idea to block this PR for that reason.

Copy link
Member

Choose a reason for hiding this comment

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

The code redundancy must be fixed either way, preferably in a way I proposed - change tunnel server silently only in case the user didn't select the tunnel manually before.

PR shouldn't introduce more inoptimal solutions code-wise, in my opinion.

}

Logger.Log($"Launching Game using tunnel server '{tunnelHandler.CurrentTunnel.Name}'");

if (playerPorts.Count < Players.Count)
{
ShowTunnelSelectionWindow("An error occured while contacting " +
"the CnCNet tunnel server." + Environment.NewLine +
"the CnCNet tunnel server." + Environment.NewLine +
"Try picking a different tunnel server:");
AddNotice("An error occured while contacting the specified CnCNet " +
"tunnel server. Please try using a different tunnel server ", ERROR_MESSAGE_COLOR);
Expand Down Expand Up @@ -635,7 +644,7 @@ protected override void RequestReadyStatus()
{
if (Map == null || GameMode == null)
{
AddNotice("The game host needs to select a different map or " +
AddNotice("The game host needs to select a different map or " +
"you will be unable to participate in the match.");
return;
}
Expand Down Expand Up @@ -697,8 +706,8 @@ private void HandleOptionsRequest(string playerName, int options)
if (team < 0 || team > 4)
return;

if (side != pInfo.SideId
|| start != pInfo.StartingLocation
if (side != pInfo.SideId
|| start != pInfo.StartingLocation
|| team != pInfo.TeamId)
{
ClearReadyStatuses();
Expand Down Expand Up @@ -806,7 +815,7 @@ private void ApplyPlayerOptions(string sender, string message)

// If we can't find the player from the channel user list,
// ignore the player
// They've either left the channel or got kicked before the
// They've either left the channel or got kicked before the
// player options message reached us
if (channel.Users.Find(pName) == null)
{
Expand Down Expand Up @@ -890,7 +899,7 @@ protected override void OnGameOptionChanged()

// Let's pack the booleans into bytes
List<byte> byteList = Conversions.BoolArrayIntoBytes(optionValues).ToList();

while (byteList.Count % 4 != 0)
byteList.Add(0);

Expand Down Expand Up @@ -1165,7 +1174,7 @@ protected override void GameProcessExited()
}

/// <summary>
/// Handles the "START" (game start) command sent by the game host.
/// Handles the "START" (game start) command sent by the game host.
/// </summary>
private void NonHostLaunchGame(string sender, string message)
{
Expand Down Expand Up @@ -1381,7 +1390,7 @@ private void CheaterNotification(string sender, string cheaterName)
if (sender != hostName)
return;

AddNotice("Player " + cheaterName + " has different files compared to the game host. Either " +
AddNotice("Player " + cheaterName + " has different files compared to the game host. Either " +
cheaterName + " or the game host could be cheating.", Color.Red);
}

Expand Down Expand Up @@ -1464,7 +1473,7 @@ protected override void BanPlayer(int playerIndex)
}
}

private void HandleCheatDetectedMessage(string sender) =>
private void HandleCheatDetectedMessage(string sender) =>
AddNotice(sender + " has modified game files during the client session. They are likely attempting to cheat!", Color.Red);

private void HandleTunnelServerChangeMessage(string sender, string tunnelAddressAndPort)
Expand Down Expand Up @@ -1504,7 +1513,7 @@ private void HandleTunnelServerChange(CnCNetTunnel tunnel)

#region CnCNet map sharing

private void MapSharer_MapDownloadFailed(object sender, SHA1EventArgs e)
private void MapSharer_MapDownloadFailed(object sender, SHA1EventArgs e)
=> WindowManager.AddCallback(new Action<SHA1EventArgs>(MapSharer_HandleMapDownloadFailed), e);

private void MapSharer_HandleMapDownloadFailed(SHA1EventArgs e)
Expand Down Expand Up @@ -1615,7 +1624,7 @@ private void HandleMapUploadRequest(string sender, string mapSHA1)
{
Logger.Log("HandleMapUploadRequest: Map is official, so skip request");

AddNotice(string.Format("{0} doesn't have the map '{1}' on their local installation. " +
AddNotice(string.Format("{0} doesn't have the map '{1}' on their local installation. " +
"The map needs to be changed or {0} is unable to participate in the match.",
sender, map.Name));

Expand Down
57 changes: 55 additions & 2 deletions DXMainClient/Domain/Multiplayer/CnCNet/CnCNetTunnel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public class CnCNetTunnel
public CnCNetTunnel() { }

/// <summary>
/// Parses a formatted string that contains the tunnel server's
/// Parses a formatted string that contains the tunnel server's
/// information into a CnCNetTunnel instance.
/// </summary>
/// <param name="str">The string that contains the tunnel server's information.</param>
Expand All @@ -34,7 +34,7 @@ public static CnCNetTunnel Parse(string str)

string address = parts[0];
string[] detailedAddress = address.Split(new char[] { ':' });

tunnel.Address = detailedAddress[0];
tunnel.Port = int.Parse(detailedAddress[1]);
tunnel.Country = parts[1];
Expand Down Expand Up @@ -85,6 +85,59 @@ public static CnCNetTunnel Parse(string str)
public double Distance { get; private set; }
public int PingInMs { get; set; } = -1;

/// <summary>
/// Rating Factor:
/// The latency in milleseconds that ping times are rounded off to.
/// </summary>
private const int LATENCY_PRECISION = 70;

/// <summary>
/// Rating Factor:
/// The importance of latency compared to client count.
/// </summary>
private const int LATENCY_WEIGHT = 100000;

/// <summary>
/// Rating Factor:
/// The value of client count compared to latency
/// </summary>
private const int CLIENTS_WEIGHT = 1;

/// <summary>
/// Rating Factor:
/// Base Rating for untrusted servers and unpingable servers.
/// </summary>
private const int UNTRUSTED_RATING = int.MaxValue - 100000;

/// <summary>
/// Rating Factor:
/// The amount of free client slots needed for the tunnel to be eligible for autoselection
/// </summary>
private const int CLIENTS_HEADROOM = 24;
Copy link
Member

Choose a reason for hiding this comment

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

Why don't use some sort of a percentage instead? Hardcaps are bad in this case, imagine a tunnel with 50 players max. being filled with 26 players and stopping receiving players.

Copy link
Member

Choose a reason for hiding this comment

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

Live example: Berryville tunnel has 50 slots, so the server will be running at 26 players max. in case of autochoosing it?
I think the best would be to use tangent function here, so when the tunnel clients amount is very close to max - the rating will skyrocket thus making it never chosen automatically.

Copy link
Member Author

Choose a reason for hiding this comment

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

The CLIENTS_HEADROOM is not for performance reasons. It's meant to make sure that when we go to use a tunnel server it has enough free slots to fill our request. The problem occurs because there is a time difference between when we select the tunnel server (game lobby creation) to when we request the user slots (game start).

Copy link
Member

Choose a reason for hiding this comment

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

I understood that already, yet the issue I mentioned is still here. Servers with low max. player amounts would never be chosen or chosen rarely.

IMO the issue of time difference should be dealt with some other sort of measure, f.ex. a task in game lobby checking for availability of the current tunnel on tunnel refresh. Ideally the players in lobby would be counted as players on servers even before starting but that would require server-side changes.


public int Rating
{
get
{
if (Clients + CLIENTS_HEADROOM >= MaxClients)
return int.MaxValue;
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion it would be a lot better to use something like a quadratic or exponential function to calculate rating than hardcap at MaxClients - 24. Also, magic numbers antipattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think a more complicated algorithm might be better in narrow cases but IMO the complexity out weighs the benefit.

Copy link
Member

Choose a reason for hiding this comment

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

I've already written a proto. algorithm, it isn't so hard, just needs adjustments to be done.

Copy link
Member Author

Choose a reason for hiding this comment

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

The point of this code is not to select tunnels with fewer clients (that's handled elsewhere). The point of this code comes from the fact that the tunnel chooser first runs when the game lobby is created and after each ping, there is no way to know if the selected tunnel will have enough resources to host our game when we're ready to start (ie, resource is requested/granted on game start). So we're creating some headroom that will give us a reasonable chance to get our first tunnel choice on game start.


if (Official || Recommended)
{
if (PingInMs <= -1)
return UNTRUSTED_RATING + Clients;

int latency = 1 + PingInMs / LATENCY_PRECISION;

return (latency * LATENCY_WEIGHT) + (Clients * CLIENTS_WEIGHT);
}
else
{
return UNTRUSTED_RATING + Clients;
Copy link
Member

@Metadorius Metadorius Sep 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

With this code it looks like the clients would almost never get an unofficial and non-recommended server selected. This is not the behavior I see as good, CnCNet is meant to be a community service, and IMO recommendation should boost the server, not basically "whitelist" recommended servers.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the best case, the recommended servers have all been tested and proven to be good. The goal is for the users to never want or need to select a tunnel server themselves. When they do manually select a tunnel server it should be their own server that they want to use for personal reasons. This provides the added benefit that we can do some hacks on the master-list side to balance tunnel servers during the high load times.

Copy link
Member

@Metadorius Metadorius Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I still object against verified and official tunnels being effectively whitelisted against regular ones, this is not future-proof at all. I mean the example is already at hand. I have a feeling no one actually bothered to test Berryville, yet it has very low ping for Europe and I never had issues using it. Imagine CnCNet staff going very busy all at once, then some of the verified tunnel will also go off and the players will be automatically balanced to 2-3 verified servers in USA, for example, even if they are from EU and there is regular, non-verified EU servers. This is absolutely not a desirable outcome IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason we should never use untrusted tunnel servers is because they fail more often. When a tunnel server fails and it breaks the game for a group of players, the players don't blame the tunnel server owner they blame CnCNet, we can't stake our reputation on untrusted servers.

Imagine CnCNet staff going very busy all at once, then some of the verified tunnel will also go off and the players will be automatically balanced to 2-3 verified servers in USA, for example, even if they are from EU and there is regular, non-verified EU servers. This is absolutely not a desirable outcome IMO.

That's still more desirable than a broken tunnel server with low ping destroying everyone's game.

Copy link
Member

Choose a reason for hiding this comment

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

My view of future CnCNet is as of a community service, not an organization, so I don't think this outcome is more desirable. I don't say that the system should be ditched. Per my idea verified servers would be "boosted" and chosen most of the time, but in case there's no local verified servers with low enough ping - regular one would be chosen. This would never happen in the current implementation.

Again, you say that untrusted servers are bad because all good servers are made verified. Is that so? Not really. Someone has to test them manually, and as I said I don't see anyone testing Berryville server or some other EU servers which are OK despite the lack of EU tunnels. I always get Canada East chosen and I live in Europe.

}
}
}

/// <summary>
/// Gets a list of player ports to use from a specific tunnel server.
/// </summary>
Expand Down