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.

Sign up for GitHub

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

Jump to bottom

Fix some arguments being passed as references in virtual functions #92175

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Fix some arguments being passed as references in virtual functions #92175

wants to merge 1 commit into from
+89 −51

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented May 20, 2024 •
edited

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):

Todo:

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

@AThousandShips AThousandShips added this to the 4.x milestone May 20, 2024
@xkisu xkisu mentioned this pull request May 20, 2024
Open
78 tasks
@AThousandShips
Copy link
Member Author

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
Copy link

xkisu commented May 21, 2024 •
edited

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.

image

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
Copy link
Member Author

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

@AThousandShips
Fix some arguments being passed as references in virtual functions
ddced1a 
These can't be used safely in extensions, replaced with `Dictionary`
returns
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
breaks compat bug discussion topic:gdextension
Projects
None yet
Milestone
4.x
Development

Successfully merging this pull request may close these issues.

Output parameters generated as const references
2 participants
@AThousandShips @xkisu