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: On company closure, only sell shares back after all company value calculations are done #9921

Conversation

SamuXarick
Copy link
Contributor

Motivation / Problem

When a company closes, shares are sold. To get the value of these shares, company value calculations are issued. But the way it's currently implemented, it sells the share, increasing the value of that company. This is done in a loop.

If another share value is required involving the same company which recently got its value increased, the value is now different. In essence, what I'm trying to say is that the same company can have different values while the closure command is still running, skewing subsequent share sells.

Description

	/* We're going to check if there are shares to sell, and if there are
	 * we're not selling them immediately, as that would create different
	 * company values for the same company while the command is still running.
	 * Instead, we will first gather the costs from selling the shares and
	 * their owners, and only after that we actually sell them. */

Limitations

This may not be an issue at all, depending on who you ask. After all, the calculations that are in place can be seen as multiple 'Sell 25% shares' happening in a sequence, meaning that having different company values is fine. However, since the execution order of which shares are sold first is not controlled by the player, I still went with this solution.

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 touches english.txt or translations? Check the guidelines
  • 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')

@2TallTyler
Copy link
Member

I don't quite understand the need for this change. When a company is closed, its value disappears into the ether. And unless I'm somehow mistaken, a company's value doesn't change based on who owns shares of it, so remaining companies wouldn't care either. Who benefits from a more accurate accounting of how much a closed company is worth? (Honest question, I think I'm missing something.)

@2TallTyler 2TallTyler added work: missing intention This Pull Request is missing "why" it exists size: small This Pull Request is small, and should be relative easy to process labels Nov 9, 2022
@2TallTyler
Copy link
Member

Closing this because the intention has not been explained after two months. Feel free to reopen if/when you want to explain. 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: small This Pull Request is small, and should be relative easy to process work: missing intention This Pull Request is missing "why" it exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants