Fix some arguments being passed as references in virtual functions

[AThousandShips]

These can't be used safely in extensions, replaced with Dictionary returns

A few cases could do with this improvement, like EditorSceneFormatImporter::_import_scene but focused on cases that had arguments that were unusable, at least generally

This is patterned on the way ScriptLanguageExtension::_validate and similar are written, alternatively this could pass GDExtensionPtr but that would make these methods inaccessible from scripting, which considering some of them have documentation showing how to use them in GDScript and C# feels like the wrong way to go. Those cases do work (as far as I know, haven't tested the unchanged code) in at least GDScript (don't know if the passed references are useable in C#) but they use constant references in at least godot-cpp, which couldn't be changed without making broad changes there without handling other cases as there's no way to distinguish the cases.

This might be considered too significant a breakage, but it feels unsafe and strange if users have to use const_cast to be able to use these, and it might not be guaranteed that these passed arguments remain linked in the future since they aren't intended to be mutable based on the interface

Edit: the ability to modify these values seems to be unreliable, and might not work correctly at least in godot-cpp, so this change appears more necessary, this could be this issue surfacing in godot-cpp (so would be specific to typed arrays):

• C++ A deep copy (COW) of TypedArray is sometimes incorrectly made #89191

Todo:

• Update documentation (will update these once the approach has been decided on, as it involves example code)
• Consider additional cases

• Closes: Output parameters generated as const references godot-cpp#1471

[AThousandShips]

I'd appreciate if someone using C# could test if the unchanged interface allows returning values from these methods through the parameters, will confirm if the GDScript even works as is later today, if either doesn't this makes this change unavoidable IMO

[xkisu]

As per godotengine/godot-cpp#1471 I believe this may be related to a wider issue - some of the reference parameters that shouldn't be const qualified also aren't being correctly serialized between a GDExtension and the engine.

In my limited testing with an EditorImportPlugin, it appears that modifications made to TypedArray reference parameters (after a const_cast to correct the qualification) that are intended to be modified don't propagate back to the engine (possibly related to #89191). However changes made to Dictionary types after a const cast (with EditorResourcePreviewGenerator::_generate's metadata parameter in my testing) do make it back to the engine correctly.

I dropped some breakpoints in editor/import/editor_import_plugin.cpp#178-184 and adding to either of the gen_files or platform_variants arrays in the called extension isn't reflected in the variables that where passed to the GDVIRTUAL_CALL.

I tried to do more testing over the last week with my godot build to create a fixed version, but I'm not familiar enough with the GDVIRTUAL system to make the necessary modifications.

I've not tested any interface beyond EditorImportPlugin::_import and EditorResourcePreviewGenerator::_generate as I'm not familiar with any other methods that utilize return parameters yet.

I'd appreciate if someone using C# could test if the unchanged interface allows returning values from these methods through the parameters, will confirm if the GDScript even works as is later today, if either doesn't this makes this change unavoidable IMO

@AThousandShips I also tested it in an EditorImportPlugin with GDScript, using the code from the Import Plugin tutorial with the example additional next_pass file, and gen_files was correctly updating the dest_files list in the .import file; whereas the same implementation done in C++ was not. I'm not familiar enough with C# though to be able to check if it's affected at the moment.

[AThousandShips]

Thank you so much for your testing! Then at least the GDScript side works, can't test with C# at the moment but might be able to later

[dsnopek]

Thanks! The code looks good to me. I'm very happy to see the virtual method compat system getting some use :-)

But I think the docs could give more information about the returned Dictionarys and their expected keys - more specific notes about that below

[dsnopek]

This can't return null because it now returns a Dictionary. This applies to _generate_from_path also

[dsnopek]

Looking at the code, I can see that the Texture2D is supposed to go in the Dictionary under "result" - but this should be explained in the docs. This applies to _generate_from_path also

[dsnopek]

This should probably still say the same things but refer to the "metadata" key in the Dictionary instead

[AThousandShips]

Absolutely, the documentation updates was (as noted in the task list) waiting for approval of the actual specific implementations, so any and all suggestions are welcome