Fixed editor filesystem/import properties not being caught by the doctool.

[KurtBliss]

So in this issue it was mentioned that some editor settings properties were not being caught by the doctool. And after running it myself, the properties for filesystem/import in the editor settings wouldn't show up in the docs.

I found a solution where adding the Editor Setting configs to the editor_settings.cpp and taking the now-not-needed editor setting configs out of modules/gltf/register_types.cpp fixes this issue after running the doctool now, hopefully in a preferred manner.

Addresses #63695

• Bugsquad edit, fixes: Add comment to Blender/FBX import paths in Editor Settings #63695

[AThousandShips]

Thank you for your contribution!

While this is important, you have changed the behaviour of these properties, the suffix _RST means that the changing of these properties require restart, this cannot be removed, a different solution is required

Edit: suggested the changes required, will test soon

You also have to add the documentation descriptions to these in the appropriate place, or at least add the entries, do this by compiling the engine and running --doctool

[KurtBliss]

I tried writing some docs for the editor settings filesystem/import/blender/rpc_port and rpc_server_uptime but wasn't sure exactly if I got it right. Here's what I came up with that I didn't didn't put in the commit...

<member name="filesystem/import/blender/rpc_port" type="int" setter="" getter="">
	The port number used for Remote Procedure Call (RPC) communication with Godot's created process of the blender executable
	Setting this to 0 effectively disables communication with Godot and the blender process, making performance slower.
</member>
<member name="filesystem/import/blender/rpc_server_uptime" type="float" setter="" getter="">
	The maximum idle uptime (in seconds) of the Blender process
	This prevents Godot from having to create a new process for each import within the given seconds.
</member>

Thanks for the feedback!

[AThousandShips]

LGTM, I'd suggest adding the other two descriptions as well

[AThousandShips]

Specific wording can be altered

[KurtBliss]

I tried renaming my main branch and cause it to be deleted for this pull, it's restored now.

[akien-mga]

Suggested change

	// Import (for glft module)
	// Import (for gltf module).

[akien-mga]

Seems good overall.

I'm not too fond of moving module specific settings outside those modules, as it breaks encapsulation and means those settings will be defined but useless if the module is compiled out/removed.

But fixing that properly requires some architecture changes to all modules to register their settings at the right time for doctool.

[akien-mga]

For the record, I did a quick attempt at adding a way for modules to register editor settings in a way that doctool could find:

diff --git a/editor/editor_settings.cpp b/editor/editor_settings.cpp
index 5a490fcfc0..c7bacff99a 100644
--- a/editor/editor_settings.cpp
+++ b/editor/editor_settings.cpp
@@ -56,6 +56,8 @@
 
 Ref<EditorSettings> EditorSettings::singleton = nullptr;
 
+Vector<EditorSettingsInitCallback> EditorSettings::_init_callbacks;
+
 // Properties
 
 bool EditorSettings::_set(const StringName &p_name, const Variant &p_value) {
@@ -784,6 +786,11 @@ void EditorSettings::_load_defaults(Ref<ConfigFile> p_extra_config) {
 	EDITOR_SETTING(Variant::STRING, PROPERTY_HINT_ENUM, "project_manager/default_renderer", "forward_plus", "forward_plus,mobile,gl_compatibility")
 #endif
 
+	// Module/extension registered settings.
+	for (int i = 0; i < _init_callbacks.size(); i++) {
+		_init_callbacks[i]();
+	}
+
 	if (p_extra_config.is_valid()) {
 		if (p_extra_config->has_section("init_projects") && p_extra_config->has_section_key("init_projects", "list")) {
 			Vector<String> list = p_extra_config->get_value("init_projects", "list");
diff --git a/editor/editor_settings.h b/editor/editor_settings.h
index 660a9501a2..e0d39ecd42 100644
--- a/editor/editor_settings.h
+++ b/editor/editor_settings.h
@@ -37,6 +37,8 @@
 #include "core/os/thread_safe.h"
 #include "core/templates/rb_set.h"
 
+typedef void (*EditorSettingsInitCallback)();
+
 class EditorPlugin;
 
 class EditorSettings : public Resource {
@@ -77,6 +79,8 @@ private:
 
 	static Ref<EditorSettings> singleton;
 
+	static Vector<EditorSettingsInitCallback> _init_callbacks;
+
 	HashSet<String> changed_settings;
 
 	HashMap<String, PropertyInfo> hints;
@@ -124,6 +128,10 @@ public:
 	static void destroy();
 	void set_optimize_save(bool p_optimize);
 
+	static void add_init_callback(EditorSettingsInitCallback p_callback) {
+		_init_callbacks.push_back(p_callback);
+	}
+
 	bool has_default_value(const String &p_setting) const;
 	void set_setting(const String &p_setting, const Variant &p_value);
 	Variant get_setting(const String &p_setting) const;
diff --git a/modules/gltf/register_types.cpp b/modules/gltf/register_types.cpp
index 1788ffac3a..691b399d59 100644
--- a/modules/gltf/register_types.cpp
+++ b/modules/gltf/register_types.cpp
@@ -47,15 +47,7 @@
 #include "editor/editor_node.h"
 #include "editor/editor_settings.h"
 
-static void _editor_init() {
-	Ref<EditorSceneFormatImporterGLTF> import_gltf;
-	import_gltf.instantiate();
-	ResourceImporterScene::add_importer(import_gltf);
-
-	// Blend to glTF importer.
-
-	bool blend_enabled = GLOBAL_GET("filesystem/import/blender/enabled");
-	// Defined here because EditorSettings doesn't exist in `register_gltf_types` yet.
+static void _editor_settings_init() {
 	EDITOR_DEF_RST("filesystem/import/blender/rpc_port", 6011);
 	EditorSettings::get_singleton()->add_property_hint(PropertyInfo(Variant::INT,
 			"filesystem/import/blender/rpc_port", PROPERTY_HINT_RANGE, "0,65535,1"));
@@ -64,9 +56,23 @@ static void _editor_init() {
 	EditorSettings::get_singleton()->add_property_hint(PropertyInfo(Variant::FLOAT,
 			"filesystem/import/blender/rpc_server_uptime", PROPERTY_HINT_RANGE, "0,300,1,or_greater,suffix:s"));
 
-	String blender3_path = EDITOR_DEF_RST("filesystem/import/blender/blender3_path", "");
+	EDITOR_DEF_RST("filesystem/import/blender/blender3_path", "");
 	EditorSettings::get_singleton()->add_property_hint(PropertyInfo(Variant::STRING,
 			"filesystem/import/blender/blender3_path", PROPERTY_HINT_GLOBAL_DIR));
+
+	EDITOR_DEF_RST("filesystem/import/fbx/fbx2gltf_path", "");
+	EditorSettings::get_singleton()->add_property_hint(PropertyInfo(Variant::STRING,
+			"filesystem/import/fbx/fbx2gltf_path", PROPERTY_HINT_GLOBAL_FILE));
+}
+
+static void _editor_init() {
+	Ref<EditorSceneFormatImporterGLTF> import_gltf;
+	import_gltf.instantiate();
+	ResourceImporterScene::add_importer(import_gltf);
+
+	// Blend to glTF importer.
+	bool blend_enabled = GLOBAL_GET("filesystem/import/blender/enabled");
+	String blender3_path = EDITOR_GET("filesystem/import/blender/blender3_path");
 	if (blend_enabled) {
 		Ref<DirAccess> da = DirAccess::create(DirAccess::ACCESS_FILESYSTEM);
 		if (blender3_path.is_empty()) {
@@ -87,12 +93,7 @@ static void _editor_init() {
 	EditorNode::get_singleton()->add_child(EditorImportBlendRunner::get_singleton());
 
 	// FBX to glTF importer.
-
 	bool fbx_enabled = GLOBAL_GET("filesystem/import/fbx/enabled");
-	// Defined here because EditorSettings doesn't exist in `register_gltf_types` yet.
-	String fbx2gltf_path = EDITOR_DEF_RST("filesystem/import/fbx/fbx2gltf_path", "");
-	EditorSettings::get_singleton()->add_property_hint(PropertyInfo(Variant::STRING,
-			"filesystem/import/fbx/fbx2gltf_path", PROPERTY_HINT_GLOBAL_FILE));
 	if (fbx_enabled) {
 		Ref<EditorSceneFormatImporterFBX> importer;
 		importer.instantiate();
@@ -161,6 +162,7 @@ void initialize_gltf_module(ModuleInitializationLevel p_level) {
 		GLOBAL_DEF_RST("filesystem/import/fbx/enabled.web", false);
 
 		ClassDB::set_current_api(prev_api);
+		EditorSettings::add_init_callback(_editor_settings_init);
 		EditorNode::add_init_callback(_editor_init);
 	}

But this fails because EDITOR_DEF expects EditorSettings to be a singleton, and that's not the case when running doctool.

It could be made a proper singleton with some more work but that has other coupled implications like creating a config file, etc.

Moreover this approach only solves part of the problem - settings definitions could stay in the modules, but the actual documentation for those settings would still end up in doc/classes/EditorSettings.xml, so it still breaks encapsulation and doesn't make it possible for thirdparty modules to provide documentation.

[akien-mga]

Thanks!