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

Fix Game starts with 1 player on ready #13271

Merged
merged 1 commit into from Jun 27, 2017

Conversation

Projects
None yet
5 participants
@rob-v
Contributor

rob-v commented May 10, 2017

  1. rename Session.IsSinglePlayer to IsSingleHuman for clarity - this is optional, 11 of 12 cases seem to be correct, 1 case where it isn't is fixed in commit 2
  2. Lobby - updated auto start on 'Ready' checkbox and on 'Start Game' button to fix case when on dedicated non SinglePlayer server game could be started with only 1 human player and admin as spectator.

This PR solves this specific case: admin as player or spectator is ready. One of 2 players moves to spectator slot. At this moment 'all' = 1 player is ready and game starts. As it is immediate, this player might not notice and starts playing without opponent.
In general this PR prevents starting game by changing 'Ready' checkbox if there aren't at least 2 players (humans or bots). In case admin might really want to start a game with only 1 player or bot, he can still click 'Start Game'.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 10, 2017

Contributor

As discussed, this is not good idea, so closing.

Contributor

rob-v commented May 10, 2017

As discussed, this is not good idea, so closing.

@rob-v rob-v closed this May 10, 2017

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 10, 2017

Member

There is a real bug here, which is that servers will autostart if the there is only one player, even if server.LobbyInfo.GlobalSettings.EnableSingleplayer is enabled, if the admin is spectating and ready.

Member

pchote commented May 10, 2017

There is a real bug here, which is that servers will autostart if the there is only one player, even if server.LobbyInfo.GlobalSettings.EnableSingleplayer is enabled, if the admin is spectating and ready.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 10, 2017

Contributor

Don't we want to allow 1 admin spectating 1 human player vs 1 bot on dedicated server?

Contributor

rob-v commented May 10, 2017

Don't we want to allow 1 admin spectating 1 human player vs 1 bot on dedicated server?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 10, 2017

Member

No. We don't allow one human player spectating one admin vs 1 bot on a dedicated server, and this case is no different.

Member

pchote commented May 10, 2017

No. We don't allow one human player spectating one admin vs 1 bot on a dedicated server, and this case is no different.

@rob-v rob-v reopened this May 11, 2017

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 11, 2017

Contributor

Updated - please see PR's description.
I think MapGlobal.IsSinglePlayer should be also updated:
[Desc("Returns true if there is only one human player.")]
public bool IsSinglePlayer { get { return Context.World.LobbyInfo.IsSingleHuman; } }
even without renaming IsSinglePlayer, there is something wrong - either it should really check for only 1 human player or description isn't precise (True also when only 1 human as spectator)

Contributor

rob-v commented May 11, 2017

Updated - please see PR's description.
I think MapGlobal.IsSinglePlayer should be also updated:
[Desc("Returns true if there is only one human player.")]
public bool IsSinglePlayer { get { return Context.World.LobbyInfo.IsSingleHuman; } }
even without renaming IsSinglePlayer, there is something wrong - either it should really check for only 1 human player or description isn't precise (True also when only 1 human as spectator)

@chrisforbes

Not tested, but this looks OK.

@pchote

This has opened up a small can of worms:

Show outdated Hide outdated OpenRA.Game/Network/Session.cs
Show outdated Hide outdated OpenRA.Game/Network/Session.cs
Show outdated Hide outdated OpenRA.Game/Network/UnitOrders.cs
Show outdated Hide outdated OpenRA.Game/Server/Server.cs
Show outdated Hide outdated OpenRA.Game/Server/Server.cs
Show outdated Hide outdated OpenRA.Game/Server/Server.cs
Show outdated Hide outdated OpenRA.Game/Traits/Player/DeveloperMode.cs
Show outdated Hide outdated OpenRA.Mods.Common/Scripting/Global/MapGlobal.cs
Show outdated Hide outdated OpenRA.Mods.Common/ServerTraits/PlayerPinger.cs
Show outdated Hide outdated OpenRA.Mods.Common/Widgets/Logic/Ingame/IngameChatLogic.cs
@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 16, 2017

Contributor

Updated

Contributor

rob-v commented May 16, 2017

Updated

@@ -36,7 +36,7 @@ public void Tick(S server)
lastPing = Game.RunTime;
// Ignore client timeout in singleplayer games to make debugging easier
if (server.LobbyInfo.IsSinglePlayer && !server.Dedicated)
if (server.LobbyInfo.NonBotClients.Count() < 2 && !server.Dedicated)

This comment has been minimized.

@abcdefg30

abcdefg30 May 18, 2017

Member

Is it even possible to have less then one non-bot client? (Otherwise NonBotClient.Count() == 1 would work as well, wouldn't it?) Anyway, just wondering, this shouldn't make any difference in practice.

@abcdefg30

abcdefg30 May 18, 2017

Member

Is it even possible to have less then one non-bot client? (Otherwise NonBotClient.Count() == 1 would work as well, wouldn't it?) Anyway, just wondering, this shouldn't make any difference in practice.

This comment has been minimized.

@rob-v

rob-v May 18, 2017

Contributor

I kept it as suggested by pchote. It isn't possible in OpenRA IMO atm, but in theory there could be bot only game simulated.

@rob-v

rob-v May 18, 2017

Contributor

I kept it as suggested by pchote. It isn't possible in OpenRA IMO atm, but in theory there could be bot only game simulated.

@abcdefg30

Looks good otherwise.

Show outdated Hide outdated OpenRA.Game/Network/UnitOrders.cs
@@ -108,7 +108,7 @@ public string TerrainType(CPos cell)
}
[Desc("Returns true if there is only one human player.")]
public bool IsSinglePlayer { get { return Context.World.LobbyInfo.IsSinglePlayer; } }
public bool IsSinglePlayer { get { return Context.World.LobbyInfo.NonBotPlayers.Count() == 1; } }

This comment has been minimized.

@abcdefg30

abcdefg30 May 19, 2017

Member

Although this is in line with the description: Do we really want to count bot-only matches as non-singleplayer? Imho we should rather adjust the description and use NonBotPlayers.Count() == 1.

@abcdefg30

abcdefg30 May 19, 2017

Member

Although this is in line with the description: Do we really want to count bot-only matches as non-singleplayer? Imho we should rather adjust the description and use NonBotPlayers.Count() == 1.

This comment has been minimized.

@rob-v

rob-v May 19, 2017

Contributor

You mean NonBotClients.Count() == 1?

@rob-v

rob-v May 19, 2017

Contributor

You mean NonBotClients.Count() == 1?

This comment has been minimized.

@abcdefg30

abcdefg30 May 19, 2017

Member

Er, yeah.

@abcdefg30

abcdefg30 May 19, 2017

Member

Er, yeah.

This comment has been minimized.

@rob-v

rob-v May 20, 2017

Contributor

Done, reverted to previous condition using the new property.

@rob-v

rob-v May 20, 2017

Contributor

Done, reverted to previous condition using the new property.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 20, 2017

Contributor

Updated.

Contributor

rob-v commented May 20, 2017

Updated.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 May 20, 2017

Member

@chrisforbes does your 👍 still stand?

Member

abcdefg30 commented May 20, 2017

@chrisforbes does your 👍 still stand?

@obrakmann

Looks fine, just one minor nit.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 4, 2017

Contributor

Updated.

Contributor

rob-v commented Jun 4, 2017

Updated.

Code changed

Code changed

@pchote

pchote approved these changes Jun 27, 2017

@pchote pchote merged commit 78bedb0 into OpenRA:bleed Jun 27, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment