Improving efficiency and future proofing command line argument parsing in main.cpp

[6nodder6]

The command line parsing logic in Main::setup (main.cpp) has been worked on over the course of a decade. This has led to a few sections of code that could be improved to enhance maintainability. This would prevent accidental introduction of bugs or other unexpected behavior.

For example:

• In one code section there are three separate if statements that must be evaluated separately (despite string's value not changing between them). If unrelated code was inserted between the if statements it could lead to unexpected behavior that could be annoying to debug. It would be beneficial for both readability and efficiency to change this into a single control structure.
• In another code section the variable accessibility_mode_set is set to true in each branch of the if statement. This could be simplified by setting it to true once and overriding it if needed.

I propose refactoring these specific sections slightly to fix the issues above. I'd be happy to work on a PR if people think these changes are a good idea.

[ArchercatNEO]

Maybe we could use something like ServerBuilders? Instead of setup parsing most of the arguments, we have a dedicated class responsible for consuming useful arguments and keeping them around. Then after all arguments have been properly dealt with we call builder->build() to actually do something with what it's parsed.

Here's an example for the quite simple AudioServer (DisplayServer and RenderingServer have way more going on).
Instead of constantly looking back for whether we're headless or not in main we can just set headless on and eventually once we actually build the server we can do everything at once. Unfortunetly, I don't actually get any audio when using this but it's the idea that counts.

#include "audio_server_builder.h"

#include "core/config/engine.h"
#include "core/config/project_settings.h"
#include "core/error/error_macros.h"
#include "main/main.h"
#include "servers/audio_server.h"

AudioServer *AudioServerBuilder::build() {
	OS::get_singleton()->benchmark_begin_measure("Servers", "Audio");

	// If we haven't already found the driver do so now
	if (headless) {
		audio_driver_index = AudioDriverManager::get_driver_count() - 1;
	} else if (audio_driver.is_empty()) {
		String default_audio_driver = GLOBAL_GET("audio/driver/driver");
		for (int i = 0; i < AudioDriverManager::get_driver_count(); i++) {
			if (audio_driver == AudioDriverManager::get_driver(i)->get_name()) {
				audio_driver_index = i;
				break;
			}
		}
	}

	AudioDriverManager::initialize(0);
	AudioServer *server = memnew(AudioServer);
	server->init();

	if (audio_output_latency >= 1) {
		Engine::get_singleton()->set_audio_output_latency(audio_output_latency);
	} else {
		Engine::get_singleton()->set_audio_output_latency(GLOBAL_GET("audio/driver/output_latency"));
	}

	//server->set_use_threads(threaded);
	server->set_debug_mute(debug_mute_audio);

	OS::get_singleton()->benchmark_end_measure("Servers", "Audio");
	return server;
}

int AudioServerBuilder::parse_args(List<String>::Element *args) {
	String arg = args->get();
	List<String>::Element *next = args->next();

	if (arg == "--audio-driver") {
		if (!next) {
			ERR_PRINT("Missing argument for `--audio-driver`");
			return 1;
		}

		audio_driver = next->get();
		for (int i = 0; i < AudioDriverManager::get_driver_count(); i++) {
			if (audio_driver == AudioDriverManager::get_driver(i)->get_name()) {
				audio_driver_index = i;
				return 2;
			}
		}

		audio_driver = "";
		String msg = vformat("Unknown audio driver %s. Known audio drivers [", audio_driver);
		for (int i = 0; i < AudioDriverManager::get_driver_count(); i++) {
			msg += vformat("%s,", AudioDriverManager::get_driver(i)->get_name());
		}
		msg += "]";

		ERR_PRINT(msg);
		return 1;
	} else if (arg == "--audio-output-latency") {
		if (!next) {
			ERR_PRINT("Missing argument for `--audio-output-latency`");
			return 1;
		}

		audio_output_latency = next->get().to_int();
		return 2;
	}

#if defined(TOOLS_ENABLED)
	if (arg == "--debug-mute-audio") {
		debug_mute_audio = true;
		return 1;
	}
#endif

	return 0;
}

void AudioServerBuilder::print_help() {
	OS::get_singleton()->print("\n--AUDIO OPTIONS--\n\n");

	Main::print_help_option("--audio-driver <driver>", "Audio driver [");
	for (int i = 0; i < AudioDriverManager::get_driver_count(); i++) {
		if (i > 0) {
			OS::get_singleton()->print(", ");
		}
		OS::get_singleton()->print("\"%s\"", AudioDriverManager::get_driver(i)->get_name());
	}
	OS::get_singleton()->print("].\n");


	Main::print_help_option("--audio-output-latency <ms>", "Override audio output latency in milliseconds (default is 15 ms).\n");
	OS::get_singleton()->print("Lower values make sound playback more reactive but increase CPU usage, and may result in audio cracking if the CPU can't keep up.\n");

	OS::get_singleton()->print("\n--AUDIO OPTIONS--\n\n");
}

void AudioServerBuilder::set_headless() {
	headless = true;
}

void AudioServerBuilder::set_threads(bool enable) {
	threaded = enable;
}

[Calinou]

There are some good arguments for switching to a third-party library to handle CLI arguments (e.g. shell completion files could be generated automatically by Godot itself; they are hand-written right now). However, this would also add more complexity and increase binary size somewhat.

[6nodder6]

That makes sense. Do you think it'd be reasonable to fix these two problems now as another solution is considered?

[Calinou]

Pull request opened:

• Improve logic for delta smoothing and accessibility cmd line options godot#107106