bugfix(player): Fix rounding inaccuracies with money awarded by Cash Bounty#2330
Conversation
|
| Filename | Overview |
|---|---|
| Generals/Code/GameEngine/Source/Common/RTS/Player.cpp | Fixed floating-point rounding error in cash bounty calculation by subtracting epsilon before ceiling and replacing macro with standard ceil function |
| GeneralsMD/Code/GameEngine/Source/Common/RTS/Player.cpp | Fixed floating-point rounding error in cash bounty calculation by subtracting epsilon before ceiling and replacing macro with standard ceil function |
Last reviewed commit: 340d2bb
|
Is there a point to fixing this particular issue instead of fixing the |
Ideally yes, but given its extensive usage across the code base, there could be many unforeseen/unpredictable consequences. |
|
You always get +1$ no matter what unit you kill. When you have cashbounty. |
|
Does using the standard math ceil function correct the behaviour? |
Yes. |
|
For the non retail path it might be better to just use the standard ceil function instead of correcting the macro version. we want to move away from those in non retail code. |
We'll need to use math functions that produce the same output with different compilers / architectures if possible. For example, |
Yes but that is no concern right now because we are not yet multi platform. |
|
By the time we break retail compatibility it'll be relevant, though, which is when this applies, anyway. |
I think we can safely change the GameClient calls without worrying about mismatches, but GameLogic ones will probably need to be behind flags. I think it'd be worth while to fix fast_float_ceil and see if anything noticeably breaks. While it wouldn't surprise me to see things that depend on an off by one bug to work correctly here, it could be fine - we should just test it. To be fair, the risk reward isn't huge here, in terms of where we should be spending our time. I just like the idea of fixing an off by one bug |
|
I think there is a broader issue here: always rounding up - whether it is due to floating point inaccuracy or just decimal math. A better way would be to do proper rounding (I know, difficult in VC6). As it involves money, one could argue that the best way is actually bankers rounding (round to even number). Further along, one could also argue using fixed decimal point types (I don't know if they exists in C++) rather then using floating point types. As any change is non-retail compatible, in my opinion any change to the rounding should immediately work towards a permanent solution rather then using a epsilon hack. |
I agree, and would have originally opted to use |
|
Did regular |
|
Probably, because 260.0000000001 is above 260. |
|
That wasn't the issue to begin with, and to answer my own question |
|
Oh. Can make a follow up change :) |
Yes - it guarantees calculations are always correct. Ceil does not eliminate floating point inaccuracies. 0.26 × 1,000 may work, but 4.01 × 1000 = 4,011. |
I don't think those numbers support your point, either. |
|
Well the point was that an epsilon is still needed. Perhaps we need a larger one for this scenario? |
|
I have yet to see an example where the epsilon would make a difference, though. |
I tested 999 multipliers between 0.01 - 9.99 on a build cost of 1000. It looks like Increasing the epsilon to 0.001 resolves these inaccuracies and can be done in a follow-up change. |
Closes #251
This change fixes an issue with the money awarded by Cash Bounty, where particular percentage calculations can cause a rounding error and return the incorrect amount.
For example, a Cash Bounty value of 26% causes an object with a value of $1,000 to unexpectedly yield $261. This is because calculations like 1000 × 0.26 can effectively return 260.0000000001 due to floating point inaccuracies, which is then rounded up.