-
Notifications
You must be signed in to change notification settings - Fork 186
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
base: master
Are you sure you want to change the base?
System18 northern italy #11216
Conversation
…nBoardGames/18xx into System18NorthernItaly
…ull cap, share issueing
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 |
Nice map! I've just spotted a few spelling mitsakes: |
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 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. |
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. |
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. |
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: |
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)? |
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. |
In case you haven't already resolved the error, this was caused by the changes to the build system introduced by #11042. Running |
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? |
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. |
I think the special rule here for EMR sales is to simulate how it works in
1841 where the companies lose a lot of value by issuing shares.
…On Sat, Oct 26, 2024 at 7:21 PM Steve ***@***.***> wrote:
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.
—
Reply to this email directly, view it on GitHub
<#11216 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AG6CEXBLCLZJBKE2V5IKPZDZ5RE2BAVCNFSM6AAAAABOEDQZXOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZZHAYDQMRRHA>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
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). |
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. |
…ce per share issued
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? |
There was a problem hiding this comment.
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".
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"
master
-- Yes, I think sopins
orarchive_alpha_games
label if this change will break existing games -- I do not know, hopefully notdocker compose exec rack rubocop -a
-- I did managed to run this command with help of philcampeau and committed the fixed issuesdocker compose exec rack rake
-- I did managed to run this command with help of philcampeau and it found no issuesImplementation 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
Any Assumptions / Hacks
none