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

Fix /givecashall debug-command #15945

Merged
merged 1 commit into from May 1, 2019

Conversation

@skruw
Copy link
Contributor

commented Dec 24, 2018

Closes #15207

Hi there. This is my second PR. Hope my solution is correct :). Tested it in all four mods and it worked for me.

  • Fix Order-Validaiton for the order "DevGiveCashAll"
  • Fix Game.Debug-Outputfor the order "DevGiveCashAll"
@Smittytron

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2018

Can you squash the fix-up commit?

@pchote

This comment has been minimized.

Copy link
Member

commented Dec 24, 2018

I'd really prefer not to start adding exceptions to the order validator. It would be better if the order were instead only sent to one player, and then the ResolveOrder can enumerate and give to all the players.

@skruw

This comment has been minimized.

Copy link
Contributor Author

commented Dec 24, 2018

I'll look into it.

@skruw skruw force-pushed the skruw:fix-givecashall branch from 719a34f to 6b2e2dd Jan 8, 2019

@skruw

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

Hey guys. I hope you all had great holidays. Sorry for the delayed response. I was on vacation.

@pchote I tried implementing your solution. Is this what you had in mind? I deleted all my old commits, made a new one and force pushed it to the pr. Hope that is okay.

@skruw skruw force-pushed the skruw:fix-givecashall branch from 6b2e2dd to 4111381 Jan 9, 2019

@skruw

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

Thank you for your suggestion. I changed the line.

@pchote

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

This is indeed broadly what I had in mind. There are a couple of code style nits i'll ask you to change when I have time to look at this properly - please ping me again here if I don't review this by the end of the weekend.

@skruw

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2019

@pchote Ping.

@pchote
Copy link
Member

left a comment

A few small comments, then LGTM:

OpenRA.Mods.Common/Commands/DevCommands.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Commands/DevCommands.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Commands/DevCommands.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Player/DeveloperMode.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Player/DeveloperMode.cs Outdated Show resolved Hide resolved

@skruw skruw force-pushed the skruw:fix-givecashall branch from 4111381 to 3f12dad Jan 15, 2019

@skruw

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

Done.

@abcdefg30
Copy link
Member

left a comment

Sorry for taking so long. A few final nits/questions before we finally merge this.

OpenRA.Mods.Common/Traits/Player/DeveloperMode.cs Outdated Show resolved Hide resolved
case "DevGiveCashAll":
{
var amount = order.ExtraData != 0 ? (int)order.ExtraData : info.Cash;
var receivingPlayers = self.World.Players.Where(p => p.Playable);

This comment has been minimized.

Copy link
@abcdefg30

abcdefg30 Mar 11, 2019

Member

Is there a reason you changed !NonCombatant to Playable?

This comment has been minimized.

Copy link
@pchote

pchote Apr 28, 2019

Member

IMO this change makes sense. It gives cash to all the players who have slots in the lobby, but omits scripted players who may otherwise break if they unexpectedly receive or lose cash.

This comment has been minimized.

Copy link
@abcdefg30

abcdefg30 Sep 18, 2019

Member

This really hurts now when testing campaign AIs...

Fix givecashall debug command
Issuing an order to another client causes a validation error in
ValidateOrder.OrderValidation(...). To fix this /givecashall will now
be issued as an order to the client that introduced the command. This
client will then resolve the command and give the cash to all playable
parties.

@pchote pchote force-pushed the skruw:fix-givecashall branch from 3f12dad to 1794e23 Apr 28, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

I've pushed over this with a fix for the braces and a tweak to the chat output.
Also rebased on latest bleed.

@pchote
pchote approved these changes Apr 28, 2019

@pchote pchote added the PR: Needs +2 label Apr 28, 2019

@abcdefg30 abcdefg30 merged commit 77d8908 into OpenRA:bleed May 1, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.