-
-
Notifications
You must be signed in to change notification settings - Fork 957
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
More than max_no_competitors could be created in network games #7376
Conversation
c927bd6
to
6a222fc
Compare
6a222fc
to
96e8976
Compare
239583e
to
4eabc1b
Compare
This feels to me like the wrong approach. From what I understand.
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. |
There was a problem hiding this 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.
4eabc1b
to
52c8e42
Compare
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: 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: 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: 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: 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: 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): 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: 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): 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. |
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. 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. |
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. |
1f8435b
to
5913a62
Compare
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 .. |
9c3b02b
to
f0b101d
Compare
465f6cc
to
5dcb172
Compare
cd9731a
to
c5a26a9
Compare
c5a26a9
to
7f9b0b5
Compare
7f9b0b5
to
caca782
Compare
527ac13
to
238a2cd
Compare
238a2cd
to
81c609f
Compare
81c609f
to
afe0b26
Compare
Given 011257d has long since been reverted, what is this PR doing, exactly? |
LordAro, #7376 is trying to generate the correct number of AIs in multiplayer. Just tested 20191230-master-gf6ce5c4563 and the bug is still present. |
_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); |
There was a problem hiding this comment.
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.
afe0b26
to
eb90176
Compare
eb90176
to
e379e26
Compare
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" |
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.