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

Change: Add separate setting for server sent commands per frame limit #11023

Merged
merged 1 commit into from Jun 27, 2023

Conversation

JGRennison
Copy link
Contributor

Motivation / Problem

This is the follow-on to #10913.

Currently the server and all clients are limited to 2 commands per frame by default.
However a server may be running a GS, multiple AIs, and/or have an attached user.
The GS may use GSAsyncMode from #10913.
2 commands per frame is therefore unnecessarily limiting for the server, which is in a privileged position anyway.

Server operators can't be expected to know what to do with any of these types of settings, so the defaults should be reasonable and they shouldn't need to try to change the existing commands_per_frame setting.

It is not necessary to change the commands per frame limit for clients.

Description

Add separate setting for server sent commands per frame limit

Set a higher default value for this setting.
Use the higher of this and existing commands per frame limit setting for server-originating commands.

Limitations

N/A

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')

Set a higher default value for this setting.
Use the higher of this and existing commands per frame limit
setting for server-originating commands, e.g. GS.

This is to support the GSAsyncMode class.
This also avoids undue throttling when more than one
script is in operation (e.g. AIs).
var = network.commands_per_frame_server
type = SLE_UINT16
flags = SF_NOT_IN_SAVE | SF_NO_NETWORK_SYNC | SF_NETWORK_ONLY
def = 16
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the 16 based on? If a game script need to manage a few hundred/thousand towns/industries, will that be enough?
1024/16 = 64 => ~1 game day.

Alternatively, is a 16x improvement over 13.x behaviour enough? Or is that still too limiting?
Should the server maybe not have any limits, but only for game scripts?

Or it 16x a nice middle ground for some performance improvements of game scripts without immediately overwhelming? And is it up to the server owners to (occasionally) set this setting (much) higher for specific game scripts on especially large maps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an arbitrary value which is a compromise between allowing a larger throughput of commands from the server and not allowing a misbehaving game script to cause the network/game overall to grind to a halt.

Making it a setting avoids excessive bikeshed problems whilst providing better default behavior than currently.

@PeterN PeterN merged commit 55c07ee into OpenTTD:master Jun 27, 2023
18 checks passed
mrmbernardi pushed a commit to mrmbernardi/OpenTTD that referenced this pull request Jul 2, 2023
…OpenTTD#11023)

Set a higher default value for this setting.
Use the higher of this and existing commands per frame limit
setting for server-originating commands, e.g. GS.

This is to support the GSAsyncMode class.
This also avoids undue throttling when more than one
script is in operation (e.g. AIs).
@JGRennison JGRennison deleted the server-commands-per-frame branch January 9, 2024 19:25
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

3 participants