search

godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Allow accessing EditorSettings at runtime #11097

Open KoBeWi opened 1 week ago

KoBeWi commented 1 week ago

Describe the project you are working on

Godot engine itself D:

Describe the problem or limitation you are having in your project

We have some runtime functionality that relies on Editor Settings. It's related to debugging features.

There is no easy way to pass the settings to the running project.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Allowing to access editor settings at runtime would make better debugging experience. Note that it only concerns internal engine usage, it doesn't need to be exposed to the users.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

The main challenge is that editor stuff can't be included in core/scene files. This means you can't just include EditorSettings, wrap it in TOOLS_ENABLED and call it a day; it requires a workaround.

The implementation I came up with involves the debugger. The class that needs editor setting requests it from SceneDebugger, the debugger sends a message to the editor's debugger, which fetches the requested setting and sends it back. It has multiple advantages:

See the pull request for more details: https://github.com/godotengine/godot/pull/98823 Important to note is that it's available only in editor builds. You can't access editor settings in export templates (obviously).

Alternative implementation is hardcoding a list of settings, which are always sent when the project is started. It's arguably simpler and easier to use, but it's more difficult to maintain, due to having 2 separate lists that you need to keep in sync, and it also couples the settings provider with everything that uses them.

If this enhancement will not be used often, can it be worked around with a few lines of script?

No.

Is there a reason why this should be core and not an add-on in the asset library?

N/A

Faless commented 1 week ago

The class that needs editor setting requests it from SceneDebugger, the debugger sends a message to the editor's debugger, which fetches the requested setting and sends it back. It has multiple advantages:

As said in https://github.com/godotengine/godot/pull/98823 I think the implementation proposed there has some big disadvantages:

I think it's possible to make a more generic (and simple) solution if we want to, but we need to acknowledge a couple of things:

With this in mind we have two options:

  1. We either have each plugin handle its own configuration (and in this case we don't have a generic system, and we only pass the relevant shortcuts from the editor, and have the runtime part handle that with a dedicated message).
  2. We create a truly generic system to let the EditorDebuggerPlugin configure the relative capture/profiler (runtime bit).

If we go for option 1, we can solve the original problem of this PR, without creating an half-generic system for internal use only, but we also require anyone wanting to provide setting for their custom plugin to re-implement the thing every time (I believe this is fine, but you seem to want to make a generic system).

This brings us to option 2.

Profilers and captures are right now separated (that might have been a mistake in hindsight, but :shrug: ), and profilers already have a way to be configured (the toggle part can bring a array of options), while captures doesn't have a standard way to be configured.

To configure captures, we can have a "standard" capture message (something like capture_name:update_capture_setting) that the debugger can handle internally (if has_capture("capture_name")) and expect a standard message format ([setting_name, setting_value]) which will cause the engine debugger to store that setting accordingly in a dictionary.

Debugger plugins (captures) can read settings by calling EngineDebugger.get_debugger_setting("capture_name", "setting_name", def_val), and EditorDebuggerPlugins can send settings by just calling send_debugger_setting("capture_name", "setting_name").

This makes the system truly generic and reusable, without having extra back and forth, and without giving excessive permissions to the clients.

KoBeWi commented 1 week ago

It's limited to EditorSettings, and Shortcuts (requiring different messages for settings and shortcuts)

It's limited to what is needed and it's easy to add new things as necessary. I'm not familiar with debugger, but different messages are not really a problem; they are String anyways and it's the prefix that matters (which is the same for each subtype).

it still requires to maintain 2 "lists" of settings (and 2 lists of shortcuts)

The difference is that you have both lists in the same place, as opposed to 2 different, unrelated files. It's much easier to track them. Also settings and shortcuts are pretty much unified; I split them only because they have to be fetched and serialized differently.

The code in the PR also still requires a bunch of #ifdef TOOLS_ENABLED in the debugger plugin (I'm not sure why).

EditorSettings exist only in editor builds. Technically wrapping it is not required, but the code is effectively no-op without editor (you will never get the requested setting).

we also require anyone wanting to provide setting for their custom plugin to re-implement the thing every time (I believe this is fine, but you seem to want to make a generic system).

That sounds like code duplication, so no.

idk, I just learned about debugger plugins a couple weeks ago and thought it's a good solution for the problem of passing settings. I don't care about a specific implementation tbh, as long as it doesn't require significant boilerplate code every time you want to access something. Also tracking and syncing things across multiple files is annoying, so it should also be avoided.

Faless commented 1 week ago

The difference is that you have both lists in the same place, as opposed to 2 different, unrelated files.

They are not unrelated, as I explained, any debugger plugin has 2 parts, the editor part, and the runtime part, they are absolutely related.

If anything, we should finish the splitting of internal debuggers into multiple plugins, something we already started but hasn't been enforced enough recently (as I explained in https://github.com/godotengine/godot/pull/60134#pullrequestreview-1179882170, and has been only in part done in godotengine/godot#97257 by making a debugger plugin, but reusing the generic SceneDebugger, whose if/elseif is growing indefinitely).

There is a good example of how things should be kept separate between the runtime and editor while keeping them self-contained by functionality (so they can be selectively disabled) in multiplayer_debugger and multiplayer_editor_plugin

EditorSettings exist only in editor builds. Technically wrapping it is not required, but the code is effectively no-op without editor (you will never get the requested setting).

The editor debugger (server) is only present in editor builds, so I don't understand in which case the runtime can be connected to a non-editor build.

Also tracking and syncing things across multiple files is annoying, so it should also be avoided.

This is a non problem, splitting code across different files is perfectly fine, especially if one part of the code is editor, and the other is runtime. That's the reason why many modules have an editor folder that gets completely excluded in non-editor builds.

Also, we have many occasion where settings are defined in one place, and used in another, it's perfectly normal.

Faless commented 1 week ago

The newly added Game tab allows moving the camera at runtime. For best experience, it should use the user's customized settings (for panning etc.)

* Right now it just uses defaults.

For reference, this can be implemented like this with 20 lines of code, and the same unchecked remote code execution present in godotengine/godot#98823 (so neither should be merged like that)

Faless commented 1 week ago

The Stop shortcut (F8 by default) which is editor shortcut, but also used at runtime

* Currently passed via a hacky environment variable:
  https://github.com/godotengine/godot/blob/b00e1cbf743dcb6a2b7f6bd14e348a1a7cf3d403/editor/editor_run.cpp#L229

I've added to the aforementioned branch a fix for this too (+21 - 36).

KoBeWi commented 1 week ago

Looks like you have pull request ready to supersede mine? Feel free to open it.

Faless commented 1 week ago

Opened https://github.com/godotengine/godot/pull/98891 as draft.