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

[18ESP] fix a few minor bugs #11393

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

Conversation

bentziaxl
Copy link
Collaborator

@bentziaxl bentziaxl commented Dec 2, 2024

transfer all but one tender to the bank when buying p5.
fix broken game when buying p5
fix having two tokens in the same city when merging major with minor

Fixes #11337

token = corporation.tokens.first
hex = hex_by_id(corporation.coordinates)
city = hex.tile.cities.size > 1 ? city_by_id("#{hex.tile.id}-#{corporation.city}") : hex.tile.cities.first
city = if corporation_by_id('MZ')&.ipoed
# mza special case if mz already exists on the map
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general I think the corporation abbreviations in texts and comments should use upper case, so MZA and MZ in this case. Same comment for line 886.

@@ -90,10 +90,14 @@ def choices
end

def can_swap?
Copy link
Collaborator

Choose a reason for hiding this comment

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

An alternative implementation could be to move line 95-97 plus the unless into a method that returns true/false.
And then
return true if is_some_method

followed by merged_token_in_shared_city? content.
Not sure which is most readable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i think this is readable as it is, i don't think refactoring will make it more readable


transfer_amount = amount - 1
@luxury_carriages['bank'] += transfer_amount
@luxury_carriages[owner] = 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this correct? I do not know the code so I might be wrong. Feels strange to set this to one here. Just checking.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, there is a private that comes with 5 tenders, but tenders can end up in the "bank"
other corps can buy tenders either from player owner of the private or the bank.
If private is bought in, all the tenders are transferred the the bank, except one which is owned by the corp buying the private in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

18ESP allows two tokens in the same city.
2 participants