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

Feature #4115: default company colour setting #6998

Merged
merged 1 commit into from Jan 10, 2019
Merged

Conversation

GabdaZM
Copy link
Contributor

@GabdaZM GabdaZM commented Dec 29, 2018

Works only in single player.

Copy link
Contributor

@nielsmh nielsmh left a comment

The commit message should be "Add #4115: Default company colour setting", there isn't any "Feature" keyword.

src/openttd.cpp Outdated Show resolved Hide resolved
src/table/settings.ini Outdated Show resolved Hide resolved
@nielsmh
Copy link
Contributor

@nielsmh nielsmh commented Jan 6, 2019

Seems to work as intended, also in multiplayer games. In multiplayer, if two players have the same preferred company colour, the second to create their company just gets assigned a different colour.

Tentatively approving this, but I'd like someone else to review the UI placement and strings too, I'm not entirely sure I like it.

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Jan 6, 2019

I'll review the UI and strings.

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Jan 6, 2019

UI is fine.

Strings, we could argue back and forth about 'Starting company colour' vs 'Default company colour'.

I think 'Default company colour' is a little better, but it's pretty much potato / potato.

Copy link
Contributor

@andythenorth andythenorth left a comment

+1 to this, the string can always be changed later as a single-line commit. Thanks

@@ -1258,6 +1259,9 @@ STR_CONFIG_SETTING_DYNAMIC_ENGINES_EXISTING_VEHICLES :{WHITE}Changing
STR_CONFIG_SETTING_INFRASTRUCTURE_MAINTENANCE :Infrastructure maintenance: {STRING2}
STR_CONFIG_SETTING_INFRASTRUCTURE_MAINTENANCE_HELPTEXT :When enabled, infrastructure causes maintenance costs. The cost grows over-proportional with the network size, thus affecting bigger companies more than smaller ones

STR_CONFIG_SETTING_COMPANY_STARTING_COLOUR :Starting company colour: {STRING2}
Copy link
Contributor

@planetmaker planetmaker Jan 9, 2019

Choose a reason for hiding this comment

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

My comment feels a bit like bike shedding, yet: AI also start with a company colour. Maybe "Default company colour" with a help text of "Choose a company colour the player starts with by default" would make it ever so slightly clearer?

Copy link
Contributor Author

@GabdaZM GabdaZM Jan 10, 2019

Choose a reason for hiding this comment

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

Yes, it sound better, I will change it. It is always hard to make a short and clear description, so thanks for the help.

@GabdaZM
Copy link
Contributor Author

@GabdaZM GabdaZM commented Jan 10, 2019

Seems to work as intended, also in multiplayer games. In multiplayer, if two players have the same preferred company colour, the second to create their company just gets assigned a different colour.

In this case I think I should put in a check before assigning the colour, as I don't know at what point the color changing fails, and some variable values might be different for server and client.
Is it possible that in single player an AI company start earlier than the player company?

@planetmaker
Copy link
Contributor

@planetmaker planetmaker commented Jan 10, 2019

Seems to work as intended, also in multiplayer games. In multiplayer, if two players have the same preferred company colour, the second to create their company just gets assigned a different colour.

I don't see a problem there. The 2nd player simply gets another colour... whichever of two that is. In multiplayer the order of who is first will be determine by the server and is unique.

Is it possible that in single player an AI company start earlier than the player company?

I don't think so. There always is first a player company.

@planetmaker planetmaker merged commit a0293d3 into OpenTTD:master Jan 10, 2019
1 check passed
@planetmaker
Copy link
Contributor

@planetmaker planetmaker commented Jan 10, 2019

Nvm my comment from yesterday. Andy is right - we can always change the wording of strings later if we feel like it.

@GabdaZM GabdaZM deleted the colour branch Feb 2, 2019
nielsmh pushed a commit to nielsmh/OpenTTD that referenced this issue Mar 11, 2019
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.

None yet

4 participants