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

Added widget showing company slots remaining #10054

Closed
wants to merge 1 commit into from
Closed

Added widget showing company slots remaining #10054

wants to merge 1 commit into from

Conversation

zachtyson
Copy link
Contributor

@zachtyson zachtyson commented Sep 26, 2022

Motivation / Problem

I've seen a few new players join and try to create a company, not realizing that the game has reached the maximum amount of companies, e.g #9703 , which is easier to see at a glance.

Description

Adds a single line above company/client count to display the amount of remaining companies that can be created.

Limitations

The addition simply shows the amount of existing companies minus the max amount of companies. Can be changed from '0 company slots' to 'FULL' if desired

Closes #9703 - edit 2TT

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@James103
Copy link
Contributor

James103 commented Sep 26, 2022

In the lines you added, the code must be indented with tabs instead of spaces.
Commit checker will fail until you resolve this.

Also, the commit title must be changed to read as follows: Add: Widget showing company slots remaining

@LordAro
Copy link
Member

LordAro commented Oct 3, 2022

Seems reasonable. Just a few things:

  • Can you add a screenshot of what it looks like?
  • "Company slots" feels like odd English to me... not that I can think of anything better at the moment. I guess "Companies remaining" is too long? "Open slots" "Unused companies" ? IDK
  • Don't modify any language files other than english.txt - everything else is exclusively handled via our translator teams (even if it's identical!)
  • We like rebasing rather than merge commits :) (As admins we can forcibly squash PRs into a single commit and fix it on merge, but it's much easier to deal with)

@zachtyson zachtyson closed this Oct 3, 2022
@zachtyson
Copy link
Contributor Author

zachtyson commented Oct 3, 2022

Untitled

Gotcha, I changed it from company slots to open slots also

@zachtyson zachtyson reopened this Oct 3, 2022
@Bouke
Copy link
Contributor

Bouke commented Oct 3, 2022

The server list is using x/y notation (x used, y available):

image
image

Maybe we should use the same here for consistency, instead of introducing a new concept "open slot(s)".

@James103
Copy link
Contributor

James103 commented Oct 3, 2022

If it is desired to keep it as one line only at the bottom, it could say something like "3 / 100 clients - 5 / 15 companies"

@LordAro
Copy link
Member

LordAro commented Oct 3, 2022

Mm, does seem reasonable to keep it consistent. Total possible number of clients seems redundant when you're already connected to the server, but again, consistent.

@zachtyson
Copy link
Contributor Author

Yea I get the need for consistency, I can show a preview of those proposed changes if y'all want once I get off work

@2TallTyler 2TallTyler added size: trivial This Pull Request is trivial work: minor details This Pull Request has some minor details left to do labels Oct 23, 2022
@LC-Zorg
Copy link

LC-Zorg commented Oct 23, 2022

In my opinion and for me the problem is a bit that the bar and the create a new company button disappears. For example, it will disappear if the list is long and you scroll down. The same is true when it is not possible to start a company because all available slots have been taken. Honestly, I still haven't got used to it until today.

I like the idea of ​​Bouke and James103. It takes up less space, is equally pithy and consistent with what is in the server list window.

I would also have some idea what else can be changed in this or other PR.

  1. If the number of companies reaches the maximum number, the number is red - this is more noticeable and I think it is understandable.
  2. The button for establishing a new company can be moved to the bottom of the window (I have already proposed something like that before - here).
  • When there is free space, the button is active
  • When there is no free space, the button is still visible but inactive and shaded
  • When the player is in some company, the bar with the create company button is collapsed
    Numbers of clients and companies v1 0

By the way, I would like to remind about the problem of the inability to reduce the width of this window, which is almost always much wider than it should (#9697). I mention it, if only so that this PR doesn't somehow reinforce this bug.

@2TallTyler
Copy link
Member

@zachtyson Are you still interested in working on this? Would be nice to have in the upcoming OpenTTD 13 release. 🙂

@@ -1785,6 +1786,10 @@ struct NetworkClientListWindow : Window {
break;
}

case WID_CL_CLIENT_COMPANY_REMAINING:
SetDParam(0, _settings_client.network.max_companies - Company::GetNumItems());
Copy link
Contributor

Choose a reason for hiding this comment

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

This setting does not get updated from the server. So, it will not be right when joining the server and whenever the server changes it won't be right. Unless "accidentally" the configuration value at the server happens to be the same as for your client, which is quite likely when testing it locally.

I would suggest you look at NetworkMaxCompaniesReached and add a similar function at those locations that returns the number of companies that are remaining. I would then also suggest changing NetworkMaxCompaniesReached so it uses that new function and compares that count with 0. That way the logic that chooses between the variables containing the maximum number of companies does not get duplicated and if the logic requires changes it is less likely that the other function is missed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: trivial This Pull Request is trivial work: minor details This Pull Request has some minor details left to do
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: No indication that server is full within the game
7 participants