godot-cpp icon indicating copy to clipboard operation
godot-cpp copied to clipboard

Output parameters generated as `const` references

Open xkisu opened this issue 1 year ago • 5 comments

Godot version

4.2-stable

godot-cpp version

4.2-stable

System information

Godot v4.2.1.stable - macOS 14.4.1 - Vulkan (Forward+) - integrated Apple M1 - Apple M1 (8 Threads)

Issue description

The generated interface for both EditorImportPlugin::_import and EditorResourcePreviewGenerator::_generate have const-qualified parameters that are actually intended to be modified.

In the docs for EditorImportPlugin::_import, the platform_variants and gen_files parameters are suppose to be lists that can be used to inform the editor of additional files related to the import. Because they're const-qualified in the godot-cpp interface they can't be updated without a const_cast.

image

Same goes for EditorResourcePreviewGenerator::_generate's metadata parameter which is suppose to updated by the preview generator to provide any applicable metadata to EditorResourceTooltipPlugin::_make_tooltip_for_path for the editor tooltip.

image

Steps to reproduce

N/A

Minimal reproduction project

N/A

xkisu avatar May 20 '24 01:05 xkisu

This might be difficult to fix without breaking compatibility, but will look into it

They would likely have to be passed as pointers instead as I don't think the interface supports non-const references, which would break compatibility quite significantly, unsure how to proceed

Edit: This would likely be best solved by using the same method as ScriptLanguageExtension::_validate which returns a Dictionary with the relevant content, will test this for this and some other cases

AThousandShips avatar May 20 '24 08:05 AThousandShips

@AThousandShips I'm also not sure if this is related (will make an issue in the main repository when I have done more tests) but thought it would be worth raising it here if it is - the gen_files and platform_variants parameters for EditorImportPlugin::_import appear to not be making it back to the engine. I've made a few EditorImportPlugins and never successful gotten gen_files to update the file list in the .import file since 4.0 was released (almost the same setup worked in GDNative).

Note that I const_cast gen_files to make it writable, which works fine for other Dictionary-based reference parameters (expanded on below).

godot::TypedArray<godot::String>& gen_files_writable = const_cast<godot::TypedArray<godot::String>&>(gen_files);

I dropped some breakpoints in editor/import/editor_import_plugin.cpp#178-184 and adding to either of those arrays doesn't seem to be making it back out of the GDVIRTUAL_CALL call. image

However other parameters that use Dictionary (i.e. the aforementioned ScriptLanguageExtension::_validate or EditorResourcePreviewGenerator::_generate) are passing the modified structures back to the caller in the engine fine. I'm wondering if this is due to Array types being COW and Dictionary not being COW so the changes are being thrown away?

Will do more testing to see if I can pinpoint it if I have time in the next week or so.

xkisu avatar May 20 '24 21:05 xkisu

If you can confirm it doesn't even work with const_cast that'd help a lot, I suspect it might be due to serialisation or similar, that adds further support for the need for this change other than just convenience, please report your findings on the PR if you confirm it, thank you!

AThousandShips avatar May 21 '24 07:05 AThousandShips

If you can confirm it doesn't even work with const_cast that'd help a lot, I suspect it might be due to serialisation or similar, that adds further support for the need for this change other than just convenience, please report your findings on the PR if you confirm it, thank you!

Yes I can confirm it still doesn't work, will add it to the PR shortly!

xkisu avatar May 21 '24 07:05 xkisu

Typed array specific issues are likely the same issue as:

  • https://github.com/godotengine/godot/issues/89191

Which could be fixed in Godot-cpp as well, but might still be problematic in other bindings making assumptions about the data etc.

AThousandShips avatar May 21 '24 07:05 AThousandShips