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

More than max_no_competitors could be created in network games #7376

Closed

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Mar 16, 2019

Description: There is a max number of commands_per_frame that can cause the starting of a company to be delayed by another tick. I was seeing 5 AIs being started when I've set max_no_competitors only to 4.

011257d causes it.

src/company_cmd.cpp Outdated Show resolved Hide resolved
@SamuXarick SamuXarick force-pushed the enforce-max_no_competitors-test branch from c927bd6 to 6a222fc Compare March 16, 2019 21:10
@SamuXarick SamuXarick changed the title Fix: Enforce the max_no_competitors test before creating an AI compan… Fix: Enforce the max_no_competitors test before creating an AI company in multiplayer. Mar 17, 2019
src/company_cmd.cpp Outdated Show resolved Hide resolved
src/company_cmd.cpp Outdated Show resolved Hide resolved
src/company_cmd.cpp Outdated Show resolved Hide resolved
src/company_cmd.cpp Outdated Show resolved Hide resolved
@SamuXarick SamuXarick force-pushed the enforce-max_no_competitors-test branch from 6a222fc to 96e8976 Compare March 21, 2019 16:08
@SamuXarick SamuXarick changed the title Fix: Enforce the max_no_competitors test before creating an AI company in multiplayer. Fix 001257d: Enforce the max_no_competitors and network.max_companies test before creating an AI company in multiplayer. Mar 21, 2019
@SamuXarick SamuXarick force-pushed the enforce-max_no_competitors-test branch 2 times, most recently from 239583e to 4eabc1b Compare March 21, 2019 17:53
@SamuXarick SamuXarick changed the title Fix 001257d: Enforce the max_no_competitors and network.max_companies test before creating an AI company in multiplayer. Fix 001257d: Enforce the max_no_competitors and network.max_companies tests before creating an AI company in multiplayer. Mar 21, 2019
@LordAro LordAro added backport requested This PR should be backport to current release (RC / stable) bug Something isn't working labels Mar 21, 2019
@TrueBrain
Copy link
Member

This feels to me like the wrong approach. From what I understand.

  • The server creates a new AI; this causes a command to be queued
  • Clients + server receive this command; they validate again if the server should have said this. If the server clearly should not have said this, everyone ignores the command.

This feels like a very dirty hack. The clients should never ignore a command from the server like that. It feels this needs more thinking, and possibly a better approach to resolve.

Copy link
Contributor

@glx22 glx22 left a comment

Choose a reason for hiding this comment

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

Wrong commit reference, should be 011257d. Please edit the commit message.

@SamuXarick SamuXarick changed the title Fix 001257d: Enforce the max_no_competitors and network.max_companies tests before creating an AI company in multiplayer. Fix 011257d: Enforce the max_no_competitors and network.max_companies tests before creating an AI company in multiplayer. Mar 22, 2019
@SamuXarick SamuXarick force-pushed the enforce-max_no_competitors-test branch from 4eabc1b to 52c8e42 Compare March 22, 2019 22:51
glx22
glx22 previously approved these changes Mar 22, 2019
@glx22 glx22 dismissed their stale review March 22, 2019 23:05

wrong button

@SamuXarick
Copy link
Contributor Author

SamuXarick commented Mar 23, 2019

This feels to me like the wrong approach. From what I understand.

The server creates a new AI; this causes a command to be queued
Clients + server receive this command; they validate again if the server should have said this. If the server clearly should not have said this, everyone ignores the command.

This feels like a very dirty hack. The clients should never ignore a command from the server like that. It feels this needs more thinking, and possibly a better approach to resolve.

This is a problem that happens in multiplayer. Single player does not apply.

Problem is difficult to follow, but gonna try explain with an example.

In multiplayer, when multiple AIs are set to start with 0 days delay between each other, OnTick_Companies() is called every tick to start them. Let's imagine I've set max_no_competitors to 5 and network.max_companies to 15.

Tick 1:
On the first tick, OnTick_Companies() is called, MaybeStartNewCompany() is tested and it will give Company::GetNumItems() == 0 (assuming dedicated server), and will count the number of AIs, n == 0. Both tests fail, which allows DoCommandP to be sent.

This command will run in DC_TEST mode. CmdCompanyCtrl returns with CommandCost(). Since this is a network, the command is sent into a queue, it's not executed right away. It does a NetworkSendCommand and puts it in _local_wait_queue, then returns Succeeded.

For the rest of this tick, nothing happens. Game proceeds to next tick.

Tick 2:
NetworkDistributeCommands() is called, which runs DistributeQueue(). There is a commands_per_frame setting there, set to 2. I see it moving the command from _local_wait_queue into _local_execution_queue via DistributeCommandPacket. Since there's only 1 command in _local_wait_queue for now, commands_per_frame limit is not reached.

After that splitting, NetworkExecuteLocalCommandQueue() is called. The command from _local_execution_queue is run. CmdCompanyCtrl is first run in DC_TEST mode, returning with CommandCost(), and then it is run again in DC_EXEC mode, where it actually creates the AI company.

Now the cycle repeats, OnTick_Companies() is called, MaybeStartNewCompany() is tested and it will give Company::GetNumItems() == 1, and will count the number of AIs, n == 1. Both tests fail, which allows DoCommandP to be sent.

This command will run in DC_TEST mode. CmdCompanyCtrl returns with CommandCost(). Since this is a network, the command is sent into a queue, it's not executed right away. It does a NetworkSendCommand and puts it in _local_wait_queue, then returns Succeeded.

For the rest of this tick, the first AI that started does its stuff and also queues one command in _local_wait_queue. So _local_wait_queue now has 2 commands.

Tick 3:
NetworkDistributeCommands() is called, which runs DistributeQueue(). It is now moving 2 commands from _local_wait_queue to _local_execution_queue. commands_per_frame limit is reached, but since there was only 2 commands, _local_wait_queue still becomes empty.

NetworkExecuteLocalCommandQueue() will run the commands in _local_execution_queue in the order they were queued. First, CmdCompanyCtrl in DC_TEST mode and DC_EXEC mode, where the 2nd AI company is created. And second, the command that the 1st AI had queued.

The cycle repeats, OnTick_Companies() is called, MaybeStartNewCompany() is tested and it will give Company::GetNumItems() == 2, and will count the number of AIs, n == 2. Both tests fail, which allows DoCommandP to be sent.

CmdCompanyCtrl() is run in DC_TEST mode, then NetworkSendCommand puts it in _local_wait_queue and the command returns Succeeded.

For the rest of this tick, the first AI queues a command and the second AI also queues a command. _local_wait_queue now has 3 commands.

Tick 4:
NetworkDistributeCommands() is called, which runs DistributeQueue(). It is moving 2 commands from _local_wait_queue to _local_execution_queue. commands_per_frame limit is reached, but there were 3 commands, _local_wait_queue remains with one command, the last that was queued, which is the command from the 2nd AI.

NetworkExecuteLocalCommandQueue() will run the commands in _local_execution_queue in the order they were queued. First, CmdCompanyCtrl in DC_TEST mode and DC_EXEC mode, where the 3nd AI company is created. And second, the command that the 1st AI had queued. The command from the 2nd AI is not executed.

Repeat, OnTick_Companies() is called, MaybeStartNewCompany() is tested and it will give Company::GetNumItems() == 3, and will count the number of AIs, n == 3. Both tests fail, which allows DoCommandP to be sent.

CmdCompanyCtrl() is run in DC_TEST mode, then NetworkSendCommand puts it in _local_wait_queue and the command returns Succeeded. _local_wait_queue now contains 2 commands, the command from the 2nd AI, and creation of the 4th AI company.

For the rest of this tick, the first AI queues a command, the second AI does nothing, and the third AI queues a command. _local_wait_queue now has 4 commands: command from the 2nd AI, creation of the 4th AI company, command from 1st AI, and command from 3rd AI.

Tick 5:
NetworkDistributeCommands() moves 2 commands from _local_wait_queue to _local_execution_queue.

NetworkExecuteLocalCommandQueue() will run 2 commands from _local_execution_queue, first, the command from the 2nd AI, then CmdCompanyCtrl in DC_TEST, then DC_EXEC mode, creating the 4th AI company.

OnTick_Companies(), MaybeStartNewCompany()… Company::GetNumItems() == 4, n == 4. Both tests fail, which allows DoCommandP to be sent.

CmdCompanyCtrl() is run in DC_TEST mode, then NetworkSendCommand puts it in _local_wait_queue and the command returns Succeeded. _local_wait_queue now contains 3 commands, the command from the 1st AI and 3rd AIs, and creation of the 5th AI company.

For the rest of this tick, 2nd AI queues a command, and 4th AI also queues a command. _local_wait_queue contains 5 commands: command from 1st AI, command from 3rd AI, creation of 5th company, command from 2nd AI, and command from 4th AI.

Tick 6 (is where the problem first arises):
NetworkDistributeCommands() moves 2 commands from _local_wait_queue to _local_execution_queue. _local_wait_queue will remain with 3 commands, which are creation of 5th company, command from 2nd AI, and command from 4th AI.

NetworkExecuteLocalCommandQueue() will run 2 commands from _local_execution_queue, which are command from 1st AI and command from 3rd AI. This means the 5th company is not created this tick!

OnTick_Companies() happens, MaybeStartNewCompany() is doing tests and Company::GetNumItems() == 4, n == 4. It does not know that there was a 5th company! The tests in this situation are incorrect, but they will still fail, allowing the queue of the 6th AI company.

CmdCompanyCtrl() is run in DC_TEST mode, then NetworkSendCommand puts it in _local_wait_queue and the command returns Succeeded. _local_wait_queue now contains 4 commands: the creation of the 5th AI company, command from the 2nd AI, command from the 4th AI and creation of the 6th company.

For the rest of the tick, commands from 1st and 3rd AIs are queued. _local_wait_queue now contains 6 commands: the creation of the 5th AI company, command from the 2nd AI, command from the 4th AI, creation of the 6th company, command from 1st AI and command from 3rd AI.

Tick 7:
NetworkDistributeCommands() moves 2 commands from _local_wait_queue to _local_execution_queue. _local_wait_queue will remain with 4 commands which are command from the 4th AI, creation of 6th company, command from 1st AI and command from 3rd AI.

NetworkExecuteLocalCommandQueue() runs 2 commands from _local_execution_queue. First, CmdCompanyCtrl in DC_TEST and DC_EXEC modes. The 5th company is created. And then, the command from the 2nd AI is executed.

OnTick_Companies(), MaybeStartNewCompany() this time gets Company::GetNumItems() == 5 and n == 5. Since 5 was the limit for the max_no_competitors, the test will pass, allowing this function to return false. No more companies are created… but this comes too late. The 6th AI company is still in the queue, remember.

For the rest of the tick, commands from 2nd and 5th AI are queued. _local_wait_queue contains 6 commands: command from 4th AI, creation of 6th AI company, command from 1st AI, command from 3rd AI, command from 2nd AI and command from 5th AI.

Tick 8 (the final rest of the problem):
NetworkDistributeCommands() moves 2 commands from _local_wait_queue to _local_execution_queue. _local_wait_queue will remain with 4 commands: command from 1st AI, command from 3rd AI, command from 2nd AI and command from 5th AI.

NetworkExecuteLocalCommandQueue() runs 2 commands from _local_execution_queue. First, the command from the 4th AI is executed. And finally, CmdCompanyCtrl in DC_TEST and DC_EXEC modes. Without this patch, the 6th company is created. With it, both DC_TEST and DC_EXEC fail. No 6th AI company is created.

The rest doesn't matter anymore.

This big example may not truly reflect what happens in the code, it's just my mere observations following the code in visual studio. I have not really checked how different the execution is on a client other than the server. I assumed that, since _settings_client.network.max_companies is not a game setting, I decided to pass it through p2. Client and server could probably have different values for this setting, that's why I avoid testing against it.

@TrueBrain
Copy link
Member

Tnx for the lengthy explanation. So basically what I said holds. So that means your solution is more a hack than a real solution, as you didn't tackle the real issue here.
Why I say it is a hack, if you look a bit further: say there are 100 commands in the queue, and you want 10 AIs. This would mean that for 20 ticks (100 / 5), 10 AI commands are queued. So by the time the first AI wants to be create, 20 * 10 commands more are in the queue. Of course 19 * 10 commands are later dismissed. This is what I mean with: this feels like a hack. You are not solving the issue, but an underlying symptom.

So we should look for how to solve this properly.

I think it all originates in the original patch: looping over a function to create multiple sounds like a dirty approach on its own. If you want to start 10 AIs in a single tick, there should be a single command creating those 10 AIs. Not loop over creating 1 AI 10 times, hoping it can be done in a single tick.
So possibly, instead of looking in this direction, you can look in that (or any other) direction?

@nielsmh
Copy link
Contributor

nielsmh commented Mar 23, 2019

Idea for making a single command to start multiple AIs: Use 16 bits of either p1 or p2 to CCA_NEW_AI as a bitmask of companies to start AI for. Then loop over all set bits in that mask inside the command execution.

@SamuXarick SamuXarick force-pushed the enforce-max_no_competitors-test branch from 1f8435b to 5913a62 Compare March 23, 2019 17:38
@SamuXarick SamuXarick changed the title Fix 011257d: Enforce the max_no_competitors and network.max_companies tests before creating an AI company in multiplayer. Fix 011257d: Allow multiple AI companies to be started with just 1 command, but revert the starting of companies back to once per day. At the start of a new game, check whether to start AI companies immediately, to avoid having to wait one day for that to happen. Mar 23, 2019
src/ai/ai_core.cpp Outdated Show resolved Hide resolved
src/company_cmd.cpp Outdated Show resolved Hide resolved
src/company_cmd.cpp Outdated Show resolved Hide resolved
src/company_cmd.cpp Outdated Show resolved Hide resolved
src/company_cmd.cpp Outdated Show resolved Hide resolved
src/openttd.cpp Outdated Show resolved Hide resolved
@TrueBrain TrueBrain changed the title Fix 011257d: Allow multiple AI companies to be started with just 1 command, but revert the starting of companies back to once per day. At the start of a new game, check whether to start AI companies immediately, to avoid having to wait one day for that to happen. More than max_no_competitors could be created in network games Mar 23, 2019
@TrueBrain
Copy link
Member

PS: please fix commit message length. And please please, never ever again make title of Pull Request this long please. It is not helping, and kinda makes me want to walk away ..

src/company_cmd.cpp Outdated Show resolved Hide resolved
src/company_cmd.cpp Outdated Show resolved Hide resolved
src/ai/ai.hpp Outdated Show resolved Hide resolved
@SamuXarick SamuXarick force-pushed the enforce-max_no_competitors-test branch from 9c3b02b to f0b101d Compare March 24, 2019 03:55
src/ai/ai.hpp Outdated Show resolved Hide resolved
@TrueBrain TrueBrain added this to the 1.9.0 milestone Mar 24, 2019
@TrueBrain TrueBrain modified the milestones: 1.9.0, 1.10.0 Mar 24, 2019
src/company_cmd.cpp Outdated Show resolved Hide resolved
@SamuXarick SamuXarick force-pushed the enforce-max_no_competitors-test branch from 465f6cc to 5dcb172 Compare March 24, 2019 19:15
@SamuXarick SamuXarick force-pushed the enforce-max_no_competitors-test branch 6 times, most recently from cd9731a to c5a26a9 Compare April 11, 2019 14:18
@SamuXarick SamuXarick force-pushed the enforce-max_no_competitors-test branch from c5a26a9 to 7f9b0b5 Compare April 13, 2019 21:36
@SamuXarick SamuXarick force-pushed the enforce-max_no_competitors-test branch from 7f9b0b5 to caca782 Compare July 18, 2019 21:35
@SamuXarick SamuXarick force-pushed the enforce-max_no_competitors-test branch from 527ac13 to 238a2cd Compare November 5, 2019 13:28
@SamuXarick SamuXarick force-pushed the enforce-max_no_competitors-test branch from 238a2cd to 81c609f Compare December 22, 2019 14:37
@SamuXarick SamuXarick force-pushed the enforce-max_no_competitors-test branch from 81c609f to afe0b26 Compare December 31, 2019 10:18
@LordAro
Copy link
Member

LordAro commented Jan 1, 2020

Given 011257d has long since been reverted, what is this PR doing, exactly?

@SamuXarick
Copy link
Contributor Author

LordAro, #7376 is trying to generate the correct number of AIs in multiplayer. Just tested 20191230-master-gf6ce5c4563 and the bug is still present.
Unnamed, 1950-01-0116

_next_competitor_start = max(1, AI::GetStartNextTime() * DAY_TICKS);
/* If GetStartNextTime is zero, wait at least a day to re-evaluate.
* This avoids checking every tick if an AI can be started. */
_next_competitor_start = max(DAY_TICKS, AI::GetStartNextTime() * DAY_TICKS);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I revert this line? The whole point of this PR was so that it could create the correct number of AIs while checking every tick.

@nielsmh nielsmh modified the milestones: 1.10.0, 1.11.0 Feb 9, 2020
@SamuXarick SamuXarick force-pushed the enforce-max_no_competitors-test branch from afe0b26 to eb90176 Compare February 9, 2020 20:50
@SamuXarick SamuXarick force-pushed the enforce-max_no_competitors-test branch from eb90176 to e379e26 Compare July 12, 2020 10:36
@LordAro
Copy link
Member

LordAro commented Sep 24, 2020

Going to close this one. It's not been touched in a while, and there's been no real consensus on it, other than "this is the wrong solution"

@LordAro LordAro closed this Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants