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

Add: Client setting gui.start_spectator #7158

Closed
wants to merge 1 commit into from

Conversation

SamuXarick
Copy link
Contributor

Activates a spectator slot in single player and initiate games as a spectator

@@ -67,8 +67,8 @@ static int32 ClickMoneyCheat(int32 p1, int32 p2)
*/
static int32 ClickChangeCompanyCheat(int32 p1, int32 p2)
{
while ((uint)p1 < Company::GetPoolSize()) {
if (Company::IsValidID((CompanyID)p1)) {
while ((uint)p1 <= COMPANY_SPECTATOR) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's wrong, you should still check Company::GetPoolSize(), but you can add something to the test

src/company_gui.cpp Outdated Show resolved Hide resolved
src/company_gui.cpp Outdated Show resolved Hide resolved
src/company_gui.cpp Outdated Show resolved Hide resolved
src/network/network.cpp Outdated Show resolved Hide resolved
src/openttd.cpp Outdated Show resolved Hide resolved
} else {
*list->Append() = new DropDownListStringItem(STR_NETWORK_TOOLBAR_LIST_SPECTATOR, CTMN_SPECTATOR, false);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

that's a lot of copy pasting, but maybe no other way

@SamuXarick SamuXarick force-pushed the start-as-spectator branch 2 times, most recently from af2ed56 to 02f3bda Compare February 2, 2019 03:38
plane = CWP_MP_C_PWD;
} else {
plane = CWP_MP_C_JOIN;
}
Copy link
Member

Choose a reason for hiding this comment

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

Excessive expansion, plane = local ? CWP_MP_C_PWD : CWP_MP_C_JOIN; would be fine, and similar for the block above.

Copy link
Contributor

@glx22 glx22 left a comment

Choose a reason for hiding this comment

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

I think you should not touch economy.cpp and bankruptcy handling

src/company_gui.cpp Outdated Show resolved Hide resolved
src/toolbar_gui.cpp Outdated Show resolved Hide resolved
@SamuXarick SamuXarick force-pushed the start-as-spectator branch 4 times, most recently from 43ee27c to bb1c94d Compare February 8, 2019 03:21
@SamuXarick SamuXarick force-pushed the start-as-spectator branch 4 times, most recently from 5e94b8c to 9ec1d48 Compare February 17, 2019 01:12
Activates a spectator slot in single player and initiate games as a spectator
@TrueBrain
Copy link
Member

I fail to see the usecase for this behavior. Why would I want to be a spectator in a SinglePlayer game? Feels like an anti-pattern. And given the PR does not argue why it should be added, going to close the PR.

From a short chat on IRC, I understand it is to watch the AI do its thing. I don't see how it is useful to change so many lines of code for a very low usercount that will ever use it that way. So if that is the argument, I am still going to close the PR.

Sorry, not every feature can make it into OpenTTD :) But thank you for your contribution nevertheless!

Feel free to rebuttal. But at always, do this with new arguments. Tnx!

@TrueBrain TrueBrain closed this Mar 2, 2019
@stale
Copy link

stale bot commented Mar 20, 2019

This pull request has been automatically marked as stale because it has not had any activity in the last month.
Please feel free to give a status update now, ping for review, or re-open when it's ready.
It will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale Stale issues label Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants