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

[1894] Fix a bug allowing for upgradin a green city that you don't have access to #11408

Merged
merged 6 commits into from
Dec 20, 2024

Conversation

Galatolol
Copy link
Contributor

@Galatolol Galatolol commented Dec 17, 2024

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 a corporation can upgrade a green city tile it is next to but doesn't have access to the city (like Ouest on the first screenshot). It shouldn't be possible, it first has to connect to the city.

⚠️ This requires pinning ⚠️

Screenshots

1
2

@ollybh ollybh added pins PR that will require some games to be pinned 1894 labels Dec 19, 2024
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.

This isn't the correct way to do this. The player shouldn't be offered the option to place those tiles if the corporation does not have access to any paths on the placed tile.

1894’s implementation of Step::Tracker::legal_tile_rotation? is, for brown city tiles, only checking whether the replacement tile has track connected to the same exits. It should also be checking that one of the paths on the tile is connected to an exit on the corporation’s graph.

This is the test in Engine::Step::Tracker::legal_tile_rotation? that you need to keep:

entity_reaches_a_new_exit = !(new_exits & hex_neighbors(entity, hex)).empty?
return false unless entity_reaches_a_new_exit

@Galatolol
Copy link
Contributor Author

Galatolol commented Dec 20, 2024

@ollybh Thank you for the suggestion, I agree that it's better to not offer upgrades at all.

Comment on lines 19 to 22
if @game.class::GREEN_CITY_TILES.include?(hex.tile.name)
entity_reaches_a_new_exit = !(tile.exits & hex_neighbors(entity, hex)).empty?
return false unless entity_reaches_a_new_exit
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

The logic here looks correct. The only thing I would change is to use a slightly more efficient way for testing for array intersection: arr1.intersect?(arr2) instead of (arr1 & arr2).empty?. You could also stick it as a single liner, but Rubocop might object to the line length.

Suggested change
if @game.class::GREEN_CITY_TILES.include?(hex.tile.name)
entity_reaches_a_new_exit = !(tile.exits & hex_neighbors(entity, hex)).empty?
return false unless entity_reaches_a_new_exit
end
return false if @game.class::GREEN_CITY_TILES.include?(hex.tile.name) && !tile.exits.intersect?(hex_neighbors(entity, hex))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Robocop accepts the "one-liner" divided in two:

image

@ollybh ollybh merged commit 52bf196 into tobymao:master Dec 20, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1894 pins PR that will require some games to be pinned
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants