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

Add: [YAPF] Rail depot penalty based on the number of trains waiting in it #9958

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nchappe
Copy link
Contributor

@nchappe nchappe commented Jul 26, 2022

Motivation / Problem

Non-stopped trains waiting in depot are not taken into account in YAPF cost calculations, because they may not have any track reserved. This can for instance be a problem for 'go to nearest depot' orders, as a train may choose to go to a depot full of waiting trains when it could go to a slightly more distant empty depot instead.

Description

This PR adds a penalty for trains in depot to YAPF.
rail_pbs_cross_penalty seemed to be the most relevant penalty as it is used for reserved tracks.

Limitations

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'm in favor of this, but I recognize that it could be confusing to players when a vehicle bypasses a nearby depot to go further away. I'd be curious to see what happens when a large number of vehicles is in a depot (say, after cloning 20+ trains and starting them all) — maybe a maximum penalty would be a good idea, if there's any precedent for that in other penalty calculations.

@nchappe
Copy link
Contributor Author

nchappe commented Oct 29, 2022

To answer your question, the default value of rail_pbs_cross_penalty is 3 * YAPF_TILE_LENGTH so after cloning 20 trains, a train going to the nearest depot would prefer an empty depot 60 (free) tiles away.

I agree this can be confusing, so I added a cap of 8 trains. Beyond 8 trains stuck in the same depot, I guess the player should probably rethink their network anyways.
The max "trains in depot" penalty is now equivalent to a distance of 24 free tiles, or (I think) 6 occupied tiles.

@PeterN
Copy link
Member

PeterN commented Oct 29, 2022

Building lists of vehicles in a depot within the pathfinder seems like it might be bad idea for pathfinder performance.

@2TallTyler
Copy link
Member

Maybe having depots cache the current number of non-stopped trains inside them would be a better solution.

@2TallTyler
Copy link
Member

@nchappe Any plans to resume work on this based on the review comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants