Fix/incorrect company values #9768

benda · 2021-12-24T19:31:37Z

Motivation / Problem

Company value calculation is missing the value of the company's shares in other companies

Discussed as issue 1 below:

I view this as a defect.

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 think this PR includes a commit that was already merged (the first commit), I'm not sure how to remove that from this PR.

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.

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')
- newgrf_debug_data.h may need updating.
- PR must be added to API tracker

…ound value.

ldpl · 2021-12-24T19:39:06Z

src/economy.cpp

+	for (const Company* co : Company::Iterate()) {
+		for (int i = 0; i < 4; i++){
+			if (co->share_owners[i] == c->index) {
+				companyValue += (companyValuesExcludingShares[co->index] / 4);


Why calculate and store company values that aren't even going to be used? CalculateCompanyValueExcludingShares can just be called here once for each relevant company.

I wasn't sure how to get which shares the relevant company owns without iterating everything first. On this particular line though (126), CalculateCompanyValueExcludingShares will be called up to 3 times for each 25% share, so it would be called multiple times for the same company, no ?

they're all the same, you can just count and multiply

OK, I committed with the implementation I think you are recommending.

2TallTyler

I'm +1 to this change, and my comments are just code style things.

Personally, I would consider simply counting owned shares in company value a fix, while the other items in #9765 would be changes which warrant a separate PR.

2TallTyler · 2021-12-25T03:54:44Z

src/economy.cpp

 {
 	Owner owner = c->index;

 	uint num = 0;

-	for (const Station *st : Station::Iterate()) {
+	for (const Station* st : Station::Iterate()) {


Pointers are written with the asterisk after the space, as in Station *st. This applies to several places in your PR. The way this alone is changed here, though, makes me wonder if your IDE is autocorrecting it to the wrong style (a problem I've experienced previously).

Thanks and sorry - yes I think this was my IDE (VS2019) - I made the requested changes.

2TallTyler · 2021-12-25T03:59:32Z

src/economy.cpp

+		if (sharesOwned > 0)
+		{


The opening curly brace for if statements stays on the same line.

Suggested change

if (sharesOwned > 0)

{

if (sharesOwned > 0) {

I made the requested change.

2TallTyler · 2021-12-25T04:07:13Z

src/economy.cpp

 * @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 companySharesValue = 0;


Variables in OpenTTD are all lowercase with underscores. Also, a better name might be:

Suggested change

Money companySharesValue = 0;

Money owned_shares_value = 0;

I made the requested change.

2TallTyler · 2021-12-25T05:00:36Z

src/economy.cpp

+{
+	Money companySharesValue = 0;
+
+	int sharesOwned = 0;


The OpenTTD code base is fairly careful about right-sizing variables (no matter the "best practice" internet arguments about unsigned integers). This, and your i iterators, would fit in uint8, which has a range of 0-255.

Suggested change

int sharesOwned = 0;

uint8 shares_owned = 0;

I made the requested change. Thanks for all your help!

2TallTyler · 2021-12-25T05:18:06Z

COMPILING.md

@@ -69,7 +69,7 @@ that comes with vcpkg. After that, you can run something similar to this:
 ```powershell
 mkdir build
 cd build
-cmake.exe .. -G'Visual Studio 16 2019' -DCMAKE_TOOLCHAIN_FILE="<location of vcpkg>\vcpkg\scripts\buildsystems\vcpkg.cmake" -DVCPKG_TARGET_TRIPLET="x64-windows-static"
+cmake.exe .. -G"Visual Studio 16 2019" -DCMAKE_TOOLCHAIN_FILE="<location of vcpkg>\vcpkg\scripts\buildsystems\vcpkg.cmake" -DVCPKG_TARGET_TRIPLET="x64-windows-static"


Assuming you've set the upstream properly as described in CONTRIBUTING.md and are using the command line (as opposed to GitHub Desktop, which doesn't really have full functionality), you should be able to rebase this fork on OpenTTD:master with the commands:
git fetch upstream
git rebase upstream/master

Then you can do git rebase -i HEAD~4 to start an interactive rebase, which allows you to drop unwanted commits, squash commits together, and reword them to match OpenTTD commit message format.

To push your changed commits to this PR, you'll need to force-push your commits with git push -f origin HEAD.

Also, assuming you're new to GitHub, I quite like the built-in Git GUI tool for building commits, particularly when you're adding things in multiple commits. It's also good for checking trailing whitespace, which is failing the commit checker.. You can start it with git gui. 😃

OK this is a bit complex for me, I'll try to get to this when I have more time. Thanks for the detailed instructions.

I ran into a lot of conflicts - I think the simplest thing is for me to delete this PR, delete my branch, make a new branch, redo my changes in one commit manually, and make a new PR. Is that OK?

Sorry for all the hassle; new PR is here: #9770

LC-Zorg · 2021-12-25T13:08:41Z

This is not a good change. Overall, incorporating the value of other companies shares into goodwill is fine, but I don't like the fact that buying 100% of the stock takes over the money of the acquired company. In practice, this means that taking over any company costs nothing, because in fact you pay for the infrastructure itself and the vehicles which are no longer worth anything. This change further strengthens this highly flawed mechanism.

In my opinion, when introducing any changes to the function of shares, it would be worth first to set a goal, how it should work, what is the purpose of buying shares and what it brings to the game. Currently, the purchase of shares in practice doesn't serve any other purpose than the possibility of unilateral takeover of companies using the robbery method (without the consent of the company owner). Such a mechanism might be ok in the past when only offline games existed. Nowadays, with a lot of games being played in multiplayer, such an arbitrary way of buying shares and taking over companies is definitely flawed.

…/OpenTTD into fix/incorrectCompanyValues

Me added 3 commits December 21, 2021 18:34

Fix documentation - need to use double quotes not apostrophes to surr…

167d9af

…ound value.

Initial fix - needs cleanup

37fa89a

Cleanup

a571ec7

ldpl reviewed Dec 24, 2021

View reviewed changes

Better implementation, don't need to calculate all company values first.

0912c15

2TallTyler reviewed Dec 25, 2021

View reviewed changes

Me added 11 commits December 25, 2021 07:56

Style change - restore pointers to correct style.

66d938e

Style changes

2c98cae

Use correct types

1b4b4e8

Initial fix - needs cleanup

878dfa0

Cleanup

9a48a0e

Better implementation, don't need to calculate all company values first.

d73766f

Style change - restore pointers to correct style.

72eca09

Style changes

5bf7210

Use correct types

0d04046

Merge branch 'fix/incorrectCompanyValues' of https://github.com/benda…

b0b2428

…/OpenTTD into fix/incorrectCompanyValues

Style changes

b629059

benda closed this Dec 25, 2021

benda pushed a commit to benda/OpenTTD that referenced this pull request Dec 25, 2021

Fix OpenTTD#9768: Incorrect Company Values

7109567

benda mentioned this pull request Dec 25, 2021

Fix: Company values do not properly account for shares #9770

Merged

LordAro pushed a commit to benda/OpenTTD that referenced this pull request Apr 30, 2022

Fix OpenTTD#9768: Incorrect Company Values

c9f3d10

LordAro added a commit to benda/OpenTTD that referenced this pull request Apr 30, 2022

Fix OpenTTD#9768: Company values did not account for shares

cb2fa73

LordAro mentioned this pull request Jun 1, 2023

Remove: buying/selling/owning company shares #10709

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/incorrect company values #9768

Fix/incorrect company values #9768

benda commented Dec 24, 2021 •

edited

ldpl Dec 24, 2021

benda Dec 24, 2021

ldpl Dec 24, 2021

benda Dec 24, 2021

2TallTyler left a comment •

edited

2TallTyler Dec 25, 2021

benda Dec 25, 2021

2TallTyler Dec 25, 2021

benda Dec 25, 2021

2TallTyler Dec 25, 2021

benda Dec 25, 2021

2TallTyler Dec 25, 2021

benda Dec 25, 2021

2TallTyler Dec 25, 2021

benda Dec 25, 2021

benda Dec 25, 2021

benda Dec 25, 2021

LC-Zorg commented Dec 25, 2021

Fix/incorrect company values #9768

Fix/incorrect company values #9768

Conversation

benda commented Dec 24, 2021 • edited

Motivation / Problem

Description

Limitations

Checklist for review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2TallTyler left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LC-Zorg commented Dec 25, 2021

benda commented Dec 24, 2021 •

edited

2TallTyler left a comment •

edited