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

[Lunatix89]

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++

void EditorSceneFormatImporter::get_import_options(const String &p_path, List<ResourceImporter::ImportOption> *r_options) {
    GDVIRTUAL_CALL(_get_import_options, p_path);
}

Glue definition

GDVIRTUAL1(_get_import_options, String)

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

[YuriSizov]

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

[Lunatix89]

You're right, the title was a bit misleading, I've adjustet it so it should be more clear now.

[Riamuwilko]

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!

[Zireael07]

@Riamuwilko Godot engine does not practice assigning issues. If you think you can tackle this, just open a PR

[YuriSizov]

@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.

[Lunatix89]

I've already worked on this and will open a PR for it - sorry for the response delay.

[lyuma]

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

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