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

Feature: Setting to disable loading speed penalty for trains at short platforms #11682

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

EmperorJake
Copy link
Contributor

@EmperorJake EmperorJake commented Jan 4, 2024

Motivation / Problem

Trains get a penalty to loading speed if the platform is shorter than the train. There is currently no way to get around this, however there are several use cases why a player might want to do so:

  • Underground stations where the rest of the platform is "hidden" inside the tunnel or on a bridge. This applies to eyecandy players who might do creative things with overlapping station tiles and such.
  • Saving space by loading trains in curved tracks rather than one long platform. Similar to "through loading" as found in JGRPP, but without the extra train movement.
  • Saving infrastructure costs by not needing as many station tiles. This applies to min-maxer players.

There have been various patches in the past to the same effect. Some have added a cheat, others have straight up deleted the code responsible for the loading speed penalty. This feature has been in demand by a subsection of the playerbase for a long time now, it's time it was made a proper setting.

Description

This PR adds a setting which allows the player to disable the loading speed penalty for trains at shorter platforms.

The setting is an "Expert" setting as it has relatively niche uses and new players don't really need to see/change it.

The setting is under the "Limitations" category as this is more in line with #11683

Limitations

The setting does not affect the train pathfinder avoiding short platforms. But that's a good thing as it would mess up many rail layouts otherwise.

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, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

src/economy.cpp Fixed Show fixed Hide fixed
Copy link
Member

@2TallTyler 2TallTyler left a comment

Choose a reason for hiding this comment

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

I'm not sure how I feel about this. It feels like a very niche feature. That said, I agree that having it as a cheat would be the proper way to do it.

You'll notice that commit checker has failed for invalid tab usage. We only use tabs for the leftmost indentation. Any spacing/indentation besides that is created with spaces.

src/economy.cpp Outdated Show resolved Hide resolved
src/saveload/saveload.h Outdated Show resolved Hide resolved
@EmperorJake
Copy link
Contributor Author

EmperorJake commented Jan 4, 2024

I agree that it's a fairly niche feature, but I think there is a small but important part of the playerbase who would greatly appreciate this feature. If the developers feel that it's still too niche, perhaps it would be better ported to JGRPP.

Here is a Discord coversation involving players who support this feature and how they would use it: https://discord.com/channels/142724111502802944/919788785217179738/1077537785759674388

@ldpl
Copy link
Contributor

ldpl commented Jan 4, 2024

I completely disagree with it being a cheat, it achieves nothing but making this setting inconvenient to use (can't change in mp, put into a config etc.).

@EmperorJake
Copy link
Contributor Author

That's a valid point, it would work better as a setting. However, in the past devs have voiced concerns about "significant gameplay implications" (see JGRennison/OpenTTD-patches#330) so I thought it would be more palatable as a cheat.

@ldpl
Copy link
Contributor

ldpl commented Jan 4, 2024

Cargodist, breakdowns and build on pause have much more significant "gameplay implications". This just allows you to build one station tile instead of TL, big deal. Also, you can already unload train on a small station quickly by reversing it a few times.

I can even make an opposite argument - this is a joke of a cheat because it only saves you a few pennies :p

@EmperorJake
Copy link
Contributor Author

I just found a patch to the same end on a forum thread from over 10 years ago. This goes to show that this feature has been in demand for a very long time https://www.tt-forums.net/viewtopic.php?p=1074153

@PeterN
Copy link
Member

PeterN commented Jan 4, 2024

That's a valid point, it would work better as a setting. However, in the past devs have voiced concerns about "significant gameplay implications" (see JGRennison/OpenTTD-patches#330) so I thought it would be more palatable as a cheat.

I don't see any vanilla dev's opinions on that link...

I agree (shock horror) with @ldpl here.

@PeterN PeterN closed this Jan 4, 2024
@PeterN PeterN reopened this Jan 4, 2024
@EmperorJake EmperorJake changed the title Feature: cheat to remove loading speed penalty for trains at short platforms Feature: Setting to disable loading speed penalty for trains at short platforms Jan 4, 2024
@EmperorJake EmperorJake marked this pull request as ready for review January 4, 2024 13:52
Copy link
Member

@2TallTyler 2TallTyler left a comment

Choose a reason for hiding this comment

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

Okay, your comments here and this morning's Discord discussion have changed my mind. I like this as a setting. 🙂

Oh, and you'll want to squash your commits.

src/settings_type.h Outdated Show resolved Hide resolved
src/lang/english.txt Outdated Show resolved Hide resolved
@rubidium42
Copy link
Contributor

Is this setting properly saved in the save game? As far as I can see it's not; at least, I'm not seeing a save game version and such.

@JGRennison
Copy link
Contributor

The settings table doesn't require a savegame bump for new settings.
It isn't otherwise necessary to bump to savegame version because there isn't any new/changed state which prior versions couldn't understand.

@EmperorJake
Copy link
Contributor Author

I tested saving and loading a game, and the state of the setting got saved too.

The original cheat version of the PR had some saveload code, but I assumed that was due to cheats being recorded by the save. I copied a different PR that added a setting and it didn't come with saveload code either.

@rubidium42 rubidium42 merged commit 6522351 into OpenTTD:master Jan 4, 2024
20 checks passed
2TallTyler added a commit to 2TallTyler/OpenTTD that referenced this pull request Jan 4, 2024
2TallTyler added a commit that referenced this pull request Jan 4, 2024
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

6 participants