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 command-line option to run a MainLoop by its global class name #78045

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

rburing
Copy link
Member

@rburing rburing commented Jun 9, 2023

This PR allows e.g. creating language bindings in the same way as C#, but without hacking main.cpp.

See 7f89a1c for an example of usage.

Original (outdated) description

Removes this hack from main.cpp:

godot/main/main.cpp

Lines 2519 to 2526 in 300748e

#if defined(MODULE_MONO_ENABLED) && defined(TOOLS_ENABLED)
// Hacky to have it here, but we don't have good facility yet to let modules
// register command line options to call at the right time. This needs to happen
// after init'ing the ScriptServer, but also after init'ing the ThemeDB,
// for the C# docs generation in the bindings.
List<String> cmdline_args = OS::get_singleton()->get_cmdline_args();
BindingsGenerator::handle_cmdline_args(cmdline_args);
#endif

With this PR, the bindings generator can be run as:

/bin/godot.linuxbsd.editor.dev.x86_64.llvm.mono --headless --main-loop CSharpBindingsGenerator --generate-mono-glue modules/mono/glue

Here I named the main loop class CSharpBindingsGenerator. The class could also be renamed CSharpGlueGenerator, and then the argument parsing could be simplified to use OS::get_cmdline_user_args:

/bin/godot.linuxbsd.editor.dev.x86_64.llvm.mono --headless --main-loop CSharpGlueGenerator -- modules/mono/glue

This PR allows creating other language bindings in the same way without hacking main.cpp (which was my motivation).

@akien-mga
Copy link
Member

akien-mga commented Jun 9, 2023

That's a pretty good approach! It definitely makes things cleaner and easier to extend from thirdparty modules.

Some minor concerns or nitpicks:

  • It makes the syntax to generate the mono glue more convoluted, and remembering the name of the MainLoop can be tricky, especially as it won't be present in the --help (but --generate-mono-glue wasn't either). Make it easier to build a working mono-enabled Godot from scons #76542 might solve this issue as most users might start relying on a SCons argument to generate the glue, instead of calling Godot manually. (CC @shana)
  • This is a feature for power users, I wouldn't add a single letter -M shortcut for it, those should be reserved for options which are used frequently.

@rburing rburing requested a review from a team as a code owner June 9, 2023 13:05
@rburing rburing force-pushed the main_loop_command branch 2 times, most recently from 375f00c to 7f89a1c Compare June 9, 2023 15:34
@rburing
Copy link
Member Author

rburing commented Jun 9, 2023

@akien-mga I agree the command is more convoluted, so it would be great to combine this with #76542. I removed the unnecessary -M shortcut.

Is there something I can do about the CSharpBindingsGenerator class being exposed e.g. to docs? Or should it just be documented?

@raulsntos
Copy link
Member

I don't really like the idea of exposing the bindings generator class. Also, as previously mentioned, it makes building Godot with .NET more convoluted.

I wonder if we could have a method implemented by modules to register options instead. Then, main could show registered options in --help and if one of them is provided it could forward it to the module that registered it. What I'm thinking is something similar to the initialize_*_module method in register_types.h.

// Called by main before parsing args.
void bind_options(ArgsDB p_args_db) {
    p_args_db.bind_arg(sarray("-e", "--editor"), "Start the editor instead of running the scene.");
    p_args_db.bind_arg(sarray("-p", "--project-manager"), "Start the project manager, even if a project is auto-detected.");
    p_args_db.bind_arg(sarray("--quit"), "Quit after the first iteration.");
}

// Called by main after handling args.
void handle_options(const List<String> &p_cmdline_args) {
    const List<String>::Element *elem = p_cmdline_args.front();
    while (elem) {
        if (elem->get() == "--my-option") {
            // Handle option.
        }
    }
}

On second thought, we probably don't need to make it that complicated. We could just have a handle_options method implemented by modules that gets called by main after it has handled args. This method would be called for every module that implements it regardless of whether there's any option provided for the module. Then we wouldn't need to register options (bind_options), although we may still want to implement a mechanism to add lines to --help.

@rburing rburing changed the title Add command-line option to run a MainLoop by its global class name; use for C# bindings generator Add command-line option to run a MainLoop by its global class name Jun 13, 2023
@rburing
Copy link
Member Author

rburing commented Jun 13, 2023

@raulsntos I've removed the C# changes from this PR.

The fact that the previous version did work shows that the MainLoop-running feature is very powerful, at an extremely low maintenance cost. Personally it would help me continue working on godot-julia (without forking Godot).

Adding more elegant argument handling to modules would be nice to do in the future indeed.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks nice and clean!

@rburing rburing removed the request for review from a team June 13, 2023 16:26
@adamscott
Copy link
Member

Personally it would help me continue working on godot-julia (without forking Godot).

If we merge this PR right after the 4.1 release, wouldn't it remove the need for you to fork Godot, @rburing?

@akien-mga
Copy link
Member

Personally it would help me continue working on godot-julia (without forking Godot).

If we merge this PR right after the 4.1 release, wouldn't it remove the need for you to fork Godot, @rburing?

Technically this can be worked around by running a Godot project (just a project.godot file) with application/run/main_loop_type set.

This PR is pretty harmless but since the main use case for it (godot-julia) is in early development, I think there's no need to breach the feature freeze for 4.1, we can keep this for 4.2.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Jun 13, 2023
@shana
Copy link
Contributor

shana commented Jun 14, 2023

This is a very nice hook @rburing, really cool! I'm going to keep this in mind for #76542 👍

@YuriSizov YuriSizov merged commit 008f32c into godotengine:master Jul 12, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@rburing rburing deleted the main_loop_command branch July 12, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants