Allow editor plugins to modify run arguments

[dsnopek]

This PR aims to allow editor plugins to control the arguments that are used when launching a project from the editor

The ultimate goal is to allow the godot_openxr_vendor extension to simulate running a hybrid immersive/flat VR app on desktop, so you don't need to deploy to the headset for testing.

See GodotVR/godot_openxr_vendors#312 for the rest of the implementation of that (also depends on #108852)

This is a DRAFT for now, because I need to do more testing (especially with the new embedded game view on multiple platforms)

[dsnopek]

This could still use more testing, although, it's working well for me on Linux (both with the embedded game view and not) so I think I'll take it out of draft

NOTE: this is squarely targeting Godot 4.6 at this point

[KoBeWi]

I think it would be more efficient if you just passed the Vector as argument and don't expect it to be returned. Since it's passed as reference, the plugin can modify it all the same, without making an unnecessary copy. Although the copy is only made when a plugin implements the method, so it's probably fine either way.

I also noticed that there is discrepancy between arguments passed to the new method and the ones returned by OS.get_cmdline_args() in running project. Specifically, the _run_scene() is called with empty array, while at runtime the array includes --scene and scene path. Not a big deal though.

[KoBeWi]

What are the debugger changes for?

[dsnopek]

@KoBeWi Thanks for the review!

I think it would be more efficient if you just passed the Vector as argument and don't expect it to be returned. Since it's passed as reference, the plugin can modify it all the same, without making an unnecessary copy.

While passing it as a reference and modifying it in place would work in the engine itself, I don't think it would work from GDScript or GDExtension. In that case, I think a copy would be made as soon as any elements were removed or added to the Vector, and the whole point of this virtual method is to add or remove stuff.

Since this only happens when the user clicks the "Run project" or "Run scene" button, I don't think we have to worry about being very efficient here.

I also noticed that there is discrepancy between arguments passed to the new method and the ones returned by OS.get_cmdline_args() in running project. Specifically, the _run_scene() is called with empty array, while at runtime the array includes --scene and scene path.

This is by design. I don't think we want plugins to mess with all the arguments that the editor adds automatically, just the additional ones. Otherwise a plugin could sneakily change which scene is being run, or the port the debugger is going to connect to, etc.

What are the debugger changes for?

To allow the application to trigger itself to relaunch with different arguments, while still allowing the new instance to run in the embedded game view (if enabled) and connect to the debugger.

These changes are to enable PR GodotVR/godot_openxr_vendors#312 for the godot_openxr_vendors GDExtension.

That PR aims to allow basically the same as PR #103972 (which only works when running the editor on Android), but instead simulating it on the desktop, so you can test hybrid apps (XR apps that can switch between immersive and 2D panel modes) without having run on a real headset.

[KoBeWi]

To allow the application to trigger itself to relaunch with different arguments, while still allowing the new instance to run in the embedded game view (if enabled) and connect to the debugger.

How can this be tested?

[dsnopek]

@KoBeWi:

How can this be tested?

Well, you can test it via GodotVR/godot_openxr_vendors#312 and the Meta XR Simulator, but that's a lot of extra stuff to setup :-)

So, here's a minimal demo app to test it:

editor-plugin-run-test.zip

[KoBeWi]

Here's a scene that does (almost) the same in vanilla Godot:
main2.tscn.txt
The only difference is that the project re-launches without debugger (though that can be fixed probably), and set_restart_on_exit() sometimes fails.

I guess adding request_run_scene is fine, but it's not really related to this PR and can be done separately.

[dsnopek]

Here's a scene that does (almost) the same in vanilla Godot:
main2.tscn.txt
The only difference is that the project re-launches without debugger (though that can be fixed probably), and set_restart_on_exit() sometimes fails.

I tried a whole bunch of times, but could not get this to work for me on Linux (X11). I also suspect that it would cause problems with the embedded game view even on platforms where it's working, but I haven't tested that

I guess adding request_run_scene is fine, but it's not really related to this PR and can be done separately.

Sure, I can move it to its own PR!

However, I really need both in order to implement the feature that I'm aiming to implement (which is a way to test hybrid apps - similar to PR #103972 - but on desktop, rather than on Android)

UPDATE: Actually, I don't need both, only this one! See this comment for the details

[dsnopek]

I've moved the debugger part into its own PR: #108852

UPDATE: I've managed to find a way to implement what #108852 did using only this PR and an EditorDebuggerPlugin, so that PR is no longer needed, only this one here

[KoBeWi]

While passing it as a reference and modifying it in place would work in the engine itself, I don't think it would work from GDScript or GDExtension.

I tested this and indeed GDScript makes a copy.
However you can change call_run_scene() and EditorPlugin.run_scene() to modify the argument. This way only the virtual method will replace the vector.
You can also avoid replacing the vector unnecessarily by using GDVIRTUAL_IS_OVERRIDDEN.

Since this only happens when the user clicks the "Run project" or "Run scene" button, I don't think we have to worry about being very efficient here.

Being inefficient may affect project starting time. It's likely not a problem here, but optimizing it won't make the code much more complex. Though it only involves internal changes, so it can be done in a follow-up PR if you don't want to do it now.

[Calinou]

Tested locally, it works as expected. Code looks good to me.

Testing project: test_pr_107671.zip
This project contains an editor plugin that implements "play from position" functionality.

play_from_position.mp4

Note that one limitation with this approach is that arguments are always appended at the end, with no way to control the order between plugins. This can have an importance if you're passing user command line arguments (i.e. --engine-arg -- --user-arg), as anything after -- will be ignored by the engine itself and treated as an user argument.

Having support for specifying user command line arguments in a way that isn't reliant on order (e.g. +user-arg) could help resolve this in the long run.

[dsnopek]

@KoBeWi: Thanks for all the review!

Being inefficient may affect project starting time. It's likely not a problem here, but optimizing it won't make the code much more complex.

I still really don't think this will be inefficient: Vector does copy-on-write, so if there have been no changes, we're really just re-assigning the reference.

Anyway, I've gone ahead and modified the code as you described here:

However you can change call_run_scene() and EditorPlugin.run_scene() to modify the argument. This way only the virtual method will replace the vector.
You can also avoid replacing the vector unnecessarily by using GDVIRTUAL_IS_OVERRIDDEN.

We actually don't need GDVIRTUAL_IS_OVERRIDDEN() because GDVIRTUAL_CALL() will only return true if it's overridden. But, otherwise, this is included in my latest push.

@Calinou:

Tested locally, it works as expected. Code looks good to me.

Thanks for the testing and review! That's a really cool use for this feature :-)

[KoBeWi]

Note that one limitation with this approach is that arguments are always appended at the end, with no way to control the order between plugins.

Not really. You get access to the whole argument array, so you can check if another plugin has already added -- and insert your arguments accordingly.

[Mickeon]

I kind of get why, and maybe it has been discussed in the past, but why does this function have to return a new array derived from the original, instead of modifying the array in-place (which would probably require an Array[String], or potentially having the returned value appended onto the original array internally?
I just personally find the approach of "take the array as a parameter, then return almost the same array, if you want" to be awkward API.

[dsnopek]

I kind of get why, and maybe it has been discussed in the past, but why does this function have to return a new array derived from the original, instead of modifying the array in-place (which would probably require an Array[String], or potentially having the returned value appended onto the original array internally?

While modifying the array in place works in C++ within the engine itself, in either GDScript or GDExtension, modifying the array in-place won't update the original because of copy-on-write.

I'm not sure what you mean by "having the returned value appended onto the original array internally"?

[Mickeon]

I'm not sure what you mean by "having the returned value appended onto the original array internally"?

Essentially the same behavior as Object's _get_property_list and Node's _get_configuration_warnings, which would limit this function to only being able to add extra arguments, but is potentially more expected.

[dsnopek]

Essentially the same behavior as Object's _get_property_list and Node's _get_configuration_warnings, which would limit this function to only being able to add extra arguments, but is potentially more expected.

Unfortunately, if this is limited to only adding arguments, then it won't work for the use case I created this PR for - I need to be able to remove and modify arguments as well.

[Repiteo]

Thanks!