Repository: godotengine/godot (language: cpp)

Issue:
C# glue for `EditorSceneFormatImporter::get_import_options` overload is missing return value for `options`
### Godot version

v4.2.beta3.mono.official [e8d57afae]

### System information

Godot v4.2.beta3.mono - Windows 10.0.19045 - Vulkan (Forward+) - dedicated AMD Radeon RX 6700 XT (Advanced Micro Devices, Inc.; 31.0.14043.7000) - AMD Ryzen 9 3900X 12-Core Processor (24 Threads)

### Issue description

The glue method generated for `EditorSceneFormatImporter::get_import_options` misses the argument `r_options` which makes it completely useless because there is no way to return the option list to the engine.

C++ 
```cpp
void EditorSceneFormatImporter::get_import_options(const String &p_path, List<ResourceImporter::ImportOption> *r_options) {
	GDVIRTUAL_CALL(_get_import_options, p_path);
}
```

Glue definition
```cpp
GDVIRTUAL1(_get_import_options, String)
```

C#
```c#
public virtual void _GetImportOptions(string path) { }
```

In `EditorImportPlugin` this is solved by allowing the C# code to return a dictionary which is then mapped to the list of import options.

### Steps to reproduce

Try to write a `EditorSceneFormatImporter` plugin which needs to display import options to a user.

### Minimal reproduction project

N/A


Discussion / hints:
It should be a return value, rather than a reference argument (we don't have those in the API for maximum compatibility across different languages). But indeed, this method seems to be bound without a return value in both `EditorSceneFormatImporter` and `EditorScenePostImportPlugin`, which given its name is a clear mistake. cc @godotengine/import 
You're right, the title was a bit misleading, I've adjustet it so it should be more clear now.
Hi, this is my first time contributing and one of my school project is to contribute to a project. Is it okay if I can get assigned to this issue? Should EditorSceneFormatImporter::get_import_options have a return value instead of being a void function? Based on a previous comment, if r_options shouldn't be passed as an argument and instead should be a return value, what should the  EditorSceneFormatImporter::get_import_options be changed to have a return value? Sorry for the questions. Any help would be appreciated! 
@Riamuwilko Godot engine does not practice assigning issues. If you think you can tackle this, just open a PR
@Riamuwilko As mentioned above, we don't assign issues, so just feel free to work on it.

As for how to resolve this, the method itself should not be changed, as internally it's totally fine to have an argument passed by reference to be populated inside of a method. The problem is with what is bound to the API. The GDVIRTUAL bits. Both the definition in the header and the `GDVIRTUAL_CALL` need to be modified to work with a return value.

For the definition, there are several `GDVIRTUALXXX` macros depending on the number of arguments, on the const-ness, and on the fact that it has a return value or not, and we must use the correct one to indicate it has a return value. Its return type should also be added to the definition.

For the call, `GDVIRTUAL_CALL` needs to be called with an extra argument to collect the return value. This cannot be the `r_options` argument of the parent method, because we don't want to override it. Instead you need to create a new collection, pass it to the call to collect the output, and then append the results to `r_options`.

Both of these things should have plenty of examples in the codebase, which makes it a good first issue. But I must admit it's on the upper level of what beginners may be comfortable with.
I've already worked on this and will open a PR for it - sorry for the response delay.
Just wanted to comment that theoretically, there is a workaround in the form of an even lesser-known but more powerful API:
[EditorScenePostImportPlugin.get_import_options](https://docs.godotengine.org/en/stable/classes/class_editorscenepostimportplugin.html#class-editorscenepostimportplugin-private-method-get-import-options)

Basically, the theory is options can be added by creating a simple EditorScenePortImportPlugin (not to be confused with a EditorScenePostImport script which is completely different) and those options should be accessible in the `options` argument to `import_scene`.

If not, they should definitely be available in the .import file which can be read from disk using ConfigFile. Hopefully it won't come to that, but that's another workaround if needed.

I'll try to look into improving the import options in Godot 4.4, but there simply wasn't enough time to fix the bugs with this for 4.3

Fix patch:
diff --git a/doc/classes/EditorSceneFormatImporter.xml b/doc/classes/EditorSceneFormatImporter.xml
index b14810133e79..886185ff9cdc 100644
--- a/doc/classes/EditorSceneFormatImporter.xml
+++ b/doc/classes/EditorSceneFormatImporter.xml
@@ -13,9 +13,10 @@
 		<method name="_get_extensions" qualifiers="virtual const">
 			<return type="PackedStringArray" />
 			<description>
+				Return supported file extensions for this scene importer.
 			</description>
 		</method>
-		<method name="_get_import_flags" qualifiers="virtual const">
+		<method name="_get_import_flags" qualifiers="virtual const" deprecated="Unused by the engine, and will be removed in a future version. Implementing this has no effect.">
 			<return type="int" />
 			<description>
 			</description>
@@ -24,6 +25,9 @@
 			<return type="void" />
 			<param index="0" name="path" type="String" />
 			<description>
+				Override to add general import options. These will appear in the main import dock on the editor. Add options via [method add_import_option] and [method add_import_option_advanced].
+				[b]Note:[/b] All [EditorSceneFormatImporter] and [EditorScenePostImportPlugin] instances will add options for all files. It is good practice to check the file extension when [param path] is non-empty.
+				When the user is editing project settings, [param path] will be empty. It is recommended to add all options when [param path] is empty to allow the user to customize Import Defaults.
 			</description>
 		</method>
 		<method name="_get_option_visibility" qualifiers="virtual const">
@@ -32,6 +36,7 @@
 			<param index="1" name="for_animation" type="bool" />
 			<param index="2" name="option" type="String" />
 			<description>
+				Should return [code]true[/code] to show the given option, [code]false[/code] to hide the given option, or [code]null[/code] to ignore.
 			</description>
 		</method>
 		<method name="_import_scene" qualifiers="virtual">
@@ -40,6 +45,27 @@
 			<param index="1" name="flags" type="int" />
 			<param index="2" name="options" type="Dictionary" />
 			<description>
+				Perform the bulk of the scene import logic here, for example using [GLTFDocument] or [FBXDocument].
+			</description>
+		</method>
+		<method name="add_import_option">
+			<return type="void" />
+			<param index="0" name="name" type="String" />
+			<param index="1" name="value" type="Variant" />
+			<description>
+				Add a specific import option (name and default value only). This function can only be called from [method _get_import_options].
+			</description>
+		</method>
+		<method name="add_import_option_advanced">
+			<return type="void" />
+			<param index="0" name="type" type="int" enum="Variant.Type" />
+			<param index="1" name="name" type="String" />
+			<param index="2" name="default_value" type="Variant" />
+			<param index="3" name="hint" type="int" enum="PropertyHint" default="0" />
+			<param index="4" name="hint_string" type="String" default="&quot;&quot;" />
+			<param index="5" name="usage_flags" type="int" default="6" />
+			<description>
+				Add a specific import option. This function can only be called from [method _get_import_options].
 			</description>
 		</method>
 	</methods>
diff --git a/editor/import/3d/resource_importer_scene.cpp b/editor/import/3d/resource_importer_scene.cpp
index 4a8837ce5b0d..5e9557537dbe 100644
--- a/editor/import/3d/resource_importer_scene.cpp
+++ b/editor/import/3d/resource_importer_scene.cpp
@@ -91,8 +91,20 @@ Node *EditorSceneFormatImporter::import_scene(const String &p_path, uint32_t p_f
 	ERR_FAIL_V(nullptr);
 }
 
+void EditorSceneFormatImporter::add_import_option(const String &p_name, const Variant &p_default_value) {
+	ERR_FAIL_NULL_MSG(current_option_list, "add_import_option() can only be called from get_import_options().");
+	add_import_option_advanced(p_default_value.get_type(), p_name, p_default_value);
+}
+
+void EditorSceneFormatImporter::add_import_option_advanced(Variant::Type p_type, const String &p_name, const Variant &p_default_value, PropertyHint p_hint, const String &p_hint_string, int p_usage_flags) {
+	ERR_FAIL_NULL_MSG(current_option_list, "add_import_option_advanced() can only be called from get_import_options().");
+	current_option_list->push_back(ResourceImporter::ImportOption(PropertyInfo(p_type, p_name, p_hint, p_hint_string, p_usage_flags), p_default_value));
+}
+
 void EditorSceneFormatImporter::get_import_options(const String &p_path, List<ResourceImporter::ImportOption> *r_options) {
+	current_option_list = r_options;
 	GDVIRTUAL_CALL(_get_import_options, p_path);
+	current_option_list = nullptr;
 }
 
 Variant EditorSceneFormatImporter::get_option_visibility(const String &p_path, const String &p_scene_import_type, const String &p_option, const HashMap<StringName, Variant> &p_options) {
@@ -103,6 +115,9 @@ Variant EditorSceneFormatImporter::get_option_visibility(const String &p_path, c
 }
 
 void EditorSceneFormatImporter::_bind_methods() {
+	ClassDB::bind_method(D_METHOD("add_import_option", "name", "value"), &EditorSceneFormatImporter::add_import_option);
+	ClassDB::bind_method(D_METHOD("add_import_option_advanced", "type", "name", "default_value", "hint", "hint_string", "usage_flags"), &EditorSceneFormatImporter::add_import_option_advanced, DEFVAL(PROPERTY_HINT_NONE), DEFVAL(""), DEFVAL(PROPERTY_USAGE_DEFAULT));
+
 	GDVIRTUAL_BIND(_get_import_flags);
 	GDVIRTUAL_BIND(_get_extensions);
 	GDVIRTUAL_BIND(_import_scene, "path", "flags", "options");
diff --git a/editor/import/3d/resource_importer_scene.h b/editor/import/3d/resource_importer_scene.h
index 03102f9a3114..df2fe3d4827c 100644
--- a/editor/import/3d/resource_importer_scene.h
+++ b/editor/import/3d/resource_importer_scene.h
@@ -50,6 +50,8 @@ class Material;
 class EditorSceneFormatImporter : public RefCounted {
 	GDCLASS(EditorSceneFormatImporter, RefCounted);
 
+	List<ResourceImporter::ImportOption> *current_option_list = nullptr;
+
 protected:
 	static void _bind_methods();
 
@@ -73,6 +75,8 @@ class EditorSceneFormatImporter : public RefCounted {
 		IMPORT_FORCE_DISABLE_MESH_COMPRESSION = 64,
 	};
 
+	void add_import_option(const String &p_name, const Variant &p_default_value);
+	void add_import_option_advanced(Variant::Type p_type, const String &p_name, const Variant &p_default_value, PropertyHint p_hint = PROPERTY_HINT_NONE, const String &p_hint_string = String(), int p_usage_flags = PROPERTY_USAGE_DEFAULT);
 	virtual uint32_t get_import_flags() const;
 	virtual void get_extensions(List<String> *r_extensions) const;
 	virtual Node *import_scene(const String &p_path, uint32_t p_flags, const HashMap<StringName, Variant> &p_options, List<String> *r_missing_deps, Error *r_err = nullptr);


Test patch: