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

Editor: Allow to override CLI args on a per-instance basis #57975

Closed

Conversation

mhilbrunner
Copy link
Member

@mhilbrunner mhilbrunner commented Feb 11, 2022

When running the Project from the Editor, this allows the user to override the CLI arguments each instance is started with per instance using the new advanced project settings editor/run/instance_1_run_args through editor/run/instance_4_run_args (overriding editor/run/main_run_args for each instance if set).

This is useful for multiplayer testing or when testing with different settings/profiles to reduce manual configuration on each test/run.

These new advanced settings, like the main run args one, are hidden by default.
Supersedes #57910. May also be a first step towards this proposal: godotengine/godot-proposals#522

When running the Project from the Editor, this allows the user to override the CLI arguments each instance is started with per instance using the new project settings editor/run/instance_1_run_args through editor/run/instance_4_run_args (overriding editor/run/main_run_args for each instance if set).

This is useful for multiplayer testing or when testing with different settings/profiles to reduce manual configuration on each test/run.
@mhilbrunner mhilbrunner added this to the 4.0 milestone Feb 11, 2022
@mhilbrunner mhilbrunner requested review from a team as code owners February 11, 2022 15:04
@mhilbrunner mhilbrunner requested review from Faless and removed request for a team February 11, 2022 15:04
@mhilbrunner mhilbrunner changed the title Allow to override CLI args on a per-instance basis Editor: Allow to override CLI args on a per-instance basis Feb 11, 2022
@mhilbrunner
Copy link
Member Author

mhilbrunner commented Feb 11, 2022

Some thoughts on the current implementation:

  1. This overrides the main args setting. Some in the proposal suggested these being additional, i.e. being added to the main args for that instance. This comes with two problems:
    1. Overriding an arg given in the main args for a single instance would be difficult. You now need to either define it for all instances and remove it from main, or we need to implement matching/replacement logic.
    2. Due to the %command% handling we support, this would be brittle and complex code, unless we decided you can only provide additional args per instance, not make use of %command%. That seems bad, because e.g. having some instances run with optimus, or in different environments for testing seems useful.
  2. These are currently Project Settings like the main args one. I can see the argument given in the proposal for these not being stored as Project Settings, but only locally, so they aren't committed to VCS etc. Open for suggestions where to store them instead.
    1. However, them being Project Settings does come with some niceties: docs, tooltips (online and googleable, so easy to discover!), and them being right under the main args setting, which, again, improves discoverability. Maybe it would be nice to have a way to override Project Settings locally.

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

See my comments, I think these options should be project metadata in EditorSettings, and not stored in project settings.

Maybe it's time we add some form of "run configuration" page like many IDEs do? I'm not great at UX so I'm a bit unsure what a good simple design would be here.


const String raw_custom_args = ProjectSettings::get_singleton()->get("editor/run/main_run_args");
void EditorRun::parse_run_args(String *exec_path, List<String> *args, const String raw_custom_args) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This fits more the godot codebase:

Suggested change
void EditorRun::parse_run_args(String *exec_path, List<String> *args, const String raw_custom_args) {
void EditorRun::parse_run_args(String &r_exec_path, List<String> &r_args, const String p_raw_custom_args) {

Comment on lines +195 to +196
String instance_arg_setting = "editor/run/instance_" + itos(instance_num) + "_run_args";
String raw_custom_args = ProjectSettings::get_singleton()->get(instance_arg_setting);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like these should be in editor settings meta too (i.e. EditorSettings::get_singleton()->get_project_metadata("debug_options", instance_arg_setting)).

We don't really want to export them, they are debug/editor configurations, not something to store in the project settings.

I guess that raises the question of where to show those info, and maybe an hint to proceed towards an initial implementation of godotengine/godot-proposals#522 ?

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Any reason to limit it to the 4 instances?

Note: I have not tested suggested changes, but PackedStringArray project setting should work fine.

*Edit: * Ah, I see, it's already limited to 4 instances (took me a few minutes to find it).
But it's probably better to add instances_to_run as another project setting (or just use size of the argument list as an instance number), the current UI is not intuitive.

Screenshot 2022-02-22 at 14 45 14

Comment on lines +1224 to +1227
GLOBAL_DEF("editor/run/instance_1_run_args", "");
GLOBAL_DEF("editor/run/instance_2_run_args", "");
GLOBAL_DEF("editor/run/instance_3_run_args", "");
GLOBAL_DEF("editor/run/instance_4_run_args", "");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
GLOBAL_DEF("editor/run/instance_1_run_args", "");
GLOBAL_DEF("editor/run/instance_2_run_args", "");
GLOBAL_DEF("editor/run/instance_3_run_args", "");
GLOBAL_DEF("editor/run/instance_4_run_args", "");
GLOBAL_DEF("editor/run/instance_run_args", PackedStringArray());

@@ -184,9 +184,44 @@ Error EditorRun::run(const String &p_scene) {
args.push_back(p_scene);
}

String exec = OS::get_singleton()->get_executable_path();
int instances = EditorSettings::get_singleton()->get_project_metadata("debug_options", "run_debug_instances", 1);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
int instances = EditorSettings::get_singleton()->get_project_metadata("debug_options", "run_debug_instances", 1);
int instances = EditorSettings::get_singleton()->get_project_metadata("debug_options", "run_debug_instances", 1);
PackedStringArray instance_arg_setting = ProjectSettings::get_singleton()->get("editor/run/instance_run_args");

Comment on lines +195 to +196
String instance_arg_setting = "editor/run/instance_" + itos(instance_num) + "_run_args";
String raw_custom_args = ProjectSettings::get_singleton()->get(instance_arg_setting);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
String instance_arg_setting = "editor/run/instance_" + itos(instance_num) + "_run_args";
String raw_custom_args = ProjectSettings::get_singleton()->get(instance_arg_setting);
String raw_custom_args;
if (instance_num >= instance_arg_setting.size()) {
raw_custom_args = instance_arg_setting[instance_num - 1];
}

@Faless
Copy link
Collaborator

Faless commented Feb 22, 2022

@bruvzg as mentioned above, I don't think these should be project settings at all.
They are not meaningful during export, only for the editor debug, so it should go in the EditorSettings project metadata.
Instances to run are limited to 4 because it's set from the debug menu -> run multiple instance in the editor and we decided to hardcode it at the time, so it has to be adjusted there too in case:

instances_menu->add_radio_check_item(TTR("Run 1 Instance"));
instances_menu->set_item_metadata(0, 1);
instances_menu->add_radio_check_item(TTR("Run 2 Instances"));
instances_menu->set_item_metadata(1, 2);
instances_menu->add_radio_check_item(TTR("Run 3 Instances"));
instances_menu->set_item_metadata(2, 3);
instances_menu->add_radio_check_item(TTR("Run 4 Instances"));
instances_menu->set_item_metadata(3, 4);
instances_menu->set_item_checked(0, true);
instances_menu->connect("index_pressed", callable_mp(this, &DebuggerEditorPlugin::_select_run_count));

void DebuggerEditorPlugin::_select_run_count(int p_index) {
int len = instances_menu->get_item_count();
for (int idx = 0; idx < len; idx++) {
instances_menu->set_item_checked(idx, idx == p_index);
}
EditorSettings::get_singleton()->set_project_metadata("debug_options", "run_debug_instances", instances_menu->get_item_metadata(p_index));
}

@akien-mga
Copy link
Member

akien-mga commented Feb 22, 2022

We discussed this on #editor and I propose making the configuration of these multiple instances a full featured dialog where the number of instances and their arguments can be configured. Quick and dirty mockup:
image

As @groud pointed it out this could use a Tree like e.g. the AutoLoad settings dialog. This was just to illustrate the feature set, UX should still be worked on.

I'll comment further in the proposal, it's probably best to keep things there.
Edit: Done: godotengine/godot-proposals#522 (comment)

@warent

This comment was marked as off-topic.

@cyraid
Copy link

cyraid commented Oct 4, 2023

@Faless If it were Editor Settings, then how would I specify -id 1 to one Project, and -someotherarg in a different one? It would have to be project settings.

@bruvzg I like it! I was hoping for such a change. Would someone be able to add this in?

@Faless
Copy link
Collaborator

Faless commented Oct 5, 2023

@Faless If it were Editor Settings, then how would I specify -id 1 to one Project, and -someotherarg in a different one? It would have to be project settings.

EditorSettings project metadata are per project, so no, it doesn't have to be project settings.

See: https://docs.godotengine.org/en/stable/classes/class_editorsettings.html#class-editorsettings-method-set-project-metadata

@cyraid
Copy link

cyraid commented Oct 7, 2023

@Faless Okeys. Would you be able to review #57975 or commit this one? ^_^ (since I have you here lol)

@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Oct 26, 2023
@mhilbrunner
Copy link
Member Author

Closing, as I currently lack the bandwidth for this one. Still should be done however IMO :)

@mhilbrunner mhilbrunner removed this from the 4.3 milestone Jun 29, 2024
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.

8 participants