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 17 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.

@BlueChickenBoardGames
Copy link
Author

I saw that the britain map was merged to master. So i will try to take the parlement round changes from there and fix the merg conflicts here. Hopefully I have time early next week.

@BlueChickenBoardGames
Copy link
Author

BlueChickenBoardGames commented Oct 11, 2024

I merged master (with general Parliament round as introduced in Britain map) into this branch.

However, something seems off (as I get an error when I try to test using port 9292). I am investigating errors:
ERROR -- : ["/18xx/lib/assets.rb:34:in read'", "/18xx/lib/assets.rb:34:in extend'", "/18xx/lib/assets.rb:204:in `block (2 levels) in combine
ERROR -- : No such file or directory @ rb_sysopen - public/assets/opal.js.map

@roseundy
Copy link
Collaborator

How are you handling Ian's rule that issuing shares during EMR drops the price once per share issued (instead of the default for System18 of only a single drop)?

@ianwilson156
Copy link

In effect, the share price adjustments in this map are the same as a full-cap version i.e. shares always drop one space per share sold.

@ollybh
Copy link
Collaborator

ollybh commented Oct 24, 2024

I merged master (with general Parliament round as introduced in Britain map) into this branch.

However, something seems off (as I get an error when I try to test using port 9292). I am investigating errors: ERROR -- : ["/18xx/lib/assets.rb:34:in read'", "/18xx/lib/assets.rb:34:in extend'", "/18xx/lib/assets.rb:204:in `block (2 levels) in combine ERROR -- : No such file or directory @ rb_sysopen - public/assets/opal.js.map

In case you haven't already resolved the error, this was caused by the changes to the build system introduced by #11042. Running make clean and then make again should fix this.

@BlueChickenBoardGames
Copy link
Author

Life has been busy lately and progress here slow.

@ollybh Thanks a lot, that indeed solved the issue.

So, the merge with the more general Parliament round seems successful

@roseundy Thanks for asking! I just checked it and I am afraid it doesn't work as intended. Issuing shares during normal OR should work more or less the same as during EMR: as Ian mentioned, for each share issued the price marker should move one step down on the 2d market. During normal OR this works, but during EMR it doesn't. I will look into that. Did you encounter this for another System18 map?

@roseundy
Copy link
Collaborator

Scott's default EMR rules for System18 are that prices only drop once no matter how many shares are sold, and that's what I have coded. Ian's rules are different, which is why I asked.

@scottredracecar
Copy link
Collaborator

scottredracecar commented Oct 27, 2024 via email

@ianwilson156
Copy link

Indeed - Italy is a mixture of full-cap and partial-cap rules. So: you may issue shares in EMR (as partial-cap) but the price drops per share (as full-cap in SRs).

@BlueChickenBoardGames
Copy link
Author

Thanks for your comments all!

Makes sense. I naively assumed that if the normal issuing would work, the EMR issuing would also work, but they don't use the same code. I will look into it, but I am afraid I won't have time this week. Hopefully next week.

@BlueChickenBoardGames
Copy link
Author

BlueChickenBoardGames commented Nov 6, 2024

I think I found a solution for "issuing shares during EMR drops the price once per share issued" and committed it. Hope it makes sense. Thanks again for spotting this issue!

@@ -12,7 +12,7 @@ class Dividend < Engine::Step::Dividend
include Engine::Step::HalfPay

def share_price_change(entity, revenue = 0)
return super unless @game.game_capitalization == :incremental
return super if @game.share_price_change_as_full_cap_by_map?
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be conflating price changes on issuing shares with price changes on dividends. I'd prefer to see separate game methods for each "control".

Choose a reason for hiding this comment

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

Good point, it is cleaner to have these "controls" separate.

I committed an improved version (using a slightly different approach) that separates these controls.

Thank you!

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
6 participants