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 #7656: destroying a tunnel/bridge now first removes the tracks for cost calculation #8508

Merged
merged 2 commits into from Jan 8, 2021

Conversation

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 6, 2021

Fixes #7656 .

Motivation / Problem

When you build a road bridge, put tram tracks on it, and destroy the road+tram bridge, you had to pay money.
If you first removed the tram tracks and destroyed the road bridge, you gained money.

This is rather unfair, to expect a player to know the difference.

This is an effect of allowing road + tram on the same bridge, where you can build either/or on them. A player that pays attention will, without a solution like this, always notice a discrepancy. If you build a road bridge and a tram bridge and a road+tram bridge, you notice quickly enough that something is not adding up.

Description

To not over-complicate the code, I went for a simple approach: destroying a bridge is now a fixed-free + the price to remove the road/tram/rail from the bridge. In most cases this benefits the player, in some it does not.

Removing rail and tram makes you receive money, so bridges that have either of the two, are now (a lot) cheaper to destroy. Road bridges on the other hand are now slightly more expensive. I consider this an acceptable compromise.

While looking into this, I also found that destroying a bridge didn't asked you money for the end-tile (off-by-one).

Limitations

  • Adding tram-tracks to a road-bridge by building a tram-bridge over a road-bridge calculates too much money. I moved this out of scope of this PR.

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')
@James103
Copy link
Contributor

@James103 James103 commented Jan 6, 2021

Note the test results (discrepancy coming from a £360 extra cost on bridge removal):

7462: -   GetBankBalance():     1999979664
7462: +   GetBankBalance():     1999979304'
7475: -   GetBankBalance():     1999965070
7475: +   GetBankBalance():     1999964710'
7494: -   GetBankBalance():     1999959675
7494: +   GetBankBalance():     1999959315'

Also, why are there the stray apostrophes next to the number?

@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Jan 6, 2021

Note the test results (discrepancy coming from a £360 extra cost on bridge removal):

7462: -   GetBankBalance():     1999979664
7462: +   GetBankBalance():     1999979304'
7475: -   GetBankBalance():     1999965070
7475: +   GetBankBalance():     1999964710'
7494: -   GetBankBalance():     1999959675
7494: +   GetBankBalance():     1999959315'

Also, why are there the stray apostrophes next to the number?

So I have not only the CI telling me regression failed, but also a user that tells me .. now I feel even worse for forgetting to run the regression before commit :'(

@TrueBrain TrueBrain force-pushed the TrueBrain:road-money branch from 2aa84f8 to 8b388f3 Jan 6, 2021
@TrueBrain TrueBrain added this to the 1.11.0 milestone Jan 6, 2021
TrueBrain added 2 commits Jan 6, 2021
It only considered the end-tile (or start-tile) for the bridge,
instead of both. This is obvious in the rest of the code which
constantly does "+ 2"; this being the only place that does a "+ 1".
…r cost calculation

This means that for rail tunnel/bridges, the rail is first sold,
and the tunnel/bridge is destroyed after. This means destroying
tunnels/ bridges now often makes you money, instead of costing.

Similar, with road/tram tracks. Destroying a road+tram
tunnel/bridge now costs the same amount of money as first
removing the tram tracks and than destroying the road
tunnel/bridge. Especially as tram tracks generate money when
removing, this is a noticeable difference.
@TrueBrain TrueBrain force-pushed the TrueBrain:road-money branch from 8b388f3 to 237745e Jan 8, 2021
@LordAro
LordAro approved these changes Jan 8, 2021
@TrueBrain TrueBrain merged commit aac8c28 into OpenTTD:master Jan 8, 2021
8 checks passed
8 checks passed
Emscripten
Details
Commit checker
Details
Check preview needs update Check preview needs update
Details
Linux (clang, clang++)
Details
Linux (gcc, g++)
Details
Mac OS (x64, x86_64)
Details
Windows (x86) Windows (x86)
Details
Windows (x64) Windows (x64)
Details
@TrueBrain TrueBrain deleted the TrueBrain:road-money branch Jan 8, 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
Linked issues

Successfully merging this pull request may close these issues.

3 participants