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

Add a CLI command parser #137

Closed
lupoDharkael opened this issue Oct 9, 2019 · 11 comments
Closed

Add a CLI command parser #137

lupoDharkael opened this issue Oct 9, 2019 · 11 comments

Comments

@lupoDharkael
Copy link

lupoDharkael commented Oct 9, 2019

I'm ignoring the base proposal template as it doesn't fit the needs of this issue.

As some of you may know I started a WIP PR in the main Godot repository: godotengine/godot#26213

I stoped working on the implementation due to the lack of feedback about it but it is obvious that it requires defining the design first before I continue the PR.
I'm going to explain a few design concepts so we can debate the best fit for Godot or even consider options not mentioned here.

Why is the parser needed

A parser allows the unification of the command parsing which in is separated in multiple steps in the actual main.cpp and the process is kind of chaotic.
The delegation of that work to a parser keeps the main.cpp smaller and more organized. The detection of some errors in format is better and more clean and we can fail earlier as a consequence.

Argument ownership

This is the tricky part, not because it is hard but because it requires a decision based on personal preferences.

We have 2 options:

  • Strict command parsing:
    requires to register all the commands, if an invalid command is provided it can suggest the most similar known by the parser.
    The problem here are the game related commands. A very standardized solution is to define a separator between engine commands and game commands.
    For example, with -- as a separator, the command godot --quiet -- test 55 3 would identify --quiet as a godot command and test 55 3 as game command. The separator could be configurable.

  • Non strict command parsing:
    It an unknown command is provided it just ignores the content that doesn't match the registered information in the command parser.
    It may be interesting as the user could process the commands later and extract the relevant information. Command suggestion is not possible here.

Godot module exposed

Godot modules could benefit from the command parser.
The actual structure of the modules need to define the following functions:
void register_xxx();
void unregister_xxx();
(where xxx is the name of the module).
using the register function to register new commands is not possible as it is called after we parse the commands and initialize a few things in the engine.
I propose defining an optional function (that way it is backward compatible) so every module can register new commands and validate them before they start processing.
It could be called setup_xxx or something clearer. And it would be called before the command parsing stage.

That would also mean having a pointer to a Command parser in a singleton so the modules can access the parser and register commands there.
Preserving the command parser in memory would mean delegating al the command related functionality to the command parser.

GDS exposed classes

The command parser could be exposed to gds for a general purpose command parsing solution.
That has the drawback of creating public API so it would be a limitation to change how to parser works.
On the other hand it would be code already implemented in the engine and a command parser shouldn't change that much. It could be very useful to validate game commands.
It would also be understandable not to expose this as some games could require very custom solutions.

@Calinou
Copy link
Member

Calinou commented Oct 9, 2019

A very standardized solution is to define a separator between engine commands and game commands.
[…]
The separator could be configurable.

But then, it means the separator is no longer standardized 😉

I've only seen -- used for this purpose in the wild.

@willnationsdev
Copy link
Contributor

I would prefer the first option of the two, and I like the other ideas you've presented. I personally like the idea of exposing the Godot executable's command line arguments to GDScript in some capacity.

And I agree with @Calinou regarding the use of -- as a separator. We should stick to the conventions. If it were configurable, that would hurt the readability of code shared on the Internet as someone might have configured it to look different and people would have trouble understanding what the arguments meant.

@lupoDharkael
Copy link
Author

I just noticed a part of the proposal was not finished (it did it little by little so I forgot to finish the redaction of the godot modules part).
I was considering the options of preserving the parser in memory the whole time or just until the commands are parsed but I think it is better to have it in memory because Godot modules could use them anytime. If we expose something to gds it would require keeping it in memory as it contains the dictionary with the occurrences of commands and other data.
It requires an almost irrelevant amount of memory and helps encapsulating the information of the commands that now is saved in the OS singleton.

@vnen
Copy link
Member

vnen commented Oct 14, 2019

I personally like the idea of exposing the Godot executable's command line arguments to GDScript in some capacity.

They are already exposed "in some capacity" by calling OS.get_cmdline_args().

But I wouldn't mind a proper command line argument parser that is also exposed to GDScript.

@Xrayez
Copy link
Contributor

Xrayez commented Oct 15, 2019

I think GUT would greatly benefit from this on the scripting side (it does mix and match built-in and plugin-specific commands).

@bitwes do you have any suggestions on how godotengine/godot#26213 could be improved?

@Xrayez
Copy link
Contributor

Xrayez commented Oct 15, 2019

Godot modules could benefit from the command parser.

Yes, did you know GDNative "pollutes" the command line options while still being a module? 🙂

--gdnative-generate-json-api Generate JSON dump of the Godot API for GDNative bindings.

Though I think this option/method should be renamed and moved to core API instead.

@bitwes
Copy link

bitwes commented Oct 15, 2019

I was able to get command line arguments from a gdscript when it was run directly. I made a simple option parser. If Godot doesn't know the option it will pass through. For that reason all the GUT options start with a g since none of the Godot options did and to avoid name clashes. You also can't do name/value pairs with a space this way. I get around that by requiring an = between the option and the value. This is the only real "gotcha" I had to overcome. Anything without a - gets swallowed up by Godot, so if you put a space in there your value doesn't show up.

I've thought about breaking out the logic I used into another plugin but I haven't had need for it outside GUT and I didn't want to make GUT dependent on another plugin. If this was built into the engine that would be great.

Here's the gdscript that parses and uses the command line options.
https://github.com/bitwes/Gut/blob/master/addons/gut/gut_cmdln.gd

Here's a wiki page that gives more context on how to use the script.
https://github.com/bitwes/Gut/wiki/Command-Line

@lupoDharkael
Copy link
Author

lupoDharkael commented Dec 13, 2019

I propose defining an optional function (that way it is backward compatible) so every module can register new commands and validate them before they start processing.

I've realized this isn't needed as the module initialization occurs in setup2() and the commands are parsed in setup(). I thought modules were initialized before (and thinking about it that doesn't make sense). Modules are able to query the parser in the register_xxx() function with the actual state of the PR. They just need to #include "core/os/os.h" and use OS::get_singleton()->get_command_parser()

@Calinou
Copy link
Member

Calinou commented Mar 11, 2020

@bitwes Note that since Godot 3.2, custom command line arguments are no longer considered to be positional arguments thanks to this pull request. This means you can now parse arguments that use a space instead of a = sign to separate the option and value.

@Calinou Calinou changed the title Command parser Add a command parser Mar 11, 2020
@Calinou Calinou changed the title Add a command parser Add a CLI command parser Jun 18, 2020
@reduz
Copy link
Member

reduz commented May 29, 2021

I'm ignoring the base proposal template as it doesn't fit the needs of this issue.

Proposals need to make the point that they are solving an actual real-world problem, proposing improvements for the sake of it will result in the proposal most likely being ignored. If there is still any interest about this, I suggest re-opening another proposal by properly following the template.

@reduz reduz closed this as completed May 29, 2021
@reduz
Copy link
Member

reduz commented May 29, 2021

I also opened a counter proposal: #2797

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants