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

System18 northern italy #11216

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

BlueChickenBoardGames
Copy link

@BlueChickenBoardGames BlueChickenBoardGames commented Sep 12, 2024

Fixes #11215 (System18: Add map Northern Italy by Ian D. Wilson)

This my first PR and first time working with ruby and github (and codespaces), so please be kind and give feedback, I am trying to learn!

Before clicking "Create"

  • Branch is derived from the latest master -- Yes, I think so
  • Add the pins or archive_alpha_games label if this change will break existing games -- I do not know, hopefully not
  • Code passes linter with docker compose exec rack rubocop -a -- I did managed to run this command with help of philcampeau and committed the fixed issues
  • Tests pass cleanly with docker compose exec rack rake -- I did managed to run this command with help of philcampeau and it found no issues

Implementation Notes

I used the Poland map as an example. I tried to keep it consistent and minimize the changes to the system.

Bankruptcy is handled as in a standard System18 Incremental cap game. I.e. the second and third sentence of the rule
"If a player goes bankrupt, they drop out of the game with a score of zero, and the game continues. Their remaining assets are sold to the Pool (in this special case, presidencies and over 50% can be in the Pool); this money is given to the company in EMR and must be used to buy a train if possible. If bankruptcy causes another player to become director of the company in EMR, the new director must carry on with the EMR process."
are not implemented yet.

During programming I used quite some copy-paste and trail-and-error to get things to work. Testing revealed some issues that then later solved. I made several commits, but unfortunately not all commits are clean/clear parts of the implementation.

Explanation of Change

Map and tiles are as https://boardgamegeek.com/thread/3238633/article/44685771#44685771
Note that the region names are shown within brackets (as in the Poland map) and part of the border between Tuscana and Emilia-Romagra is hidden behind edge-mountain-costs.

Trains, tiles, companies, phases, etc. are as described in the issue.

The code for the regions is the same as in the Poland map.

The game is partial/incremental cap, but uses the 2D stockmarket and full cap share movement rules. For this I had to make some small changes to the system code. Please check if it makes sense and if this might influence other maps.

Parliament Round (and SR) works the same as the Poland map, so I copied some of that code and files (and renamed the copies). Not sure if there is a cleaner way to do this.

As a last action in an OR a company may issue shares. I copied code from 18Texas to get Share Issueing to work. I added this (partly) the System18 code, so it could also be used by other maps. Please check if that makes sense.

Screenshots

CaptureMapNorthernItaly

Any Assumptions / Hacks

none

@roseundy
Copy link
Collaborator

My pending Britain map generalizes the parliament round. This PR should hold off and merge with my new code.

@BlueChickenBoardGames
Copy link
Author

My pending Britain map generalizes the parliament round. This PR should hold off and merge with my new code.

I had a quick look at your change and it seems to make a lot of sense, would be great if we could also use it here.

I also added a screenshot of the map above

@ianwilson156
Copy link

Nice map! I've just spotted a few spelling mitsakes:
C12 should be Padova & Venezia
D7 should be (Emilia-Romagna)
F11 should be (Toscana) - my error, it's Tuscany in English.

@philcampeau
Copy link
Collaborator

I loaded this onto my local machine and ran Rubocop, and it identified quite a few spacing errors. You're going to want to figure out how to run that and push any changes it makes.

From what I've seen, you're doing this in Codespaces. This should be fairly straightforward. Assuming the Codespace is using Visual Studio Code (I think they all do), open a terminal window (View > Terminal) if it's not open already. In there, launch the site code by entering the command make. Then open a new terminal window and enter the command docker compose exec rack rubocop -a. The -a part will auto-correct any style errors it identifies.

The errors I saw were all minor. Spacing, indentation, etc. It will list out all the errors it found and the changes made in the terminal.

@BlueChickenBoardGames
Copy link
Author

Thanks Ian and Phil!

This helps a lot! The step that I missed (when I was trying to run those docker commands) was opening a second terminal... Cool that it fixes the issues for me!

I will try to make a commit to this PR with the fixed docker issues and fixed spelling mistakes.

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.

System18: Add map Northern Italy by Ian D. Wilson
5 participants