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

[18SJ] Corrects edge case nationalization calculation #11205

Merged
merged 5 commits into from
Oct 21, 2024

Conversation

philcampeau
Copy link
Collaborator

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

Explanation of Change

Currently, the nationalization method uses this code to determine which corp of the eligible candidates will be nationalized:

          merged = candidates.max_by { |c| [c.share_price.price, [email protected]_index(c)] }

The problem is that the second sorting criteria uses the initial round order, meaning that if one corp runs and withholds, and moves into the same space as another corp, and they then are both eligible for nationalization, the corp that withheld will be nationalized, even though its token is under the other corp. This issue was reported on boardgamegeek here: https://boardgamegeek.com/thread/3366312/nationalisation-the-first-corporation-in-descendin

I've replaced this with the sorting used by def sort_order_key in lib/engine/corporation.rb:

          merged = candidates.max_by do |c|
            [c.share_price.price, -c.share_price.coordinates.last, -c.share_price.coordinates.first]
          end

which produces the desired result.

This will require pins for any games that have done the wrong nationalization, of course.

Screenshots

Any Assumptions / Hacks

@philcampeau philcampeau added pins PR that will require some games to be pinned 18SJ labels Sep 11, 2024
@philcampeau philcampeau reopened this Sep 11, 2024
Copy link
Collaborator

@perwestling perwestling left a comment

Choose a reason for hiding this comment

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

Looks good.

lib/engine/game/g_18_sj/game.rb Outdated Show resolved Hide resolved
lib/engine/game/g_18_sj/game.rb Outdated Show resolved Hide resolved
@ollybh ollybh merged commit 54f5118 into tobymao:master Oct 21, 2024
1 check passed
@philcampeau philcampeau deleted the 18sj-nationalization-fix branch October 28, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
18SJ pins PR that will require some games to be pinned
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants