Clamp to avoid moving the loan out of bounds.#3645
Clamp to avoid moving the loan out of bounds.#3645DarthSashna wants to merge 9 commits intoOpenLoco:masterfrom
Conversation
…oco/src/GameCommands/Company/ChangeLoan.cpp
| { | ||
| const auto maxLoan = Economy::getInflationAdjustedCost(CompanyManager::getMaxLoanSize(), 0, 8); | ||
| if (newLoan > maxLoan) | ||
| if (company->currentLoan >= maxLoan) |
There was a problem hiding this comment.
why the change to >=?
There was a problem hiding this comment.
I could have left the original check, but that would have preserved an old quirk where one cannot maximise the loan simply by holding the increase loan button. A large increment from holding the button for a while could exceed the difference between the old and new loan and would thus be rejected even when the company could borrow a little bit more.
It would not make sense to simply drop clampedNewLoan in as it is clamped to never exceed the maximum loan, so the check would never trip and the user would never know why the loan suddenly stops increasing. Thus, I changed the check to be based on the company's currently loan: if the loan is already maxed out (the company's current loan should never exceed the maximum in non-sandbox gameplay, of course.), report an error. In theory, strict equality (i.e. company->currentLoan == maxLoan) would work just the same, but I didn't see any reason to not use >=, which may also help if the current loan were to somehow exceed the maximum loan.
There was a problem hiding this comment.
Using sandbox mode to reduce the maximum loan when the current loan is maxed out causes the loan to be clamped to the new maximum when one tries to reduce or increase the loan. This of course causes the company->currentLoan > clampedNewLoan branch to be taken, so one does not get an error when the company's loan exceeds the maximum loan. In some sense, it's not wrong - the loan does get back in bounds. But it doing so silently is a bit quirky. Then again, you can only see that behaviour using sandbox mode and it doesn't crash the game, so I'm not sure that it's worth 'fixing'. It may be more honest to use a strict equality in the check.
There was a problem hiding this comment.
Then again, you can only see that behaviour using sandbox mode and it doesn't crash the game, so I'm not sure that it's worth 'fixing'. It may be more honest to use a strict equality in the check.
Actually, the game could clamp the loan to be between std::min<currency32_t>(company->currentLoan, 0) and std::max<currency32_t>(company->currentLoan, maxLoan). So if the user has a negative loan from an old save, it won't decrease, but they can increase it. Conversely if sandbox mode is used to have maxLoan < company->currentLoan, attempting to increase the loan won't cause the loan to clamp downwards and will thus error out, but they can repay it normally. That wouldn't immediately bring the loan back in bounds if it was already out of bounds, but it stops the user from making things worse.
It might be possible to have it clamp in bounds all at once when adjusting it towards the bounds, but I think that that would require additional checks, so simply letting users gradually bring things back in bounds is probably simpler.
There was a problem hiding this comment.
While my latest commit can allow clampedNewLoan > maxLoan when company->currentLoan > maxLoan, it can still only be increased to maxLoan if it was less than or equal to it, so we will still need to check company->currentLoan to get the error in normal gameplay without forbidding increases to maxLoan. And the comparison needs to be >= to retain the old behaviour. If it were ==, then attempting to increase an oversized loan would silently clamp. If it were >, then attempting to increase an exactly maxed-out loan would silently clamp.
I don't think that it makes sense for attempting to decrease a negative loan (from an old save) to increases it zero. It also does not make sense for trying to increase a previously-maxed out loan after decreasing the maximum loan using sandbox mode to decrease the loan.
DarthSashna
changed the title
DarthSashna
changed the title
|
One outcome that I find interesting is that the maximum loan displayed in the scenario editor is evidently rounded down to the nearest £100, but the actual maximum loan is continuous. This shouldn't be hard to fix: either divide by 100 - thus rounding down - before multiply by 100 again or subtract |
|
One consequence of that is that logic is duplicated between |
This is so that the maximum loan the user can reach is consistent with the maximum loan in the scenario editor/sandbox mode. Previously, the lack of rounding could not be observed as attempting to increase the loan when you were close to the maximum would fail.
| if (company->currentLoan > newLoan) | ||
| // Old saves may have a negative loan as per #3836; allow gradual increases, but not decreases. | ||
| const currency32_t clampLow = std::min<currency32_t>(company->currentLoan, 0); | ||
| // Sandbox mode can be used to decrease `maxLoan` such that `company->maxLoan > maxLoan`. Don't clamp the loan downwards |
There was a problem hiding this comment.
We don't need to care about that. If you are in sandbox mode you already can and do want to be outside of bounds of normal play. Just use the normal clamp.
|
I'm starting to think we shouldn't be changing the parameters at all here and just reject a negative loan outright no clamping. The callers are the ones that should ensure they pass up to the max. |
Don't worry about certain old saves or sandbox mode and just clamp between 0 and maxLoan.
2 participants
This should have three observable effects:
It was and remains possible to decrease the maximum loan in sandbox mode. If this causes the maximum loan to be less than the company's current loan, then attempting to adjust the company's loan will clamp it to the new maximum. Previously in this case, companies could gradually decrease their loan, but would get an error when increasing it.