Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Output parameters generated as const references #1471

Open
xkisu opened this issue May 20, 2024 · 5 comments · May be fixed by godotengine/godot#92175
Open

Output parameters generated as const references #1471

xkisu opened this issue May 20, 2024 · 5 comments · May be fixed by godotengine/godot#92175
Labels
breaks compat bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation

Comments

@xkisu
Copy link

xkisu commented May 20, 2024

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

@AThousandShips AThousandShips added bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation labels May 20, 2024
@AThousandShips
Copy link
Member

AThousandShips commented May 20, 2024

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

@xkisu
Copy link
Author

xkisu commented May 20, 2024

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

@AThousandShips
Copy link
Member

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!

@xkisu
Copy link
Author

xkisu commented May 21, 2024

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!

@AThousandShips
Copy link
Member

Typed array specific issues are likely the same issue as:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks compat bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants