Skip to content

1837: fix for multiple train discards #11705 #11711

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

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

Conversation

timpelican
Copy link
Contributor

Fixes #11705

Before clicking "Create"

  • Branch is derived from the latest master
  • Add the pins or archive_alpha_games label if this change will break existing games
  • Code passes linter with docker compose exec rack rubocop -a
  • Tests pass cleanly with docker compose exec rack rake

Implementation Notes

@round.merged_trains[action.entity].delete(action.train) is a silent no-op (returns nil, but we're not using the return value) if either:

  • action.train (the discarded train) is not a member of @round.merged_trains[action.entity] (the list of merged trains)
  • The list is empty.

It can be safely called every time we discard a train.

Explanation of Change

Phase change which both lowers the train limit and merges coal companies can leave a corporation needing to discard multiple trains to meet the new train limit. This could be trains from both the merged coal companies, which must be discarded first, and also from the corporation's trains that were already owned before the merge.

The existing code chooses either the list of 'merged' trains, if non-empty, or the list of already-owned trains, and tries to satisfy ALL of the required discards from that list. If more trains must be discarded than are in the 'merged' list, the game stalls. It does not empty the list of 'merged' trains until after the corporation is back down to the train limit.

By removing each train from the list of 'merged' trains as it is discarded, we can get to an empty list of 'merged' trains during the discard step, which triggers the logic to show the list of already-owned trains to complete the discard process.

Screenshots

Any Assumptions / Hacks

@crericha
Copy link
Collaborator

Thanks for this; however, I want to go in a little more involved direction.

I want to see if the corp is train tight or over the limit without merged merged trains and if so discard the train during the merger and include a log message saying why.

If you'd like to give that a try, let me know. Otherwise, I'll try to get to it next week.

@timpelican
Copy link
Contributor Author

Ah, I get it, not making people click to discard a train they are forced to discard. That does sound a better user experience.

I'm not going to any time for a try at that for at least a week now, so if you have bandwidth next week, please do go ahead.

@ollybh ollybh added the 1837 label May 4, 2025
Copy link
Collaborator

@ollybh ollybh left a comment

Choose a reason for hiding this comment

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

Setting this PR to changes requested, so it doesn't get merged until it has been updated.

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

Successfully merging this pull request may close these issues.

[1837] Companies are not given full choice of what train to discard on phase change
3 participants