Fix: Company values do not properly account for shares #9770
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benda
changed the title
LordAro
requested changes
Jan 5, 2022
src/economy.cpp
Outdated
| { | ||
| Money owned_shares_value = 0; | ||
|
|
||
| uint8 shares_owned = 0; |
There was a problem hiding this comment.
variable is only used inside the loop, no need to declare it before it's used (very old-style C ! )
src/economy.cpp
Outdated
| * @param including_loan include the loan in the company value. | ||
| * @return the value of the company. | ||
| */ | ||
| Money CalculateCompanyValue(const Company *c, bool including_loan) | ||
| Money CalculateCompanyValue(const Company* c, bool including_loan) |
There was a problem hiding this comment.
codestyle: * goes on the variable, not the type
src/economy.cpp
Outdated
Comment on lines
129
to
131
| if (shares_owned > 0) { | ||
| owned_shares_value += (CalculateCompanyValueExcludingShares(co) / 4) * shares_owned; | ||
| } |
There was a problem hiding this comment.
probably not worth bothering with the if statement here - CalculateCompanyValueExcludingShares isn't a particularly costly function to call needlessly (if shares_owned == 0)
There was a problem hiding this comment.
CalculateCompanyValueExcludingShares iterates all vehicles and stations so it is somewhat costly. Enough to add a check to skip it in the most common case.
src/economy.cpp
Outdated
| if (including_loan) value -= c->current_loan; | ||
|
|
||
| /* Add real money value */ |
There was a problem hiding this comment.
Comment applies to line below the comment. There was an unnecessary space.
src/company_base.h
Outdated
| @@ -166,6 +166,7 @@ struct Company : CompanyProperties, CompanyPool::PoolItem<&_company_pool> { | |||
| }; | |||
|
|
|||
| Money CalculateCompanyValue(const Company *c, bool including_loan = true); | |||
| Money CalculateCompanyValueExcludingShares(const Company* c, bool including_loan = true); | |||
LordAro
previously approved these changes
Mar 20, 2022
LordAro
force-pushed
the
fix/IncorrectCompanyValues
branch
from
af9a7ef
to
5b2181c
Compare
LordAro
force-pushed
the
fix/IncorrectCompanyValues
branch
from
5b2181c
to
cb2fa73
Compare
LordAro
approved these changes
Apr 30, 2022
LordAro
changed the title
LordAro
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation / Problem
This is a redo of
#9768
to have everything into 1 commit. Had some rebase conflicts with prior PR.
Company value calculation is missing the value of the company's shares in other companies
Discussed as issue 1 below:
#9765
Description
The problem is solved by first calculating all of the company values without including value of shares in other companies, then adding in the value of the shares which now is known. I do not think this causes issues with bankruptcy or buying/selling shares; it does affect those of course since they now use the new company values.
Note that when buying shares, the company value does not change, since instead of holding the cash it now holds the shares of an equivalent value.
Note that when selling shares, the company value does not change, since instead of holding the shares it now holds cash of an equivalent value.
Limitations
I haven't tested multiplayer, this works fine on saved games in single player.
I am not a C++ programmer so my change should be thoroughly reviewed.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.