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

Added checks to make sure cash can't be <0. #14424

Merged
merged 4 commits into from May 7, 2018

Conversation

@GSonderling
Copy link
Contributor

GSonderling commented Nov 26, 2017

Prevents cash from going into negative values as described in #14385 .

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Nov 26, 2017

I'm not sure if we wanna disable negative cash totally. It may be useful for missions to simulate debt and set with lua. I think we should just ensure /givecash(all) doesn't cause it to go negative.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 26, 2017

That same argument could be applied to the /givecash command too. If someone wants to set up a mission with negative funds, then they might want to use /givecash to help test that. If we want to support one, then we should support both.

@GSonderling

This comment has been minimized.

Copy link
Contributor Author

GSonderling commented Nov 26, 2017

I asked about this exact thing in #14385 .

If you want, I will kill the PR and move on. But if so, you might also want to close the issue. So it doesn't confuse anyone else.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 26, 2017

IMO negative cash in any situation is a bug, so would prefer to fix it everywhere.

@LipkeGu
Copy link
Member

LipkeGu left a comment

Pleaase dont set things to Zero without a specified reason.
Use my Example what i have shown you. Result is safer and performance is the same.

In your PR i can crash the Game with an Exception because i can still assign values like "-99999999999" but your check comes after the (miss)assignment!

@@ -105,6 +105,7 @@ public void GiveCash(int num)
checked
{
Cash += num;
if (Cash < 0) Cash = 0;

This comment has been minimized.

@LipkeGu

LipkeGu Nov 27, 2017

Member

please DONT change the cash value! (leave it AS IS!
use this;

    Cash += (num > 0) ? num : 0;

same in bottom case for Earned!

This comment has been minimized.

@GSonderling

GSonderling Nov 27, 2017

Author Contributor

But wouldn't the cash go into negative if num == -(Cash+1)?

Edit: Very well, this option would work as you want it. But what if someone wanted to decrease cash of all players?
Are you absolutely sure nobody will ever need to do that?

This comment has been minimized.

@GSonderling

GSonderling Nov 27, 2017

Author Contributor

Also, one more thing. The OverflowException, is already taken care of. The game can't crash because the entire code already is in try/check block.

The worst case scenario you get cash and Earned set to int.max.

This comment has been minimized.

@MustaphaTR

MustaphaTR Nov 28, 2017

Member

I think TakeCash already checks for such situtations. So i think this should be fixed by making /givecash all use TakeCash instead of GiveCash when a negative value is given as an argument.

If there is, another place that only use GiveCash without a negative check should also be changed to use TakeCash.

This comment has been minimized.

@GSonderling

GSonderling Nov 29, 2017

Author Contributor

So how about wrapping TakeCash and GiveCash in method that decides which to use, based on sign of the argument?

This comment has been minimized.

@penev92

penev92 Nov 29, 2017

Member

Sure, check the usages of both methods and if the dev command is the only one that can do both just do a check which to use in there.

@@ -120,6 +121,7 @@ public void GiveCash(int num)
checked
{
Earned += num;
if (Earned < 0) Earned = 0;

This comment has been minimized.

@LipkeGu

LipkeGu Nov 27, 2017

Member

Same here...

@GSonderling

This comment has been minimized.

Copy link
Contributor Author

GSonderling commented Dec 4, 2017

If we are in agreement I will create a wrapper method that decides between TakeCash and GiveCash depending on the amount requested.

The method will also adjust the requested change so that cash doesnt drop below 0.

@GSonderling GSonderling force-pushed the GSonderling:ResourceCheck branch from f439332 to 235aee6 Dec 9, 2017

@GSonderling

This comment has been minimized.

Copy link
Contributor Author

GSonderling commented Dec 9, 2017

I have adjusted the code as requested.

public void ChangeCash(int amount)
{
if (amount >= 0)
{

This comment has been minimized.

@GraionDilach

GraionDilach Dec 27, 2017

Contributor

Code style nit: Braces are dropped in such single-line code blocks.

You coulda consider simplifying this into amount >= 0 ? GiveCash(amount) : TakeCash(-amount) as well.

This comment has been minimized.

@GSonderling

GSonderling Dec 27, 2017

Author Contributor

I can drop the braces, although it doesn't affect functionality in any way.
As for the ternary, I would prefer to avoid it for several reasons.

  1. Ternary operator is slightly less efficient, not enough to cause concern but still.
  2. The readability kind of suffers when you use it. It is pretty obvious what it is. But more complex conditions can get confusing for the reader. On the other hand if-else are completely obvious from the first glance.

That being said, if it is real issue and a requirement, I will change it. No problem.

This comment has been minimized.

@GraionDilach

GraionDilach Dec 27, 2017

Contributor

Well, I like ternaries, but their use is optional. The braces however are still against the employed coding style - see point 7 of the list at https://github.com/OpenRA/OpenRA/wiki/Coding-Standard.

@GSonderling GSonderling force-pushed the GSonderling:ResourceCheck branch from 235aee6 to 0a1f9a7 Jan 3, 2018

@GSonderling GSonderling force-pushed the GSonderling:ResourceCheck branch from 0a1f9a7 to bd0eb5f Jan 13, 2018

@GSonderling

This comment has been minimized.

Copy link
Contributor Author

GSonderling commented Jan 14, 2018

Removed the braces as requested.

@GSonderling

This comment has been minimized.

Copy link
Contributor Author

GSonderling commented Feb 1, 2018

Guys, either merge this, tell me what to do with it or delete it. Otherwise it will inevitably fall behind and need rebase.

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Sorry for the delayed review.
Looks good to me other than that small nit. 👍

public void ChangeCash(int amount)
{
if (amount >= 0) GiveCash(amount);
else TakeCash(-amount);

This comment has been minimized.

@abcdefg30

abcdefg30 Feb 1, 2018

Member

Please change this to

if (amount >= 0)
	GiveCash(amount);
else
	TakeCash(-amount);

This comment has been minimized.

@GSonderling

GSonderling Feb 1, 2018

Author Contributor

Ok, it will be done.

@MunWolf
Copy link
Contributor

MunWolf left a comment

Works as advertised, 👍 from me

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Feb 1, 2018

Can you please review the other uses of GiveCash/TakeCash and update them where necessary to avoid duplication and fix all the variations on #14385 from other traits? For example, this now mostly supersedes the ModifyCash method in CashTricker, and a negative payload on DeliversCash can still trigger negative cash.

@GSonderling

This comment has been minimized.

Copy link
Contributor Author

GSonderling commented Feb 1, 2018

I will take a look at that.

There seems to be just one other example of deciding between give/take behavior, in GivesCashOnCapture.

All other methods that use either give or take cash, already take care of necessary checks and adding another layer in between would just slow things down.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Feb 1, 2018

The main issue is code that uses GiveCash without checking the sign first, which is the underlying cause of #14385. DeliversCash was the first example I looked at that could also trigger this, but there are probably others.

@GSonderling

This comment has been minimized.

Copy link
Contributor Author

GSonderling commented Feb 1, 2018

In most cases it doesn't matter. Because the method never receives negative values in the first place.
The #14385 is an exception, because unlike other examples, it's the player who determines the amount and not a developer.

But if you really want to check the sign every time, then it shall be done. It's just unnecessary.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Feb 1, 2018

I was thinking about modders, who might want to create cash-draining units or abilities with the current traits.

@GSonderling

This comment has been minimized.

Copy link
Contributor Author

GSonderling commented Feb 1, 2018

Well in that case it will need more comprehensive rewrite, because the code doesn't count with that.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Feb 1, 2018

Is replacing GiveCash with ModifyCash not sufficient?

@GSonderling

This comment has been minimized.

Copy link
Contributor Author

GSonderling commented Feb 1, 2018

ChangeCash the method I have added and that I'm using for this, can handle it, in theory.

I can't think of any reason why it should fail, but that isn't exactly assuring. The issue is that I tested it for this specific thing, and in that context it should work. But I didn't take into account specific circumstances (value ranges, usage) in other classes.

It might not be an issue, if the methods are similar enough I can use the code as it is and just replace original method with the new one. But if there are things I didn't consider, and I have to assume that there will be, it will require some work to make sure it doesn't break anything.

So far I haven't encountered anything that could lead to trouble. But I have to be careful.

Edit: The initial test match just ended. No crashes and AI behaved normally.
The good news is, that most methods already have mechanics that prevent any illegal values.
And that includes things modders might want to use.

@GSonderling GSonderling force-pushed the GSonderling:ResourceCheck branch from 663476d to 098bdef Feb 1, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Feb 1, 2018

Sell activity is also using GiveCash you may wanna change that too.

@GSonderling

This comment has been minimized.

Copy link
Contributor Author

GSonderling commented Mar 3, 2018

Ok, is everything as it should be? Should I change something else?

// Check whether the amount of cash to be removed would exceed available player cash, in that case only remove all the player cash
var drain = Math.Min(resources.Cash + resources.Resources, -amount);
resources.TakeCash(drain);
resources.ChangeCash(amount);

This comment has been minimized.

@pchote

pchote Mar 4, 2018

Member

Here and in GivesCashOnCapture the amount is first tweaked so that it will take the player to 0 cash (otherwise TakeCash won't do anything) and then this tweaked amount is shown in the cash tick.

I'm pretty sure that we want this same behaviour for the other traits that now use ChangeCash, so could you please add this to the else branch of ChangeCash and make that method return the actual changed amount? The callers can then use this return value in their cash ticks.

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 19, 2018

Member

This needs to be amount = resources.ChangeCash(amount); then if I'm not mistaken?

This comment has been minimized.

@GSonderling

GSonderling Mar 19, 2018

Author Contributor

I guess. Anything else while Im changing this?

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 19, 2018

Member

I didn't test it ingame yet, the code looks good though. (So, no. ^^)

This comment has been minimized.

@GSonderling

GSonderling Mar 20, 2018

Author Contributor

Ok the new code is up.

@GSonderling

This comment has been minimized.

Copy link
Contributor Author

GSonderling commented Mar 4, 2018

Oops, forgot about rebase.

Edit: Ok, rebasing at this moment. I will push again when it's done.

@GSonderling GSonderling force-pushed the GSonderling:ResourceCheck branch from 827f438 to 43073f0 Mar 4, 2018

@@ -34,7 +34,7 @@ protected override void OnInside(Actor self)
if (target.IsDead)
return;

target.Owner.PlayerActor.Trait<PlayerResources>().GiveCash(payload);
target.Owner.PlayerActor.Trait<PlayerResources>().ChangeCash(payload);

This comment has been minimized.

@pchote

pchote Mar 10, 2018

Member

This will need to be payload = target.Owner.PlayerActor.Trait<PlayerResources>().ChangeCash(payload); so that the CashTick can show the correct value.

This comment has been minimized.

@GSonderling

GSonderling Mar 10, 2018

Author Contributor

Payload is readonly. Are you sure you want me to change it?

This comment has been minimized.

@pchote

pchote Mar 10, 2018

Member

Ah ok, better to define a new donated variable then.

This comment has been minimized.

@GSonderling

GSonderling Mar 10, 2018

Author Contributor

Ok. I'm on it.

@@ -102,7 +102,7 @@ public void GiveResource(int amount)
playerResources.GiveResources(amount);
}
else
playerResources.GiveCash(amount);
playerResources.ChangeCash(amount);

This comment has been minimized.

@pchote

pchote Mar 10, 2018

Member

Likewise here

AddCashTick(self, amount);
}
if (info.ShowTicks)
AddCashTick(self, resources.ChangeCash(amount));

This comment has been minimized.

@pchote

pchote Mar 10, 2018

Member

The ChangeCash needs to be outside the if, otherwise it won't run!

@@ -39,7 +39,7 @@ public override void Activate(Actor collector)
{
collector.World.AddFrameEndTask(w =>
{
collector.Owner.PlayerActor.Trait<PlayerResources>().GiveCash(info.Amount);
collector.Owner.PlayerActor.Trait<PlayerResources>().ChangeCash(info.Amount);

This comment has been minimized.

@pchote

pchote Mar 10, 2018

Member

var amount = for use in the FloatingText below.

}
else
resources.GiveCash(amount);
amount = resources.ChangeCash(amount);

This comment has been minimized.

@pchote

pchote Mar 10, 2018

Member

This can simplify down to var amount = resources.ChangeCash(info.Amount);

@GSonderling GSonderling force-pushed the GSonderling:ResourceCheck branch 2 times, most recently from 5b1da05 to f915446 Mar 10, 2018

@GSonderling

This comment has been minimized.

Copy link
Contributor Author

GSonderling commented Mar 10, 2018

It's all there now.

@GSonderling GSonderling force-pushed the GSonderling:ResourceCheck branch from f915446 to 71985b3 Mar 20, 2018

@abcdefg30
Copy link
Member

abcdefg30 left a comment

This doesn't seem to work at the moment. You can't seem to subtract cash using a CashTrickler or the dev commands at all.

else
{
amount = Math.Min(Cash + Resources, -amount);
TakeCash(-amount);

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 21, 2018

Member

Are you sure this is supposed to be -amount?

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 21, 2018

Member

It seems like this is causing negative cash to go to the player's resources instead!

This comment has been minimized.

@GSonderling

GSonderling Mar 21, 2018

Author Contributor

With all the rebasing I'm not sure when was the last time I touched this. It feels that I wouldn't code something like this.

Anyway, are there any other issues I should know about? I know I keep asking this, but every time I do you tell me it's ok only for more issues to pop-up.

This comment has been minimized.

@GSonderling

GSonderling Mar 21, 2018

Author Contributor

I took a look and as unlikely as it may seem, it is supposed to be -amount.
The take cash method operates with positive numbers. I could give it a rewrite, but honestly it makes logical sense for it to behave as it does now.
TakeCash(howMuchCashIWantToTake)

I will take a look at the first issue you raised later.

This comment has been minimized.

@GSonderling

GSonderling Mar 21, 2018

Author Contributor

Well turns out I was wrong, again. The Min method already flips the sign on this one so it should have been TakeCash(amount)

Sorry.

@pchote pchote force-pushed the GSonderling:ResourceCheck branch from 71985b3 to 80785e6 May 7, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 7, 2018

I have rebased this and fixed the issue with ChangeCash. I also added a couple of commits to fix some polish issues with cash ticks. 👍 with my changes.

@pchote

pchote approved these changes May 7, 2018

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍

PR was fixed

@reaperrr reaperrr merged commit 579cc09 into OpenRA:bleed May 7, 2018

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
You can’t perform that action at this time.