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: Prevent showing company is in trouble again after about 260 months due to overflow #8597

Closed

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Jan 20, 2021

Motivation / Problem

When in single player, the company the player is in cannot bankrupt, but c->months_of_bankruptcy counter keeps going up every month the company is in red. It overflows once it reaches 255, restarting at 0. So, at aproximatelly 260 months, (case 4), the newspaper displays again that the company is in trouble.

Description

Bug is fixed by decreasing months_of_bankruptcy counter by 1 inside the check where it maintains the company playing.

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 22, 2021

I never considered this a bug, just a kind reminder that you are still in trouble. So I am very unsure this needs fixing.

But if we do fix it, it is lacking a comment to indicate -why- we should decrease the variable at that point again. It also seems like the wrong place to fix it, but that might just be me. First we need to figure out if we want to fix it. My vote: no.

@James103
Copy link
Contributor

@James103 James103 commented Jan 22, 2021

If I were you, I would consider rejecting this PR for a similar reason. To be more specific:

  • A "company is in trouble" news message pops up for the company you're playing as, but the player is AFK.
  • The news message expires and is deleted from the news history, while the player is still AFK.
  • The player then is no longer AFK and checks the news history, never realizing that a "company is in trouble" message has popped up.

Without that second reminder, the player would never know that his/her company is in trouble besides the negative bank balance. With that reminder (current behavior before PR), the player finally gets notified that his/her company is in trouble, regardless of if he/she ever checked the bank balance.

@@ -609,6 +609,7 @@ static void CompanyCheckBankrupt(Company *c)
* is no THE-END, otherwise mark the client as spectator to make sure
* he/she is no long in control of this company. However... when you
* join another company (cheat) the "unowned" company can bankrupt. */
c->months_of_bankruptcy--;
Copy link
Contributor

@perezdidac perezdidac Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we just change the variable type to a short or int? This seems a workaround.

@SamuXarick
Copy link
Contributor Author

@SamuXarick SamuXarick commented Jan 27, 2021

Does it really matter for the player to know he's in trouble at that point? If the player is 260 consecutive months with a negative balance, I don't think he cares about the company, or be bothered with those news. For context, that's 21 consecutive years in the red.

For me, this usually happens when observing AIs in single player, my company does nothing the whole time, it will eventually get the "in trouble" news once. Displaying it again every 21 years feels kind of pointless.

Also of note, the 7th month the company is in negative, the company is offered to its competitors. Sure, it's single player, AIs can't buy human companies, but it's just to say that the months_of_bankruptcy counter is restarting from 0 months, and all inherent code runs again.

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 31, 2021

It seems 3 people mention it is either the wrong fix, or considered a hack. I guess that settles it :)

I agree that the current code is not all that nice, but I strongly think this is the wrong solution. Open for alternatives, but going to close this PR for now.

@TrueBrain TrueBrain closed this Jan 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants