TShock icon indicating copy to clipboard operation
TShock copied to clipboard

Command line conflicts with config file precedence

Open kubedzero opened this issue 4 years ago • 5 comments

  • TShock version: 4.4.0.0 pre-3 vanilla running on Windows 10 1607

Reproduction steps

  1. Start server using TerrariaServer.exe
  2. Choose a world, choose any player count other than the default 8 (even though it says the default is 16)
  3. go through the rest of the steps and wait for the world to come online
  4. The title bar of the CMD window will always say x/8 players, even though the default is apparently 16 when going through the max player option and you can set it to any number (I tried 2, 10, 30, it didn't seem to matter and would always report 8)

Any screenshots?

  • Initial loading screen showing Terraria API version and stuff 2020-05-19 22_22_17-10 1 1 9 - Remote Desktop Connection

  • Changing the player count to something other than 16 (the supposed default) or 8 (the reported count) 2020-05-19 22_22_35-10 1 1 9 - Remote Desktop Connection

  • Changing the port to something other than the default 2020-05-19 22_22_49-10 1 1 9 - Remote Desktop Connection

  • This is the important screenshot showing the title bar as 0/8 even though we set 2 player max before 2020-05-19 22_23_08-10 1 1 9 - Remote Desktop Connection

I realize this is a UI only/visual bug, but it might indicate deeper problems; I'm unable to test with more than one client at the moment and am unable to verify if the default of 8 ever changes.

kubedzero avatar May 20 '20 05:05 kubedzero

This is down to a bit of a mess of configuration options. There are 3 ways to configure max slots:

  • CLI argument (eg, launch with terrariaserver.exe -maxplayers 16)
  • Config (MaxSlots: 8 by default)
  • Console input (Max players (press enter for 16):)

Currently the precedence order is CLI > Config > Input. This means that the input argument will always be ignored while TShock is running. If we change precedence order to CLI > Input > Config then the Config will always be ignored unless enough CLI arguments are provided to skip input.

I will discuss with the team and determine how we should go about adjusting the precedence order. Thanks for raising the issue :)

QuiCM avatar May 22 '20 05:05 QuiCM

Thanks for the information! I suppose it made for a confusing user experience because some in-window-based commands work (the port number, for instance) while others (max player count) don't seem to take effect, even though both are supplied in the default JSON file.

If I imagine a perfect world, I'd love to see executing TerrariaServer.exe with optional arguments (such as port, world path, player count, etc) skip all the interactive inputs for which we already provided config values. Then for any remaining parameters necessary to start the server, we'd pull from the interactive input and then the JSON config.

As another scenario building on this plan, if no optional args are passed in when TerrariaServer.exe is started, then we rely first on interactive input and then on the JSON config.

In implementation, I'd imagine this to look the following way (say for maxPlayerCount)

effectiveMaxPlayerCount = 8; //hardcoded fallback value if no json provided? or it could error out
if (json.contains(jsonMaxPlayerCount) {
  effectiveMaxPlayerCount = jsonMaxPlayerCount;
}

if (args.contains(maxPlayerCount)) {
  effectiveMaxPlayerCount = args.get(maxPlayerCount);
  skipInteractiveMaxPlayerCountPrompt();
} else {
  interactiveMaxPlayerCount = getMaxPlayerCountFromInteractivePrompt();
 if (interactiveMaxPlayerCount != null) {
    effectiveMaxPlayerCount = interactiveMaxPlayerCount;
 }
}
return effectiveMaxPlayerCount;

With this, we'd set a hardcoded ultimate fallback first. The we'd update it with whatever we got from the JSON, if anything (lowest priority, so to speak). Then we'd update it with the optional arg passed in when opening the executable OR update it with the interactive prompt.

That way the optional arg takes precedence, followed by interactive input, followed by JSON, followed by a hardcoded fallback (or an error message saying it must be defined in JSON?)

Sorry, I don't want to impose and force a certain implementation! I greatly appreciate all the hard work put in by the Pryaxis team and want to assist in any way I can.

Cheers!

kubedzero avatar May 22 '20 06:05 kubedzero

I propose just ripping all of this out of the config. CLI is standard practice and you can even set ENV variables or other things when executing. The config/CLI/interactive order of operations is honestly nonsensical. If a user defines something on the CLI, I would expect that to be superseded by following interactive input. A user can change their mind between pressing enter and the next prompt. The config file is the first thing set, therefore the last priority. But that means it's always masked.

We could add a whole bunch of things like --skip-interactive or --prefer-config but I would rather we just ditch the config and let people use the CLI.

Fundamentally the TShock config is an operating layer config, not a startup config. The config file is mostly intended to change parameters about the game world or specific subsystem behaviors. The fact that TShock even makes any attempt to reconcile the server's port is a gross violation of the fact that TShock is a plugin on TSAPI. TSAPI inits TShock after it starts execution of the program. Just because we can continue to do this doesn't mean we should.

Let's eliminate the config options in the config file.

hakusaro avatar May 22 '20 07:05 hakusaro

Thanks for the response! My only concern with that is that some users may prefer the all-in-one TerrariaServer.exe --config=path/to/config.json command to start their servers, especially if they run many simultaneous instances. Having some options only configurable via optional arguments while others only configurable via the JSON could be misleading.

Just to confirm, what options would then be made CLI/interactive only? Looking through the config file, there would be a decent amount to move out that isn't relevant to the game world or subsystem behaviors? All the REST configs, the SQLite db path, mysql DB/port/username, server port and password, maxSlots, etc.

Also, I think the current defaults chosen in the interactive window are pulled from the config. When I updated the maxSlots in the JSON from 16 to 12, it showed up in the interactive window Max Players (press enter for 12) Moving stuff out of the config would mean hardcoding it somewhere for that default to still work, right?

kubedzero avatar May 22 '20 17:05 kubedzero

The only things that would be moved would be part of the interactive setup. I don’t mind breaking people’s workflows because the current design is confusing and makes no sense.

hakusaro avatar May 22 '20 18:05 hakusaro