[WIP] Add command parser

[lupoDharkael]

Initial work for #20000

Proposal: godotengine/godot-proposals#137

The parser is pretty simple. We have the class CommandFlag which represents a single commands and its properties (the code is self explanatory). The class CommandParser contains the list of CommandFlag and process the arguments in the parse method.
The method init_defaults adds all the commands of the engine.
With a centralised point with all the data of the commands the --help can be generated dynamically. This has some advantages such as proper alignment of descriptions based on the largest line, and even splitting the lines if they exceed the 80 characters.

Engine arguments and project arguments are separated by a -- as it was suggested in the issue:
godot --godot-command -- project-command 333 4

TODO:

• Implement a way to add CommandFlag checkers from gdscript.
• Method name cleanup.
• Decide about List vs Vector internal storage.
• Add class documentation.

[neikeq]

Didn't read the description, I guess this is still WIP. This is great any ways and it would be great if the main CommandParser could be publicly exposed to make it possible to add custom commands from modules (maybe with callback support). This would specially benefit the mono module, which has some hard-coded commands.

[lupoDharkael]

I'm adding a [WIP] to the title of the PR so it is more clear.
@neikeq I need to change all the copypasted parts like the ones you noticed. Godot has a ton of arguments and it was tedious creating every single one.
This PR is still very WIP. I just pushed it so it can receive some feedback.

[Calinou]

Nice work! 🙂

If that makes sense, would it be possible to expose the command-line parsing interface to GDScript so that it can be used directly in projects? I can see it being useful to quickly jump to a specific level in a game (or similar).

[lupoDharkael]

@Calinou I'll leave the potential GDScript API for a future issue so it can be debated. It would require some minor tweaks in the parser.

@neikeq having the modules register their commands would be ideal but I'm a little lost about how it could be done as it would require the registration to be ready early on Main::setup.
Maybe the register* functions like register_mono_types() could be used as the point where commands are defined.
The problem is, it would be good to notify when the parser has processed the command args so the modules can initialise values based on the results.
Any hints would be really helpful.

[neikeq]

Hmm, It's indeed not that simple since command line is parsed before calling register_module_types and I don't think moving the latter is a good idea, so it would require a custom callback for initializing command line options.
I wouldn't bother too much with it for now. It can be done in a new PR after thinking this well.

[lupoDharkael]

Now Godot suggests commands if you wrote them wrong:

[lupoDharkael]

I've exposed the argument check functions so they can be reused when the command registration in modules is implemented.

[lupoDharkael]

Ok so this is ready for testing. The get_cmdline_args thing still needs feedback. Once this is merged I'll start working on the PR so godot modules can register their commands in the parser.

[lupoDharkael]

@akien-mga I'm reworking this PR based on the feedback of the proposal and this issue and I'm adapting the code I didn't touch in the actual state of the PR (for example the usage of cmdline in the engine tests)

I have a doubt about a check condition you implemented:

godot/main/tests/test_math.cpp

Lines 489 to 496 in 5ca7460

	String test = cmdlargs.back()->get();
	if (test == "math") {
	// Not a file name but the test name, abort.
	// FIXME: This test is ugly as heck, needs fixing :)
	return NULL;
	}
	FileAccess *fa = FileAccess::open(test, FileAccess::READ);

What should that test file be? a gds script file? I don't think I'm understanding the usage of that test.

[lupoDharkael]

I also think this PR should be moved to the 4.0 milestone.

[akien-mga]

Yeah that's 4.0 material indeed, I just didn't want to change milestone before taking time to review any of the proposed changes... But time is scarce :)

I fully support the idea to refactor our command line interface as it's currently very limited and unforgiving. Haven't checked this PR's code yet so I can't comment on the implementation, but it's likely going in the right direction :)

What should that test file be? a gds script file? I don't think I'm understanding the usage of that test.

I didn't implement this test, I only fixed up some issues. I also have no clue about what kind of files is expected here, but I guess it used to be a custom GDScript file with some math stuff to evaluate.

These tests are also obsolete and will be replaced in the future, so I wouldn't bother too much about the --test interface in this PR. See #30743 for the likely replacement.

[lupoDharkael]

I think I'll need to change the cmdline from List<String> to Vector<String> so I can prevent creating an extra class to expose to gds. I don't think keeping it as List<String> makes sense anyways as the cmdline doesn't change and exposing them to gds requires copying them to a Vector<String> anyways.

[aaronfranke]

@lupoDharkael The majority of the core refactoring work done by the core devs is finished, so anytime soon would be a good time to rebase this.

[lupoDharkael]

I was waiting because I expected a lot of things to be changed not knowing when will this be considered to be reviewed more in depth.
I'm updating this soon™️ (it may take a while because I need to improve a few things, not only fix the conflicts).

[Anutrix]

Requesting rebase.

[lupoDharkael]

Oh I forgot about this one. I'll try to find time to resume this PR

[Shatur]

@lupoDharkael, are you still interested in it? Maybe some of us can just pick this PR and continue working on it?

[lupoDharkael]

I don't know if I'll be able to resume this work soon as I have university exams and other stuff going on. I wouldn't mind If someone would like to continue my work.

[Shatur]

No problem. Could you briefly provide a description of what is already done (architecture, maybe how it works is short) and what needs to be completed?

[lupoDharkael]

My first comment in this PR contains a very basic overview of the parser but I'll try to describe everything better.

core/cli_parser.h contains 2 classes CommandFlag and CommandParser.

CommandFlag

Every object of this type contains the information of a command.
I think almost every member variable is self explanatory and I have a few comments too.
The only thing that may create some confusion is the Vector<CommandChecker *> check_list;
which is a list of pointer to CommandChecker, if you look at the definition of CommandChecker you'll see it is just a function that validates arguments. So check_list is just a vector of functions to be called by the parser in order to validate the arguments. I thought about changing this to use functions exposed to ClassDB so you can define validation functions from gds too.

CommandParser

Here is a breakdown of the member variables of the class:

• Vector<Ref > _commands: the list of CommandFlag available for the parser.

• Vector _game_args: arguments found after a --

• Vector _args; arguments found before a --
  for example, with the arguments godot clean-the-house 3 -- do-the-dishes. _game_args would contain [do-the-dishes] and _args would contain [clean-the-house, 3].

• HashMap<String, String> _data_found: this is where we save the ocurrences of the command flags, the key is the command itself to see if it was wound in the parsed arguments. A key-value example would be <godot clean-the-house, 3> for the example arguments above.

• String _project_file: when we parse one of the first things the parser does is search of a project file as (when I wrote this PR) Godot checks the last argument to be a .godot or scene.

• String _help_header: the text shown before the list of flags in the --help.

• String _help_footer: the text shown after the list of flags in the --help.

• String _version: the text shown by --version.

• bool _separator_enabled: if this is true we split the arguments into _game_args and _args. When this is false all the arguments go to _args. The engine always process --, this can be disabled for the convenience of user defined parsers in gds.

• bool _search_project_file: when this is disabled the parser doesn't try to find a project file at the end of the arguments.

The parse()

This function is concise and I think this doesn't need very much explanation, it does the following:

• split the arguments between _game_args and _args.
• Try to find the project file if _search_project_file is true.
• process the arguments in _args, if it finds a non existent one it shows an error. If it finds a defined one checks if it needs an argument and validates the command with all its checkers. Save that command and argument in _data_found

Considerations

• I think now some godot commands can receive multiple arguments, in order to implement that CommandFlag should have a method int arguments_needed() const instead of bool needs_argument() const; and adapt the parse() method.

• To update this PR you need to check the date of the last commit in this PR and start implementing the changes in Godot's main.cpp from that date.
  Here's the list of commits in that file:
  https://github.com/godotengine/godot/commits/master/main/main.cpp

• Godot's main.cpp may difer a lot from this in the parts where the commands are processed, before the changes in this PR the command processing was very chaotic as it needed validation and processing of commands in multiple parts of the engine initialization. With this parser the validation is centralized. Keep that in mind and check the actual state of the main.cpp in this PR for examples on how to use the parser class.

[Shatur]

Thank you a lot! I will try. Should I open another PR or or will you add me as a contributor to your fork?

[lupoDharkael]

I think it would be better to open a new one as this has a lot of comments already and it makes navigation through the feedback a bit harder.
If you have any doubts with anything you can ask me.

[Xrayez]

I think it's better to take this PR as a reference for implementation, if you attempt to rebase the branch to resolve conflicts then it's easy to miss out potential fixes and enhancements made to master so far, this is a massive change.

[lupoDharkael]

It's a lot of work to adapt main.cpp
I think it is better to go through the list of commits that change something on the main and adapt it. I already did that when I resumed the work on this PR after a few months of conflicts and most of the commits are easy to adapt as you only have to touch a few lines.

[lupoDharkael]

The changes on:
core/bind/core_bind.cpp
core/bind/core_bind.h
core/os/os.cpp
core/os/os.h
Are very small, I would start from clean Godot master, add the changes in those 4 files, replace main.cpp with the one in this PR and start adding the changes of individual commits in main starting from the date of my last commit.
That's how I did it the last time.

[Shatur]

Thanks for the tips. Almost a year has passed and there was a lot of commits related to main.cpp. I think that it is worth using both proposed approaches: use main.cpp as a reference implementation and fix conflicts in all other files.

I think now some godot commands can receive multiple arguments, in order to implement that CommandFlag should have a method int arguments_needed() const instead of bool needs_argument() const; and adapt the parse() method.

I noticed that --export and --export-debug already accepts two arguments. How it works in current implementation?

[lupoDharkael]

This PR only receives a single argument as that's how it was when I wrote it.
As we now require supporting multiple arguments per commands the following elements need to be adapted in cli_parser.h and cli_parser.cpp

• CommandChecker needs to be able to check a list of strings instead of a single String.
• CommandFlag::set_argument_name defines the name of the argument to show in --help, it should receive a list of names or change it to add_argument_name and append a new one with each call.
• CommandParser::needs_argument and CommandFlag::needs_argument should return an int and renamed accordingly.
• CommandParser::_data_found value should be a list of String instead as a single String as multiple arguments can be owned by a single command.
• CommandParser::get_argument as it returns the argument defined in the flag, it can be changed to return the list of arguments.
• the parse() function obviously.

this change also affects main.cpp but the conversion is more or less straightforward.

[Shatur]

Thank you!

[Shatur]

Still working on it. I refactored code a lot, improved error checking, added usage generation, support for positional arguments (it's better to support it instead of manually checking for scene / script), multiply arguments, default values, required arguments, compound arguments (-abc), sticky arguments (-dvalue), adjacent arguments (-d=value), allowed values lists, any number of flags in single command, support for use with different prefixes (e.g. /command, +command, -command, etc, this can be useful for chat commands parsing, for example) with the ability to disable unneeded syntax (for example disable compound arguments) and other API improvements.
I currently writing tests for all this stuff. But I didn't make any changes to main.cpp. I going to first send a PR with only this class and tests for it (as a class for GDScript, because it can be used as is), and then, after we agree on API and merge it, I will send a separate PR with changes to main.cpp to simplify review process (and then another PR with the ability to disable engine arguments to be able to define your own). What do you think?

[lupoDharkael]

That was fast :)
Yeah that's better option right now because the PR will accumulate almost no conflicts that way and it will be simpler to review and merge. The changes seem good, lets see what others think about them.

[Shatur]

I decided to add a couple of additional features before posting :) Finally finished: #44594.